Split validationinterface into parallel validation/mempool interfaces #12979

TheBlueMatt commented at 6:58 PM on April 13, 2018: member

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.

laanwj added this to the "Blockers" column in a project

fanquake added the label Refactoring on Apr 13, 2018

fanquake added the label Validation on Apr 13, 2018

in src/validationinterface.h:112 in 16e4c4bed6 outdated

 106 | @@ -93,12 +107,22 @@ class CValidationInterface {
 107 |      /**
 108 |       * Notifies listeners of a block being disconnected
 109 |       *
 110 | +     * The ordering of BlockDisconnected and TransactionRemovedFromMempool
 111 | +     * (for transactions removed due to memory constraints or lock time/
 112 | +     * coinbase maturity chenges during the disconnection/reorg) is undefined,

MarcoFalke commented at 5:50 PM on April 16, 2018:

in commit 16e4c4bed66e436be606b9cfc6fa982819f00b86: Should say "changes"?

in src/txmempool.cpp:592 in ff936aeea5 outdated

 587 |          txiter it = mapTx.find(tx->GetHash());
 588 |          if (it != mapTx.end()) {
 589 |              setEntries stage;
 590 |              stage.insert(it);
 591 |              RemoveStaged(stage, true, MemPoolRemovalReason::BLOCK);
 592 | +            txn_removed_in_block.push_back(tx); // Use the block's copy

MarcoFalke commented at 6:39 PM on April 16, 2018:

in commit ff936aeea569a2ae56a3d9096a486fbd94b6dde7: 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?

TheBlueMatt commented at 3:20 PM on April 17, 2018:

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").

in src/txmempool.cpp:423 in 6b4e2a79e4 outdated

 417 | @@ -418,8 +418,10 @@ bool CTxMemPool::addUnchecked(const uint256& hash, const CTxMemPoolEntry &entry,
 418 |  
 419 |  void CTxMemPool::removeUnchecked(txiter it, MemPoolRemovalReason reason)
 420 |  {
 421 | -    if (reason != MemPoolRemovalReason::BLOCK && reason != MemPoolRemovalReason::CONFLICT) {
 422 | -        // BLOCK and CONFLICT callbacks are generated in removeForBlock
 423 | +    if (reason != MemPoolRemovalReason::BLOCK && reason != MemPoolRemovalReason::CONFLICT &&
 424 | +        reason != MemPoolRemovalReason::REPLACED) {
 425 | +        // BLOCK and CONFLICT callbacks are generated in removeForBlock REPLACED

MarcoFalke commented at 7:34 PM on April 16, 2018:

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

in src/zmq/zmqnotificationinterface.cpp:170 in 6b4e2a79e4 outdated

 164 | @@ -165,18 +165,22 @@ void CZMQNotificationInterface::TransactionAddedToMempool(const CTransactionRef&
 165 |      }
 166 |  }
 167 |  
 168 | +void CZMQNotificationInterface::TransactionAddedToMempool(const CTransactionRef& ptx, const std::vector<CTransactionRef>& txn_replaced) {
 169 | +    NotifyTransaction(ptx);
 170 | +}

MarcoFalke commented at 7:52 PM on April 16, 2018:

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

in src/txmempool.cpp:602 in db156ea327 outdated

 598 | @@ -599,7 +599,7 @@ void CTxMemPool::removeForBlock(const std::vector<CTransactionRef>& vtx, unsigne
 599 |          removeConflicts(*tx, txn_conflicts);
 600 |          ClearPrioritisation(tx->GetHash());
 601 |      }
 602 | -    GetMainSignals().MempoolUpdatedForBlockConnect(std::move(txn_removed_in_block), std::move(txn_conflicts));
 603 | +    GetMainSignals().MempoolUpdatedForBlockConnect(std::move(txn_removed_in_block), std::move(txn_conflicts), nBlockHeight);

MarcoFalke commented at 7:58 PM on April 16, 2018:

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

TheBlueMatt commented at 3:20 PM on April 17, 2018:

Its used in #11775.

MarcoFalke commented at 7:58 PM on April 16, 2018: member

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

TheBlueMatt force-pushed on Apr 17, 2018

MarcoFalke commented at 4:02 PM on April 17, 2018: member

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

in src/wallet/wallet.cpp:1234 in e95db39559 outdated

1204 | @@ -1205,8 +1205,11 @@ void CWallet::SyncTransaction(const CTransactionRef& ptx, const CBlockIndex *pin
1205 |      }
1206 |  }
1207 |  
1208 | -void CWallet::TransactionAddedToMempool(const CTransactionRef& ptx) {
1209 | +void CWallet::TransactionAddedToMempool(const CTransactionRef& ptx, const std::vector<CTransactionRef>& txn_replaced) {
1210 |      LOCK2(cs_main, cs_wallet);
1211 | +
1212 | +    for (const CTransactionRef& txit : txn_replaced) TransactionRemovedFromMempool(txit, MemPoolRemovalReason::REPLACED);

Empact commented at 11:05 PM on April 18, 2018:

Could you break this line?

MarcoFalke commented at 12:12 PM on April 19, 2018:

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.

TheBlueMatt commented at 4:15 PM on April 20, 2018:

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

sipa commented at 7:11 PM on April 26, 2018: member

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

ajtowns commented at 10:58 PM on April 26, 2018: member

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 commented at 4:47 PM on April 27, 2018: member

@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?

in src/validationinterface.h:70 in e7d7edc898 outdated

  66 | +     *
  67 | +     * Is called after a series of BlockConnected/BlockDisconnected events once
  68 | +     * the chain has made forward progress and is now at the best-known-tip.
  69 | +     *
  70 | +     * If a block is found to be invalid, this event may trigger without
  71 | +     * forward-progress, only to trigger again soon thereafter.

ryanofsky commented at 4:59 PM on April 30, 2018:

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 (review))

sdaftuar commented at 2:29 PM on May 8, 2018:

@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.

MarcoFalke commented at 3:26 PM on September 24, 2018:

#13023 was merged so this TODO can be removed?

ryanofsky commented at 5:33 PM on April 30, 2018: member

Started review (will update this comment with progress).

e7d7edc898c31ef37d6d64e61c72979c32253a43 Clarify validationinterface notification ordering (1/10)
165d2f1594e56d523797494a20e0b2f7a768244e Add a parallel validation interface for mempool events. (2/10)
4305ee98f5c5e558c3b6f229890f0240b12ed5cc Pass MemPoolRemovalReason out through TransactionRemovedFromMempool (3/10)
2100e0dca5f786d85f0e74f7515a4797e6d5541a Split removeRecursive into calculate/remove steps (4/10)
df6c6edc2e8758a6511ba8ee9f66469d9ccc9d95 Track the set of transactions removed/conflicted in removeForBlock (5/10)
07418e7d12de22402dfc805d201565f539a47b9b BlockConnected(vtxConflicted) -> New MempoolInterface callback (6/10)
9ca7be6992f9230a9a9dcd5f69f9695bf8b9d0b3 Remove boost::signals from txmempool, call GetMainSignals() directly (7/10)
ab3f927538021a89e38cfa55a312bb8c0a0adb46 Remove useless scope in AcceptToMemoryPoolWorker (8/10)
3febd61adb8955d4eee5294ab5554f6613d591ff Add txn_replaced to Added callback, remove lReplaced ATMP arg (9/10)
e95db395592c763e8c77f619db7a8f5dbaa0a604 Pass new block height through MempoolUpdatedForBlock (10/10)

in src/validationinterface.h:93 in e95db39559 outdated

  93 | +     * This only fires for transactions which leave mempool because of expiry,
  94 | +     * size limiting, or reorg (changes in lock times/coinbase maturity). This
  95 | +     * does not include any transactions which are included in
  96 | +     * MempoolUpdatedForBlockConnect or TransactionAddedToMempool(txn_replaced)
  97 | +     *
  98 | +     * reason == REORG is not ordered with BlockDisconnected/BlockDisconnected!

jnewbery commented at 3:59 PM on May 1, 2018:

Should this be BlockDisconnected/BlockConnected?

in src/validationinterface.h:98 in e95db39559 outdated

  98 | +     * reason == REORG is not ordered with BlockDisconnected/BlockDisconnected!
  99 | +     *
 100 | +     * Note that in some rare cases (eg mempool limiting) a
 101 | +     * TransactionRemovedFromMempool event may fire with no corresponding
 102 | +     * TransactionAddedToMempool event.
 103 | +     * (TODO: remove this edge case)

jnewbery commented at 4:00 PM on May 1, 2018:

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.

jnewbery commented at 6:57 PM on May 1, 2018:

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

TheBlueMatt commented at 4:46 AM on May 2, 2018:

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.

jnewbery commented at 8:48 PM on May 2, 2018:

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

in src/validationinterface.h:30 in e95db39559 outdated

  22 | @@ -23,15 +23,20 @@ class uint256;
  23 |  class CScheduler;
  24 |  class CTxMemPool;
  25 |  enum class MemPoolRemovalReason;
  26 | +class MempoolInterface;
  27 |  
  28 |  // These functions dispatch to one or all registered wallets
  29 |  
  30 |  /** Register a wallet to receive updates from core */

jnewbery commented at 4:04 PM on May 1, 2018:

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

in src/txmempool.cpp:597 in e95db39559 outdated

 592 |          txiter it = mapTx.find(tx->GetHash());
 593 |          if (it != mapTx.end()) {
 594 |              setEntries stage;
 595 |              stage.insert(it);
 596 |              RemoveStaged(stage, true, MemPoolRemovalReason::BLOCK);
 597 | +            txn_removed_in_block.push_back(tx); // Use the block's copy (as per spec)

jnewbery commented at 4:28 PM on May 1, 2018:

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

in src/validation.cpp:2314 in e95db39559 outdated

2386 | - * The block is added to connectTrace if connection succeeds.
2387 |   */
2388 | -bool CChainState::ConnectTip(CValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexNew, const std::shared_ptr<const CBlock>& pblock, ConnectTrace& connectTrace, DisconnectedBlockTransactions &disconnectpool)
2389 | +bool CChainState::ConnectTip(CValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexNew, const std::shared_ptr<const CBlock>& pblock, DisconnectedBlockTransactions &disconnectpool)
2390 |  {
2391 |      assert(pindexNew->pprev == chainActive.Tip());

jnewbery commented at 5:32 PM on May 1, 2018:

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).

TheBlueMatt commented at 5:59 PM on May 2, 2018:

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).

practicalswift commented at 6:19 PM on May 2, 2018:

@TheBlueMatt I'm standing ready! :-)

jnewbery commented at 8:48 PM on May 2, 2018:

sure, sounds good

in src/validation.cpp:2363 in e95db39559 outdated

2368 | @@ -2441,7 +2369,7 @@ bool CChainState::ConnectTip(CValidationState& state, const CChainParams& chainp
2369 |      LogPrint(BCLog::BENCH, "  - Connect postprocess: %.2fms [%.2fs (%.2fms/blk)]\n", (nTime6 - nTime5) * MILLI, nTimePostConnect * MICRO, nTimePostConnect * MILLI / nBlocksTotal);
2370 |      LogPrint(BCLog::BENCH, "- Connect block: %.2fms [%.2fs (%.2fms/blk)]\n", (nTime6 - nTime1) * MILLI, nTimeTotal * MICRO, nTimeTotal * MILLI / nBlocksTotal);
2371 |  
2372 | -    connectTrace.BlockConnected(pindexNew, std::move(pthisBlock));
2373 | +    GetMainSignals().BlockConnected(std::move(pthisBlock), pindexNew);

jnewbery commented at 6:32 PM on May 1, 2018:

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

in src/validationinterface.h:91 in e95db39559 outdated

  91 | +     * Notifies listeners of a transaction leaving mempool.
  92 | +     *
  93 | +     * This only fires for transactions which leave mempool because of expiry,
  94 | +     * size limiting, or reorg (changes in lock times/coinbase maturity). This
  95 | +     * does not include any transactions which are included in
  96 | +     * MempoolUpdatedForBlockConnect or TransactionAddedToMempool(txn_replaced)

jnewbery commented at 6:33 PM on May 1, 2018:

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)

jnewbery commented at 7:01 PM on May 1, 2018: member

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)

