bitshares / bitshares/bitshares-core
`assert()` in db_market.cpp and other source code?
- Dominant language
- C++
- Stars
- 1.2k
- Forks
- 660
- Avg merge
- 8h 17m
- Merged PRs (30d)
- 26
Description
Update: quoted from https://github.com/bitshares/bitshares-core/pull/669#issuecomment-387308111:
>it is too dangerous to simply search&replace. Each of these asserts needs to be carefully reviewed
>* if it is (still) correct
>* if it is a useful check
>
>We should ... rather remove/replace assert's whenever we come across them. Once we've got rid of most of them we can tackle the rest in one go.
Related discussion in https://github.com/steemit/steem/issues/1815:
>There are lots of `assert()` statements in `database::match()`. This ticket proposes changing them to FC_ASSERT(). (**The difference is that assert() is only triggered in debug builds.**) Some things to keep in mind about this kind of change:
> * (a) In general, recently the core devs' policy has been that it is acceptable to convert assert() to FC_ASSERT() statements for conditions that should never happen. The slight performance penalty is worth extra checking, defense in depth against potential bugs or exploits.
> * (b) Newly-added asserts must "fail safe" with regard to block production, FC_ASSERT() in per-block processing could shut down the blockchain, so we can't put any there.
> * (c) Newly-added asserts must "fail safe" with regard to consensus changes, any newly added FC_ASSERT() must be gated by a hardfork (which can be removed once the hardfork passes, assuming nothing that triggered the newly added FC_ASSERT() got into the chain).
>
> The implementation strategy for adding new FC_ASSERT() is fairly straightforward:
> * Change to FC_ASSERT(), replay, see if anything on-chain triggers assertions
> * If something triggers, we need to further analyze the issue
> * If nothing triggers, move FC_ASSERT() to if( has_hf_20 ) block
> * Once HF20 passes, remove if( has_hf_20 ) block, if we can replay then all is well (we can merge the > * removal of the if( has_hf_20 ) check and do FC_ASSERT() unconditionally)
For BitShares, we need to do:
- [ ] Change all `asset(...)` to `FC_ASSERT(...)`, replay, see if anything on-chain triggers assertions
- [ ] If something triggers, we need to further analyze the issue
- [ ] If nothing triggers, surround `FC_ASSERT(...)` with a `if( has_hf_xxx )` check, e.g.
`if( head_block_time() > HARDFORK_CORE_511_TIME ) FC_ASSERT(...)`
- [ ] Once the hard fork time passed, remove the `if( has_hf_xxx )` check, if we can replay then all is well (we can merge the
- [ ] removal of the `if( has_hf_xxx )` check and do `FC_ASSERT(...)` unconditionally)
Contributor guide
No contributing guide indexed for this repository
Research direction
Start by reviewing the assert() occurrences in db_market.cpp and the database::match() discussion, checking each condition rather than applying a search-and-replace. Replay the chain after any FC_ASSERT() changes and investigate anything triggered; done requires the reviewed assertions and hardfork-gated behavior to remain replay-safe.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- blockchain
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 20/100