Notify on removal #9371

morcos · 2016-12-16T19:27:16Z

This is my alternative to reverting #9240

I think we have to decide which one of these 3 choices we like best:

walletnotify, zmq notification and UI notification on all transactions evicted from the mempool for any reason => merge this PR or something similar
walletnotify, zmq notification and UI notification on only transactions evicted from the mempool due to conflicts (prior behavior) => revert Remove txConflicted #9240
No walletnotify, zmq notification and UI notification on any transactions evicted from the mempool including due to conflicts => do nothing

I'm not sure this PR is the perfect solution, but I found txConflicted confusing, and I'm hoping this is at least a bit of a clearer overall picture.

Feedback welcome.

sipa · 2016-12-16T20:38:46Z

src/txmempool.cpp

+    if (trackRemovalsCount == 0) {
+        std::vector<CTransactionRef> temp(std::move(removedTxs));
+        removedTxs.clear();
+        return std::move(temp);


It's better to have a single return statement that returns a local variable over using std::move:

... std::vector<CTransactionRef> temp; if (trackRemovalsCount == 0) { temp.emplace_back(std::move(removedTxs)); removedTxs.clear(); } return temp; }

This allows the compiler to use NVRO (named return value optimization) - the temp variable essentially becomes an alias for the returned object.

Using swap in the middle could be a nice way to simplify this:

std::vector<CTransactionRef> temp; if (trackRemovalsCount == 0) temp.swap(removedTxs); return temp;

I like @ryanofsky's suggestion. Will do.

rebroad · 2016-12-19T03:56:33Z

src/txmempool.cpp

+            // If there are ever intentionally removals that are not
+            // meant to be tracked (so they can be notified on), then
+            // this log message can be removed.
+            LogPrint("mempool", "MemPoolRemovalTracker not tracking removed transaction %s\n",hash.ToString());


Is this the only notification, or is the GUI notified also?

about the error? this is the only log of the fact that a removal happened without notification.

dcousens · 2016-12-19T04:09:07Z

Is there a way to isolate these changes to the wallet code only?
I understand it is to fix a problem there, but ideally I'd rather see just a primitive mechanism in the mempool for removal events, and the wallet then handle that with a "mempool removal tracker" and such.

AFAIK, non-wallet users don't need this or #9240 reverted.

laanwj · 2016-12-19T07:22:03Z

I'd rather see just a primitive mechanism in the mempool for removal events, and the wallet then handle that with a "mempool removal tracker" and such.

In #8549 I had added a specific signal on the mempool to be notified of removed transactions, even with a reason field.
I think this was slightly more elegant than having a stateful StartTracking/StopTracking in the mempool itself and keeping a vector there (which means there can only be one client at a time). A client could easily handle the part of accumulating notifications into a vector itself, if that is desired.

jonasschnelli · 2016-12-19T07:35:14Z

Agree with @laanwj: the reason enum is really what we should have for mempool removal notifications: https://github.com/bitcoin/bitcoin/pull/8549/files#diff-d8e6fe13399f13c42a93ec8326c60614R150

dcousens · 2016-12-19T07:44:59Z

Agree with @laanwj, I actually thought that was merged and was wondering why this wasn't using it.

morcos · 2016-12-19T11:56:21Z

@laanwj ah, sorry I hadn't seen #8549. The reason I did it this way instead of trying to do notifications from the mempool directly was because we had just gone to such much effort to remove other notifications from happening within cs_main during the block connection logic.

