Sanity assert GetAncestor() != nullptr where appropriate#17232
Sanity assert GetAncestor() != nullptr where appropriate#17232adamjonas wants to merge 1 commit intobitcoin:masterfrom
Conversation
|
Concept ACK Explicit is better than implicit. In case the assumptions do not hold then a predictable assertion failure is the better alternative. |
adamjonas
force-pushed
the
assert_get_ancestor
branch
from
ddad4b0 to
22cca81
Compare
adamjonas
force-pushed
the
assert_get_ancestor
branch
from
22cca81 to
59f51d5
Compare
|
Code review ACK 59f51d5 |
|
@adamjonas Thanks for making implicit assumptions explicit by adding assertions and documentation. If you want to continue working on making currently implicit FWIW gcc 7.4.0 with |
I'm not sure there's much benefit to fixing these disabled warnings relative to the cost of implementing and reviewing the fixes. But there are going to be future PRs, another way of going about them instead of calling functions that return pointers and assert Lines 761 to 762 in ddd4629 bitcoin/src/wallet/rpcdump.cpp Lines 90 to 97 in ddd4629 |
|
Hitting an assertion is always better than a segmentation fault, so while none of the |
|
I'm happy to follow up and keep working on these, but I'd prefer to open a separate PR(s) to address the other instances and maintain a low burden of review. |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
Add sanity asserts for return value of `CBlockIndex::GetAncestor()` where appropriate. In validation.cpp `CheckSequenceLocks`, check the return value of `tip->GetAncestor(maxInputHeight)` stored into `lp->maxInputBlock`. If it ever returns `nullptr` because the ancestor isn't found, it's going to be a bad bug to keep going, since a `LockPoints` object with the `maxInputBlock` member set to `nullptr` signifies no relative lock time. In the other places, the added asserts would prevent accidental dereferencing of a null pointer which is undefined behavior. Co-Authored-By: danra <[email protected]>
adamjonas
force-pushed
the
assert_get_ancestor
branch
from
59f51d5 to
b11df4a
Compare
|
Going to close this for lack of interest. Maybe @MarcoFalke or @fanquake could label it up for grabs if someone wants to pick it back up again? |
|
|
||
| if (txin.nSequence & CTxIn::SEQUENCE_LOCKTIME_TYPE_FLAG) { | ||
| int64_t nCoinTime = block.GetAncestor(std::max(nCoinHeight-1, 0))->GetMedianTimePast(); | ||
| const CBlockIndex* ancestor = block.GetAncestor(std::max(nCoinHeight-1, 0)); |
There was a problem hiding this comment.
- const CBlockIndex* ancestor = block.GetAncestor(std::max(nCoinHeight-1, 0));
+ const CBlockIndex* ancestor = block.GetAncestor(std::max(nCoinHeight - 1, 0));| for (int i = 0; i < CBlockIndex::nMedianTimeSpan; i++) | ||
| ::ChainActive().Tip()->GetAncestor(::ChainActive().Tip()->nHeight - i)->nTime += 512; //Trick the MedianTimePast | ||
| // However if we advance height by 1 and time by SEQUENCE_LOCK_TIME, all of them should be mined | ||
| for (int i = 0; i < CBlockIndex::nMedianTimeSpan; i++) { |
There was a problem hiding this comment.
- for (int i = 0; i < CBlockIndex::nMedianTimeSpan; i++) {
+ for (int i = 0; i < CBlockIndex::nMedianTimeSpan; ++i) {
|
@adamjonas Would you be willing to re-open this given renewed reviewer interest? :) |
8 participants
Re-opening #11342 (taking suggestions from comments) which adds sanity asserts for return value of
CBlockIndex::GetAncestor()where appropriate.In validation.cpp
CheckSequenceLocks, check the return value oftip->GetAncestor(maxInputHeight)stored intolp->maxInputBlock. If it ever returnsnullptrbecause the ancestor isn't found, it's going to be a bad bug to keep going, since aLockPointsobject with themaxInputBlockmember set tonullptrsignifies no relative lock time.In other places, the added asserts would prevent accidental dereferencing of a null pointer which is undefined behavior.