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

Remove the mempool's NotifyEntryAdded and NotifyEntryRemoved signals #17477

Open
wants to merge 6 commits into
base: master
from

Conversation

@jnewbery
Copy link
Member

jnewbery commented Nov 14, 2019

These boost signals were added in #9371, before we had an asynchronous 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 an asynchronous TransactionRemovedFromMempool callback, we can fire that signal directly from the mempool for conflicted transactions without having to worry about the performance impact of sync'ing wallet transactions during block connection.

@jnewbery

This comment has been minimized.

Copy link
Member Author

jnewbery commented Nov 14, 2019

Request review from @TheBlueMatt and @morcos

@TheBlueMatt - you introduced the vtxConflicted vector in BlockConnected in 461e49f with comment 'This change adds a parameter to BlockConnectedDisconnected which lists the transactions which were removed from mempool due to confliction as a result of this operation. While its somewhat of a shame to make block-validation-logic generate a list of mempool changes to be included in its generated callbacks, fixing this isnt too hard.'

@morcos - you attempted something similar in #9240

@fanquake fanquake added the Validation label Nov 14, 2019
@jnewbery jnewbery force-pushed the jnewbery:2019-11-remove-mempool-signals2 branch from 63611e7 to 906b478 Nov 14, 2019
@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Nov 14, 2019

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #17951 (Use rolling bloom filter of recent block txs for AlreadyHave() check by sdaftuar)
  • #15606 ([experimental] UTXO snapshots by jamesob)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@jnewbery

This comment has been minimized.

Copy link
Member Author

jnewbery commented Nov 14, 2019

Note that this can change the order of notifications for the wallet. If the node connects block A with conflicted txs a1 a2 and block B with conflicted txs b1 and b2 in one ActivateBestChainStep, the old ordering would be:

  • BlockConnected(A(conflicted=[a1,a2]))
  • BlockConnected(B(conflivted=[b1,b2]))

and the new ordering is:

  • TransactionRemovedFromMempool(a1)
  • TransactionRemovedFromMempool(a2)
  • TransactionRemovedFromMempool(b1)
  • TransactionRemovedFromMempool(b2)
  • BlockConnected(A)
  • BlockConnected(B)

I'm pretty sure that's not a problem. Take a look at how the wallet currently handles conflicted transactions in the BlockConnected() callback:

TransactionRemovedFromMempool(ptx);

All the TransactionRemovedFromMempool does is toggle the fInMempool flag to false.

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Nov 14, 2019

I'm pretty sure that's not a problem

Are you sure about that? Note that change that is unconfirmed and not in the mempool is not counted toward our balance, so this would open up race conditions where a send* might fail when there is no reason for it to fail.

@jnewbery

This comment has been minimized.

Copy link
Member Author

jnewbery commented Nov 15, 2019

this would open up race conditions where a send* might fail when there is no reason for it to fail.

I don't think that's true:

  • Before this change: conflicted transactions are marked as not in the mempool when the wallet hears about them in a BlockConnected's vtxConflicted vector.
  • After this change: conflicted transactions are marked as not in the mempool slightly earlier when the wallet hears about them in a TransactionRemovedFromMempool callback.

