bitshares / bitshares/bitshares-core

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

Open
#511 3 comments 0 reactions 0 assignees View on GitHub
2a Discussion Needed hardfork question security
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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.