Clarify validationinterface notification ordering bad68e9a50

Add a parallel validation interface for mempool events.

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).

ae5e07196c

Pass MemPoolRemovalReason out through TransactionRemovedFromMempool 07095f3b45

Split removeRecursive into calculate/remove steps ec0f81f6be

Track the set of transactions removed/conflicted in removeForBlock 1ea56d14ae

TheBlueMatt force-pushed on May 2, 2018

BlockConnected(vtxConflicted) -> New MempoolInterface callback

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.

9550c830d7

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

Remove useless scope in AcceptToMemoryPoolWorker 15205068e6

Add txn_replaced to Added callback, remove lReplaced ATMP arg 53b90df75c

TheBlueMatt force-pushed on May 2, 2018

in src/validationinterface.h:38 in 53b90df75c

  37 | -void UnregisterAllValidationInterfaces();
  38 | +/** Register a listener to receive updates from mempool */
  39 | +void RegisterMempoolInterface(MempoolInterface* listener);
  40 | +/** Unregister a listener from mempool */
  41 | +void UnregisterMempoolInterface(MempoolInterface* listener);
  42 | +/** Unregister all listeners from core and mempool */

jnewbery commented at 8:31 PM on May 2, 2018:

s/core and mempool/validation and mempool

jnewbery commented at 9:00 PM on May 2, 2018: member

