highperfocused/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2026-01-19 23:03:45 +01:00
Code Issues Packages Projects Releases Wiki Activity

Merge bitcoin/bitcoin#24080: policy: Remove unused locktime flags

Browse Source 
fa8d4d9128 scripted-diff: Clarify CheckFinalTxAtTip name (MarcoFalke)
fa4e30b0f3 policy: Remove unused locktime flags (MarcoFalke)

Pull request description:

  The locktime flags have many issues:
  * They are passed in by a default argument, which is fragile. It has already lead to bugs like the one fixed in commit e30b6ea194.
  * They are negative (signed), which doesn't make sense for flags (unsigned in general). According to the review comments when the code was added: "The max on the flags is a fairly weird operation." (https://github.com/bitcoin/bitcoin/pull/6566#issuecomment-150310861)
  * No call site relies on the default argument and they all pass in a single compile-time constant, rendering most of the code dead and untested.
  * The dead code calls `GetAdjustedTime` (network adjusted time), which has its own issues. See https://github.com/bitcoin/bitcoin/issues/4521

  Fix all issues by removing them

ACKs for top commit:
  ajtowns:
    ACK  fa8d4d9128
  theStack:
    Code-review ACK fa8d4d9128
  glozow:
    ACK fa8d4d9128, agree the default arg `flags` is a massive footgun and just setting max flags is weird. Adding `AtTip` to the names makes sense to me, since they're both testing for *next* block and only ever used for {,re}addition to mempool.

Tree-SHA512: 79f4a52f34909eb598d88bbae7afe8abe5f85f45c128483d16aa83dacd0e5579e561b725d01b1e9a931d1821012a51ad2bc6fb2867f8d09ee541f9d234d696f8
This commit is contained in:
MarcoFalke
2022-03-14 16:32:17 +01:00
parent 25d045a9ec fa8d4d9128
commit 28bdaa3f76
3 changed files with 38 additions and 54 deletions
Download Patch File Download Diff File Expand all files Collapse all files

45
src/validation.cpp
View File

