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

pull aureleoules wants to merge 1 commits into bitcoin:master from aureleoules:assert_get_ancestor_rebase changing 5 files +39 −28
  1. aureleoules commented at 2:01 am on April 8, 2022: member

    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

  2. DrahtBot added the label Consensus on Apr 8, 2022
  3. DrahtBot added the label RPC/REST/ZMQ on Apr 8, 2022
  4. DrahtBot added the label Validation on Apr 8, 2022
  5. in src/test/miner_tests.cpp:452 in d297649836 outdated 
    450-        m_node.chainman->ActiveChain().Tip()->GetAncestor(m_node.chainman->ActiveChain().Tip()->nHeight - i)->nTime += 512; //Trick the MedianTimePast
451-
452+    const int SEQUENCE_LOCK_TIME = 512; // Sequence locks pass 512 seconds later
453+    for (int i = 0; i < CBlockIndex::nMedianTimeSpan; ++i)
454+        m_node.chainman->ActiveChain().Tip()->GetAncestor(m_node.chainman->ActiveChain().Tip()->nHeight - i)->nTime += SEQUENCE_LOCK_TIME; //Trick the MedianTimePast
455+
    jonatack commented at 7:00 am on April 8, 2022:
    The CI whitespace linter needs appeasing by removing the extra spaces here. You can test this locally on the command line by running test/lint/lint-whitespace.sh in this branch from your local bitcoin repo root.
  6. in src/test/miner_tests.cpp:506 in d297649836 outdated 
    501@@ -501,8 +502,12 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
