Split validationinterface into parallel validation/mempool interfaces by TheBlueMatt · Pull Request #12979 · bitcoin/bitcoin

TheBlueMatt · 2018-04-13T18:58:38Z

These are the non-feeestimator-specific refactoring commits from #11775, mostly focusing on splitting the validationinterface into two parlell interfaces.

There were no changes except reordering commits and rebasing on latest master.

maflcko

weak utACK db156ea (Just did a first pass and left some nits on comments)

maflcko · 2018-04-16T17:50:32Z

src/validationinterface.h

     *
+     * The ordering of BlockDisconnected and TransactionRemovedFromMempool
+     * (for transactions removed due to memory constraints or lock time/
+     * coinbase maturity chenges during the disconnection/reorg) is undefined,


in commit 16e4c4b: Should say "changes"?

maflcko · 2018-04-16T18:39:30Z

src/txmempool.cpp

            setEntries stage;
            stage.insert(it);
            RemoveStaged(stage, true, MemPoolRemovalReason::BLOCK);
+            txn_removed_in_block.push_back(tx); // Use the block's copy


in commit ff936ae: I don't understand what this comment means. Mind to elaborate or remove?

Edit: I presume instead of "copy" you mean "witness", since the witness might be different and it is no longer a copy?

I just added a "as per spec" note that its clarified more in MempoolUpdatedForBlockConnect docs ("The txn_removed_in_block txn are as they appear in the block, and may have different witnesses from the version which was previously in the mempool").

maflcko · 2018-04-16T19:34:41Z

src/txmempool.cpp

-        // BLOCK and CONFLICT callbacks are generated in removeForBlock
+    if (reason != MemPoolRemovalReason::BLOCK && reason != MemPoolRemovalReason::CONFLICT &&
+        reason != MemPoolRemovalReason::REPLACED) {
+        // BLOCK and CONFLICT callbacks are generated in removeForBlock REPLACED


nit in commit db156ea: Could add a semicolon, dot or new line to indicate the start of a new sentence. (before REPLACED)

maflcko · 2018-04-16T19:52:17Z

src/zmq/zmqnotificationinterface.cpp


+void CZMQNotificationInterface::TransactionAddedToMempool(const CTransactionRef& ptx, const std::vector<CTransactionRef>& txn_replaced) {
+    NotifyTransaction(ptx);
+}


nit in commit 6b4e2a7: Could add a comment to explain why txn_replaced is dropped?

maflcko · 2018-04-16T19:58:03Z

src/txmempool.cpp

        ClearPrioritisation(tx->GetHash());
    }
-    GetMainSignals().MempoolUpdatedForBlockConnect(std::move(txn_removed_in_block), std::move(txn_conflicts));
+    GetMainSignals().MempoolUpdatedForBlockConnect(std::move(txn_removed_in_block), std::move(txn_conflicts), nBlockHeight);


in commit db156ea: You pass the block height, but it doesn't appear to be used anywhere?

Its used in #11775.

maflcko · 2018-04-17T16:02:09Z

weak re-utACK e95db395592c763e8c77f619db7a8f5dbaa0a604 (Only changes were a rebase to solve conflicts and three minor comment clarifications)

Empact · 2018-04-18T23:05:54Z

src/wallet/wallet.cpp

Could you break this line?

Please no trivial changes to pull request with a ton of commits. It is always a pain to re-ACK, since a diff has to be done for every commit.

Will do when another round of changes comes in after more review, until then agree with Marco, not worth a rebase for it.

sipa · 2018-04-26T19:11:26Z

I'm confused, this removes the connect trace, but it's only refactoring?

ajtowns · 2018-04-26T22:58:20Z

Still a big set of commits.

It seems like it would have been simpler to do:

+void CTxMemPool::removeRecursive(const CTransaction &origTx, MemPoolRemovalReason reason, std::vector<CTransactionRef>* txn_removed) {
     // Remove transaction from memory pool
     LOCK(cs);
     setEntries setAllRemoves;
     calculateRemoveRecursive(origTx, setAllRemoves);
+    if (txn_removed != nullptr) {
+        for (const txiter& it : setAllRemoves) {
+            txn_removed->push_back(it->GetSharedTx());
+        }
+    }
     RemoveStaged(setAllRemoves, false, reason);
 }

