Merge bitcoin/bitcoin#23630: Remove GetSpendHeight

fa3942fc4c66d7624bd3578d1e7f4a6a7721c11a Remove GetSpendHeight (MarcoFalke)

Pull request description:

  It is unclear what the goal of the helper is, as the caller already
  knows the spend height before calling the helper.

  Also, in case the coins view is corrupted, LookupBlockIndex will return
  nullptr. Dereferencing a nullptr is UB.

  Fix both issues by removing it. Also, add a sanity check, which aborts
  if the coins view is corrupted.

ACKs for top commit:
  laanwj:
    Code review ACK fa3942fc4c66d7624bd3578d1e7f4a6a7721c11a
  ryanofsky:
    Code review ACK fa3942fc4c66d7624bd3578d1e7f4a6a7721c11a. I'm not aware of cases where coins GetBestBlock could be different from active chain tip, and asset seems sufficient to guarantee PR doesn't change behavior if that doesn't happen.

Tree-SHA512: 29f65d72e116ec5a4509e0947ceeaa5bb6b7dfd5d174d3c7945cb15fa266d590c4f8b48e6385de74ef7d7c84ebd2255de902ad9c87c24955348a91b12e5bffd5

1 files changed, 4 insertions, 9 deletions

diff --git a/src/validation.cpp b/src/validation.cpp
index b532888f37..203bd57676 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -723,6 +723,8 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
     // to coins_to_uncache)
     m_view.SetBackend(m_dummy);
 
+    assert(m_active_chainstate.m_blockman.LookupBlockIndex(m_view.GetBestBlock()) == m_active_chainstate.m_chain.Tip());
+
     // Only accept BIP68 sequence locked 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.
@@ -731,7 +733,8 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
     if (!CheckSequenceLocks(m_active_chainstate.m_chain.Tip(), m_view, tx, STANDARD_LOCKTIME_VERIFY_FLAGS, &lp))
         return state.Invalid(TxValidationResult::TX_PREMATURE_SPEND, "non-BIP68-final");
 
-    if (!Consensus::CheckTxInputs(tx, state, m_view, m_active_chainstate.m_blockman.GetSpendHeight(m_view), ws.m_base_fees)) {
+    // 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)) {
         return false; // state filled in by CheckTxInputs
     }
 
@@ -1311,14 +1314,6 @@ bool CScriptCheck::operator()() {
     return VerifyScript(scriptSig, m_tx_out.scriptPubKey, witness, nFlags, CachingTransactionSignatureChecker(ptxTo, nIn, m_tx_out.nValue, cacheStore, *txdata), &error);
 }
 
-int BlockManager::GetSpendHeight(const CCoinsViewCache& inputs)
-{
-    AssertLockHeld(cs_main);
-    CBlockIndex* pindexPrev = LookupBlockIndex(inputs.GetBestBlock());
-    return pindexPrev->nHeight + 1;
-}
-
-
 static CuckooCache::cache<uint256, SignatureCacheHasher> g_scriptExecutionCache;
 static CSHA256 g_scriptExecutionCacheHasher;