one more comment on a comment

in src/validationinterface.h:98 in 53b90df75c

  98 | +     * reason == REORG is not ordered with BlockConnected/BlockDisconnected!
  99 | +     *
 100 | +     * Note that in some rare cases (eg mempool limiting) a
 101 | +     * TransactionRemovedFromMempool event may fire with no corresponding
 102 | +     * TransactionAddedToMempool event for the same transaction.
 103 | +     * (TODO: remove this edge case)

laanwj commented at 2:01 PM on May 3, 2018:

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).

in src/txmempool.h:651 in 4da0342753 outdated

 646 | @@ -648,9 +647,6 @@ class CTxMemPool
 647 |  
 648 |      size_t DynamicMemoryUsage() const;
 649 |  
 650 | -    boost::signals2::signal<void (CTransactionRef)> NotifyEntryAdded;

laanwj commented at 2:17 PM on May 3, 2018:

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.

TheBlueMatt commented at 7:05 PM on May 3, 2018:

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.

laanwj commented at 7:04 AM on May 12, 2018:

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.

sdaftuar commented at 12:39 PM on May 16, 2018:

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.

TheBlueMatt renamed this:
~~Split validationinterface into paralell validation/mempool interfaces~~
Split validationinterface into parallell validation/mempool interfaces
on May 10, 2018