It would be very easy to add to this PR the ability to distinguish between: "added", "removed", "appeared in a disconnected block", but slightly more involved to distinguish between the various "removed" reasons. (EDIT: I suppose the approach used in #8549 would work just as easily, we could just save the reason state in the vector.)

Whichever approach we take, I think it's a step in the right direction towards cleaning up the notification design and we should keep going. Note for example that even after #8549, I think "rawtx" and "hashtx" notifications are also happening on transactions in disconnected blocks (and before #9240 in transactions removed due to conflict), which doesn't seem to be the intent.

morcos · 2017-01-04T17:56:25Z

rebased

morcos · 2017-01-04T18:03:42Z

.. and added @ryanofsky's suggested cleanup

sipa · 2017-01-09T20:22:10Z

It seems strange to have a constraint that the mempool is either in a tracking or non-tracking state... the mempool object itself shouldn't need to know or care that there is at most one entity interested in seeing its removals.

Would it be possible to instead have a callback installed from the mempool to notify whatever is interested of removed transactions?

morcos · 2017-01-09T20:43:43Z

Yes @laanwj had a similar comment.
What I wanted to accomplish was that the notifications (or at least some of the potentially time consuming ones) happened outside of the critical path. The same as the change we recently made with transactions in connected blocks. But perhaps there is a better way to design that where the mempool just fires off its notifications immediately, but there can be a helper class that subscribes and appropriately batches them before passing them on? It's a bit tricky to imagine how some removals caused by limiting inside ATMP should be fired immediately and some shouldn't, but there ought to be a way.

In any case I'm open to a suggestion on redesigning that aspect of it.

sipa · 2017-01-09T20:56:54Z

The receiver of the mempool-removal event notifications can also still add things to a queue for later processing, i mean. It doesn't need to be the mempool that does the batching - as that inherently only works for a single consumer of event.

morcos · 2017-01-18T19:14:12Z

OK I've redone this in a way that is hopefully more appealing.

The first commit is stolen (and slightly tweaked) from #8549 and will lay the ground work for a more flexible notification system on mempool actions.

Then I introduce a minimal MemPoolConflictRemovalTracker which should exactly mimic the behavior of txConflicted that was removed in #9240. I wouldn't argue that we want to keep this class in the long run, but as a regression fix for 0.14, I think this most clearly accomplishes the goal while moving us in a forward direction.

In the future we will want to rethink exactly what notifications need to fire on mempool removals and when. Most of the wallet functionality that is called by SyncTransaction is a no-op on mempool removals except for downstream notifications. This is documented.

theuni · 2017-01-18T19:56:01Z

src/validation.cpp

+    ~MemPoolConflictRemovalTracker() {
+        pool.NotifyEntryRemoved.disconnect(boost::bind(&MemPoolConflictRemovalTracker::NotifyEntryRemoved, this, _1, _2));
+        for (const auto& tx : conflictedTxs) {
+            GetMainSignals().SyncTransaction(*tx, NULL, CMainSignals::SYNC_TRANSACTION_NOT_IN_BLOCK);


Seems like a good time to split up SyncTransaction.

How about dropping the SYNC_TRANSACTION_NOT_IN_BLOCK kludge, and adding a new signal for mempool removals? I suspect @TheBlueMatt's recent-removed-tx-cache could benefit from that as well.

TheBlueMatt · 2017-01-18T22:39:09Z

src/validation.cpp

+          // notification of a connected conflict might cause an outside process
+          // to abandon a transaction and then have it inadvertantly cleared by
+          // the notification that the conflicted transaction was evicted.
+        MemPoolConflictRemovalTracker mrt(mempool);


I believe you need some lock held here so that no two threads could be calling mempool.NotifyEntryRemoved.connect at the same time, however I agree you dont want to SyncTransaction with cs_main held. You could add a static mutex here, or you could create the object with cs_main, then std::move it to a dummy which will be destroyed without cs_main.

Hmm I guess you're right.. Perhaps I need to go back to reference counting these things..

TheBlueMatt · 2017-01-18T22:43:44Z

I believe this breaks #9570. Even if its rebased on #9570 (so that SyncTransaction becomes SyncTransactions and they're all batched together), the multiple per-block SyncTransactions calls will still expose wallet state as of mid-block to RPC clients. I'm not sure how exactly we should go about fixing this, but one way might be to take @theuni's suggestion for 0.14 and split SyncTransaction to SyncTransactionsFromBlock(vTxnConnected, vTxnConflicted) and TransactionAddedToMemPool(tx) so that we can hold the cs_wallet lock the whole time in SyncTransactionsFromBlock.

Of course, as noted in #9240, the only visible state from the SyncTransaction calls from CONFLICTED transaction removals is to un-abandon some transactions and update the UI/call walletnotify, both of which I think maybe are OK.

morcos · 2017-01-18T23:09:47Z

Yeah .... frustrating... Ideally we wouldn't actually be calling the wallet part of SyncTransaction for these transactions anyway. But I suspect that for 0.14 we should just be fine with wallet state being inspectable between these. As you point out it should have basically no effect... (it really shouldn't be unabandoning anything, that would imply some other logic was broken)

morcos · 2017-01-19T17:07:15Z

OK this has been rebased on #9583 which automagically fixes the lesser synchronization issues here if these notifications are also moved inside cs_main.

The ugly scope hack to have removals notified before connections I think should be removed after we tidy up abandoned logic in some future PR. I believe the natural order of notifications should be chainstate changes before mempool changes...

Add notification signals to make it possible to subscribe to mempool changes: - NotifyEntryAdded(CTransactionRef)> - NotifyEntryRemoved(CTransactionRef, MemPoolRemovalReason)> Also add a mempool removal reason enumeration, which is passed to the removed notification based on why the transaction was removed from the mempool.

Analogue to ConnectTrace that tracks transactions that have been removed from the mempool due to conflicts and then passes them through SyncTransaction at the end of its scope.

TheBlueMatt · 2017-01-23T21:46:26Z

utACK 094e4b3

laanwj · 2017-01-24T09:04:27Z

src/validation.cpp

@@ -3597,7 +3642,7 @@ bool CVerifyDB::VerifyDB(const CChainParams& chainparams, CCoinsView *coinsview,
            return error("VerifyDB(): *** ReadBlockFromDisk failed at %d, hash=%s", pindex->nHeight, pindex->GetBlockHash().ToString());
        // check level 1: verify block validity
        if (nCheckLevel >= 1 && !CheckBlock(block, state, chainparams.GetConsensus()))
-            return error("%s: *** found bad block at %d, hash=%s (%s)\n", __func__, 
+            return error("%s: *** found bad block at %d, hash=%s (%s)\n", __func__,


μ-nit: please don't include space changes in otherwise unchanged lines/functions

heh. sure. i'll pass that on to the commit author. :)

laanwj · 2017-01-24T09:06:41Z

Looks good to me now. utACK 094e4b3

094e4b3 Better document usage of SyncTransaction (Alex Morcos) 4afbde6 Introduce MemPoolConflictRemovalTracker (Alex Morcos) ff25c32 mempool: add notification for added/removed entries (Wladimir J. van der Laan)

ryanofsky · 2020-03-12T21:36:13Z

@jnewbery pointed out the PR description here became out of date and doesn't reflect the change that was actually merged

re: #9371 (comment)

I think we have to decide which one of these 3 choices we like best:

walletnotify, zmq notification and UI notification on all transactions evicted from the mempool for any reason => merge this PR or something similar

walletnotify, zmq notification and UI notification on only transactions evicted from the mempool due to conflicts (prior behavior) => revert Remove txConflicted #9240

No walletnotify, zmq notification and UI notification on any transactions evicted from the mempool including due to conflicts => do nothing

Initially this PR implemented the first choice behavior (commits), but it was changed in #9371 (comment) to implement second choice behavior instead. That behavior got broken again in 7e89994 from #16624 and we are currently discussing how to restore it in #18325

…moved signals e57980b [mempool] Remove NotifyEntryAdded and NotifyEntryRemoved callbacks (John Newbery) 2dd561f [validation] Remove pool member from ConnectTrace (John Newbery) 969b65f [validation] Remove NotifyEntryRemoved callback from ConnectTrace (John Newbery) 5613f98 [validation] Remove conflictedTxs from PerBlockConnectTrace (John Newbery) cdb8934 [validation interface] Remove vtxConflicted from BlockConnected (John Newbery) 1168394 [wallet] Notify conflicted transactions in TransactionRemovedFromMempool (John Newbery) Pull request description: These boost signals were added in #9371, before we had a `TransactionRemovedFromMempool` method in the validation interface. The `NotifyEntryAdded` callback was used by validation to build a vector of conflicted transactions when connecting a block, which the wallet was notified of in the `BlockConnected` CValidationInterface callback. Now that we have a `TransactionRemovedFromMempool` callback, we can fire that signal directly from the mempool for conflicted transactions. Note that #9371 was implemented to ensure `-walletnotify` events were fired for these conflicted transaction. We inadvertently stopped sending these notifications in #16624 (Sep 2019 commit 7e89994). We should probably fix that, but in a different PR. ACKs for top commit: jonatack: Re-ACK e57980b ryanofsky: Code review ACK e57980b, no code changes since previous review, but helpful new code comments have been added and the PR description is now more clear about where the old code came from Tree-SHA512: 3bdbaf1ef2731e788462d4756e69c42a1efdcf168691ce1bbfdaa4b7b55ac3c5b1fd4ab7b90bcdec653703600501b4224d252cfc086aef28f9ce0da3b0563a69

Add test coverage for conflicted wallet transaction notifications so we improve current behavior and avoid future regressions bitcoin#9240 - accidental break bitcoin#9479 - bug report bitcoin#9371 - fix bitcoin#16624 - accidental break bitcoin#18325 - bug report bitcoin#18600 - potential fix

f963a68 test: Add test for conflicted wallet tx notifications (Russell Yanofsky) Pull request description: Add test coverage for conflicted wallet transaction notifications so we can improve current behavior and avoid future regressions #9240 - accidental break #9479 - bug report #9371 - fix #16624 - accidental break #18325 - bug report #18600 - potential fix ACKs for top commit: laanwj: ACK f963a68 jonatack: re-ACK f963a68 MarcoFalke: ACK f963a68 Tree-SHA512: d3a7952a2d3dc2ff0800ef857575ea4ef9759c0917d58a7fc91e2db0ca3cc3baf0dd0cf9af61683f691e5fefb11afe8120bb5810c7037ed9ecedee385dd4aa07

…tions f963a68 test: Add test for conflicted wallet tx notifications (Russell Yanofsky) Pull request description: Add test coverage for conflicted wallet transaction notifications so we can improve current behavior and avoid future regressions bitcoin#9240 - accidental break bitcoin#9479 - bug report bitcoin#9371 - fix bitcoin#16624 - accidental break bitcoin#18325 - bug report bitcoin#18600 - potential fix ACKs for top commit: laanwj: ACK f963a68 jonatack: re-ACK f963a68 MarcoFalke: ACK f963a68 Tree-SHA512: d3a7952a2d3dc2ff0800ef857575ea4ef9759c0917d58a7fc91e2db0ca3cc3baf0dd0cf9af61683f691e5fefb11afe8120bb5810c7037ed9ecedee385dd4aa07

Add test coverage for conflicted wallet transaction notifications so we improve current behavior and avoid future regressions bitcoin#9240 - accidental break bitcoin#9479 - bug report bitcoin#9371 - fix bitcoin#16624 - accidental break bitcoin#18325 - bug report bitcoin#18600 - potential fix Github-Pull: bitcoin#18878 Rebased-From: f963a68

Add test coverage for conflicted wallet transaction notifications so we improve current behavior and avoid future regressions bitcoin/bitcoin#9240 - accidental break bitcoin/bitcoin#9479 - bug report bitcoin/bitcoin#9371 - fix bitcoin/bitcoin#16624 - accidental break bitcoin/bitcoin#18325 - bug report bitcoin/bitcoin#18600 - potential fix

…llet 98d770f CScheduler boost->std::function, use millisecs for times, not secs (Matt Corallo) 67e068c Remove now unneeded ChainTip signal (furszy) bcdd3e9 Move ChainTip sapling update witnesses and nullifiers to BlockConnected/BlockDisconnected. (furszy) b799070 Take a CTransactionRef in AddToWalletIfInvolvingMe to avoid a copy (Matt Corallo) d77244c Remove dead-code tracking of requests for blocks we generated (Matt Corallo) 10ccbbf Hold cs_wallet for whole block [dis]connection processing (Matt Corallo) 1a45036 fix compiler warning member functions not marked as 'override' (furszy) d3867a7 An adaptation of Corallo's btc@461e49fee2935b1eb4d4ea7bae3023e655c0a6d8 (Matt Corallo) f5ac648 [Refactor] Move Sapling ChainTip signal notification loop to use the latest connectTrace class structure (furszy) 8704d9d Handle SyncTransaction in ActivateBestChain instead of ConnectTrace (Matt Corallo) 6dcb6b0 Keep conflictedTxs in ConnectTrace per-block (Matt Corallo) 50d3e0e Handle conflicted transactions directly in ConnectTrace (furszy) 27fb897 Make ConnectTrace::blocksConnected private, hide behind accessors (Matt Corallo) 60329da Add pblock to connectTrace at the end of ConnectTip, not start (Matt Corallo) e7c2789 Include missing #include in zmqnotificationinterface.h (Matt Corallo) 1b396b8 Move `NotifyEntryRemoved` to use the general interfaces::MakeHandler (furszy) 4cb5820 Better document usage of SyncTransaction (Alex Morcos) 21be4e2 Introduce MemPoolConflictRemovalTracker (Alex Morcos) 7326acb mempool: add notification for added/removed entries (Wladimir J. van der Laan) a8605d2 Clean up tx prioritization when conflict mined (Casey Rodarmor) e7db9ff remove internal tracking of mempool conflicts for reporting to wallet (Alex Morcos) 76c72c6 remove external usage of mempool conflict tracking (Alex Morcos) ef429af tests: Stop node before removing the notification file (furszy) 15a21c2 tests: write the notification to different files to avoid race condition (Chun Kuan Lee) 466e97a [Wallet] Simplify InMempool (furszy) 85e18f0 Rename FlushWalletDB -> CompactWalletDB, add function description (Matt Corallo) 00f36ea Use CScheduler for wallet flushing, remove ThreadFlushWalletDB (Matt Corallo) Pull request description: Revamped the validation interface interaction with the wallet, encapsulating and improving the mempool and block signaling and each of the wallet signals handler. Restructured the Sapling witnesses and nullifiers update under the new signals. Solved several bugs that found on the way as well (Check each commit description). This PR concludes with #1726 long road :). Pushing our repository around 2 years ahead in btc time in the mempool and validation interface areas (without including RBF). The new validation -> wallet interaction architecture will enable further, and much more user facing important, improvements for the syncing process, overall software responsiveness and multithreading locking issues. Adapted backports: dashpay#6464 --> Always clean up manual transaction prioritization (mempool) bitcoin#9240 --> Remove txConflicted (mempool) bitcoin#9371 --> Notify on removal (mempool) bitcoin#9605 --> Use CScheduler for wallet flushing, remove ThreadFlushWalletDB (walletdb) bitcoin#9725 --> CValidationInterface Cleanups (wallet, validation and validation interface) ACKs for top commit: random-zebra: utACK 98d770f Fuzzbawls: utACK 98d770f Tree-SHA512: 84c86567c2bff36b859b2ae73c558956a70dff0fffb4f73208708d92165b851bf42d35246410238c66c7a4b77e5bf51ec93885234a75fa48901fd182d2f70a28

Add test coverage for conflicted wallet transaction notifications so we improve current behavior and avoid future regressions bitcoin/bitcoin#9240 - accidental break bitcoin/bitcoin#9479 - bug report bitcoin/bitcoin#9371 - fix bitcoin/bitcoin#16624 - accidental break bitcoin/bitcoin#18325 - bug report bitcoin/bitcoin#18600 - potential fix Github-Pull: #18878 Rebased-From: f963a68

sipa reviewed Dec 16, 2016

View reviewed changes

fanquake added the Mempool label Dec 17, 2016

rebroad reviewed Dec 19, 2016

View reviewed changes

morcos force-pushed the notifyOnRemoval branch from f3baf89 to 45adeb9 Compare January 4, 2017 17:55

morcos force-pushed the notifyOnRemoval branch from 45adeb9 to 52f6c35 Compare January 4, 2017 18:03

TheBlueMatt mentioned this pull request Jan 5, 2017

-walletnotify isn't fired on transactions that were in the mempool and then evicted due to a tx in a new block conflicting them. #9479

Closed

morcos force-pushed the notifyOnRemoval branch 2 times, most recently from 370061b to f0574d6 Compare January 18, 2017 19:08

theuni reviewed Jan 18, 2017

View reviewed changes

TheBlueMatt reviewed Jan 18, 2017

View reviewed changes

morcos force-pushed the notifyOnRemoval branch from f0574d6 to af8d1c5 Compare January 19, 2017 17:04

laanwj and others added 3 commits January 23, 2017 15:43

Introduce MemPoolConflictRemovalTracker

4afbde6

Analogue to ConnectTrace that tracks transactions that have been removed from the mempool due to conflicts and then passes them through SyncTransaction at the end of its scope.

Better document usage of SyncTransaction

094e4b3

morcos force-pushed the notifyOnRemoval branch from af8d1c5 to 094e4b3 Compare January 23, 2017 21:07

laanwj reviewed Jan 24, 2017

View reviewed changes

laanwj merged commit 094e4b3 into bitcoin:master Jan 24, 2017

karelbilek mentioned this pull request Mar 17, 2017

zmq: mempool notifications #8549

Closed

CryptoCentric pushed a commit to absolute-community/absolute that referenced this pull request Feb 27, 2019

Merge bitcoin#9371: Notify on removal

5b8a8b0

jnewbery mentioned this pull request Nov 14, 2019

Remove the mempool's NotifyEntryAdded and NotifyEntryRemoved signals #17477

Merged

jnewbery mentioned this pull request Mar 11, 2020

Restore UI notifications and -walletnotify behavior for block conflicted transactions #18325

Closed

ryanofsky mentioned this pull request May 5, 2020

test: Add test for conflicted wallet tx notifications #18878

Merged

ryanofsky mentioned this pull request Jan 11, 2021

Marking chains of txs conflicted properly #8692

Closed

This was referenced Jan 21, 2021

Revamping block validation interface interaction with wallet PIVX-Project/PIVX#2150

Merged

Up to CTransactionRef point back ports to do list. PIVX-Project/PIVX#1726

Closed

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Notify on removal #9371

Notify on removal #9371

Uh oh!

Conversation

morcos commented Dec 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky Dec 16, 2016 • 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!

dcousens commented Dec 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Dec 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonasschnelli commented Dec 19, 2016

Uh oh!

dcousens commented Dec 19, 2016

Uh oh!

morcos commented Dec 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

morcos commented Jan 4, 2017

Uh oh!

morcos commented Jan 4, 2017

Uh oh!

sipa commented Jan 9, 2017

Uh oh!

morcos commented Jan 9, 2017

Uh oh!

sipa commented Jan 9, 2017

Uh oh!

morcos commented Jan 18, 2017

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 Jan 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

morcos commented Jan 18, 2017

Uh oh!

morcos commented Jan 19, 2017

Uh oh!

TheBlueMatt commented Jan 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Jan 24, 2017

Uh oh!

ryanofsky commented Mar 12, 2020

Uh oh!

Uh oh!

morcos commented Dec 16, 2016 •

edited

Loading

ryanofsky Dec 16, 2016 •

edited

Loading

dcousens commented Dec 19, 2016 •

edited

Loading

laanwj commented Dec 19, 2016 •

edited

Loading

morcos commented Dec 19, 2016 •

edited

Loading

TheBlueMatt commented Jan 18, 2017 •

edited

Loading