Mempool refactor #3154

pull gavinandresen wants to merge 1 commits into bitcoin:master from gavinandresen:mempool_refactor changing 11 files +317 −246

gavinandresen commented at 4:26 AM on October 26, 2013: contributor
I want to get these cleanups into master now to save work on merge conflicts later.

This mostly just moves mempool-related code from main.cpp to txmempool.{cpp,h}

There are a few small refactors:
- nTransactionsUpdated moved from a global var to a mempool private var (with accessor functions)
- Made all the CTxMemPool methods thread safe (they take the mempool.cs lock)
- Folded mempool.exists() into mempool.lookup() to avoid possible multithreading bugs and simplify calling code
- Found and fixed a potential bug in main.cpp where mempool.mapNextTx was accessed without holding the mempool.cs lock
Diapolo commented at 11:11 AM on October 26, 2013: none

Is it good / wise or just unneded to enable the -checkmempool with a normal node? Do we work towards enabling this as default in the future? Just want to get some insight on it :).
jgarzik commented at 12:07 PM on October 26, 2013: contributor

Looks mostly OK. One issue: the locking for the mempool lookup inside ProcessMessage() seems to have changed.
sipa commented at 12:24 PM on October 26, 2013: member
I like the idea of moving this out, but this doesn't really encapsulate it cleanly. A suggestion to obtain that:
- CTxMempool should just be a data structure with associated logic to remain consistent, and not contain part of the validation logic. So, CTxMempool::accept should remain in main (as a function, not a CTxMempool method).
  
  That means no forward declaration of CValidationState in txmempool.h.
  
  Also no need to move the EraseFromWallets callback from main to txmempool (that certainly doesn't belong there).
  
  The mempool object should remain inside main, and not move to txmempool (main just becomes a client that uses one instance of it).
- txmempool.h/.cpp can just include core.h then, which means no circular dependencies at all.
jgarzik commented at 12:24 PM on October 26, 2013: contributor

@sipa +1
gavinandresen commented at 1:07 AM on October 28, 2013: contributor

@sipa : good idea.
petertodd commented at 4:37 AM on October 28, 2013: contributor

@sipa Agreed.

FWIW I wound up implementing a CTxMempool style thing myself when I was looking at doing a child-pays-for-parent mempool, so I think that's the right general direction to go in.
gavinandresen commented at 12:35 AM on October 30, 2013: contributor

Rebased and updated as per @sipa's suggestions. @jgarzik: locking has changed, but should be safer than before because there are many fewer cases of "reach inside and LOCK(mempool.cs)". I'd like to make the mempool critical section private, but I think that should be done in a future refactor (we will probably need a "give me a snapshot copy of the memory pool"; but that should wait until after implementing a memory-limited mempool, I think).
in src/main.cpp:None in 6542e5d41d outdated
```
 300 | @@ -300,6 +301,11 @@ unsigned int CCoinsViewCache::GetCacheSize() {
 301 |      return cacheCoins.size();
 302 |  }
 303 |  
 304 | +/** Helper; lookup from tip (used calling mempool.check() **/
```
sipa commented at 11:32 AM on October 30, 2013:

Passing this method to mempool.check() requires that you're holding cs_main. I'd feel safer documenting that :)

gavinandresen commented at 10:58 PM on October 30, 2013:

I'll add a comment about holding cs_main, and will note that when we switch to C++11 replacing it with lambda expressions will probably make sense.
sipa commented at 11:47 AM on October 30, 2013: member

ACK design and implementation. Haven't tested, and haven't checked that the moves are really move-only.
sipa commented at 2:01 PM on November 2, 2013: member
An alternative and perhaps cleaner solution to passing the LookupFromTip pointer:
- Move CCoinsView, CCoinsViewBacked and CCoinsViewCache to core
- Pass a CCoinsViewCache object (pcoinsTip) to CTxMempool::check (which the caller knows the corresponding lock is held for).
- CCoinsViewMempool can then move to txmempool as well
petertodd commented at 7:09 AM on November 3, 2013: contributor

ACK

I checked that the moves were all really move-only.
Refactor: CTxMempool class to its own txmempool.{cpp,h} 319b11607f
gavinandresen commented at 1:42 AM on November 4, 2013: contributor

Rebased, and renamed AcceptToMempool to AcceptToMemoryPool for consistency. @sipa: Moving CCoinsView/etc to core is non-trivial; CBlockIndex (at least) would have to move also. Lets save that for a future even-more-perfect refactor.
BitcoinPullTester commented at 1:50 AM on November 4, 2013: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/319b11607f8592d7ef67ec82fa73545ad7430974 for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.
gavinandresen referenced this in commit a95a1c06b1 on Nov 4, 2013
gavinandresen merged this on Nov 4, 2013
gavinandresen closed this on Nov 4, 2013
gavinandresen deleted the branch on Nov 4, 2013
DrahtBot locked this on Sep 8, 2021

Contributors