doc: Remove fee delta TODO from txmempool.cpp #23416
Conversation
|
I am suspecting that this refactor request might have been a feature request on how prioritization should be handled for packages? Though, in that case it also makes more sense to discuss in an issue. ping @sdaftuar in case I am missing something obvious. |
| CAmount delta{0}; | ||
| ApplyDelta(entry.GetTx().GetHash(), delta); |
There was a problem hiding this comment.
Don't have much background on what was intended, but I interpret this TODO to be "move these 2 lines up before the insert call, and delete the mapTx.modify() call below?
There was a problem hiding this comment.
Yes, it is possible to fix it in that way, see commit fa300f5. Are you suggesting I open a pull with that?
There was a problem hiding this comment.
Yeah I think it's better, regardless of whether that's what the todo is referring to
There was a problem hiding this comment.
Should we just convert this PR into the suggested fix?
|
Code review ACK fa32cc0
Just going to go ahead and merge this. I fully agree with the OP. Any next steps can be done whether there's a TODO comment in the source code or not. |
fa32cc0 doc: Remove fee delta TODO from txmempool.cpp (MarcoFalke) Pull request description: This refactor request was added in commit eb30666, though it didn't explain why the refactor is needed and what the goal is. Given that this wasn't touched for more than 5 years, it doesn't seem critical. Generally, non-trivial `TODO`s make more sense as GitHub issues, so that they can be discussed and triaged more easily. ACKs for top commit: laanwj: Code review ACK fa32cc0 Tree-SHA512: 6629fef543e815136c82c38aa8ba2c4de68a5fe94c6954f2559e468f7e59052e02dd7c221d3b159be0314eaf0dbb18f74814297c58f76e2289c47e8d4f49be4e
Summary: > This refactor request was added in commit eb30666, though it didn't explain why the refactor is needed and what the goal is. Given that this wasn't touched for more than 5 years, it doesn't seem critical. Generally, non-trivial TODOs make more sense as GitHub issues, so that they can be discussed and triaged more easily. This is a backport of [[bitcoin/bitcoin#23416 | core#23416]] Test Plan: NA Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D12594
fa32cc0 doc: Remove fee delta TODO from txmempool.cpp (MarcoFalke) Pull request description: This refactor request was added in commit eb30666, though it didn't explain why the refactor is needed and what the goal is. Given that this wasn't touched for more than 5 years, it doesn't seem critical. Generally, non-trivial `TODO`s make more sense as GitHub issues, so that they can be discussed and triaged more easily. ACKs for top commit: laanwj: Code review ACK fa32cc0 Tree-SHA512: 6629fef543e815136c82c38aa8ba2c4de68a5fe94c6954f2559e468f7e59052e02dd7c221d3b159be0314eaf0dbb18f74814297c58f76e2289c47e8d4f49be4e
fa32cc0 doc: Remove fee delta TODO from txmempool.cpp (MarcoFalke) Pull request description: This refactor request was added in commit eb30666, though it didn't explain why the refactor is needed and what the goal is. Given that this wasn't touched for more than 5 years, it doesn't seem critical. Generally, non-trivial `TODO`s make more sense as GitHub issues, so that they can be discussed and triaged more easily. ACKs for top commit: laanwj: Code review ACK fa32cc0 Tree-SHA512: 6629fef543e815136c82c38aa8ba2c4de68a5fe94c6954f2559e468f7e59052e02dd7c221d3b159be0314eaf0dbb18f74814297c58f76e2289c47e8d4f49be4e
backport: bitcoin#23416,bitcoin#18997,bitcoin#18996, partial bitcoin#27483 (TODO cleanup)
This refactor request was added in commit eb30666, though it didn't explain why the refactor is needed and what the goal is. Given that this wasn't touched for more than 5 years, it doesn't seem critical. Generally, non-trivial
TODOs make more sense as GitHub issues, so that they can be discussed and triaged more easily.