refactor: Avoid locking tx pool cs thrice #13786

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:Mf1807-txPoolCsThrice changing 8 files +18 −15

MarcoFalke commented at 3:08 PM on July 28, 2018: member

addUnchecked is (outside the tests) only called by ATMP, which already takes the tx pool read lock. So locking it twice more in both addUnchecked methods seems redundant.

Similarly CalculateMemPoolAncestors is (beside once in the wallet) only called in contexts, where the tx pool lock is already taken. So remove the lock there as well.
MarcoFalke added the label Refactoring on Jul 28, 2018
DrahtBot commented at 3:24 PM on July 28, 2018: contributor
Note to reviewers: This pull request conflicts with the following ones:
- #10443 (Add fee_est tool for debugging fee estimation code by ryanofsky)
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.
DrahtBot cross-referenced this on Jul 28, 2018 from issue Return void instead of bool for functions that cannot fail by practicalswift
MarcoFalke force-pushed on Jul 28, 2018
promag commented at 8:35 PM on July 28, 2018: member

Tested ACK fa6b4b5. Verified all OP claims.

CalculateMemPoolAncestors is called in: https://github.com/bitcoin/bitcoin/blob/ef4fac0ea5b4891f4529e4b59dfd1f7aeb3009b5/src/miner.h#L173 https://github.com/bitcoin/bitcoin/blob/ef4fac0ea5b4891f4529e4b59dfd1f7aeb3009b5/src/policy/rbf.h#L26 https://github.com/bitcoin/bitcoin/blob/ef4fac0ea5b4891f4529e4b59dfd1f7aeb3009b5/src/txmempool.h#L680 which already have the annotation.
DrahtBot cross-referenced this on Jul 29, 2018 from issue tx pool: Avoid passing redundant hash into addUnchecked (scripted-diff) by MarcoFalke
refactor: Avoid locking tx pool cs thrice fa5ed4f8d2
MarcoFalke force-pushed on Jul 29, 2018
MarcoFalke commented at 12:05 PM on July 29, 2018: member

Rebased

in src/test/mempool_tests.cpp:58 in fa5ed4f8d2

  54 | @@ -55,6 +55,7 @@ BOOST_AUTO_TEST_CASE(MempoolRemoveTest)
  55 |  
  56 |  
  57 |      CTxMemPool testPool;
  58 | +    LOCK(testPool.cs);

Empact commented at 4:34 PM on July 30, 2018:

nit: could move to 65 since removeRecursive locks separately

in src/test/policyestimator_tests.cpp:21 in fa5ed4f8d2

  17 | @@ -18,6 +18,7 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates)
  18 |  {
  19 |      CBlockPolicyEstimator feeEst;
  20 |      CTxMemPool mpool(&feeEst);
  21 | +    LOCK(mpool.cs);

Empact commented at 4:35 PM on July 30, 2018:

nit: could move to 55

Empact commented at 4:36 PM on July 30, 2018: member

utACK fa5ed4f
kallewoof commented at 6:57 PM on July 30, 2018: member

utACK fa5ed4f8d2c4cb3507bcc2460725d483f2e5789c
MarcoFalke referenced this in commit 84d5a6210c on Jul 30, 2018
MarcoFalke merged this on Jul 30, 2018
MarcoFalke closed this on Jul 30, 2018
MarcoFalke deleted the branch on Jul 30, 2018
ryanofsky cross-referenced this on Aug 3, 2018 from issue Refactor: separate wallet from node by ryanofsky
Bushstar cross-referenced this on Aug 13, 2018 from issue commits from bitcoin/master by Bushstar
ryanofsky cross-referenced this on Jan 7, 2019 from issue Add fee_est tool for debugging fee estimation code by ryanofsky
jasonbcox referenced this in commit 5822a6e6ef on Oct 11, 2019
PastaPastaPasta referenced this in commit 76bd594609 on Feb 2, 2021
PastaPastaPasta referenced this in commit 4a7387fba9 on Feb 4, 2021
bitcoin locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#10443 Add fee_est tool for debugging fee estimation code #10973 Refactor: separate wallet from node