doc: Remove fee delta TODO from txmempool.cpp #23416

1. 
   maflcko commented at 2:52 pm on November 2, 2021: member

   This refactor request was added in commit eb306664e786ae43d539fde66f0fbe2a3e89d910, 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.
2. doc: Remove fee delta TODO from txmempool.cpp fa32cc0682
3. DrahtBot added the label Mempool on Nov 2, 2021
4. 
   maflcko commented at 6:07 pm on November 2, 2021: member

   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.

5. in src/txmempool.cpp:426 in fa32cc0682

   421@@ -422,8 +422,6 @@ void CTxMemPool::addUnchecked(const CTxMemPoolEntry &entry, setEntries &setAnces
   422     indexed_transaction_set::iterator newit = mapTx.insert(entry).first;
   423 
   424     // Update transaction for any feeDelta created by PrioritiseTransaction
   425-    // TODO: refactor so that the fee delta is calculated before inserting
   426-    // into mapTx.
   427     CAmount delta{0};
   428     ApplyDelta(entry.GetTx().GetHash(), delta);
   

   ---

   ---

   

   glozow commented at 3:17 pm on November 4, 2021:

   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?

   ---

   

   maflcko commented at 7:26 pm on November 4, 2021:

   Yes, it is possible to fix it in that way, see commit fa300f581fe6cd4b37c7cefb191722ff970e0116. Are you suggesting I open a pull with that?

   ---

   

   glozow commented at 3:58 pm on November 14, 2021:

   Yeah I think it’s better, regardless of whether that’s what the todo is referring to

   ---

   

   fanquake commented at 12:07 pm on March 24, 2022:

   Should we just convert this PR into the suggested fix?
6. 
   laanwj commented at 7:41 am on April 14, 2022: member

   Code review ACK fa32cc0682a0aa3420e6a11031721fcb6c50fa44

   Generally, non-trivial TODOs make more sense as GitHub issues, so that they can be discussed and triaged more easily.

   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.

7. laanwj merged this on Apr 14, 2022
8. laanwj closed this on Apr 14, 2022

   ---

9. sidhujag referenced this in commit 6b9411d880 on Apr 14, 2022
10. Fabcien referenced this in commit 89aa566f3a on Nov 23, 2022
11. maflcko deleted the branch on Feb 28, 2023
12. knst referenced this in commit 0bf2430184 on Oct 19, 2023
13. PastaPastaPasta referenced this in commit bbcb2d3998 on Oct 23, 2023
14. PastaPastaPasta referenced this in commit dab44cd0b0 on Oct 23, 2023
15. bitcoin locked this on Feb 28, 2024

Contributors
maflcko glozow fanquake laanwj

Labels
Mempool