refactor: remove duplicate code from BlockAssembler #24364
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Concept ACK |
|
code review ACK 0f40d65 |
|
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. |
|
Ah that's true, but it's very weird that they're not the same. Why does |
|
Edit: This is nonsense |
|
What do you mean by recursively? Isn't |
Oh snap, I obviously totally overlooked this in my review :/ |
|
This can be reopened, now that ##24538 is merged. |
maflcko
pushed a commit
to maflcko/bitcoin-core
that referenced
this pull request
May 6, 2022
…ing modified fees, not base e4303c3 [unit test] prioritisation in mining (glozow) 7a8d606 [miner] bug fix: update for parent inclusion using modified fee (glozow) 0f9a444 MOVEONLY: group miner tests into MinerTestingSetup functions (glozow) Pull request description: Came up while reviewing bitcoin#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. ACKs for top commit: ccdle12: tested ACK e4303c3 MarcoFalke: ACK e4303c3 🚗 Tree-SHA512: 4cd94106fbc9353e9f9b6d5af268ecda5aec7539245298c940ca220606dd0737264505bfaae1f83d94765cc2d9e1a6e913a765048fe6c19292482241761a6762
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
May 9, 2022
…ing modified fees, not base e4303c3 [unit test] prioritisation in mining (glozow) 7a8d606 [miner] bug fix: update for parent inclusion using modified fee (glozow) 0f9a444 MOVEONLY: group miner tests into MinerTestingSetup functions (glozow) Pull request description: Came up while reviewing bitcoin#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. ACKs for top commit: ccdle12: tested ACK e4303c3 MarcoFalke: ACK e4303c3 🚗 Tree-SHA512: 4cd94106fbc9353e9f9b6d5af268ecda5aec7539245298c940ca220606dd0737264505bfaae1f83d94765cc2d9e1a6e913a765048fe6c19292482241761a6762
|
Concept ACK to reopen. Agree not necessary for v24.0. |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
glozow
reviewed
Oct 6, 2022
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Oct 6, 2022
0f40d65 refactor: remove duplicate code from BlockAssembler (James O'Beirne) Pull request description: Found while reminding myself how transactions are chosen for blocks. Take it or leave it! ACKs for top commit: glozow: ACK 0f40d65 theStack: Concept and code-review ACK 0f40d65 Tree-SHA512: 8a2694e670ce3fe897ab8f64f64c8df5f8487fc1264527a3abbcba0e5b921fb693416497ccd62508295bc33f202c65556b91b6af463acb91aab43138d2492c14
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Jan 6, 2023
Summary: There is no change in behavior. Backport of [[bitcoin/bitcoin#24364 | core#24364]]. Depends on D12953. Test Plan: ninja check-all Reviewers: #bitcoin_abc, sdulfari Reviewed By: #bitcoin_abc, sdulfari Differential Revision: https://reviews.bitcoinabc.org/D12954
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Found while reminding myself how transactions are chosen for blocks. Take it or leave it!