TheBlueMatt renamed this:
~~Split validationinterface into parallell validation/mempool interfaces~~
Split validationinterface into parallel validation/mempool interfaces
on May 10, 2018

jimpo commented at 12:19 AM on May 11, 2018: contributor

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.

in src/txmempool.cpp:592 in 1ea56d14ae outdated

 587 |          txiter it = mapTx.find(tx->GetHash());
 588 |          if (it != mapTx.end()) {
 589 |              setEntries stage;
 590 |              stage.insert(it);
 591 |              RemoveStaged(stage, true, MemPoolRemovalReason::BLOCK);
 592 | +            txn_removed_in_block.push_back(tx); // Use the block's copy as witness may be different

sdaftuar commented at 12:23 PM on May 16, 2018:

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.

in src/net_processing.cpp:2321 in 53b90df75c

2324 | @@ -2318,9 +2325,6 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
2325 |              }
2326 |          }
2327 |  
2328 | -        for (const CTransactionRef& removedTx : lRemovedTxn)

sdaftuar commented at 12:46 PM on May 16, 2018:

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?

in src/validationinterface.h:84 in 53b90df75c

  80 | @@ -81,14 +81,14 @@ class MempoolInterface {
  81 |       *
  82 |       * Called on a background thread.
  83 |       */
  84 | -    virtual void TransactionAddedToMempool(const CTransactionRef &ptxn) {}
  85 | +    virtual void TransactionAddedToMempool(const CTransactionRef &ptxn, const std::vector<CTransactionRef>& txn_replaced) {}

sdaftuar commented at 12:55 PM on May 16, 2018:

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

sdaftuar commented at 1:46 PM on May 16, 2018: member

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.

in src/zmq/zmqnotificationinterface.cpp:170 in 53b90df75c

 164 | @@ -165,18 +165,22 @@ void CZMQNotificationInterface::TransactionAddedToMempool(const CTransactionRef&
 165 |      }
 166 |  }
 167 |  
 168 | -void CZMQNotificationInterface::BlockConnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindexConnected, const std::vector<CTransactionRef>& vtxConflicted)
 169 | +void CZMQNotificationInterface::TransactionAddedToMempool(const CTransactionRef& ptx, const std::vector<CTransactionRef>& txn_replaced) {
 170 | +    NotifyTransaction(ptx);
 171 | +}

MarcoFalke commented at 7:09 PM on May 17, 2018:

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

MarcoFalke commented at 7:26 PM on May 21, 2018: member

Needs rebase.

MarcoFalke removed this from the "Blockers" column in a project

MarcoFalke added the label Needs rebase on Jun 6, 2018

TheBlueMatt closed this on Jun 14, 2018

laanwj removed the label Needs rebase on Oct 24, 2019

DrahtBot locked this on Dec 16, 2021