assert() in db_market.cpp and other source code?

[abitmore]

Update: quoted from #669 (comment):

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 steemit/steem#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)

[abitmore]

Also found some in db_block.cpp.

[abitmore]

Just think out loud:

Adding FC_ASSERT in the middle of processing a new transaction may be fine; however, adding it to the middle of post-transaction-processing work (e.g. clear_expired_orders()) is somehow dangerous, since it may halt the chain unexpectedly, which is not the best option in most circumstances.

That said, we should AVOID using FC_ASSERT in database class if not to process transactions or blocks.

All new code need to be reviewed.

[abitmore]

Need more discussion. Moved to future milestone.