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.
719@@ -718,7 +720,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
720 if (!CheckSequenceLocks(m_active_chainstate.m_chain.Tip(), m_view, tx, STANDARD_LOCKTIME_VERIFY_FLAGS, &lp))
721 return state.Invalid(TxValidationResult::TX_PREMATURE_SPEND, "non-BIP68-final");
722
723- if (!Consensus::CheckTxInputs(tx, state, m_view, m_active_chainstate.m_blockman.GetSpendHeight(m_view), ws.m_base_fees)) {
724+ if (!Consensus::CheckTxInputs(tx, state, m_view, m_active_chainstate.m_chain.Height() + 1, ws.m_base_fees)) {
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.
I’m pretty sure I’m ACK on this, but just to be sure (and as a helpful reminder):
coins_view.GetBestBlock() differs from chaintip? Is that only on startup when we’re recovering from a crash that happened before we could write the coins down to disk?
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.
Removing GetSpendHeight was also discussed in #21055 (review)
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
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.
do we have any reason to believe this (BestBlock != tip) could happen during mempool acceptance, or is there some kind of explicit guard against that?
I’ve added the assert in this commit, if that is what you mean with guard?
master, if the assumption didn’t hold we’d silently run out of consensus (potentially via UB). With this patch the node will crash.
Post-merge ACK https://github.com/bitcoin/bitcoin/pull/23630/commits/fa3942fc4c66d7624bd3578d1e7f4a6a7721c11a
I was a little unclear on how this change might interact with reorgs, but I verified that the coins view’s BestBlock is updated lock-step with the tip in DisconnectBlock(). It seems there is a brief period during ReplayBlocks() during which it isn’t clear to me that the two are lockstep (https://github.com/jamesob/bitcoin/blob/785f9cc46a43661c63a5ec56a9e82f4ce9d42f44/src/validation.cpp#L4277-L4287), but of course ReplayBlocks() is called in init before the chainstate is fully initialized.
Otherwise (based on a grep of SetBestBlock()), the best block should be fully in sync with the tip.
Otherwise (based on a grep of SetBestBlock()), the best block should be fully in sync with the tip.
Could you elaborate on this for my learning? It is not obvious to me why this is the case from the grep result. Thank you.