@@ -178,20 +178,12 @@ bool CheckInputScripts(const CTransaction& tx, TxValidationState& state,
std::vector<CScriptCheck>* pvChecks = nullptr)
EXCLUSIVE_LOCKS_REQUIRED(cs_main);
bool CheckFinalTx(const CBlockIndex* active_chain_tip, const CTransaction &tx, int flags)
bool CheckFinalTxAtTip(const CBlockIndex* active_chain_tip, const CTransaction& tx)
{
AssertLockHeld(cs_main);
assert(active_chain_tip); // TODO: Make active_chain_tip a reference
// By convention a negative value for flags indicates that the
// current network-enforced consensus rules should be used. In
// a future soft-fork scenario that would mean checking which
// rules would be enforced for the next block and setting the
// appropriate flags. At the present time no soft-forks are
// scheduled, so no flags are set.
flags = std::max(flags, 0);
// CheckFinalTx() uses active_chain_tip.Height()+1 to evaluate
// CheckFinalTxAtTip() uses active_chain_tip.Height()+1 to evaluate
// nLockTime because when IsFinalTx() is called within
// AcceptBlock(), the height of the block *being*
// evaluated is what is used. Thus if we want to know if a
@@ -203,18 +195,15 @@ bool CheckFinalTx(const CBlockIndex* active_chain_tip, const CTransaction &tx, i
// less than the median time of the previous block they're contained in.
// When the next block is created its previous block will be the current
// chain tip, so we use that to calculate the median time passed to
// IsFinalTx() if LOCKTIME_MEDIAN_TIME_PAST is set.
const int64_t nBlockTime = (flags & LOCKTIME_MEDIAN_TIME_PAST)
? active_chain_tip->GetMedianTimePast()
: GetAdjustedTime();
// IsFinalTx().
const int64_t nBlockTime{active_chain_tip->GetMedianTimePast()};
return IsFinalTx(tx, nBlockHeight, nBlockTime);
}
bool CheckSequenceLocks(CBlockIndex* tip,
bool CheckSequenceLocksAtTip(CBlockIndex* tip,
const CCoinsView& coins_view,
const CTransaction& tx,
int flags,
LockPoints* lp,
bool useExistingLockPoints)
{
@@ -222,7 +211,7 @@ bool CheckSequenceLocks(CBlockIndex* tip,
CBlockIndex index;
index.pprev = tip;
// CheckSequenceLocks() uses active_chainstate.m_chain.Height()+1 to evaluate
// CheckSequenceLocksAtTip() uses active_chainstate.m_chain.Height()+1 to evaluate
// height based locks because when SequenceLocks() is called within
// ConnectBlock(), the height of the block *being*
// evaluated is what is used.
@@ -252,7 +241,7 @@ bool CheckSequenceLocks(CBlockIndex* tip,
prevheights[txinIndex] = coin.nHeight;
}
}
lockPair = CalculateSequenceLocks(tx, flags, prevheights, index);
lockPair = CalculateSequenceLocks(tx, STANDARD_LOCKTIME_VERIFY_FLAGS, prevheights, index);
if (lp) {
lp->height = lockPair.first;
lp->time = lockPair.second;
@@ -268,7 +257,7 @@ bool CheckSequenceLocks(CBlockIndex* tip,
// 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
// CheckSequenceLocks to indicate the LockPoints validity
// 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
@@ -358,26 +347,26 @@ void CChainState::MaybeUpdateMempoolForReorg(
// Also updates valid entries' cached LockPoints if needed.
// If false, the tx is still valid and its lockpoints are updated.
// If true, the tx would be invalid in the next block; remove this entry and all of its descendants.
const auto filter_final_and_mature = [this, flags=STANDARD_LOCKTIME_VERIFY_FLAGS](CTxMemPool::txiter it)
const auto filter_final_and_mature = [this](CTxMemPool::txiter it)
EXCLUSIVE_LOCKS_REQUIRED(m_mempool->cs, ::cs_main) {
AssertLockHeld(m_mempool->cs);
AssertLockHeld(::cs_main);
const CTransaction& tx = it->GetTx();
// The transaction must be final.
if (!CheckFinalTx(m_chain.Tip(), tx, flags)) return true;
if (!CheckFinalTxAtTip(m_chain.Tip(), tx)) return true;
LockPoints lp = it->GetLockPoints();
const bool validLP{TestLockPointValidity(m_chain, lp)};
CCoinsViewMemPool view_mempool(&CoinsTip(), *m_mempool);
// CheckSequenceLocks checks if the transaction will be final in the next block to be
// 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 (!CheckSequenceLocks(m_chain.Tip(), view_mempool, tx, flags, &lp, validLP)) {
// If CheckSequenceLocks fails, remove the tx and don't depend on the LockPoints.
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 CheckSequenceLocks succeeded, it also updated the LockPoints.
// 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); });
}
@@ -722,8 +711,9 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
// Only accept nLockTime-using transactions that can be mined in the next
// block; we don't want our mempool filled up with transactions that can't
// be mined yet.
if (!CheckFinalTx(m_active_chainstate.m_chain.Tip(), tx, STANDARD_LOCKTIME_VERIFY_FLAGS))
if (!CheckFinalTxAtTip(m_active_chainstate.m_chain.Tip(), tx)) {
return state.Invalid(TxValidationResult::TX_PREMATURE_SPEND, "non-final");
}
if (m_pool.exists(GenTxid::Wtxid(tx.GetWitnessHash()))) {
// Exact transaction already exists in the mempool.
@@ -803,8 +793,9 @@ 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 (!CheckSequenceLocks(m_active_chainstate.m_chain.Tip(), m_view, tx, STANDARD_LOCKTIME_VERIFY_FLAGS, &lp))
if (!CheckSequenceLocksAtTip(m_active_chainstate.m_chain.Tip(), m_view, tx, &lp)) {
return state.Invalid(TxValidationResult::TX_PREMATURE_SPEND, "non-BIP68-final");
}
// The mempool holds txs for the next block, so pass height+1 to CheckTxInputs
if (!Consensus::CheckTxInputs(tx, state, m_view, m_active_chainstate.m_chain.Height() + 1, ws.m_base_fees)) {