Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Notify on removal #9371

Merged
merged 3 commits into from Jan 24, 2017
Merged

Notify on removal #9371

merged 3 commits into from Jan 24, 2017

Conversation

@morcos
Copy link
Member

morcos commented Dec 16, 2016

Fixes #9479

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.

src/txmempool.cpp Outdated
if (trackRemovalsCount == 0) {
std::vector<CTransactionRef> temp(std::move(removedTxs));
removedTxs.clear();
return std::move(temp);

This comment has been minimized.

Copy link
@sipa

sipa Dec 16, 2016

Member

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.

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Dec 16, 2016

Contributor

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;

This comment has been minimized.

Copy link
@morcos

morcos Dec 19, 2016

Author Member

I like @ryanofsky's suggestion. Will do.

@fanquake fanquake added the Mempool label Dec 17, 2016
src/txmempool.cpp Outdated
// 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());

This comment has been minimized.

Copy link
@rebroad

rebroad Dec 19, 2016

Contributor

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

This comment has been minimized.

Copy link
@morcos

morcos Dec 19, 2016

Author Member

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

@dcousens

This comment has been minimized.

Copy link
Contributor

dcousens commented Dec 19, 2016

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

This comment has been minimized.

Copy link
Member

laanwj commented Dec 19, 2016

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

This comment has been minimized.

Copy link
Member

jonasschnelli commented Dec 19, 2016

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

This comment has been minimized.

Copy link
Contributor

dcousens commented Dec 19, 2016

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

@morcos

This comment has been minimized.

Copy link
Member Author

morcos commented Dec 19, 2016

@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 morcos force-pushed the morcos:notifyOnRemoval branch Jan 4, 2017
@morcos

This comment has been minimized.

Copy link
Member Author

morcos commented Jan 4, 2017

rebased

@morcos morcos force-pushed the morcos:notifyOnRemoval branch Jan 4, 2017
@morcos

This comment has been minimized.

Copy link
Member Author

morcos commented Jan 4, 2017

.. and added @ryanofsky's suggested cleanup

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Jan 9, 2017

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

This comment has been minimized.

Copy link
Member Author

morcos commented Jan 9, 2017

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

This comment has been minimized.

Copy link
Member

sipa commented Jan 9, 2017

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 morcos force-pushed the morcos:notifyOnRemoval branch 2 times, most recently Jan 18, 2017
@morcos

This comment has been minimized.

Copy link
Member Author

morcos commented Jan 18, 2017

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.

~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);

This comment has been minimized.

Copy link
@theuni

theuni Jan 18, 2017

Member

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.

src/validation.cpp Outdated
// 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);

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Jan 18, 2017

Contributor

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.

This comment has been minimized.

Copy link
@morcos

morcos Jan 18, 2017

Author Member

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

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

TheBlueMatt commented Jan 18, 2017

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

This comment has been minimized.

Copy link
Member Author

morcos commented Jan 18, 2017

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 morcos force-pushed the morcos:notifyOnRemoval branch Jan 19, 2017
@morcos

This comment has been minimized.

Copy link
Member Author

morcos commented Jan 19, 2017

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

laanwj and others added 3 commits Mar 26, 2016
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.
@morcos morcos force-pushed the morcos:notifyOnRemoval branch to 094e4b3 Jan 23, 2017
@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

TheBlueMatt commented Jan 23, 2017

utACK 094e4b3

@@ -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__,

This comment has been minimized.

Copy link
@laanwj

laanwj Jan 24, 2017

Member

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

This comment has been minimized.

Copy link
@morcos

morcos Jan 24, 2017

Author Member

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

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Jan 24, 2017

Looks good to me now. utACK 094e4b3

@laanwj laanwj merged commit 094e4b3 into bitcoin:master Jan 24, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
laanwj added a commit that referenced this pull request Jan 24, 2017
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)
codablock added a commit to codablock/dash that referenced this pull request Jan 21, 2018
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)
andvgal added a commit to energicryptocurrency/energi that referenced this pull request Jan 6, 2019
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)
CryptoCentric added a commit to absolute-community/absolute that referenced this pull request Feb 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

10 participants
You can’t perform that action at this time.