and pass in &txn_removed rather than duplicate the code. That would also avoid the need to split out calculateRemoveRecursive() from what I can see, though maybe another PR needs it? I'm not sure that UpdateMempoolForReorg() shouldn't also be tracking the conflicting txs it removes and issuing a notification anyway?

Otherwise:

Pass new block height through MempoolUpdatedForBlock
Remove useless scope in AcceptToMemoryPoolWorker
Remove boost::signals from txmempool, call GetMainSignals() directly
Track the set of transactions removed/conflicted in removeForBlock
Split removeRecursive into calculate/remove steps
Pass MemPoolRemovalReason out through TransactionRemovedFromMempool

all look fine and straightforward (and easily reviewable/mergeable if they were all there was).

I haven't verified that the comments in

Clarify validationinterface notification ordering

are actually correct. For what it's worth, a commit that documented what the ordering ideally would be (after all these commits, eg) and noted any deviations from that as TODOs (which then get removed in subsequent commits as they're fixed) might be/have been better?

That leaves these commits I'm not confident about:

Add txn_replaced to Added callback, remove lReplaced ATMP arg
BlockConnected(vtxConflicted) -> New MempoolInterface callback
Add a parallel validation interface for mempool events.

EDIT: In particular, there's a couple of changes in the "Add txn_replaced" commit that deal with (un)registering MempoolInterface that seem like they should be in an earlier commit, possibly the "parallel validation interface for mempool events" one?

I'm not familiar with what ConnectTrace does or the impact of changing that area of code to be able to be confident in the BlockConnected... commit. Having one commit that just adds MempoolUpdatedForBlockConnect followed by one that removes ConnectTrace might be simpler to review? The MempoolUpdateForBlockConnect parts look good at least. Seems like the switch from BasicTestingSetup to TestingSetup for policyestimator_tests.cpp should be in one of the commits in "Move fee estimator thread" PR?

TheBlueMatt · 2018-04-27T16:47:36Z

@sipa yes. Because the validationinterface/mempoolinterface callbacks happen on a background thread, having an object which queues up callbacks for calling later is just redundant (as no other thread will be able to see an inconsistent state while we hold cs_main). The callbacks themselves should not change in that commit, it just removes a redundant queue.

@ajtowns thanks for taking a look. I'm not actually sure what you mean in your suggestion with the patch there, can you clarify a bit further?

ryanofsky

Started review (will update this comment with progress).

ryanofsky · 2018-04-30T16:59:49Z

src/validationinterface.h

In commit "Clarify validationinterface notification ordering" (e7d7edc898c31ef37d6d64e61c72979c32253a43)

I couldn't understand what "without forward-progress" means, and would appreciate any clarification. Does it just mean this can be called repeatedly with the same tip? (I asked about this previously in #11856 (comment))

@ryanofsky "without forward-progress" means being at a worse-tip than we were previously at before (less work, or same work but a block that sorts worse than our prior tip, eg we received it second) -- exposing such an intermediate state to listeners is a bug. Note that this would be fixed by #13023.

#13023 was merged so this TODO can be removed?

jnewbery

Looks good. I have a few requests to update/add comments.

I think the final commit (Pass new block height through MempoolUpdatedForBlock) should be left out of this PR and put in #11775)

jnewbery · 2018-05-01T15:59:21Z

src/validationinterface.h

Should this be BlockDisconnected/BlockConnected?

jnewbery · 2018-05-01T16:00:19Z

src/validationinterface.h

I don't understand why this is an edge case or needs to be removed? Seems reasonable to remove txs from the mempool without necessarily needing to add other txs.

In fact, I think this comment is basically misleading. TransactionRemovedFromMempool no longer has a corresponding TransactionAddedToMempool event (TransactionAddedToMempool carries its own replaced txs).

The comment was referencing transactions which were never actually added to mempool but which we tried to add and then removed in the mempool limit pass before we ever generated a TransactionAddedToMempool callback for. Will look into making it more clear.

