mempool, fees: stop treating different witness variants as mined #36317

Problem: The mempool estimator added in #34075 checks health by comparing the weight of mempool transactions removed for a block with the block’s non-coinbase transaction weight. It gets the removed entries from removeForBlock and sums their local weight. That function matches mempool entries to block transactions by txid, which does not commit to witness data. A matched entry can therefore have a different weight from the mined transaction, skewing the coverage check. The block policy estimator also treats the local variant as confirmed even though its wtxid was not in the block. Same-txid, different-witness transactions also appear in #35501 and #35893.

Fix: First, calculate total and covered block weight in one pass, computing each block transaction’s weight once and reusing it when a mempool entry matches. This follows the one-pass accounting in GetMempoolHealth and makes the mined-weight basis explicit. Then require an exact wtxid match in removeForBlock before reporting a mempool entry as mined. A mismatched local variant is removed as a conflict, so neither estimator treats it as mined.

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--006a51241073e994b41acfe9ec718e94-->

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/36317.

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline and AI policy for information on the review process.

If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #36074 _{<img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36074.svg">} (scripted-diff: [test] Add util/check.h includes for assertions by maflcko)
• #35713 _{<img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/35713.svg">} (Remove boost as a unit test runner by rustaceanrob)
• #35480 _{<img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/35480.svg">} (doc: document ZMQ notification behavior during reorgs and evictions by fernandguil)

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.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

Concept ACK.

This is an inherent false positive in master that also affects the block_policy, too so #34075 inherits it :(. Should be rare to occur to make a meaningful difference, but it should be fixed.

removeForBlock matches by txid, so when a block mines txid A with a different wtxid than the A we have in our mempool, MempoolTransactionsRemovedForBlock still reports our variant as the one that was mined.

The mempool_policy then miscounts its weight (this PR's concern), but the block_policy also records a false confirmation, see processBlockTx it records our mempool variant's feerate bucket with a confirmation, but it's actually a different transaction that we did not see propagate.

Rather than recomputing weight from the connected block, I think there's a cleaner fix that covers both, we should just filter at removeForBlock: only record a tx removal as BLOCK on an exact wtxid match, and evict a witness mismatch as a CONFLICT instead via TransactionRemovedFromMempool.

we should just filter at removeForBlock: only record a tx removal as BLOCK on an exact wtxid match, and evict a witness mismatch as a CONFLICT instead via TransactionRemovedFromMempool.

I considered changing removeForBlock instead of only the fee estimator, but treating a witness mismatch as CONFLICT also changes removal notifications and may count as a failure in block policy estimation so I would prefer doing that in a followup to avoid surprising side-effects.

This fix is more focused on the more urgent problem and also reuses each mined transaction’s weight in one block walk (instead of two iterations) and tests the coverage calculation, so I’d keep it focused and tackle removal separately.

<details> <summary>Possible `txmempool.cpp` change</summary>

-                txs_removed_for_block.emplace_back(*it);
-                removeUnchecked(it, MemPoolRemovalReason::BLOCK);
+                const bool mined{it->GetTx().GetWitnessHash() == tx->GetWitnessHash()};
+                if (mined) txs_removed_for_block.emplace_back(*it);
+                removeUnchecked(it, mined ? MemPoolRemovalReason::BLOCK : MemPoolRemovalReason::CONFLICT);

</details>

but treating a witness mismatch as CONFLICT also changes removal notifications and may count as a failure in block policy estimation so I would prefer doing that in a followup to avoid surprising side-effects.

That is the correct behaviour. It is a minimal fix that works for both.

Updated the stack to calculate coverage from mined transaction weights in one block walk, then require an exact wtxid match before reporting a mempool transaction as mined. I was hesitant to change removeForBlock at first, but it has only one production call site. The resulting notification changes also required documentation and release notes.

The previous implementation made the witness mismatch easy to miss in review, so I made the coverage calculation explicit before changing removal behavior. The later removeForBlock change also resolves the production mismatch, but the estimator change leaves a simpler one-pass calculation that computes each block transaction’s weight only once.

Mismatched variants now leave as conflicts, preventing false confirmations in block policy estimation and triggering the wallet and ZMQ notifications described in the release notes.

Approach ACK

This should be simplified, we do not need the refactor and the changes in the mempool estimator at all. The interface assumes all the txs reported by the notification have the same tx id and witness, and fixing the false positive in 4cc6c2c1e9da2dd4fae5175ca355765cb6013136 will naturally work for both fee rate estimators.

We should also have a test that indicates the effects of the fix for block_policy as well.

We should also have a test that indicates the effects of the fix for block_policy as well.

I added that check to remove_for_block_witness_mismatch: the block policy estimator no longer records the local variant as confirmed.

This should be simplified, we do not need the refactor and the changes in the mempool estimator at all

While there is a fix with a smaller diff, the separate sums before made it easy for us to miss that a txid match could have a different witness, so I’d like to learn from this and adjust the accounting first. GetMempoolHealth already accumulates both coverage totals in one loop, so I’d like to follow the same pattern here. Computing each block transaction’s weight once and reusing it for coverage when there’s a match keeps the code consistent and easier to review.

I added that check to remove_for_block_witness_mismatch: the block policy estimator no longer records the local variant as confirmed.

Indeed, that's good, but in addition to that, you have mempool_health_uses_mined_witness_weight too, so my suggestion was also block_policy_record_witness_mismatch_as_failure so the effect is visible and explicit.

While there is a fix with a smaller diff, the separate sums before made it easy for us to miss that a txid match could have a different witness, so I’d like to learn from this and adjust the accounting first. GetMempoolHealth already accumulates both coverage totals in one loop, so I’d like to follow the same pattern here. Computing each block transaction’s weight once and reusing it for coverage when there’s a match keeps the code consistent and easier to review.

Not sure I agree, so I think that's a refactor that is beyond the scope of this fix, hence should be separate from this fix and be in it's own PR.

Thanks for the review, I've extended remove_for_block_witness_mismatch to verify that witness mismatches produce no false confirmation and record failures for missed targets. It also checks that an unmissed target records no failure. Let me know if this is what you meant.

I think that's a refactor that is beyond the scope of this fix

I've squashed the two refactor commits and I prefer keeping the accounting change here because it is directly related to the problem. Putting total and covered weight in one block walk makes the mined-weight basis explicit and follows the accounting pattern in GetMempoolHealth. Each mined transaction's weight is computed once and reused for coverage. The tests characterize the old behavior first, and the weight and removal fixes remain separate so each behavior change can be reviewed on its own.

Concept ACK

Up for grabs