502     // For now these will still generate a valid template until BIP68 soft fork
503     BOOST_CHECK_EQUAL(pblocktemplate->block.vtx.size(), 3U);
504     // However if we advance height by 1 and time by 512, all of them should be mined
505-    for (int i = 0; i < CBlockIndex::nMedianTimeSpan; i++)
506-        m_node.chainman->ActiveChain().Tip()->GetAncestor(m_node.chainman->ActiveChain().Tip()->nHeight - i)->nTime += 512; //Trick the MedianTimePast
507+    for (int i = 0; i < CBlockIndex::nMedianTimeSpan; ++i)
508+    {
    jonatack commented at 7:03 am on April 8, 2022:
    Please use clang-format for the bracket here (like in the previous version of this pull).
  7. jonatack commented at 7:06 am on April 8, 2022: member
    A couple of initial comments. Minor detail, please [don’t use @-usernames in the PR description](https://jonatack.github.io/articles/how-to-contribute-pull-requests-to-bitcoin-core#pr-descriptions).
  8. in src/versionbits.cpp:115 in d297649836 outdated 
    108@@ -109,6 +109,10 @@ BIP9Stats AbstractThresholdConditionChecker::GetStateStatisticsFor(const CBlockI
109 
110     // Find how many blocks are in the current period
111     int blocks_in_period = 1 + (pindex->nHeight % stats.period);
112+    // Find beginning of period
113+    const CBlockIndex* pindexEndOfPrevPeriod = pindex->GetAncestor(pindex->nHeight - ((pindex->nHeight + 1) % stats.period));
114+    assert(pindexEndOfPrevPeriod != nullptr);
115+    stats.elapsed = pindex->nHeight - pindexEndOfPrevPeriod->nHeight;
    jonatack commented at 9:03 am on April 8, 2022:
    It looks like these 4 lines were superceded in a7469bcd356 and can be removed here
  9. jonatack commented at 9:08 am on April 8, 2022: member

    Concept ACK with some suggestions for the lines touched here

     0diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp
 1index 4f00d6e094..eb731ba205 100644
 2--- a/src/consensus/tx_verify.cpp
 3+++ b/src/consensus/tx_verify.cpp
 4@@ -76,7 +80,7 @@ std::pair<int, int64_t> CalculateSequenceLocks(const CTransaction &tx, int flags
 5             assert(ancestor != nullptr);
 6-            int64_t nCoinTime = ancestor->GetMedianTimePast();
 7+            const int64_t nCoinTime = ancestor->GetMedianTimePast();
 8             // NOTE: Subtract 1 to maintain nLockTime semantics
 9diff --git a/src/consensus/tx_verify.h b/src/consensus/tx_verify.h
10index 1209c0faa5..1386b029f5 100644
11--- a/src/consensus/tx_verify.h
12+++ b/src/consensus/tx_verify.h
13@@ -446,17 +446,16 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
14     BOOST_CHECK(!TestSequenceLocks(CTransaction{tx})); // Sequence locks fail
15 
16-    const int SEQUENCE_LOCK_TIME = 512; // Sequence locks pass 512 seconds later
17-    for (int i = 0; i < CBlockIndex::nMedianTimeSpan; ++i)
18-        m_node.chainman->ActiveChain().Tip()->GetAncestor(m_node.chainman->ActiveChain().Tip()->nHeight - i)->nTime += SEQUENCE_LOCK_TIME; //Trick the MedianTimePast
19-        
20-    {
21+    const int SEQUENCE_LOCK_TIME{512}; // Sequence locks pass 512 seconds later
22+    for (int i = 0; i < CBlockIndex::nMedianTimeSpan; ++i) {
23+        m_node.chainman->ActiveChain().Tip()->GetAncestor(m_node.chainman->ActiveChain().Tip()->nHeight - i)->nTime += SEQUENCE_LOCK_TIME; // Trick the MedianTimePast
24         CBlockIndex* active_chain_tip = m_node.chainman->ActiveChain().Tip();
25-        BOOST_CHECK(SequenceLocks(CTransaction(tx), flags, prevheights, CreateBlockIndex(active_chain_tip->nHeight + 1, active_chain_tip))); // Sequence locks pass 512 seconds later
26+        BOOST_CHECK(SequenceLocks(CTransaction(tx), flags, prevheights, CreateBlockIndex(active_chain_tip->nHeight + 1, active_chain_tip)));
27     }
28 
29-    for (int i = 0; i < CBlockIndex::nMedianTimeSpan; ++i)
30-        m_node.chainman->ActiveChain().Tip()->GetAncestor(m_node.chainman->ActiveChain().Tip()->nHeight - i)->nTime -= SEQUENCE_LOCK_TIME; //undo tricked MTP
31+    for (int i = 0; i < CBlockIndex::nMedianTimeSpan; ++i) {
32+        m_node.chainman->ActiveChain().Tip()->GetAncestor(m_node.chainman->ActiveChain().Tip()->nHeight - i)->nTime -= SEQUENCE_LOCK_TIME; // undo tricked MTP
33+    }
34 
35     // absolute height locked
36@@ -501,9 +500,9 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
37     BOOST_CHECK_EQUAL(pblocktemplate->block.vtx.size(), 3U);
38-    // However if we advance height by 1 and time by 512, all of them should be mined
39-    for (int i = 0; i < CBlockIndex::nMedianTimeSpan; ++i)
40-    {
41+    // However, if we advance height by 1 and time by SEQUENCE_LOCK_TIME, all of
42+    // them should be mined.
43+    for (int i = 0; i < CBlockIndex::nMedianTimeSpan; ++i) {
44         CBlockIndex* ancestor = m_node.chainman->ActiveChain().Tip()->GetAncestor(m_node.chainman->ActiveChain().Tip()->nHeight - i);
45         assert(ancestor != nullptr);
46         ancestor->nTime += SEQUENCE_LOCK_TIME; // Trick the MedianTimePast
47diff --git a/src/versionbits.cpp b/src/versionbits.cpp
48index 9218c6bf26..934bfb015f 100644
49--- a/src/versionbits.cpp
50+++ b/src/versionbits.cpp
51@@ -109,10 +109,6 @@ BIP9Stats AbstractThresholdConditionChecker::GetStateStatisticsFor(const CBlockI
52 
53     // Find how many blocks are in the current period
54     int blocks_in_period = 1 + (pindex->nHeight % stats.period);
55-    // Find beginning of period
56-    const CBlockIndex* pindexEndOfPrevPeriod = pindex->GetAncestor(pindex->nHeight - ((pindex->nHeight + 1) % stats.period));
57-    assert(pindexEndOfPrevPeriod != nullptr);
58-    stats.elapsed = pindex->nHeight - pindexEndOfPrevPeriod->nHeight;
59 
60     // Reset signalling_blocks
  10. aureleoules force-pushed on Apr 8, 2022
  11. aureleoules commented at 12:27 pm on April 8, 2022: member
    Thank you for your review @jonatack, I have addressed your comments.
  12. in src/consensus/tx_verify.cpp:77 in dcaae44e7f outdated 
    73@@ -74,7 +74,9 @@ std::pair<int, int64_t> CalculateSequenceLocks(const CTransaction &tx, int flags
74         int nCoinHeight = prevHeights[txinIndex];
75 
76         if (txin.nSequence & CTxIn::SEQUENCE_LOCKTIME_TYPE_FLAG) {
77-            int64_t nCoinTime = block.GetAncestor(std::max(nCoinHeight-1, 0))->GetMedianTimePast();
    MarcoFalke commented at 12:33 pm on April 8, 2022:
    If you don’t use the result, you can just inline Assert(bla)->…
    aureleoules commented at 1:03 pm on April 8, 2022:
    Thank you, fixed it
  13. in src/test/miner_tests.cpp:58 in dcaae44e7f outdated 
    72-              {4, 873254135},  {0, 341817335},  {6, 53501687},   {3, 179755410}, {5, 172209688},  {8, 516810279},
73-              {4, 1228391489}, {8, 325372589},  {6, 550367589},  {0, 876291812}, {7, 412454120},  {7, 717202854},
74-              {2, 222677843},  {6, 251778867},  {7, 842004420},  {7, 194762829}, {4, 96668841},   {1, 925485796},
75-              {0, 792342903},  {6, 678455063},  {6, 773251385},  {5, 186617471}, {6, 883189502},  {7, 396077336},
76-              {8, 254702874},  {0, 455592851}};
77+} BLOCKINFO[]{{8, 582909131}, {0, 971462344}, {2, 1169481553}, {6, 66147495}, {7, 427785981}, {8, 80538907}, {8, 207348013}, {2, 1951240923}, {4, 215054351}, {1, 491520534}, {8, 1282281282}, {4, 639565734}, {3, 248274685}, {8, 1160085976}, {6, 396349768}, {5, 393780549}, {5, 1096899528}, {4, 965381630}, {0, 728758712}, {5, 318638310}, {3, 164591898}, {2, 274234550}, {2, 254411237}, {7, 561761812}, {2, 268342573}, {0, 402816691}, {1, 221006382}, {6, 538872455}, {7, 393315655}, {4, 814555937}, {7, 504879194}, {6, 467769648}, {3, 925972193}, {2, 200581872}, {3, 168915404}, {8, 430446262}, {5, 773507406}, {3, 1195366164}, {0, 433361157}, {3, 297051771}, {0, 558856551}, {2, 501614039}, {3, 528488272}, {2, 473587734}, {8, 230125274}, {2, 494084400}, {4, 357314010}, {8, 60361686}, {7, 640624687}, {3, 480441695}, {8, 1424447925}, {4, 752745419}, {1, 288532283}, {6, 669170574}, {5, 1900907591}, {3, 555326037}, {3, 1121014051}, {0, 545835650}, {8, 189196651}, {5, 252371575}, {0, 199163095}, {6, 558895874}, {6, 1656839784}, {6, 815175452}, {6, 718677851}, {5, 544000334}, {0, 340113484}, {6, 850744437}, {4, 496721063}, {8, 524715182}, {6, 574361898}, {6, 1642305743}, {6, 355110149}, {5, 1647379658}, {8, 1103005356}, {7, 556460625}, {3, 1139533992}, {5, 304736030}, {2, 361539446}, {2, 143720360}, {6, 201939025}, {7, 423141476}, {4, 574633709}, {3, 1412254823}, {4, 873254135}, {0, 341817335}, {6, 53501687}, {3, 179755410}, {5, 172209688}, {8, 516810279}, {4, 1228391489}, {8, 325372589}, {6, 550367589}, {0, 876291812}, {7, 412454120}, {7, 717202854}, {2, 222677843}, {6, 251778867}, {7, 842004420}, {7, 194762829}, {4, 96668841}, {1, 925485796}, {0, 792342903}, {6, 678455063}, {6, 773251385}, {5, 186617471}, {6, 883189502}, {7, 396077336}, {8, 254702874}, {0, 455592851}};
    MarcoFalke commented at 12:33 pm on April 8, 2022:
    ?
    aureleoules commented at 12:35 pm on April 8, 2022:
    I realized I clang-formatted the whole file, just fixed it
  14. aureleoules force-pushed on Apr 8, 2022
  15. aureleoules force-pushed on Apr 8, 2022
  16. in src/validation.cpp:4088 in e3250c5bff outdated 
    4084@@ -4079,6 +4085,7 @@ bool CChainState::ReplayBlocks()
4085     int nForkHeight = pindexFork ? pindexFork->nHeight : 0;
4086     for (int nHeight = nForkHeight + 1; nHeight <= pindexNew->nHeight; ++nHeight) {
4087         const CBlockIndex* pindex = pindexNew->GetAncestor(nHeight);
4088+        assert(pindex != nullptr);
    MarcoFalke commented at 2:39 pm on April 8, 2022: 
    0        const CBlockIndex& block{*Assert(pindexNew->GetAncestor(nHeight))};

    If the value is used later, you may as well convert it to a reference after the assert.

    aureleoules commented at 3:36 pm on April 8, 2022:
    Interesting, I added it
  17. in src/rpc/blockchain.cpp:1626 in e3250c5bff outdated 
    1622@@ -1623,6 +1623,7 @@ static RPCHelpMan getchaintxstats()
1623     }
1624 
1625     const CBlockIndex* pindexPast = pindex->GetAncestor(pindex->nHeight - blockcount);
1626+    CHECK_NONFATAL(pindexPast != nullptr);
    MarcoFalke commented at 2:41 pm on April 8, 2022:
    Unrelated: Could probably make sense to convert CHECK_NONFATAL to the identity function like Assert
    aureleoules commented at 10:12 pm on April 8, 2022:
    I’d be happy to work on this
    aureleoules commented at 0:20 am on April 25, 2022:
    Since #24812 got merged, I used the identity CHECK_NONFATAL macro in this PR as well.
  18. aureleoules force-pushed on Apr 8, 2022
  19. in src/test/miner_tests.cpp:454 in 93e516d909 outdated 
    452+    const int SEQUENCE_LOCK_TIME = 512; // Sequence locks pass 512 seconds later
453+    for (int i = 0; i < CBlockIndex::nMedianTimeSpan; ++i)
454+        m_node.chainman->ActiveChain().Tip()->GetAncestor(m_node.chainman->ActiveChain().Tip()->nHeight - i)->nTime += SEQUENCE_LOCK_TIME; //Trick the MedianTimePast
455     {
456         CBlockIndex* active_chain_tip = m_node.chainman->ActiveChain().Tip();
457         BOOST_CHECK(SequenceLocks(CTransaction(tx), flags, prevheights, CreateBlockIndex(active_chain_tip->nHeight + 1, active_chain_tip))); // Sequence locks pass 512 seconds later
    jonatack commented at 9:12 am on April 12, 2022:
    Duplicate comment here
  20. in src/test/miner_tests.cpp:503 in 93e516d909 outdated 
    500@@ -501,8 +501,11 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
501     // For now these will still generate a valid template until BIP68 soft fork
502     BOOST_CHECK_EQUAL(pblocktemplate->block.vtx.size(), 3U);
503     // However if we advance height by 1 and time by 512, all of them should be mined
    jonatack commented at 9:13 am on April 12, 2022: 
    0    // However if we advance height by 1 and time by SEQUENCE_LOCK_TIME, all of them should be mined

    #24804 (comment)

  21. fanquake deleted a comment on Apr 12, 2022
  22. jonatack commented at 9:58 am on April 12, 2022: member 
     0index a8701cc9aa..c03e975c03 100644
 1--- a/src/consensus/tx_verify.cpp
 2+++ b/src/consensus/tx_verify.cpp
 3@@ -75,7 +75,7 @@ std::pair<int, int64_t> CalculateSequenceLocks(const CTransaction &tx, int flags
 4         int nCoinHeight = prevHeights[txinIndex];
 5 
 6         if (txin.nSequence & CTxIn::SEQUENCE_LOCKTIME_TYPE_FLAG) {
 7-            const int64_t nCoinTime = Assert(block.GetAncestor(std::max(nCoinHeight - 1, 0)))->GetMedianTimePast();
 8+            const int64_t nCoinTime{Assert(block.GetAncestor(std::max(nCoinHeight - 1, 0)))->GetMedianTimePast()};
 9             // NOTE: Subtract 1 to maintain nLockTime semantics
10             // BIP 68 relative lock times have the semantics of calculating
11             // the first block or time at which the transaction would be
12diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp
13index 78bc93c3d4..1d7eb80d78 100644
14--- a/src/test/miner_tests.cpp
15+++ b/src/test/miner_tests.cpp
16@@ -448,14 +448,17 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
17 
18     const int SEQUENCE_LOCK_TIME = 512; // Sequence locks pass 512 seconds later
19     for (int i = 0; i < CBlockIndex::nMedianTimeSpan; ++i)
20-        m_node.chainman->ActiveChain().Tip()->GetAncestor(m_node.chainman->ActiveChain().Tip()->nHeight - i)->nTime += SEQUENCE_LOCK_TIME; //Trick the MedianTimePast
21+        m_node.chainman->ActiveChain().Tip()->GetAncestor(m_node.chainman->ActiveChain().Tip()->nHeight - i)->nTime += SEQUENCE_LOCK_TIME; // Trick the MedianTimePast
22     {
23         CBlockIndex* active_chain_tip = m_node.chainman->ActiveChain().Tip();
24-        BOOST_CHECK(SequenceLocks(CTransaction(tx), flags, prevheights, CreateBlockIndex(active_chain_tip->nHeight + 1, active_chain_tip))); // Sequence locks pass 512 seconds later
25+        BOOST_CHECK(SequenceLocks(CTransaction(tx), flags, prevheights, CreateBlockIndex(active_chain_tip->nHeight + 1, active_chain_tip)));
26     }
27 
28-    for (int i = 0; i < CBlockIndex::nMedianTimeSpan; ++i)
29-        m_node.chainman->ActiveChain().Tip()->GetAncestor(m_node.chainman->ActiveChain().Tip()->nHeight - i)->nTime -= SEQUENCE_LOCK_TIME; //undo tricked MTP
30+    for (int i = 0; i < CBlockIndex::nMedianTimeSpan; ++i) {
31+        CBlockIndex* ancestor = m_node.chainman->ActiveChain().Tip()->GetAncestor(m_node.chainman->ActiveChain().Tip()->nHeight - i);
32+        assert(ancestor != nullptr);
33+        ancestor->nTime -= SEQUENCE_LOCK_TIME; // undo tricked MTP
34+    }
35 
36     // absolute height locked
37     tx.vin[0].prevout.hash = txFirst[2]->GetHash();
  23. aureleoules force-pushed on Apr 12, 2022
  24. aureleoules force-pushed on Apr 12, 2022
  25. aureleoules commented at 4:25 pm on April 12, 2022: member
    Seems like there is a bug in the CI? I ran the tests on my fork and passed all tests
  26. jonatack commented at 4:50 pm on April 12, 2022: member
    Yes, it’s unrelated, see issue #24151 (and review #24454).
  27. DrahtBot commented at 7:04 pm on April 12, 2022: member

    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.

  28. jonatack commented at 11:39 am on April 13, 2022: member
    ACK 977b6b1874942474006d5d3c257b18b77c882af1
  29. MarcoFalke referenced this in commit b1c5991eeb on Apr 24, 2022
  30. aureleoules force-pushed on Apr 25, 2022
  31. in src/rpc/blockchain.cpp:1621 in 54070a5bed outdated 
    1617@@ -1618,7 +1618,7 @@ static RPCHelpMan getchaintxstats()
1618         }
1619     }
1620 
1621-    const CBlockIndex* pindexPast = pindex->GetAncestor(pindex->nHeight - blockcount);
1622+    const CBlockIndex* pindexPast = CHECK_NONFATAL(pindex->GetAncestor(pindex->nHeight - blockcount));
    MarcoFalke commented at 7:07 am on April 25, 2022: 
    0    const CBlockIndex& past_block{*CHECK_NONFATAL(pindex->GetAncestor(pindex->nHeight - blockcount))};

    nit: no need to keep the pointer while touching this line

  32. in src/rpc/blockchain.cpp:1764 in 54070a5bed outdated 
    1760@@ -1761,8 +1761,7 @@ static RPCHelpMan getblockstats()
1761 {
1762     ChainstateManager& chainman = EnsureAnyChainman(request.context);
1763     LOCK(cs_main);
1764-    const CBlockIndex* pindex{ParseHashOrHeight(request.params[0], chainman)};
1765-    CHECK_NONFATAL(pindex != nullptr);
1766+    const CBlockIndex* pindex{CHECK_NONFATAL(ParseHashOrHeight(request.params[0], chainman))};
    MarcoFalke commented at 7:08 am on April 25, 2022: 
    0    const CBlockIndex& block{*CHECK_NONFATAL(ParseHashOrHeight(request.params[0], chainman))};

    same nit

  33. MarcoFalke commented at 7:12 am on April 25, 2022: member
    Left some nits
  34. aureleoules force-pushed on Apr 25, 2022
  35. aureleoules force-pushed on Apr 25, 2022
  36. jonatack commented at 2:57 pm on April 26, 2022: member
    LGTM, modulo since the commit is authored by @adamjonas, you could change the first “Co-Authored-By:” in the commit message to yourself.
  37. aureleoules force-pushed on Apr 26, 2022
  38. jonatack commented at 8:56 am on April 27, 2022: member

    ACK c0fcbb7506e0b5a49549a1615e48781151cb4687

    If you have to retouch, maybe consider

     0--- a/src/rpc/blockchain.cpp
 1+++ b/src/rpc/blockchain.cpp
 2@@ -1619,8 +1619,8 @@ static RPCHelpMan getchaintxstats()
 3     const CBlockIndex& past_block{*CHECK_NONFATAL(pindex->GetAncestor(pindex->nHeight - blockcount))};
 4-    int nTimeDiff = pindex->GetMedianTimePast() - past_block.GetMedianTimePast();
 5-    int nTxDiff = pindex->nChainTx - past_block.nChainTx;
 6+    const int64_t nTimeDiff{pindex->GetMedianTimePast() - past_block.GetMedianTimePast()};
 7+    const int nTxDiff = pindex->nChainTx - past_block.nChainTx;
 8 
 9@@ -1772,8 +1772,8 @@ static RPCHelpMan getblockstats() 
10-    const CBlock block = GetBlockChecked(chainman.m_blockman, &pindex);
11-    const CBlockUndo blockUndo = GetUndoChecked(chainman.m_blockman, &pindex);
12+    const CBlock& block = GetBlockChecked(chainman.m_blockman, &pindex);
13+    const CBlockUndo& blockUndo = GetUndoChecked(chainman.m_blockman, &pindex);
  39. MarcoFalke removed the label RPC/REST/ZMQ on Apr 29, 2022
  40. MarcoFalke removed the label Validation on Apr 29, 2022
  41. MarcoFalke removed the label Consensus on Apr 29, 2022
  42. MarcoFalke added the label Refactoring on Apr 29, 2022
  43. aureleoules force-pushed on May 3, 2022
  44. aureleoules force-pushed on May 5, 2022
  45. in src/test/miner_tests.cpp:459 in 260a05ddaf outdated 
    460 
461-    for (int i = 0; i < CBlockIndex::nMedianTimeSpan; i++)
462-        m_node.chainman->ActiveChain().Tip()->GetAncestor(m_node.chainman->ActiveChain().Tip()->nHeight - i)->nTime -= 512; //undo tricked MTP
463+    for (int i = 0; i < CBlockIndex::nMedianTimeSpan; ++i) {
464+        CBlockIndex* ancestor = m_node.chainman->ActiveChain().Tip()->GetAncestor(m_node.chainman->ActiveChain().Tip()->nHeight - i);
465+        assert(ancestor != nullptr);
    jonatack commented at 12:21 pm on May 5, 2022:

    if you retouch

    0-        CBlockIndex* ancestor = m_node.chainman->ActiveChain().Tip()->GetAncestor(m_node.chainman->ActiveChain().Tip()->nHeight - i);
1-        assert(ancestor != nullptr);
2+        CBlockIndex* ancestor{Assert(m_node.chainman->ActiveChain().Tip()->GetAncestor(m_node.chainman->ActiveChain().Tip()->nHeight - i))};
  46. in src/validation.cpp:255 in 260a05ddaf outdated 
    246@@ -247,6 +247,12 @@ bool CheckSequenceLocksAtTip(CBlockIndex* tip,
247                 }
248             }
249             lp->maxInputBlock = tip->GetAncestor(maxInputHeight);
250+            // tip->GetAncestor(maxInputHeight) should never return a nullptr
251+            // because maxInputHeight is always less than the tip height.
252+            // It would, however, be a bad bug to continue execution, since a
253+            // LockPoints object with the maxInputBlock member set to nullptr
254+            // signifies no relative lock time.
255+            assert(lp->maxInputBlock);
    jonatack commented at 12:24 pm on May 5, 2022: 
    0-            lp->maxInputBlock = tip->GetAncestor(maxInputHeight);
1             // tip->GetAncestor(maxInputHeight) should never return a nullptr
2             // because maxInputHeight is always less than the tip height.
3             // It would, however, be a bad bug to continue execution, since a
4             // LockPoints object with the maxInputBlock member set to nullptr
5             // signifies no relative lock time.
6-            assert(lp->maxInputBlock);
7+            lp->maxInputBlock = Assert(tip->GetAncestor(maxInputHeight));
  47. in src/versionbits.cpp:162 in 260a05ddaf outdated 
    158@@ -159,6 +159,7 @@ int AbstractThresholdConditionChecker::GetStateSinceHeightFor(const CBlockIndex*
159     // if we are computing for the first block of a period, then pindexPrev points to the last block of the previous period.
160     // The parent of the genesis block is represented by nullptr.
161     pindexPrev = pindexPrev->GetAncestor(pindexPrev->nHeight - ((pindexPrev->nHeight + 1) % nPeriod));
162+    assert(pindexPrev != nullptr);
    jonatack commented at 12:26 pm on May 5, 2022:

    Noting that it would be possible to use the Assert macro here but it doesn’t seem worth including util/check.h just for one use.

     0 
 1 #include <versionbits.h>
 2 #include <consensus/params.h>
 3+#include <util/check.h>
 4 
 5 ThresholdState AbstractThresholdConditionChecker::GetStateFor(const CBlockIndex* pindexPrev, const Consensus::Params& params, ThresholdConditionCache& cache) const
 6 {
 7@@ -158,8 +159,7 @@ int AbstractThresholdConditionChecker::GetStateSinceHeightFor(const CBlockIndex*
 8     // if we are computing for the last block of a period, then pindexPrev points to the second to last block of the period, and
 9     // if we are computing for the first block of a period, then pindexPrev points to the last block of the previous period.
10     // The parent of the genesis block is represented by nullptr.
11-    pindexPrev = pindexPrev->GetAncestor(pindexPrev->nHeight - ((pindexPrev->nHeight + 1) % nPeriod));
12-    assert(pindexPrev != nullptr);
13+    pindexPrev = Assert(pindexPrev->GetAncestor(pindexPrev->nHeight - ((pindexPrev->nHeight + 1) % nPeriod)));
14 
15     const CBlockIndex* previousPeriodParent = pindexPrev->GetAncestor(pindexPrev->nHeight - nPeriod);
  48. jonatack commented at 12:30 pm on May 5, 2022: member
    ACK 260a05ddafd9d62aa478637a8d58d67ff4dbacc6 with a couple more suggestions
  49. Sanity assert GetAncestor() != nullptr where appropriate 
    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 <aurele@oules.com>
Co-Authored-By: danra <danra@users.noreply.github.com>
    308dd2e93e
  50. aureleoules force-pushed on May 5, 2022
  51. jonatack commented at 3:38 pm on May 5, 2022: member
    ACK 308dd2e93e92f4cac4e7d75478316af9bb2b77b8
  52. MarcoFalke merged this on May 6, 2022
  53. MarcoFalke closed this on May 6, 2022
  54. sidhujag referenced this in commit 2c2c03039a on May 9, 2022
  55. aureleoules deleted the branch on May 20, 2022
  56. Fabcien referenced this in commit 70912f9a54 on Mar 6, 2023
  57. DrahtBot locked this on May 20, 2023


aureleoules jonatack MarcoFalke DrahtBot

Labels
Refactoring

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-01 13:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me