Ah, I understand now. Suggested wording:

Note that a transaction may fire a TransactionRemovedFromMempool notification without having previously fired a TransactionAddedToMempool notification (for example if it passes all AcceptToMemoryPool checks, but isn't added to the mempool due to a LimitMempoolSize() step). or similar

jnewbery · 2018-05-01T16:04:46Z

src/validationinterface.h

I'm not sure if 'core' is the right word now that there are validation and mempool interfaces. Would 'chainstate' be better?

jnewbery · 2018-05-01T16:28:59Z

src/txmempool.cpp

I think this is still unclear (as per spec is not very meaningful for me). Suggested wording: Use the tx as it appears in the block. See comment for MempoolUpdatedForBlockConnect() for details. or similar

jnewbery · 2018-05-01T17:32:18Z

src/validation.cpp

Can you add an AssertLockHeld(cs_main); to this function? ConnectTip() is only called by ActivateBestChainStep(), which holds cs_main.

This would make it clearer that MempoolUpdatedForBlockConnect is called immediately before its BlockConnected (ie we can't have two BlockConnected calls racing each other).

I'll let @practicalswift get to that with EXCLUSIVE_LOCKS_REQUIRED additions. Would prefer to not add more AssertLockHelds that are just gonna get converted anyway (also, I think its assumed that everything in CChainState either takes, or requires, and certainly for private stuff requires, cs_main).

@TheBlueMatt I'm standing ready! :-)

sure, sounds good

jnewbery · 2018-05-01T18:32:14Z

src/validation.cpp

I can't comment on the exact line above (currently L2361), but can you update:

// Remove conflicting transactions from the mempool.;

to:

// Remove conflicting transactions from the mempool. This will fire the MempoolUpdatedForBlockConnect() notification.

or similar

jnewbery · 2018-05-01T18:33:44Z

src/validationinterface.h

micronit: this part of the comment was updated in the wrong commit (Remove boost::signals from txmempool, call GetMainSignals() directly instead of BlockConnected(vtxConflicted) -> New MempoolInterface callback)

Because many listeners (eg wallet) will want both types of events to be well-ordered, they are parallel and connected on the backend. However, they are exposed separately to clients to separate the logic (and because, hopefully, eventually, they can be exposed to external clients of Bitcoin Core via libconsensus or similar).

This removes the whole ConnectTrace object, which may make it slightly harder to remove the unbounded-memory-during-reorg bug by throwing blocks out of memory and re-loading them from disk later. Comments are added to validationinterface to note where this should likely happen instead of ConnectTrace.

jnewbery · 2018-05-02T20:31:21Z

src/validationinterface.h

+void RegisterMempoolInterface(MempoolInterface* listener);
+/** Unregister a listener from mempool */
+void UnregisterMempoolInterface(MempoolInterface* listener);
+/** Unregister all listeners from core and mempool */


s/core and mempool/validation and mempool

jnewbery · 2018-05-02T20:48:06Z

src/validationinterface.h

Ah, I understand now. Suggested wording:

Note that a transaction may fire a TransactionRemovedFromMempool notification without having previously fired a TransactionAddedToMempool notification (for example if it passes all AcceptToMemoryPool checks, but isn't added to the mempool due to a LimitMempoolSize() step). or similar

jnewbery

one more comment on a comment

laanwj · 2018-05-03T14:01:27Z

src/validationinterface.h

+     * Note that in some rare cases (eg mempool limiting) a
+     * TransactionRemovedFromMempool event may fire with no corresponding
+     * TransactionAddedToMempool event for the same transaction.
+     * (TODO: remove this edge case)


Thanks for adding the documentation.
I think it'd be better to open an issue to track the TODOs, instead of putting them in the source code comments (this tends to get out of date very quickly).

laanwj · 2018-05-03T14:17:03Z

src/txmempool.h


    size_t DynamicMemoryUsage() const;

-    boost::signals2::signal<void (CTransactionRef)> NotifyEntryAdded;


What is the rationale to put mempool signals on something else than on the mempool itself (or an object contained in it)?
Naively I'd think it'd be a more appropriate place than on the global validation interface.

CTxMemPool isn't really "the mempool" its more like "the data structure that stores mempool and has some of the logic from validation.cpp's mempool management stuff in it". While it'd be nice to clean that up, I don't think its as cut-and-dry as "belongs elsewhere". More directly, the reason I wanted to do this is it means one less place we have to include boost signals in an included-everywhere .h file, and I'd like to be moving towards not using it even inside the validationinterface for more paralellism as we need it, eventually.

If CTxMempool isn't really "the mempool" that doesn't mean it shouldn't be. I think grouping the functionality for a certain concern together in one module makes sense.
E.g. when trying to understand the code, "Mempool" is definitely a more useful conceptual grouping than "all notification signals".
But I don't feel strongly enough about this to NACK this.

I think it could make sense either way, with mempool signals living inside and coming from the mempool itself, or completely pulled out and coming from validation.cpp. However it does seem suboptimal to have the call-sites be split, with some coming from validation.cpp and others coming from the mempool.

Is it a design goal to eventually move all the call-sites to validation.cpp? If so then I think this intermediate state is fine for now, as this is still overall an improvement (and it's the existing logic that is already split between ATMP living in validation.cpp, but removeForBlock living in txmempool.cpp), but I would suggest that we add some comments indicating that these invocations of GetMainSignals() should be moved out eventually.

jimpo · 2018-05-11T00:19:05Z

ACK 53b90df. Tested by running with -debug=mempool, sending to wallet and checking unconfirmed balance, spending from wallet and checking balance. Did not attempt to test conflicts/double spends.

sdaftuar

Looks pretty good. I think it'd be great to also see unit tests that verify the validation interface is living up to its promises, either in this PR or queued up in a follow-on PR.

sdaftuar · 2018-05-08T14:29:58Z

src/validationinterface.h

@ryanofsky "without forward-progress" means being at a worse-tip than we were previously at before (less work, or same work but a block that sorts worse than our prior tip, eg we received it second) -- exposing such an intermediate state to listeners is a bug. Note that this would be fixed by #13023.

sdaftuar · 2018-05-16T12:23:23Z

src/txmempool.cpp

            setEntries stage;
            stage.insert(it);
            RemoveStaged(stage, true, MemPoolRemovalReason::BLOCK);
+            txn_removed_in_block.push_back(tx); // Use the block's copy as witness may be different


I think it would make sense to return both the version removed from the mempool and the version in the block, in the event that a malleated witness is mined.

My thinking is that listeners should either be expected to track everything they care about, in which case we should only return wtxid's to them when referencing something they've already seen, or they should not -- in which case we should give them full transactions everywhere. Right now we're giving full transactions for the added to mempool and removed from mempool cases, so I think we should also give the full transaction in this last edge case, for completeness.

sdaftuar · 2018-05-16T12:39:39Z

src/txmempool.h


    size_t DynamicMemoryUsage() const;

-    boost::signals2::signal<void (CTransactionRef)> NotifyEntryAdded;


I think it could make sense either way, with mempool signals living inside and coming from the mempool itself, or completely pulled out and coming from validation.cpp. However it does seem suboptimal to have the call-sites be split, with some coming from validation.cpp and others coming from the mempool.

Is it a design goal to eventually move all the call-sites to validation.cpp? If so then I think this intermediate state is fine for now, as this is still overall an improvement (and it's the existing logic that is already split between ATMP living in validation.cpp, but removeForBlock living in txmempool.cpp), but I would suggest that we add some comments indicating that these invocations of GetMainSignals() should be moved out eventually.

sdaftuar · 2018-05-16T12:46:25Z

src/net_processing.cpp

            }
        }

-        for (const CTransactionRef& removedTx : lRemovedTxn)


I haven't though through the numbers required here to make this a practical DoS, but should we make sure to drain the callback queue before processing too many tx messages to prevent unbounded memory?

sdaftuar · 2018-05-16T12:55:57Z

src/validationinterface.h

     * Called on a background thread.
     */
-    virtual void TransactionAddedToMempool(const CTransactionRef &ptxn) {}
+    virtual void TransactionAddedToMempool(const CTransactionRef &ptxn, const std::vector<CTransactionRef>& txn_replaced) {}


nit: would be nice to add a comment explaining what txn_replaced is.

maflcko · 2018-05-17T19:09:28Z

src/zmq/zmqnotificationinterface.cpp

-void CZMQNotificationInterface::BlockConnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindexConnected, const std::vector<CTransactionRef>& vtxConflicted)
+void CZMQNotificationInterface::TransactionAddedToMempool(const CTransactionRef& ptx, const std::vector<CTransactionRef>& txn_replaced) {
+    NotifyTransaction(ptx);
+}


Nit: could add a small comment in this method to explain why txn_replaced can safely be dropped?

maflcko · 2018-05-21T19:26:12Z

Needs rebase.

TheBlueMatt mentioned this pull request Apr 13, 2018

Move fee estimator into validationinterface/cscheduler thread #11775

Closed

fanquake added Refactoring Validation labels Apr 13, 2018

maflcko reviewed Apr 16, 2018

View reviewed changes

TheBlueMatt force-pushed the 2018-04-mempoolinterface branch 2 times, most recently from e530a76 to e95db39 Compare April 17, 2018 15:26

Empact reviewed Apr 18, 2018

View reviewed changes

ryanofsky mentioned this pull request Apr 30, 2018

[RFC] I Have a Hammer! (Replace parts of ui_interface with validationinterface) #11856

Closed

ryanofsky reviewed Apr 30, 2018

View reviewed changes

jnewbery reviewed May 1, 2018

View reviewed changes

TheBlueMatt added 5 commits May 2, 2018 14:09

Clarify validationinterface notification ordering

bad68e9

Pass MemPoolRemovalReason out through TransactionRemovedFromMempool

07095f3

Split removeRecursive into calculate/remove steps

ec0f81f

Track the set of transactions removed/conflicted in removeForBlock

1ea56d1

TheBlueMatt force-pushed the 2018-04-mempoolinterface branch from e95db39 to 9a48d33 Compare May 2, 2018 18:09

TheBlueMatt added 4 commits May 2, 2018 14:12

Remove boost::signals from txmempool, call GetMainSignals() directly

4da0342

Remove useless scope in AcceptToMemoryPoolWorker

1520506

Add txn_replaced to Added callback, remove lReplaced ATMP arg

53b90df

TheBlueMatt force-pushed the 2018-04-mempoolinterface branch from 9a48d33 to 53b90df Compare May 2, 2018 18:15

jnewbery reviewed May 2, 2018

View reviewed changes

laanwj reviewed May 3, 2018

View reviewed changes

maflcko mentioned this pull request May 8, 2018

Remove 2nd mapTx lookup in CTxMemPool::removeForBlock #13189

Closed

TheBlueMatt changed the title ~~Split validationinterface into paralell validation/mempool interfaces~~ Split validationinterface into parallell validation/mempool interfaces May 10, 2018

TheBlueMatt changed the title ~~Split validationinterface into parallell validation/mempool interfaces~~ Split validationinterface into parallel validation/mempool interfaces May 10, 2018

TheBlueMatt mentioned this pull request May 11, 2018

Fix some concurrency issues in ActivateBestChain() #13023

Merged

sdaftuar reviewed May 16, 2018

View reviewed changes

maflcko reviewed May 17, 2018

View reviewed changes

maflcko added the Needs rebase label Jun 6, 2018

TheBlueMatt closed this Jun 14, 2018

laanwj removed the Needs rebase label Oct 24, 2019

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021


		size_t DynamicMemoryUsage() const;

		boost::signals2::signal<void (CTransactionRef)> NotifyEntryAdded;

Conversation

TheBlueMatt commented Apr 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko Apr 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Apr 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa commented Apr 26, 2018

Uh oh!

ajtowns commented Apr 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheBlueMatt commented Apr 27, 2018

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Apr 13, 2018 •

edited

Loading

maflcko Apr 16, 2018 •

edited

Loading

ajtowns commented Apr 26, 2018 •

edited

Loading

laanwj May 3, 2018 •

edited

Loading

laanwj May 12, 2018 •

edited

Loading