Sanity assert GetAncestor() != nullptr where appropriate by aureleoules · Pull Request #24804 · bitcoin/bitcoin

aureleoules · 2022-04-08T02:01:53Z

Re-opening #17232. I have rebased the PR and addressed jonatack's nit suggestions.

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: Adam Jonas [email protected]
Co-Authored-By: danra [email protected]

jonatack

A couple of initial comments. Minor detail, please don't use @-usernames in the PR description.

src/test/miner_tests.cpp

src/versionbits.cpp

jonatack · 2022-04-08T09:08:16Z

Concept ACK with some suggestions for the lines touched here

suggested diff for your perusal

diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp
index 4f00d6e094..eb731ba205 100644
--- a/src/consensus/tx_verify.cpp
+++ b/src/consensus/tx_verify.cpp
@@ -76,7 +80,7 @@ std::pair<int, int64_t> CalculateSequenceLocks(const CTransaction &tx, int flags
             assert(ancestor != nullptr);
-            int64_t nCoinTime = ancestor->GetMedianTimePast();
+            const int64_t nCoinTime = ancestor->GetMedianTimePast();
             // NOTE: Subtract 1 to maintain nLockTime semantics
diff --git a/src/consensus/tx_verify.h b/src/consensus/tx_verify.h
index 1209c0faa5..1386b029f5 100644
--- a/src/consensus/tx_verify.h
+++ b/src/consensus/tx_verify.h
@@ -446,17 +446,16 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
     BOOST_CHECK(!TestSequenceLocks(CTransaction{tx})); // Sequence locks fail
 
-    const int SEQUENCE_LOCK_TIME = 512; // Sequence locks pass 512 seconds later
-    for (int i = 0; i < CBlockIndex::nMedianTimeSpan; ++i)
-        m_node.chainman->ActiveChain().Tip()->GetAncestor(m_node.chainman->ActiveChain().Tip()->nHeight - i)->nTime += SEQUENCE_LOCK_TIME; //Trick the MedianTimePast
-        
-    {
+    const int SEQUENCE_LOCK_TIME{512}; // Sequence locks pass 512 seconds later
+    for (int i = 0; i < CBlockIndex::nMedianTimeSpan; ++i) {
+        m_node.chainman->ActiveChain().Tip()->GetAncestor(m_node.chainman->ActiveChain().Tip()->nHeight - i)->nTime += SEQUENCE_LOCK_TIME; // Trick the MedianTimePast
         CBlockIndex* active_chain_tip = m_node.chainman->ActiveChain().Tip();
-        BOOST_CHECK(SequenceLocks(CTransaction(tx), flags, prevheights, CreateBlockIndex(active_chain_tip->nHeight + 1, active_chain_tip))); // Sequence locks pass 512 seconds later
+        BOOST_CHECK(SequenceLocks(CTransaction(tx), flags, prevheights, CreateBlockIndex(active_chain_tip->nHeight + 1, active_chain_tip)));
     }
 
-    for (int i = 0; i < CBlockIndex::nMedianTimeSpan; ++i)
-        m_node.chainman->ActiveChain().Tip()->GetAncestor(m_node.chainman->ActiveChain().Tip()->nHeight - i)->nTime -= SEQUENCE_LOCK_TIME; //undo tricked MTP
+    for (int i = 0; i < CBlockIndex::nMedianTimeSpan; ++i) {
+        m_node.chainman->ActiveChain().Tip()->GetAncestor(m_node.chainman->ActiveChain().Tip()->nHeight - i)->nTime -= SEQUENCE_LOCK_TIME; // undo tricked MTP
+    }
 
     // absolute height locked
@@ -501,9 +500,9 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
     BOOST_CHECK_EQUAL(pblocktemplate->block.vtx.size(), 3U);
-    // However if we advance height by 1 and time by 512, all of them should be mined
-    for (int i = 0; i < CBlockIndex::nMedianTimeSpan; ++i)
-    {
+    // 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) {
         CBlockIndex* ancestor = m_node.chainman->ActiveChain().Tip()->GetAncestor(m_node.chainman->ActiveChain().Tip()->nHeight - i);
         assert(ancestor != nullptr);
         ancestor->nTime += SEQUENCE_LOCK_TIME; // Trick the MedianTimePast
diff --git a/src/versionbits.cpp b/src/versionbits.cpp
index 9218c6bf26..934bfb015f 100644
--- a/src/versionbits.cpp
+++ b/src/versionbits.cpp
@@ -109,10 +109,6 @@ BIP9Stats AbstractThresholdConditionChecker::GetStateStatisticsFor(const CBlockI
 
     // Find how many blocks are in the current period
     int blocks_in_period = 1 + (pindex->nHeight % stats.period);
-    // Find beginning of period
-    const CBlockIndex* pindexEndOfPrevPeriod = pindex->GetAncestor(pindex->nHeight - ((pindex->nHeight + 1) % stats.period));
-    assert(pindexEndOfPrevPeriod != nullptr);
-    stats.elapsed = pindex->nHeight - pindexEndOfPrevPeriod->nHeight;
 
     // Reset signalling_blocks

aureleoules · 2022-04-08T12:27:31Z

Thank you for your review @jonatack, I have addressed your comments.

src/consensus/tx_verify.cpp

src/test/miner_tests.cpp

src/validation.cpp

src/rpc/blockchain.cpp

src/test/miner_tests.cpp

jonatack

A few suggestions

index a8701cc9aa..c03e975c03 100644
--- a/src/consensus/tx_verify.cpp
+++ b/src/consensus/tx_verify.cpp
@@ -75,7 +75,7 @@ std::pair<int, int64_t> CalculateSequenceLocks(const CTransaction &tx, int flags
         int nCoinHeight = prevHeights[txinIndex];
 
         if (txin.nSequence & CTxIn::SEQUENCE_LOCKTIME_TYPE_FLAG) {
-            const int64_t nCoinTime = Assert(block.GetAncestor(std::max(nCoinHeight - 1, 0)))->GetMedianTimePast();
+            const int64_t nCoinTime{Assert(block.GetAncestor(std::max(nCoinHeight - 1, 0)))->GetMedianTimePast()};
             // NOTE: Subtract 1 to maintain nLockTime semantics
             // BIP 68 relative lock times have the semantics of calculating
             // the first block or time at which the transaction would be
diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp
index 78bc93c3d4..1d7eb80d78 100644
--- a/src/test/miner_tests.cpp
+++ b/src/test/miner_tests.cpp
@@ -448,14 +448,17 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
 
     const int SEQUENCE_LOCK_TIME = 512; // Sequence locks pass 512 seconds later
     for (int i = 0; i < CBlockIndex::nMedianTimeSpan; ++i)
-        m_node.chainman->ActiveChain().Tip()->GetAncestor(m_node.chainman->ActiveChain().Tip()->nHeight - i)->nTime += SEQUENCE_LOCK_TIME; //Trick the MedianTimePast
+        m_node.chainman->ActiveChain().Tip()->GetAncestor(m_node.chainman->ActiveChain().Tip()->nHeight - i)->nTime += SEQUENCE_LOCK_TIME; // Trick the MedianTimePast
     {
         CBlockIndex* active_chain_tip = m_node.chainman->ActiveChain().Tip();
-        BOOST_CHECK(SequenceLocks(CTransaction(tx), flags, prevheights, CreateBlockIndex(active_chain_tip->nHeight + 1, active_chain_tip))); // Sequence locks pass 512 seconds later
+        BOOST_CHECK(SequenceLocks(CTransaction(tx), flags, prevheights, CreateBlockIndex(active_chain_tip->nHeight + 1, active_chain_tip)));
     }
 
-    for (int i = 0; i < CBlockIndex::nMedianTimeSpan; ++i)
-        m_node.chainman->ActiveChain().Tip()->GetAncestor(m_node.chainman->ActiveChain().Tip()->nHeight - i)->nTime -= SEQUENCE_LOCK_TIME; //undo tricked MTP
+    for (int i = 0; i < CBlockIndex::nMedianTimeSpan; ++i) {
+        CBlockIndex* ancestor = m_node.chainman->ActiveChain().Tip()->GetAncestor(m_node.chainman->ActiveChain().Tip()->nHeight - i);
+        assert(ancestor != nullptr);
+        ancestor->nTime -= SEQUENCE_LOCK_TIME; // undo tricked MTP
+    }
 
     // absolute height locked
     tx.vin[0].prevout.hash = txFirst[2]->GetHash();

aureleoules · 2022-04-12T16:25:39Z

Seems like there is a bug in the CI? I ran the tests on my fork and passed all tests

jonatack · 2022-04-12T16:50:13Z

Yes, it's unrelated, see issue #24151 (and review #24454).

DrahtBot · 2022-04-12T19:04:01Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#24912 (refactor: update CBlockIndex::nChainTx to be uint64_t by mruddy)
#24833 (refactor: consensus/tx_verify.{h,cpp} tidy-ups by jonatack)
#24567 (Enforce BIP 68 from genesis by MarcoFalke)
#23897 (refactor: Move calculation logic out from CheckSequenceLocksAtTip() by hebasto)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

jonatack · 2022-04-13T11:39:31Z

ACK 977b6b1874942474006d5d3c257b18b77c882af1

…function and NONFATAL_UNREACHABLE macro ee02c8b util/check: Add CHECK_NONFATAL identity function, NONFATAL_UNREACHABLE AND UNREACHABLE macros (Aurèle Oulès) Pull request description: This PR replaces the macro `CHECK_NONFATAL` with an identity function. I simplified the usage of `CHECK_NONFATAL` where applicable in `src/rpc`. This function is useful in sanity checks for RPC and command-line interfaces. Context: bitcoin/bitcoin#24804 (comment). Also adds `UNREACHABLE_NONFATAL` macro. ACKs for top commit: jonatack: ACK ee02c8b MarcoFalke: ACK ee02c8b 🍨 Tree-SHA512: 3cba09223cd7b22e62fe5d0b46c4a024c1d9957d4268ba6d3fb07fcc0a5854fc0886bb3266184e6a7df5df91373b3e84edd6adf6999c4e934aeef8c043b01aa2

maflcko

Left some nits

src/rpc/blockchain.cpp

jonatack

LGTM, modulo since the commit is authored by @adamjonas, you could change the first "Co-Authored-By:" in the commit message to yourself.

jonatack · 2022-04-27T08:56:24Z

ACK c0fcbb7506e0b5a49549a1615e48781151cb4687

If you have to retouch, maybe consider

--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -1619,8 +1619,8 @@ static RPCHelpMan getchaintxstats()
     const CBlockIndex& past_block{*CHECK_NONFATAL(pindex->GetAncestor(pindex->nHeight - blockcount))};
-    int nTimeDiff = pindex->GetMedianTimePast() - past_block.GetMedianTimePast();
-    int nTxDiff = pindex->nChainTx - past_block.nChainTx;
+    const int64_t nTimeDiff{pindex->GetMedianTimePast() - past_block.GetMedianTimePast()};
+    const int nTxDiff = pindex->nChainTx - past_block.nChainTx;
 
@@ -1772,8 +1772,8 @@ static RPCHelpMan getblockstats() 
-    const CBlock block = GetBlockChecked(chainman.m_blockman, &pindex);
-    const CBlockUndo blockUndo = GetUndoChecked(chainman.m_blockman, &pindex);
+    const CBlock& block = GetBlockChecked(chainman.m_blockman, &pindex);
+    const CBlockUndo& blockUndo = GetUndoChecked(chainman.m_blockman, &pindex);

jonatack

ACK 260a05ddafd9d62aa478637a8d58d67ff4dbacc6 with a couple more suggestions

src/test/miner_tests.cpp

src/validation.cpp

src/versionbits.cpp

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: Aurèle Oulès <[email protected]> Co-Authored-By: danra <[email protected]>

jonatack · 2022-05-05T15:38:19Z

ACK 308dd2e

…ropriate

Summary: 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: Aurèle Oulès <[email protected]> Co-Authored-By: danra <[email protected]> This is a backport of [[bitcoin/bitcoin#24804 | core#24804]] Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D13242

DrahtBot added Consensus RPC/REST/ZMQ Validation labels Apr 8, 2022

jonatack reviewed Apr 8, 2022

View reviewed changes

src/test/miner_tests.cpp Outdated Show resolved Hide resolved

src/test/miner_tests.cpp Outdated Show resolved Hide resolved

jonatack reviewed Apr 8, 2022

View reviewed changes

src/versionbits.cpp Outdated Show resolved Hide resolved

aureleoules force-pushed the assert_get_ancestor_rebase branch from d297649 to dcaae44 Compare April 8, 2022 12:26

maflcko reviewed Apr 8, 2022

View reviewed changes

src/consensus/tx_verify.cpp Outdated Show resolved Hide resolved

src/test/miner_tests.cpp Outdated Show resolved Hide resolved

aureleoules force-pushed the assert_get_ancestor_rebase branch 2 times, most recently from 44f4141 to e3250c5 Compare April 8, 2022 13:03

maflcko reviewed Apr 8, 2022

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved

aureleoules force-pushed the assert_get_ancestor_rebase branch from e3250c5 to 93e516d Compare April 8, 2022 15:35

aureleoules mentioned this pull request Apr 8, 2022

util/check: Add CHECK_NONFATAL identity function and NONFATAL_UNREACHABLE macro #24812

Merged

Ying091909 mentioned this pull request Apr 11, 2022

reference zglbt/LOTGA#2

Open

jonatack reviewed Apr 12, 2022

View reviewed changes

src/test/miner_tests.cpp Outdated Show resolved Hide resolved

jonatack reviewed Apr 12, 2022

View reviewed changes

src/test/miner_tests.cpp Outdated Show resolved Hide resolved

bitcoin deleted a comment from joshuadbryant1985 Apr 12, 2022

jonatack reviewed Apr 12, 2022

View reviewed changes

aureleoules force-pushed the assert_get_ancestor_rebase branch 2 times, most recently from 9f9361f to 977b6b1 Compare April 12, 2022 12:19

DrahtBot mentioned this pull request Apr 12, 2022

refactor: consensus/tx_verify.{h,cpp} tidy-ups #24833

Closed

DrahtBot mentioned this pull request Apr 12, 2022

Enforce BIP 68 from genesis #24567

Closed

DrahtBot mentioned this pull request Apr 20, 2022

refactor: Move calculation logic out from CheckSequenceLocksAtTip() #23897

Merged

aureleoules force-pushed the assert_get_ancestor_rebase branch from 977b6b1 to 54070a5 Compare April 25, 2022 00:17

maflcko reviewed Apr 25, 2022

View reviewed changes

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved

aureleoules force-pushed the assert_get_ancestor_rebase branch 2 times, most recently from 9075d79 to a06141f Compare April 25, 2022 23:59

jonatack reviewed Apr 26, 2022

View reviewed changes

aureleoules force-pushed the assert_get_ancestor_rebase branch from a06141f to c0fcbb7 Compare April 26, 2022 15:57

DrahtBot mentioned this pull request Apr 26, 2022

refactor: update CBlockIndex::nChainTx to be uint64_t #24912

Closed

maflcko added Refactoring and removed RPC/REST/ZMQ Validation Consensus labels Apr 29, 2022

aureleoules force-pushed the assert_get_ancestor_rebase branch 2 times, most recently from b0c9d04 to 260a05d Compare May 5, 2022 01:15

jonatack reviewed May 5, 2022

View reviewed changes

src/test/miner_tests.cpp Outdated Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

src/versionbits.cpp Outdated Show resolved Hide resolved

aureleoules force-pushed the assert_get_ancestor_rebase branch from 260a05d to 308dd2e Compare May 5, 2022 13:59

maflcko merged commit 59ac8ba into bitcoin:master May 6, 2022

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 9, 2022

Merge bitcoin#24804: Sanity assert GetAncestor() != nullptr where app…

2c2c030

…ropriate

aureleoules deleted the assert_get_ancestor_rebase branch May 20, 2022 22:14

bitcoin locked and limited conversation to collaborators May 20, 2023

Conversation

aureleoules commented Apr 8, 2022 • edited by maflcko Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonatack commented Apr 8, 2022

Uh oh!

aureleoules commented Apr 8, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonatack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aureleoules commented Apr 12, 2022

Uh oh!

jonatack commented Apr 12, 2022

Uh oh!

DrahtBot commented Apr 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

jonatack commented Apr 13, 2022

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jonatack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonatack commented Apr 27, 2022

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonatack commented May 5, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

aureleoules commented Apr 8, 2022 •

edited by maflcko

Loading

jonatack left a comment •

edited

Loading

jonatack left a comment •

edited

Loading

DrahtBot commented Apr 12, 2022 •

edited

Loading

jonatack left a comment •

edited

Loading