From 75db62ba4cae048e742ca02dc6a52b3b3d6727de Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Tue, 31 Jan 2023 13:26:54 +0000 Subject: [PATCH] refactor: Move calculation logic out from `CheckSequenceLocksAtTip()` --- src/test/miner_tests.cpp | 4 +- src/validation.cpp | 96 +++++++++------------------------------- src/validation.h | 17 ++----- 3 files changed, 27 insertions(+), 90 deletions(-) diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp index ba43f1926b..0d545a4619 100644 --- a/src/test/miner_tests.cpp +++ b/src/test/miner_tests.cpp @@ -36,7 +36,9 @@ struct MinerTestingSetup : public TestingSetup { bool TestSequenceLocks(const CTransaction& tx, CTxMemPool& tx_mempool) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { CCoinsViewMemPool view_mempool{&m_node.chainman->ActiveChainstate().CoinsTip(), tx_mempool}; - return CheckSequenceLocksAtTip(m_node.chainman->ActiveChain().Tip(), view_mempool, tx); + CBlockIndex* tip{m_node.chainman->ActiveChain().Tip()}; + const std::optional lock_points{CalculateLockPointsAtTip(tip, view_mempool, tx)}; + return lock_points.has_value() && CheckSequenceLocksAtTip(tip, *lock_points); } CTxMemPool& MakeMempool() { diff --git a/src/validation.cpp b/src/validation.cpp index 21c91fbdda..a93621c1ab 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -252,10 +252,7 @@ std::optional CalculateLockPointsAtTip( } bool CheckSequenceLocksAtTip(CBlockIndex* tip, - const CCoinsView& coins_view, - const CTransaction& tx, - LockPoints* lp, - bool useExistingLockPoints) + const LockPoints& lock_points) { assert(tip != nullptr); @@ -269,61 +266,7 @@ bool CheckSequenceLocksAtTip(CBlockIndex* tip, // *next* block, we need to use one more than active_chainstate.m_chain.Height() index.nHeight = tip->nHeight + 1; - std::pair lockPair; - if (useExistingLockPoints) { - assert(lp); - lockPair.first = lp->height; - lockPair.second = lp->time; - } - else { - std::vector prevheights; - prevheights.resize(tx.vin.size()); - for (size_t txinIndex = 0; txinIndex < tx.vin.size(); txinIndex++) { - const CTxIn& txin = tx.vin[txinIndex]; - Coin coin; - if (!coins_view.GetCoin(txin.prevout, coin)) { - return error("%s: Missing input", __func__); - } - if (coin.nHeight == MEMPOOL_HEIGHT) { - // Assume all mempool transaction confirm in the next block - prevheights[txinIndex] = tip->nHeight + 1; - } else { - prevheights[txinIndex] = coin.nHeight; - } - } - lockPair = CalculateSequenceLocks(tx, STANDARD_LOCKTIME_VERIFY_FLAGS, prevheights, index); - if (lp) { - lp->height = lockPair.first; - lp->time = lockPair.second; - // Also store the hash of the block with the highest height of - // all the blocks which have sequence locked prevouts. - // This hash needs to still be on the chain - // for these LockPoint calculations to be valid - // Note: It is impossible to correctly calculate a maxInputBlock - // if any of the sequence locked inputs depend on unconfirmed txs, - // except in the special case where the relative lock time/height - // is 0, which is equivalent to no sequence lock. Since we assume - // input height of tip+1 for mempool txs and test the resulting - // lockPair from CalculateSequenceLocks against tip+1. We know - // EvaluateSequenceLocks will fail if there was a non-zero sequence - // lock on a mempool input, so we can use the return value of - // CheckSequenceLocksAtTip to indicate the LockPoints validity - int maxInputHeight = 0; - for (const int height : prevheights) { - // Can ignore mempool inputs since we'll fail if they had non-zero locks - if (height != tip->nHeight+1) { - maxInputHeight = std::max(maxInputHeight, height); - } - } - // 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); + return EvaluateSequenceLocks(index, {lock_points.height, lock_points.time}); } // Returns the script flags which should be checked for a given block @@ -409,20 +352,23 @@ void Chainstate::MaybeUpdateMempoolForReorg( // The transaction must be final. if (!CheckFinalTxAtTip(*Assert(m_chain.Tip()), tx)) return true; - LockPoints lp = it->GetLockPoints(); - const bool validLP{TestLockPointValidity(m_chain, lp)}; - CCoinsViewMemPool view_mempool(&CoinsTip(), *m_mempool); + + const LockPoints& lp = it->GetLockPoints(); // CheckSequenceLocksAtTip checks if the transaction will be final in the next block to be - // created on top of the new chain. We use useExistingLockPoints=false so that, instead of - // using the information in lp (which might now refer to a block that no longer exists in - // the chain), it will update lp to contain LockPoints relevant to the new chain. - if (!CheckSequenceLocksAtTip(m_chain.Tip(), view_mempool, tx, &lp, validLP)) { - // If CheckSequenceLocksAtTip fails, remove the tx and don't depend on the LockPoints. - return true; - } else if (!validLP) { - // If CheckSequenceLocksAtTip succeeded, it also updated the LockPoints. - // Now update the mempool entry lockpoints as well. - m_mempool->mapTx.modify(it, [&lp](CTxMemPoolEntry& e) { e.UpdateLockPoints(lp); }); + // created on top of the new chain. + if (TestLockPointValidity(m_chain, lp)) { + if (!CheckSequenceLocksAtTip(m_chain.Tip(), lp)) { + return true; + } + } else { + const CCoinsViewMemPool view_mempool{&CoinsTip(), *m_mempool}; + const std::optional new_lock_points{CalculateLockPointsAtTip(m_chain.Tip(), view_mempool, tx)}; + if (new_lock_points.has_value() && CheckSequenceLocksAtTip(m_chain.Tip(), *new_lock_points)) { + // Now update the mempool entry lockpoints as well. + m_mempool->mapTx.modify(it, [&new_lock_points](CTxMemPoolEntry& e) { e.UpdateLockPoints(*new_lock_points); }); + } else { + return true; + } } // If the transaction spends any coinbase outputs, it must be mature. @@ -824,7 +770,6 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) } } - LockPoints lp; m_view.SetBackend(m_viewmempool); const CCoinsViewCache& coins_cache = m_active_chainstate.CoinsTip(); @@ -866,7 +811,8 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) // be mined yet. // Pass in m_view which has all of the relevant inputs cached. Note that, since m_view's // backend was removed, it no longer pulls coins from the mempool. - if (!CheckSequenceLocksAtTip(m_active_chainstate.m_chain.Tip(), m_view, tx, &lp)) { + const std::optional lock_points{CalculateLockPointsAtTip(m_active_chainstate.m_chain.Tip(), m_view, tx)}; + if (!lock_points.has_value() || !CheckSequenceLocksAtTip(m_active_chainstate.m_chain.Tip(), *lock_points)) { return state.Invalid(TxValidationResult::TX_PREMATURE_SPEND, "non-BIP68-final"); } @@ -902,7 +848,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) } entry.reset(new CTxMemPoolEntry(ptx, ws.m_base_fees, nAcceptTime, m_active_chainstate.m_chain.Height(), - fSpendsCoinbase, nSigOpsCost, lp)); + fSpendsCoinbase, nSigOpsCost, lock_points.value())); ws.m_vsize = entry->GetTxSize(); if (nSigOpsCost > MAX_STANDARD_TX_SIGOPS_COST) diff --git a/src/validation.h b/src/validation.h index 36c59b41df..cb9c2ab85f 100644 --- a/src/validation.h +++ b/src/validation.h @@ -271,24 +271,13 @@ std::optional CalculateLockPointsAtTip( * Check if transaction will be BIP68 final in the next block to be created on top of tip. * @param[in] tip Chain tip to check tx sequence locks against. For example, * the tip of the current active chain. - * @param[in] coins_view Any CCoinsView that provides access to the relevant coins for - * checking sequence locks. For example, it can be a CCoinsViewCache - * that isn't connected to anything but contains all the relevant - * coins, or a CCoinsViewMemPool that is connected to the - * mempool and chainstate UTXO set. In the latter case, the caller is - * responsible for holding the appropriate locks to ensure that - * calls to GetCoin() return correct coins. + * @param[in] lock_points LockPoints containing the height and time at which this + * transaction is final. * Simulates calling SequenceLocks() with data from the tip passed in. - * Optionally stores in LockPoints the resulting height and time calculated and the hash - * of the block needed for calculation or skips the calculation and uses the LockPoints - * passed in for evaluation. * The LockPoints should not be considered valid if CheckSequenceLocksAtTip returns false. */ bool CheckSequenceLocksAtTip(CBlockIndex* tip, - const CCoinsView& coins_view, - const CTransaction& tx, - LockPoints* lp = nullptr, - bool useExistingLockPoints = false); + const LockPoints& lock_points); /** * Closure representing one script verification