Sanity assert GetAncestor() != nullptr where appropriate #24804

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 jonas@chaincode.com Co-Authored-By: danra danra@users.noreply.github.com

A couple of initial comments. Minor detail, please don't use [@-usernames](/bitcoin-bitcoin/contributor/-usernames/) in the PR description.

Concept ACK with some suggestions for the lines touched here

<details><summary>suggested diff for your perusal</summary><p>

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

</p></details>

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

<details><summary>A few suggestions</summary><p>

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();

</p></details>

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

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

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

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.

ACK 977b6b1874942474006d5d3c257b18b77c882af1

Left some nits

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

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);

ACK 260a05ddafd9d62aa478637a8d58d67ff4dbacc6 with a couple more suggestions

ACK 308dd2e93e92f4cac4e7d75478316af9bb2b77b8