In both of those cases, the change output is unspendable because it conflicts with a transaction included in the block, but in the current code the wallet doesn't know about it until slightly later. So arguably there's a window condition in the existing code where the wallet might try to spend change that it thinks is in the mempool, but is actually conflicted with the latest block (although in practice this doesn't matter - it's always the case that there might be a better block that conflicts our transactions that we just don't know about yet).

(If this PR was changing the way we notify the wallet about transactions removed from the mempool because they're included in a block, then I'd agree with you. We don't want to introduce a window where the wallet knows the transaction is no longer in the mempool but doesn't yet know that it's in a block.)

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Nov 15, 2019

So arguably there's a window condition in the existing code where the wallet might try to spend change that it thinks is in the mempool, but is actually conflicted with the latest block (although in practice this doesn't matter - it's always the case that there might be a better block that conflicts our transactions that we just don't know about yet).

What if the user set min_conf to 1000 and rbf to false for that tx that spends the change that is only available for a short time erroneously? With those settings, the user should expect to create a transaction that could never possibly fail, no?

@jnewbery

This comment has been minimized.

Copy link
Member Author

jnewbery commented Nov 15, 2019

What if the user set min_conf to 1000 and rbf to false for that tx that spends the change that is only available for a short time erroneously? With those settings, the user should expect to create a transaction that could never possibly fail, no?

Maybe I don't fully understand the scenario you're describing, but if the tx is spending change that has been conflicted out of the mempool but the wallet has not yet been notified then that transaction will definitely fail, since it's spending an output that doesn't exist in the main chain.

Essentially, the difference for the wallet in this PR is that it'll get notified slightly earlier about transactions (and therefore possibly unconfirmed change outputs that it owns) that have been conflicted out of the mempool slightly before it is notified about the block that conflicts them. I can't think of any way that would cause windows of undesirable behaviour.

Copy link
Contributor

ariard left a comment

Concept ACK.

I've tried to implement most of my comments here : https://github.com/ariard/bitcoin/commits/2019-11-review-17477, I think it's possible to remove the whole ConnectTrace and replace by a simple typedef'ed vector.

What if the user set min_conf to 1000 and rbf to false for that tx that spends the change that is only available for a short time erroneously?

Your scenario is non-rbf tx B is spending change output from non-rbf confirmed tx A but tx B is finally conflicted and so your concern is about the window time between the conflict being connected in the validation logic and the wallet getting the info to act on it ? I think this change make it better as the window time is going to be shorted due to TransactionRemovedFromMempool triggered before BlockConnected.

* block are added via mempool callbacks prior to the BlockConnected() associated
* with those transactions. If any transactions are marked conflicted, it is
* assumed that an associated block will always be added.
*
* This class is single-use, once you call GetBlocksConnected() you have to throw
* it away and make a new one.
*/
class ConnectTrace {
private:
std::vector<PerBlockConnectTrace> blocksConnected;
CTxMemPool &pool;

This comment has been minimized.

Copy link
@ariard

ariard Nov 15, 2019

Contributor

Commit 0f17fd9. I think after this one CTxMemPool member is no more of any use as it was used by ConnecTrace to connect to mempool signal, can you remove it ?

This comment has been minimized.

Copy link
@jnewbery

jnewbery Nov 15, 2019

Author Member

done

*
* - EXPIRY (expired from mempool after -mempoolexpiry hours)
* - SIZELIMIT (removed in size limiting if the mempool exceeds -maxmempool megabytes)
* - REORG (removed during a reorg)

This comment has been minimized.

Copy link
@ariard

ariard Nov 15, 2019

Contributor

nit: "removed txn non-final anymore due to a reorg"

This comment has been minimized.

Copy link
@jnewbery

jnewbery Nov 15, 2019

Author Member

There are several other reasons a tx could be removed for REORG:

  • a spend from a coinbase output that is no longer mature (>100 confirmations)
  • a descendants of non-final and non-mature outputs.
  • if the re-org has been deep enough that the disconnect pool has filled up
  • if the standardness or consensus rules have changed across the reorg

I'd rather leave this high level than try to enumerate all the possible reasons for this here.

* block are added via mempool callbacks prior to the BlockConnected() associated
* with those transactions. If any transactions are marked conflicted, it is
* assumed that an associated block will always be added.
*

This comment has been minimized.

Copy link
@ariard

ariard Nov 15, 2019

Contributor

Commit 0f17fd9. Shoulnd't be more logic to remove both comments in previous commit (141f971) as it's the one where you effectively removed conflicted txn insertion and struct, Just looking at this commit atomically you can't make sense of "This class assumes (and asserts) that the conflicted transactions for a given block are added via mempool callbacks"

This comment has been minimized.

Copy link
@jnewbery

jnewbery Nov 15, 2019

Author Member

done

@@ -70,6 +70,7 @@
#include <boost/algorithm/string/replace.hpp>
#include <boost/algorithm/string/split.hpp>
#include <boost/thread.hpp>
#include <boost/signals2/signal.hpp>

This comment has been minimized.

Copy link
@ariard

ariard Nov 15, 2019

Contributor

super-nit: signal before thread?

This comment has been minimized.

Copy link
@jnewbery

jnewbery Nov 15, 2019

Author Member

done

@@ -698,9 +697,6 @@ class CTxMemPool

size_t DynamicMemoryUsage() const;

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

This comment has been minimized.

Copy link
@ariard

ariard Nov 15, 2019

Contributor

I think it can be noted in commit message than NotifiyEntryAdded signal didn't have any slot connected to and so was already useless

This comment has been minimized.

Copy link
@jnewbery

jnewbery Nov 15, 2019

Author Member

done

// one waiting for the transactions from the next block. We pop
// the last entry here to make sure the list we return is sane.
assert(!blocksConnected.back().pindex);
blocksConnected.pop_back();

This comment has been minimized.

Copy link
@ariard

ariard Nov 15, 2019

Contributor

Commit 793b45c. I think this comment, assert and pop should also be removed in commit 141f971 as this is the last commit were their logic make sense. Also in BlockConnected, the removed assert and emplace_back belonged to same logic.

This comment has been minimized.

Copy link
@jnewbery

jnewbery Nov 15, 2019

Author Member

I prefer to leave this as a separate commit since I think it's easier to review removing the logic piece-by-piece. If other reviewers agree with you, then I'm happy to squash the commits.

* Used to track blocks whose transactions were applied to the UTXO state as a
* part of a single ActivateBestChainStep call.
*
* This class is single-use, once you call GetBlocksConnected() you have to throw

This comment has been minimized.

Copy link
@ariard

ariard Nov 15, 2019

Contributor

Should you keep this comment ? The class is still single-use.

This comment has been minimized.

Copy link
@jnewbery

jnewbery Nov 15, 2019

Author Member

restored

@jnewbery jnewbery force-pushed the jnewbery:2019-11-remove-mempool-signals2 branch from 906b478 to 23c2276 Nov 15, 2019
@jnewbery

This comment has been minimized.

Copy link
Member Author

jnewbery commented Nov 15, 2019

@ariard - thanks for the review. I've taken all of your comment suggestions and some of the changes in your branch. I've gone back on changing the PerBlockConnectTrace to a std::pair. Using auto and std::pair's first and second members makes it difficult to work out what's being done at the call site, so I've just changed PerBlockConnectTrace to be a struct with two members and ConnectTrace to be a std::vector. Take a look and let me know what you think.

@jnewbery

This comment has been minimized.

Copy link
Member Author

jnewbery commented Nov 18, 2019

@ariard : I think it's possible to actually remove ConnectTrace entirely now that it's not necessary to release cs_main before firing validation interface signals. Take a look at the branch here: https://github.com/jnewbery/bitcoin/tree/2019-11-remove-connect-trace.

If you agree, I'll remove the last two commits from this PR, and then just remove ConnectTrace entirely in a follow-up PR.

@jnewbery jnewbery force-pushed the jnewbery:2019-11-remove-mempool-signals2 branch from 23c2276 to e028f34 Nov 18, 2019
@jnewbery jnewbery force-pushed the jnewbery:2019-11-remove-mempool-signals2 branch from e028f34 to cb155c9 Nov 22, 2019
@jnewbery

This comment has been minimized.

Copy link
Member Author

jnewbery commented Nov 22, 2019

rebased on master

@DrahtBot DrahtBot removed the Needs rebase label Nov 22, 2019
@jnewbery jnewbery force-pushed the jnewbery:2019-11-remove-mempool-signals2 branch from cb155c9 to a352662 Nov 22, 2019
@jnewbery

This comment has been minimized.

Copy link
Member Author

jnewbery commented Nov 22, 2019

I've removed the final two commits (which tidied up the ConnectTrace class) since I remove that class entirely in PR #17562.

@ariard

This comment has been minimized.

Copy link
Contributor

ariard commented Nov 25, 2019

@jnewbery - your approach is better than the one tried in my aforementioned branch, because you avoid passing back and forth a std::vector between ActivateBestChain, ActivateBestChainStep, ConnectTip. Furthermore, it's pretty straightforward on the lock reasoning, given we already do this for BlockDisconnected in DisconnectTip where we hold cs_main

@jnewbery jnewbery force-pushed the jnewbery:2019-11-remove-mempool-signals2 branch from a352662 to 227680f Nov 25, 2019
@jnewbery

This comment has been minimized.

Copy link
Member Author

jnewbery commented Nov 25, 2019

Force-pushed a fix for a typo in a commit message. No code change.

Copy link
Member

promag left a comment

Nice cleanup, concept ACK.

Looks like the first commit is the most sensible. Can't CWallet::ResendWalletTransactions -> CWalletTx::SubmitMemoryPoolAndRelay mess if called in between TransactionRemovedFromMempool and BlockConnected?

@@ -2468,35 +2468,21 @@ static int64_t nTimePostConnect = 0;
struct PerBlockConnectTrace {
CBlockIndex* pindex = nullptr;
std::shared_ptr<const CBlock> pblock;
std::shared_ptr<std::vector<CTransactionRef>> conflictedTxs;
PerBlockConnectTrace() : conflictedTxs(std::make_shared<std::vector<CTransactionRef>>()) {}
PerBlockConnectTrace() {}

This comment has been minimized.

Copy link
@promag

promag Nov 25, 2019

Member

nit, just drop the constructor?

This comment has been minimized.

Copy link
@jnewbery

jnewbery Nov 25, 2019

Author Member

I plan to remove PerBlockConnectTrace() entirely in #17562.

This comment has been minimized.

Copy link
@promag

promag Nov 26, 2019

Member

Sure, but if you happen to push again I still think you could remove this line.

Copy link
Member Author

jnewbery left a comment

Can't CWallet::ResendWalletTransactions -> CWalletTx::SubmitMemoryPoolAndRelay mess if called in between TransactionRemovedFromMempool and BlockConnected?

I'm not sure I fully understand the scenario. Can you explain further?

@promag

This comment has been minimized.

Copy link
Member

promag commented Nov 26, 2019

In master CWallet::BlockConnected is "atomic" - adds new transactions and updates transaction states and nothing can can "use" the wallet in between.

With this change "lots of stuff" can happen while the wallet state (transactions state really) is updated. So I was checking what can happen and one is:

bitcoin/src/wallet/wallet.cpp

Lines 1747 to 1748 in 0ee914b

bool ret = pwallet->chain().broadcastTransaction(tx, err_string, pwallet->m_default_max_tx_fee, relay);
fInMempool |= ret;

So it looks like a transaction can be broadcast again if this runs between TransactionRemovedFromMempool and BlockConnected. But then I've realised that BroadcastTransaction checks this first:

LOCK(cs_main);
// If the transaction is already confirmed in the chain, don't do anything
// and return early.
CCoinsViewCache &view = ::ChainstateActive().CoinsTip();
for (size_t o = 0; o < tx->vout.size(); o++) {
const Coin& existingCoin = view.AccessCoin(COutPoint(hashTx, o));
// IsSpent doesn't mean the coin is spent, it means the output doesn't exist.
// So if the output does exist, then this transaction exists in the chain.
if (!existingCoin.IsSpent()) return TransactionError::ALREADY_IN_CHAIN;
}
if (!mempool.exists(hashTx)) {

So I think there's no issue in this case.

@jnewbery

This comment has been minimized.

Copy link
Member Author

jnewbery commented Nov 26, 2019

So I think there's no issue in this case.

I agree, and I think this is similar to Marco's concern earlier and my response: #17477 (comment).

This PR allows the wallet to be used in a small window between being notified of conflicted transactions from the block and the block itself. However, I don't think it opens any new window conditions in the wallet that don't already exist. Imagine the following scenario in master branch:

  1. a wallet's transaction is removed from the mempool for expiry or size limiting reasons. The wallet is notified via TransactionRemovedFromMempool
  2. a new block gets mined that contains a transaction that conflicted with the removed transaction and the BlockConnected callback is added to the Validation Interface queue (but not fired in the wallet yet)
  3. some action happens in the wallet before the BlockConnected callback is fired.

This is exactly the same behaviour as you describe above, and can already happen in the existing code. The only difference is the mempool removal reason, which is not reported to the wallet.

The only CValidationInterface client that cares about transactions that
are removed from the mempool because of CONFLICT is the wallet.

Start using the TransactionRemovedFromMempool method to notify about
conflicted transactions instead of using the vtxConflicted vector in
BlockConnected.
jnewbery added 5 commits Nov 11, 2019
The wallet now uses TransactionRemovedFromMempool to be notified about
conflicted wallet, and no other clients use vtxConflicted.
Since we don't add a vtxConflicted vector to BlockConnected the
conflictedTxs member of PerBlockConnectTrace is no longer used.
ConnectTrace used to subscribe to the mempool's NotifyEntryRemoved
callback to be notified of transactions removed for conflict. Since
PerBlockConnectTrace no longer tracks conflicted transactions,
ConnectTrace no longer requires these notifications.
NotifyEntryAdded never had any subscribers so can be removed.

Since ConnectTrace no longer subscribes to NotifyEntryRemoved, there are
now no subscribers.

The CValidationInterface TransactionAddedToMempool and
TransactionRemovedFromMempool methods can now provide this
functionality. There's no need for a special notifications framework for
the mempool.
It's no longer used for anything.
@jnewbery

This comment has been minimized.

Copy link
Member Author

jnewbery commented Jan 10, 2020

rebased

@jnewbery jnewbery force-pushed the jnewbery:2019-11-remove-mempool-signals2 branch from 227680f to 100d5d0 Jan 10, 2020
@DrahtBot DrahtBot removed the Needs rebase label Jan 10, 2020
Copy link
Member

jonatack left a comment

ACK 100d5d0 code review, built, ran tests, ran bitcoind with mempool/validation logging, with the following reserves and to-do:

On the potential issues raised in the discussion above, these changes appear to reduce the window but I have not yet looked through all the existing test coverage to see if more relevant coverage can be added. At first glance there seems to be a fair amount on the topic in these testfiles but it would good to dig in deeper:

    git grep "double spend\|double-spend\|conflicted" -- 'test' -- 'src/test/*'
@@ -70,6 +70,7 @@
#include <boost/algorithm/string/classification.hpp>
#include <boost/algorithm/string/replace.hpp>
#include <boost/algorithm/string/split.hpp>
#include <boost/signals2/signal.hpp>

This comment has been minimized.

Copy link
@jonatack

jonatack Jan 22, 2020

Member

Would this line be better in src/ui_interface.h instead? It seems to be where it's used. Build and tests green.

@fjahr

This comment has been minimized.

Copy link
Contributor

fjahr commented Jan 23, 2020

Code review ACK 100d5d0

I spent some time thinking about the "window issue" and the worst scenario I could come up with is if there are high fees, a full mempool, and we do some automatic RBF-ing, so we replace a tx with a higher fee one immediately if our original is evicted from the Mempool because of low fees. We might try to RBF again although it is already in the block. That's the only scenario I could come up with and it is an edge case with the only bad effect of sending out an RBF tx that gets rejected from every mempool. And fairly easy to prevent on the user side as well. Additionally, developers will be mindful of the window when implementing further changes, but I think that is also not a real argument against the change as it is simplifying things overall.

@jonatack

This comment has been minimized.

Copy link
Member

jonatack commented Jan 23, 2020

FWIW if anyone is looking at tests, I reverted this change and built

--- a/src/txmempool.cpp
+++ b/src/txmempool.cpp
@@ -403,7 +403,7 @@ void CTxMemPool::addUnchecked(const CTxMemPoolEntry &entry, setEntries &setAnces
 
 void CTxMemPool::removeUnchecked(txiter it, MemPoolRemovalReason reason)
 {
-    if (reason != MemPoolRemovalReason::BLOCK) {
+    if (reason != MemPoolRemovalReason::BLOCK && reason != MemPoolRemovalReason::CONFLICT) {
         GetMainSignals().TransactionRemovedFromMempool(it->GetSharedTx());
     }

and all the unit and functional tests pass except

2020-01-23T14:46:58.142000Z TestFramework (INFO): Initializing test directory /dev/shm/bitcoin_func_test_rjrhubgl
2020-01-23T14:46:59.277000Z TestFramework (INFO): Mining blocks...
2020-01-23T14:47:04.035000Z TestFramework (INFO): Running tests
2020-01-23T14:47:16.692000Z TestFramework (ERROR): JSONRPC error
Traceback (most recent call last):
  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 112, in main
    self.run_test()
  File "test/functional/wallet_bumpfee.py", line 84, in run_test
    test_unconfirmed_not_spendable(rbf_node, rbf_node_address)
  File "test/functional/wallet_bumpfee.py", line 405, in test_unconfirmed_not_spendable
    rbf_node.abandontransaction(bumpid)
  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/coverage.py", line 47, in __call__
    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/authproxy.py", line 141, in __call__
    raise JSONRPCException(response['error'], status)
test_framework.authproxy.JSONRPCException: Transaction not eligible for abandonment (-5)
Copy link
Contributor

ryanofsky left a comment

Review club notes at https://bitcoincore.reviews/17477.html

Code review ACK 100d5d0

I'm struggling with the PR description, though. The PR description makes it sound like the reason the wallet was receiving conflicted transaction notifications through the BlockConnected signal instead of the TransactionRemovedFromMempool signal was about performance, because signals were synchronous at the time these notifications started being sent in #9371 (Jan 2017), and only later made asynchronous in #10179 (Jul 2017).

But I think the actual reason was that:

  • The TransactionRemovedFromMempool callback didn't exist yet. It was added in #10286 (Nov 2017)
  • #9371 wasn't really trying to mark conflicted transactions as not being in the mempool. The CWalletTx::fInMempool boolean didn't exist until #10286. #9371 was actually trying to fix missing -walletnotify events for these transactions. Since we stopped sending these notifications in #16624 (Sep 2019 commit 7e89994), it means this PR should be safe, not just because CWalletTx::fInMempool is still being kept up to date, but also because the original reason for sending these notifications is no longer there
@@ -406,7 +406,7 @@ void CTxMemPool::removeUnchecked(txiter it, MemPoolRemovalReason reason)
{
CTransactionRef ptx = it->GetSharedTx();
NotifyEntryRemoved(ptx, reason);
if (reason != MemPoolRemovalReason::BLOCK && reason != MemPoolRemovalReason::CONFLICT) {
if (reason != MemPoolRemovalReason::BLOCK) {

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Jan 23, 2020

Contributor

In commit "[wallet] Notify conflicted transactions in TransactionRemovedFromMempool" (3529e0b)

Could you add a comment about why notifications are skipped for BLOCK? I believe it's just for performance because BlockConnected notification provides the same information.

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Jan 31, 2020

Needs rebase
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

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