Skip to content

Commit

Permalink
Merge bitcoin#24804: Sanity assert GetAncestor() != nullptr where app…
Browse files Browse the repository at this point in the history 
…ropriate

308dd2e Sanity assert GetAncestor() != nullptr where appropriate (Adam Jonas)

Pull request description:

  Re-opening bitcoin#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]>

ACKs for top commit:
  jonatack:
    ACK 308dd2e

Tree-SHA512: 5bfdaab1499607ae2c3cd3e2e9e8c37850bfd0e327e680f4e36c81f9c6d98a543af78ecfac1ab0e06325d264412615a04d52005875780c7db2a4d81bd2d2259a
  • Loading branch information
MacroFake committed May 6, 2022
2 parents 77a9997 + 308dd2e commit 59ac8ba
Show file tree
Hide file tree
Showing 5 changed files with 39 additions and 28 deletions.
3 changes: 2 additions & 1 deletion src/consensus/tx_verify.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include <consensus/validation.h>
#include <primitives/transaction.h>
#include <script/interpreter.h>
#include <util/check.h>
#include <util/moneystr.h>

bool IsFinalTx(const CTransaction &tx, int nBlockHeight, int64_t nBlockTime)
Expand Down Expand Up @@ -74,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) {
int64_t nCoinTime = 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
Expand Down
23 changes: 11 additions & 12 deletions src/rpc/blockchain.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1613,9 +1613,9 @@ static RPCHelpMan getchaintxstats()
}
}

const CBlockIndex* pindexPast = pindex->GetAncestor(pindex->nHeight - blockcount);
int nTimeDiff = pindex->GetMedianTimePast() - pindexPast->GetMedianTimePast();
int nTxDiff = pindex->nChainTx - pindexPast->nChainTx;
const CBlockIndex& past_block{*CHECK_NONFATAL(pindex->GetAncestor(pindex->nHeight - blockcount))};
const int64_t nTimeDiff{pindex->GetMedianTimePast() - past_block.GetMedianTimePast()};
const int nTxDiff = pindex->nChainTx - past_block.nChainTx;

