mempool clean up: replace update_* structs with lambdas #26048

1. 
   glozow commented at 5:48 pm on September 8, 2022: member

   These were introduced in commit https://github.com/bitcoin/bitcoin/commit/5add7a74a672cb12b0a2a630d318d9bc64dd0f77, when the codebase was pre-C++11. We can use lambdas now.
2. glozow added the label Refactoring on Sep 8, 2022
3. glozow added the label Mempool on Sep 8, 2022
4. in src/txmempool.cpp:128 in a02af7ca50 outdated

   124@@ -155,7 +125,9 @@ void CTxMemPool::UpdateForDescendants(txiter updateIt, cacheMap& cachedDescendan
   125             }
   126         }
   127     }
   128-    mapTx.modify(updateIt, update_descendant_state(modifySize, modifyFee, modifyCount));
   129+    mapTx.modify(updateIt, [modifySize, modifyFee, modifyCount](CTxMemPoolEntry& e) {
   

   ---

   ---

   

   MarcoFalke commented at 6:03 pm on September 8, 2022:

   0    mapTx.modify(updateIt, [&/=](CTxMemPoolEntry& e) {
   

   Is there any downside to just taking a reference/copy of everything that is used inside the lambda? This would make the code not too much more verbose than it previously was, and it would still be clear that every symbol used in the lambda is captured by reference/copy.

   ---

   

   glozow commented at 10:10 am on September 9, 2022:

   Good point, no downside. Changed them all to default-capture by value since they are pointers and int arguments.
5. 
   amovfx commented at 1:06 am on September 9, 2022: none

   Just trying to help out here. I pulled it down and it built fine of course. But I don’t really have anything to really run to hit the code path. I assume that the tests that are already available do so.
6. 
   DrahtBot commented at 3:15 am on September 9, 2022: contributor

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

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #25673 (refactor: make member functions const when applicable by aureleoules)
   • #23962 (Use int32_t type for transaction size/weight consistently by hebasto)

   If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

7. [mempool] replace update_descendant_state with lambda

   These were introduced in commit 5add7a7, when the codebase was
   pre-C++11. They are no longer necessary.

   1b348d2725
8. glozow force-pushed on Sep 9, 2022
9. 
   glozow commented at 10:19 am on September 9, 2022: member

   @amovfx probably the best way to review this PR is to review the code manually and check that no behavior has changed. If you want to run a quick smoke test (would show if something is really wrong in mempool entry accounting), I often use make check && test/functional/test_runner.py test/functional/mempool* test/functional/rpc_packages.py test/functional/feature_rbf.py test/functional/mining_prioritisetransaction.py.
10. 
    amovfx commented at 6:15 pm on September 9, 2022: none

    @amovfx probably the best way to review this PR is to review the code manually and check that no behavior has changed. If you want to run a quick smoke check (would show if something is really wrong in mempool entry accounting), I often use make check && test/functional/test_runner.py test/functional/mempool* test/functional/rpc_packages.py test/functional/feature_rbf.py test/functional/mining_prioritisetransaction.py.

    Thanks! Ill do this. I did this and everything is working, I tried running the test thats getting caught on the check for 32-bit + dash [gui] [CentOS 8], “wallet_reorgsrestore.py” and my test passed fine on macOS.

    I imagine I should be trying to test that in a docker container?

11. w0xlt approved
12. 
    w0xlt commented at 7:34 am on September 12, 2022: contributor

    ACK https://github.com/bitcoin/bitcoin/pull/26048/commits/1b348d2725f6271d7f78b4668ab35014cdb176be
13. 
    MarcoFalke commented at 8:08 am on September 12, 2022: member

    review ACK 1b348d2725f6271d7f78b4668ab35014cdb176be 👮

    Signature:

     0-----BEGIN PGP SIGNED MESSAGE-----
     1Hash: SHA512
     2
     3review ACK 1b348d2725f6271d7f78b4668ab35014cdb176be 👮
     4-----BEGIN PGP SIGNATURE-----
     5
     6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
     7pUiypwwAgyoDOkuAp46McbdxAuSLZzb+fsg8fERzK2j86FthtnBmh4UVwpcPYMcf
     86/egvnEKi2vN+3lafv0S6wKouxp9X+vkEOfz/USygH29rgyq425DWlwHRwHO1H5l
     9Ea2bE/5bVWoVf4Zj6tF+8WGhecIMKJhHZ+KmHcBihcVFT79yHDI05sBdofFnBZmz
    10ig7DH4+xj7zfUzRRocpadEPtfkNsastbquyXEFpagI4kCUCVDJ2PL5DK4+5Zy8PL
    11nZARtu+DMBOiWZkCm6eUCtGsa9SzwCACY/bf93dFU/i80kMDNCw/UXp0Arva3LVm
    12kp6yGodkDQQGUHzMPIox7F8zhgppOvvNTZs7p0VxFsGN7o3M5zV/MMaaTb+afc8i
    13txOeBT3O5sWzKQ5SDqOR1+kBFDXkOZMcWjpDFsrq8HbnRKXusN1dFKt96UOaAdGC
    14BOcFRVf7oJBtdcibDNXIsr7Vv6OMnxAdvNlXvoc/4DBhE0q8errN7DyvLh1JvFnm
    15wJEf9iGR
    16=9yhH
    17-----END PGP SIGNATURE-----
    

14. MarcoFalke merged this on Sep 12, 2022
15. MarcoFalke closed this on Sep 12, 2022

    ---

16. in src/txmempool.cpp:118 in 1b348d2725

    113@@ -146,7 +114,9 @@ void CTxMemPool::UpdateForDescendants(txiter updateIt, cacheMap& cachedDescendan
    114             modifyCount++;
    115             cachedDescendants[updateIt].insert(mapTx.iterator_to(descendant));
    116             // Update ancestor state for each descendant
    117-            mapTx.modify(mapTx.iterator_to(descendant), update_ancestor_state(updateIt->GetTxSize(), updateIt->GetModifiedFee(), 1, updateIt->GetSigOpCost()));
    118+            mapTx.modify(mapTx.iterator_to(descendant), [=](CTxMemPoolEntry& e) {
    119+              e.UpdateAncestorState(updateIt->GetTxSize(), updateIt->GetModifiedFee(), 1, updateIt->GetSigOpCost());
    

    ---

    ---

    

    jonatack commented at 10:24 am on September 12, 2022:

    0                e.UpdateAncestorState(updateIt->GetTxSize(), updateIt->GetModifiedFee(), /*modifyCount=*/1, updateIt->GetSigOpCost());
    

17. in src/txmempool.cpp:366 in 1b348d2725

    362@@ -393,7 +363,7 @@ void CTxMemPool::UpdateForRemoveFromMempool(const setEntries &entriesToRemove, b
    363             CAmount modifyFee = -removeIt->GetModifiedFee();
    364             int modifySigOps = -removeIt->GetSigOpCost();
    365             for (txiter dit : setDescendants) {
    366-                mapTx.modify(dit, update_ancestor_state(modifySize, modifyFee, -1, modifySigOps));
    367+                mapTx.modify(dit, [=](CTxMemPoolEntry& e){ e.UpdateAncestorState(modifySize, modifyFee, -1, modifySigOps); });
    

    ---

    ---

    

    jonatack commented at 10:25 am on September 12, 2022:

    0                mapTx.modify(dit, [=](CTxMemPoolEntry& e) { e.UpdateAncestorState(modifySize, modifyFee, /*modifyCount=*/-1, modifySigOps); });
    

18. glozow deleted the branch on Sep 12, 2022
19. sidhujag referenced this in commit 604c76b1ff on Sep 12, 2022
20. bitcoin locked this on Sep 12, 2023

Contributors
glozow MarcoFalke amovfx DrahtBot w0xlt jonatack

Labels
Refactoring Mempool