Notify on removal

morcos commented at 7:27 pm on December 16, 2016: member

This is my alternative to reverting #9240 @sipa @laanwj

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

in src/txmempool.cpp: in 64712d66f7 outdated

496+    assert(trackRemovalsCount);
497+    trackRemovalsCount--;
498+    if (trackRemovalsCount == 0) {
499+        std::vector<CTransactionRef> temp(std::move(removedTxs));
500+        removedTxs.clear();
501+        return std::move(temp);

sipa commented at 8:38 pm on December 16, 2016:

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

0    ...
1    std::vector<CTransactionRef> temp;
2    if (trackRemovalsCount == 0) {
3        temp.emplace_back(std::move(removedTxs));
4        removedTxs.clear();
5    }
6    return temp;
7}

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

ryanofsky commented at 9:15 pm on December 16, 2016:

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

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

morcos commented at 11:28 am on December 19, 2016:

I like @ryanofsky’s suggestion. Will do.

fanquake added the label Mempool on Dec 17, 2016

in src/txmempool.cpp: in f3baf89e0d outdated

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

rebroad commented at 3:56 am on December 19, 2016:

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

morcos commented at 11:29 am on December 19, 2016:

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

dcousens commented at 4:09 am on December 19, 2016: contributor

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 commented at 7:22 am on December 19, 2016: member

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 commented at 7:35 am on December 19, 2016: contributor

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 commented at 7:44 am on December 19, 2016: contributor

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

morcos commented at 11:56 am on December 19, 2016: member

@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 force-pushed on Jan 4, 2017

morcos commented at 5:56 pm on January 4, 2017: member

rebased

morcos force-pushed on Jan 4, 2017

morcos commented at 6:03 pm on January 4, 2017: member

.. and added @ryanofsky’s suggested cleanup

sipa commented at 8:22 pm on January 9, 2017: member

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 commented at 8:43 pm on January 9, 2017: member

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 commented at 8:56 pm on January 9, 2017: member

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 force-pushed on Jan 18, 2017

morcos commented at 7:14 pm on January 18, 2017: member

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.

in src/validation.cpp: in 84b4c4c1d5 outdated

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

theuni commented at 7:56 pm on January 18, 2017:

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.

in src/validation.cpp: in 84b4c4c1d5 outdated

2484+          // state of transactions in our wallet is currently cleared when we
2485+          // receive another notification and there is a race condition where
2486+          // notification of a connected conflict might cause an outside process
2487+          // to abandon a transaction and then have it inadvertantly cleared by
2488+          // the notification that the conflicted transaction was evicted.
2489+        MemPoolConflictRemovalTracker mrt(mempool);

TheBlueMatt commented at 10:39 pm on January 18, 2017:

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.

morcos commented at 11:07 pm on January 18, 2017:

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

TheBlueMatt commented at 10:43 pm on January 18, 2017: member

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 commented at 11:09 pm on January 18, 2017: member

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 force-pushed on Jan 19, 2017

morcos commented at 5:07 pm on January 19, 2017: member

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…

mempool: add notification for added/removed entries

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.

ff25c32392

Introduce MemPoolConflictRemovalTracker

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.

4afbde6028

Better document usage of SyncTransaction 094e4b3383

morcos force-pushed on Jan 23, 2017

TheBlueMatt commented at 9:46 pm on January 23, 2017: member

utACK 094e4b33839404d9c18200fc30d9a993a3dc106f

in src/validation.cpp: in 094e4b3383

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

laanwj commented at 9:04 am on January 24, 2017:

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

morcos commented at 11:11 am on January 24, 2017:

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

laanwj commented at 9:06 am on January 24, 2017: member

Looks good to me now. utACK 094e4b3

laanwj merged this on Jan 24, 2017

laanwj closed this on Jan 24, 2017

laanwj referenced this in commit 4a1dc35ca5 on Jan 24, 2017

codablock referenced this in commit 184d54da86 on Jan 19, 2018

codablock referenced this in commit 3c8dea320e on Jan 20, 2018

codablock referenced this in commit dc523a1649 on Jan 21, 2018

andvgal referenced this in commit 847aeabf11 on Jan 6, 2019

CryptoCentric referenced this in commit 5b8a8b0840 on Feb 27, 2019

ryanofsky commented at 9:36 pm on March 12, 2020: member

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

re: #9371#issue-98389524

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 #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 7e89994133725125dddbfa8d45484e3b9ed51c6e from #16624 and we are currently discussing how to restore it in #18325

laanwj referenced this in commit 312d27b11c on Mar 19, 2020

ryanofsky referenced this in commit 2c1c162cd5 on May 5, 2020

ryanofsky referenced this in commit f963a68051 on May 13, 2020

laanwj referenced this in commit 99c03728c0 on May 13, 2020

sidhujag referenced this in commit a84dd1fa2e on May 14, 2020

fanquake referenced this in commit 9c193e4d0a on May 14, 2020

fanquake referenced this in commit ed0afe8c1f on May 15, 2020

metalicjames referenced this in commit 09af10dec5 on Aug 5, 2020

janus referenced this in commit f690d20f65 on Nov 15, 2020

furszy referenced this in commit 6143f803b2 on Jan 30, 2021

backpacker69 referenced this in commit 364a2d33dd on Mar 28, 2021

DrahtBot locked this on Feb 15, 2022

Notify on removal #9371