UniValue ret(UniValue::VOBJ);
ret.pushKV("time", (int64_t)pindex->nTime);
Expand Down Expand Up @@ -1756,8 +1756,7 @@ static RPCHelpMan getblockstats()
{
ChainstateManager& chainman = EnsureAnyChainman(request.context);
LOCK(cs_main);
const CBlockIndex* pindex{ParseHashOrHeight(request.params[0], chainman)};
CHECK_NONFATAL(pindex != nullptr);
const CBlockIndex& pindex{*CHECK_NONFATAL(ParseHashOrHeight(request.params[0], chainman))};

std::set<std::string> stats;
if (!request.params[1].isNull()) {
Expand All @@ -1768,8 +1767,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);

const bool do_all = stats.size() == 0; // Calculate everything if nothing selected (default)
const bool do_mediantxsize = do_all || stats.count("mediantxsize") != 0;
Expand Down Expand Up @@ -1887,25 +1886,25 @@ static RPCHelpMan getblockstats()
ret_all.pushKV("avgfee", (block.vtx.size() > 1) ? totalfee / (block.vtx.size() - 1) : 0);
ret_all.pushKV("avgfeerate", total_weight ? (totalfee * WITNESS_SCALE_FACTOR) / total_weight : 0); // Unit: sat/vbyte
ret_all.pushKV("avgtxsize", (block.vtx.size() > 1) ? total_size / (block.vtx.size() - 1) : 0);
ret_all.pushKV("blockhash", pindex->GetBlockHash().GetHex());
ret_all.pushKV("blockhash", pindex.GetBlockHash().GetHex());
ret_all.pushKV("feerate_percentiles", feerates_res);
ret_all.pushKV("height", (int64_t)pindex->nHeight);
ret_all.pushKV("height", (int64_t)pindex.nHeight);
ret_all.pushKV("ins", inputs);
ret_all.pushKV("maxfee", maxfee);
ret_all.pushKV("maxfeerate", maxfeerate);
ret_all.pushKV("maxtxsize", maxtxsize);
ret_all.pushKV("medianfee", CalculateTruncatedMedian(fee_array));
ret_all.pushKV("mediantime", pindex->GetMedianTimePast());
ret_all.pushKV("mediantime", pindex.GetMedianTimePast());
ret_all.pushKV("mediantxsize", CalculateTruncatedMedian(txsize_array));
ret_all.pushKV("minfee", (minfee == MAX_MONEY) ? 0 : minfee);
ret_all.pushKV("minfeerate", (minfeerate == MAX_MONEY) ? 0 : minfeerate);
ret_all.pushKV("mintxsize", mintxsize == MAX_BLOCK_SERIALIZED_SIZE ? 0 : mintxsize);
ret_all.pushKV("outs", outputs);
ret_all.pushKV("subsidy", GetBlockSubsidy(pindex->nHeight, Params().GetConsensus()));
ret_all.pushKV("subsidy", GetBlockSubsidy(pindex.nHeight, Params().GetConsensus()));
ret_all.pushKV("swtotal_size", swtotal_size);
ret_all.pushKV("swtotal_weight", swtotal_weight);
ret_all.pushKV("swtxs", swtxs);
ret_all.pushKV("time", pindex->GetBlockTime());
ret_all.pushKV("time", pindex.GetBlockTime());
ret_all.pushKV("total_out", total_out);
ret_all.pushKV("total_size", total_size);
ret_all.pushKV("total_weight", total_weight);
Expand Down
22 changes: 13 additions & 9 deletions src/test/miner_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -405,16 +405,18 @@ void MinerTestingSetup::TestBasicMining(const CChainParams& chainparams, const C
BOOST_CHECK(CheckFinalTxAtTip(m_node.chainman->ActiveChain().Tip(), CTransaction{tx})); // Locktime passes
BOOST_CHECK(!TestSequenceLocks(CTransaction{tx})); // Sequence locks fail

for (int i = 0; i < CBlockIndex::nMedianTimeSpan; i++)
m_node.chainman->ActiveChain().Tip()->GetAncestor(m_node.chainman->ActiveChain().Tip()->nHeight - i)->nTime += 512; //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 -= 512; //undo tricked MTP
for (int i = 0; i < CBlockIndex::nMedianTimeSpan; ++i) {
CBlockIndex* ancestor{Assert(m_node.chainman->ActiveChain().Tip()->GetAncestor(m_node.chainman->ActiveChain().Tip()->nHeight - i))};
ancestor->nTime -= SEQUENCE_LOCK_TIME; // undo tricked MTP
}

// absolute height locked
tx.vin[0].prevout.hash = txFirst[2]->GetHash();
Expand Down Expand Up @@ -459,9 +461,11 @@ void MinerTestingSetup::TestBasicMining(const CChainParams& chainparams, const C
// but relative locked txs will if inconsistently added to mempool.
// For now these will still generate a valid template until BIP68 soft fork
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++)
m_node.chainman->ActiveChain().Tip()->GetAncestor(m_node.chainman->ActiveChain().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) {
CBlockIndex* ancestor{Assert(m_node.chainman->ActiveChain().Tip()->GetAncestor(m_node.chainman->ActiveChain().Tip()->nHeight - i))};
ancestor->nTime += SEQUENCE_LOCK_TIME; // Trick the MedianTimePast
}
m_node.chainman->ActiveChain().Tip()->nHeight++;
SetMockTime(m_node.chainman->ActiveChain().Tip()->GetMedianTimePast() + 1);

Expand Down
14 changes: 10 additions & 4 deletions src/validation.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,12 @@ bool CheckSequenceLocksAtTip(CBlockIndex* tip,
maxInputHeight = std::max(maxInputHeight, height);
}
}
lp->maxInputBlock = tip->GetAncestor(maxInputHeight);
// tip->GetAncestor(maxInputHeight) should never return a nullptr
// because maxInputHeight is always less than the tip height.
// It would, however, be a bad bug to continue execution, since a
// LockPoints object with the maxInputBlock member set to nullptr
// signifies no relative lock time.
lp->maxInputBlock = Assert(tip->GetAncestor(maxInputHeight));
}
}
return EvaluateSequenceLocks(index, lockPair);
Expand Down Expand Up @@ -4108,10 +4113,11 @@ bool CChainState::ReplayBlocks()
// Roll forward from the forking point to the new tip.
int nForkHeight = pindexFork ? pindexFork->nHeight : 0;
for (int nHeight = nForkHeight + 1; nHeight <= pindexNew->nHeight; ++nHeight) {
const CBlockIndex* pindex = pindexNew->GetAncestor(nHeight);
LogPrintf("Rolling forward %s (%i)\n", pindex->GetBlockHash().ToString(), nHeight);
const CBlockIndex& pindex{*Assert(pindexNew->GetAncestor(nHeight))};

LogPrintf("Rolling forward %s (%i)\n", pindex.GetBlockHash().ToString(), nHeight);
uiInterface.ShowProgress(_("Replaying blocks…").translated, (int) ((nHeight - nForkHeight) * 100.0 / (pindexNew->nHeight - nForkHeight)) , false);
if (!RollforwardBlock(pindex, cache)) return false;
if (!RollforwardBlock(&pindex, cache)) return false;
}

cache.SetBestBlock(pindexNew->GetBlockHash());
Expand Down
5 changes: 3 additions & 2 deletions src/versionbits.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,9 @@
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

#include <versionbits.h>
#include <consensus/params.h>
#include <util/check.h>
#include <versionbits.h>

ThresholdState AbstractThresholdConditionChecker::GetStateFor(const CBlockIndex* pindexPrev, const Consensus::Params& params, ThresholdConditionCache& cache) const
{
Expand Down Expand Up @@ -158,7 +159,7 @@ int AbstractThresholdConditionChecker::GetStateSinceHeightFor(const CBlockIndex*
// if we are computing for the last block of a period, then pindexPrev points to the second to last block of the period, and
// if we are computing for the first block of a period, then pindexPrev points to the last block of the previous period.
// The parent of the genesis block is represented by nullptr.
pindexPrev = pindexPrev->GetAncestor(pindexPrev->nHeight - ((pindexPrev->nHeight + 1) % nPeriod));
pindexPrev = Assert(pindexPrev->GetAncestor(pindexPrev->nHeight - ((pindexPrev->nHeight + 1) % nPeriod)));

const CBlockIndex* previousPeriodParent = pindexPrev->GetAncestor(pindexPrev->nHeight - nPeriod);

Expand Down

0 comments on commit 59ac8ba

Please sign in to comment.