refactor: remove duplicate code from BlockAssembler #24364

pull jamesob wants to merge 1 commits into bitcoin:master from jamesob:2022-02-ba-dup-code changing 1 files +2 −6

jamesob commented at 2:19 AM on February 17, 2022: member

Found while reminding myself how transactions are chosen for blocks. Take it or leave it!
refactor: remove duplicate code from BlockAssembler 0f40d65321
DrahtBot added the label Refactoring on Feb 17, 2022
laanwj commented at 4:22 PM on February 23, 2022: member

Concept ACK
theStack approved
theStack commented at 10:19 AM on March 1, 2022: contributor

Concept and code-review ACK 0f40d653218789aa176ca2f844e3222d2ad890a3
glozow commented at 1:21 PM on March 1, 2022: member

code review ACK 0f40d653218789aa176ca2f844e3222d2ad890a3
fanquake added this to the milestone 24.0 on Mar 2, 2022
maflcko commented at 4:18 PM on March 8, 2022: member

Can anyone explain why this is correct? Why should GetModifiedFee be equal to GetFee??
jamesob commented at 4:24 PM on March 8, 2022: member

Can anyone explain why this is correct? Why should GetModifiedFee be equal to GetFee??

Good point. This is not correct. :)

Probably a comment and/or test should be added.
jamesob closed this on Mar 8, 2022
glozow commented at 4:26 PM on March 8, 2022: member

Ah that's true, but it's very weird that they're not the same. Why does update_for_parent_inclusion use GetFee() and not GetModifiedFee() to update nModFeesWithAncestors?
maflcko commented at 4:34 PM on March 8, 2022: member

<strike>modified fee is already applied recursively, so if you apply it repeatedly it would accumulate?</strike>

Edit: This is nonsense
glozow commented at 4:40 PM on March 8, 2022: member

What do you mean by recursively? Isn't nModFeesWithAncestors the sum of ours + all ancestors' modified fees? So if you're updating for parent inclusion, using parent.GetFee() instead of parent.GetModifiedFee() means you're still including the prioritisation from your parent, which would be incorrect.
theStack commented at 5:29 PM on March 8, 2022: contributor

Can anyone explain why this is correct? Why should GetModifiedFee be equal to GetFee??

Oh snap, I obviously totally overlooked this in my review :/
glozow cross-referenced this on Mar 11, 2022 from issue miner: bug fix? update for ancestor inclusion using modified fees, not base by glozow
jamesob cross-referenced this on Mar 25, 2022 from issue test: Add diamond-shape prioritisetransaction test by maflcko
maflcko commented at 9:07 AM on May 6, 2022: member

This can be reopened, now that #https://github.com/bitcoin/bitcoin/pull/24538 is merged.
maflcko referenced this in commit b2e7811c62 on May 6, 2022
sidhujag referenced this in commit c8d2687323 on May 9, 2022
fanquake commented at 2:45 PM on September 13, 2022: member

@jamesob @glozow want to reopen / pick this up?
fanquake removed this from the milestone 24.0 on Sep 13, 2022
glozow commented at 3:10 PM on September 13, 2022: member

Concept ACK to reopen. Agree not necessary for v24.0.
jamesob reopened this on Sep 13, 2022
DrahtBot commented at 1:01 PM on September 23, 2022: contributor



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



Conflicts

No conflicts as of last run.
glozow commented at 9:57 AM on October 6, 2022: member

ACK 0f40d653218789aa176ca2f844e3222d2ad890a3
glozow merged this on Oct 6, 2022
glozow closed this on Oct 6, 2022
sidhujag referenced this in commit 22d943f5ee on Oct 6, 2022
Fabcien referenced this in commit ce0b1b9a24 on Jan 6, 2023
bitcoin locked this on Oct 6, 2023