Page MenuHomePhabricator

miner: bug fix? update for ancestor inclusion using modified fees, not base
ClosedPublic

Authored by Fabien on Jan 5 2023, 19:42.

Details

Summary
Came up while reviewing #24364, where some of us incorrectly assumed that we use the same fee deduction in CTxMemPoolModifiedEntry::nModFeesWithAncestors when first constructing an entry and in update_for_parent_inclusion.

Actually, the behavior is this: when a mempool entry's ancestor is included in the block template, we create a CTxMemPoolModifiedEntry for it, subtracting the ancestor's modified fees from nModFeesWithAncestors. If another ancestor is included, we update it again, but use the ancestor's base fees instead.

I can't explain why we use GetFee in one place and GetModifiedFee in the other, but I'm quite certain we should be using the same one for both.

And should it be base or modified fees? Modified, otherwise the child inherits the prioritisation of the parent, but only until the parent gets mined. If we want prioritisation to cascade down to current in-mempool descendants, we should probably document that in the prioritsetransaction helpstring and implement it in CTxMemPool::mapDeltas, not as a quirk in the mining code?

Wrote a test in which a mempool entry has 2 ancestors, both prioritised, and both included in a block template individually. This test should fail without the s/GetFee/GetModifiedFee commit.

Backport of core#24538.

Depends on D12951.

Test Plan
ninja check-all

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Fabien requested review of this revision.Jan 5 2023, 19:42
sdulfari requested changes to this revision.Jan 5 2023, 22:03
sdulfari added a subscriber: sdulfari.
sdulfari added inline comments.
src/test/miner_tests.cpp
286 ↗(On Diff #37466)

Config does not appear used here. Use chainparams instead so we better match the backport.

694 ↗(On Diff #37466)
This revision now requires changes to proceed.Jan 5 2023, 22:03
src/test/miner_tests.cpp
286 ↗(On Diff #37466)

Ah good catch, the ProcessNewBlock call that needs it remained at the callsite

This revision is now accepted and ready to land.Jan 6 2023, 17:45