wallet: Update transactions with current mempool after load

promag commented at 10:11 am on March 23, 2019: member

Fixes #15591.

promag commented at 10:12 am on March 23, 2019: member

If it’s the right fix then it needs:

update to functional test
backport to 0.17 and 0.18.

gmaxwell commented at 10:15 am on March 23, 2019: contributor

Do we also need to think about how this interacts with restoring the mempool from disk? e.g. is there a race condition where some transactions being loaded would be missed?

fanquake added the label Wallet on Mar 23, 2019

fanquake added the label Needs backport on Mar 23, 2019

fanquake added this to the milestone 0.18.0 on Mar 23, 2019

promag commented at 10:21 am on March 23, 2019: member

IIUC if the mempool is loaded after postInitProcess then TransactionAddedToMempool is already called.

laanwj commented at 10:30 am on March 23, 2019: member

Ordering checks out:

CWallet::postInitProcess() is called first: current mempool will be used, wallet will be notified when transactions added to mempool in LoadMempool()
LoadMempool() called first: CWallet::postInitProcess() will process all the transactions

As for concurrency:

CWallet::postInitProcess() locks the mempool for the duration of loading transactions from it. LoadMempool() locks the mempool for every individual transaction inserted. This means that it is posible for CWallet::postInitProcess() to be called while the mempool is being loaded. If it manages to get the lock, it will process the current transactions in the mempool, potentially partially complete.

Is the wallet already registered at this point (before CWallet::postInitProcess()) for receiving TransactionAddedToMempool events? If so, there is no race, but it might get some transactions multiple time (I think that’s harmless?), if not, it will start receiving transactions again after registering and miss some in that timeframe.

promag commented at 10:38 am on March 23, 2019: member

Wallet is registered in CWallet::CreateWalletFromFile: https://github.com/bitcoin/bitcoin/blob/7b13c646457980f44599412f243694fa682a1abf/src/wallet/wallet.cpp#L4386 which happens before CWallet::postInitProcess, so I think there’s no race.

promag commented at 10:43 am on March 23, 2019: member

but it might get some transactions multiple time (I think that’s harmless?)

I also think that trying to improve that is not worth the pain/complexity. But it may cost a bit for large wallets and a large mempool. I’m happy to followup something to improve that.

in src/wallet/wallet.cpp:4406 in 3cb4fb15bf outdated

4400@@ -4401,6 +4401,12 @@ void CWallet::postInitProcess()
4401     // Add wallet transactions that aren't already in a block to mempool
4402     // Do this here as mempool requires genesis block to be loaded
4403     ReacceptWalletTransactions();
4404+
4405+    // Update wallet transactions with current mempool transactions.
4406+    LOCK(::mempool.cs);

promag commented at 10:51 am on March 23, 2019:

nit, this lock is not at the begin of a scope, could move this to a new CWallet member.

promag referenced this in commit 3e60cbd07b on Mar 23, 2019

laanwj commented at 10:59 am on March 23, 2019: member

I also think that trying to improve that is not worth the pain/complexity. But it may cost a bit for large wallets and a large mempool. I’m happy to followup something to improve that.

I agree (it’s not worth handling that explicitly), just wanted to cover all possible scenarios.

promag force-pushed on Mar 23, 2019

promag commented at 12:54 pm on March 23, 2019: member

Moved the fix to LoadWallet(), which is used to dynamically load wallets - and so the startup behavior isn’t changed.

Also added a test.

MarcoFalke commented at 1:47 pm on March 23, 2019: member

0 node1 2019-03-23T13:08:34.742866Z POTENTIAL DEADLOCK DETECTED 
1 node1 2019-03-23T13:08:34.742874Z Previous lock order was: 
2 node1 2019-03-23T13:08:34.742883Z  (1) ::mempool.cs  wallet/wallet.cpp:152 
3 node1 2019-03-23T13:08:34.742898Z  (2) cs_main  interfaces/chain.cpp:264 
4 node1 2019-03-23T13:08:34.742912Z Current lock order is: 
5 node1 2019-03-23T13:08:34.742921Z  (2) cs_main  init.cpp:1475 
6 node1 2019-03-23T13:08:34.742934Z  (2) cs_main  validation.cpp:4300 
7 node1 2019-03-23T13:08:34.742947Z  (1) cs  txmempool.cpp:593

MarcoFalke removed this from the milestone 0.18.0 on Mar 23, 2019

MarcoFalke added this to the milestone 0.17.2 on Mar 23, 2019

promag force-pushed on Mar 23, 2019

promag commented at 4:57 pm on March 23, 2019: member

@MarcoFalke fixed.

in src/wallet/wallet.cpp:153 in 3f70633a88 outdated

145@@ -146,6 +146,15 @@ std::shared_ptr<CWallet> LoadWallet(interfaces::Chain& chain, const WalletLocati
146     }
147     AddWallet(wallet);
148     wallet->postInitProcess();
149+
150+    {
151+        // Update wallet transactions with current mempool transactions.
152+        LOCK2(cs_main, ::mempool.cs);
153+        for (const CTxMemPoolEntry& entry : ::mempool.mapTx) {

MarcoFalke commented at 8:00 pm on March 23, 2019:

The wallet module is not allowed to call directly into the node. This should be an interface method.

promag commented at 11:11 pm on March 23, 2019:

Right, added 51cb2eb and then refactored LoadWallet to use it.

promag force-pushed on Mar 23, 2019

meshcollider commented at 8:16 am on March 25, 2019: contributor

tested ACK https://github.com/bitcoin/bitcoin/pull/15652/commits/3e31d680c55101c476a4a3db488cd29fb3dbea23, cannot reproduce #15591 with this fix

in src/wallet/wallet.cpp:154 in 3e31d680c5 outdated

145@@ -146,6 +146,15 @@ std::shared_ptr<CWallet> LoadWallet(interfaces::Chain& chain, const WalletLocati
146     }
147     AddWallet(wallet);
148     wallet->postInitProcess();
149+
150+    {
151+        // Update wallet transactions with current mempool transactions.
152+        auto locked_chain = chain.lock();
153+        for (const CTransactionRef& tx : locked_chain->getMemoryPoolTransactions()) {
154+            wallet->TransactionAddedToMempool(tx);

MarcoFalke commented at 1:43 pm on March 25, 2019:

Could there be a race, since we don’t take cs_main when removing txs from the mempool?

I.e. a tx is first removed due to eviction, the wallet is notified and then it is re-added here.

promag commented at 2:11 pm on March 25, 2019:

Indeed, between getMemoryPoolTransactions and TransactionAddedToMempool a tx can be evicted.

I see 2 solutions:

lock the wallet before the loop so that TransactionRemovedFromMempool comes after this
add a way to lock the mempool in interfaces:: but not sure if this is wanted? cc @ryanofsky

ryanofsky commented at 2:42 pm on March 25, 2019:

re: #15652 (review)

I see 2 solutions:

I think more ideally the wallet doesn’t have knowledge or control of node locks, and can just register for notifications and handle them as they come in. Maybe the ChainImpl::handleNotifications method can be changed to something like:

0std::unique_ptr<Handler> handleNotifications(Notifications& notifications) override
1{
2    LOCK2(::cs_main, ::mempool.cs);
3    for (const CTxMemPoolEntry& entry : ::mempool.mapTx) {
4        notifications.TransactionAddedToMempool(entry.GetSharedTx());
5    }
6    return MakeUnique<NotificationsHandlerImpl>(*this, notifications);
7}

MarcoFalke commented at 3:18 pm on March 25, 2019:

If you move the loop into postInitProcess, you can just take a LOCK(::mempool.cs) in there

promag commented at 3:30 pm on March 25, 2019:

@MarcoFalke postInitProcess is wallet module, should not use that right? #15652 (review)

MarcoFalke commented at 3:46 pm on March 25, 2019:

Yeah, you’d have to move it to the interface

Alternatively you could add it to handleNotifications as suggested by @ryanofsky, but then it would also be called for createwallet, which seems a bit wasteful

promag commented at 3:49 pm on March 25, 2019:

@ryanofsky nice, I thought of something like replayMemoryPoolNotifications. However I’m not sure if these notifications should come after ReacceptWalletTransactions?

ryanofsky commented at 5:38 pm on March 25, 2019:

re: #15652 (review)

Alternatively you could add it to handleNotifications as suggested by @ryanofsky, but then it would also be called for createwallet, which seems a bit wasteful

Agree, this would be too wasteful. Having the separate method to replay notifications is probably better.

I’m not sure if these notifications should come after ReacceptWalletTransactions?

I’m not 100% sure, but I don’t think I see a problem if notifications about what’s already in the mempool happen before the wallet adds more transactions to the mempool. I think what you have now basically seems good though.

in src/wallet/wallet.cpp:148 in 3e31d680c5 outdated

145@@ -146,6 +146,15 @@ std::shared_ptr<CWallet> LoadWallet(interfaces::Chain& chain, const WalletLocati
146     }
147     AddWallet(wallet);
148     wallet->postInitProcess();

MarcoFalke commented at 1:46 pm on March 25, 2019:

Any reason not to do this in postInitProcess?

I understand that this is also called when the createwallet rpc is called, but the post-init seems useless in that case anyway.

I’d suggest:

Remove postInitProcess from createwallet, because the wallet is fresh and there couldn’t be any txs that need to be exchanged between mempool and wallet
Rename postInitProcess to postLoadProcess, since it is not needed on init, but on load
Move the below code into postLoadProcess with mempool::cs hold while adding the txs.

promag commented at 2:07 pm on March 25, 2019:

I agree, postInitProcess is “pre dynamic wallet support”. However I’d prefer to refactor in a different PR, this is a bug fix.

MarcoFalke commented at 3:16 pm on March 25, 2019:

Fine with doing the rename et al. later, but is there any reason not to put it in postInitProcess? IIRC it was in there previously in this pull

promag commented at 3:29 pm on March 25, 2019:

Yes, could be moved back there.

MarcoFalke commented at 1:47 pm on March 25, 2019: member

Concept ACK

promag force-pushed on Mar 25, 2019

in src/wallet/wallet.cpp:149 in 596f91d207 outdated

145@@ -146,6 +146,7 @@ std::shared_ptr<CWallet> LoadWallet(interfaces::Chain& chain, const WalletLocati
146     }
147     AddWallet(wallet);
148     wallet->postInitProcess();
149+

MarcoFalke commented at 4:07 pm on March 25, 2019:

in commit 596f91d20732a1714395e9f6e5d1e6c41f14a2e5:

Could remove empty hunk to avoid having to solve conflicts in a backport?

DrahtBot commented at 4:21 pm on March 25, 2019: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#15713 (refactor: Replace chain relayTransactions/submitMemoryPool by higher method by ariard)
#15639 (bitcoin-wallet tool: Drop libbitcoin_server.a dependency by ryanofsky)
#15632 (Remove ResendWalletTransactions from the Validation Interface by jnewbery)

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.

in src/interfaces/chain.cpp:372 in 12fb550b08 outdated

366@@ -367,6 +367,13 @@ class ChainImpl : public Chain
367     {
368         return MakeUnique<RpcHandlerImpl>(command);
369     }
370+    void notifyMemoryPoolTransactions(Notifications& notifications) override
371+    {
372+        LOCK2(::cs_main, ::mempool.cs);

MarcoFalke commented at 5:20 pm on March 25, 2019:

Not sure how to solve the deadlock warning. Maybe take cs_wallet before both of these?

 0node1 2019-03-25T17:06:10.586193Z Leaving InitialBlockDownload (latching to false) 
 1 node1 2019-03-25T17:06:10.586301Z POTENTIAL DEADLOCK DETECTED 
 2 node1 2019-03-25T17:06:10.586314Z Previous lock order was: 
 3 node1 2019-03-25T17:06:10.586326Z  ::cs_main  interfaces/chain.cpp:372 
 4 node1 2019-03-25T17:06:10.586335Z  (1) ::mempool.cs  interfaces/chain.cpp:372 
 5 node1 2019-03-25T17:06:10.586350Z  cs_main  interfaces/chain.cpp:264 
 6 node1 2019-03-25T17:06:10.586359Z  (2) cs_wallet  wallet/wallet.cpp:1230 
 7 node1 2019-03-25T17:06:10.586386Z Current lock order is: 
 8 node1 2019-03-25T17:06:10.586397Z  cs_main  interfaces/chain.cpp:264 
 9 node1 2019-03-25T17:06:10.586406Z  (2) cs_wallet  wallet/wallet.cpp:1871 
10 node1 2019-03-25T17:06:10.586419Z  (1) pool.cs  validation.cpp:580

ryanofsky commented at 5:29 pm on March 25, 2019:

re: #15652 (review)

Not sure how to solve the deadlock warning. Maybe take cs_wallet before both of these?

It looks like you need to do the opposite and release cs_wallet before calling this.

MarcoFalke commented at 6:10 pm on March 25, 2019:

The lock order is cs_main, cs_wallet, ::mempool.cs, no?

MarcoFalke commented at 6:21 pm on March 25, 2019:

I can get the functional tests to pass with the diff

 0diff --git a/src/interfaces/chain.cpp b/src/interfaces/chain.cpp
 1index cb76461647..94e7a7ad53 100644
 2--- a/src/interfaces/chain.cpp
 3+++ b/src/interfaces/chain.cpp
 4@@ -367,9 +367,9 @@ public:
 5     {
 6         return MakeUnique<RpcHandlerImpl>(command);
 7     }
 8-    void notifyMemoryPoolTransactions(Notifications& notifications) override
 9+    void notifyMemoryPoolTransactions(Notifications& notifications, RecursiveMutex&cs_wallet) override
10     {
11-        LOCK2(::cs_main, ::mempool.cs);
12+        LOCK2(::cs_main, cs_wallet);         LOCK(::mempool.cs);
13         for (const CTxMemPoolEntry& entry : ::mempool.mapTx) {
14             notifications.TransactionAddedToMempool(entry.GetSharedTx());
15         }
16diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h
17index 3fa63d83be..b53d170645 100644
18--- a/src/interfaces/chain.h
19+++ b/src/interfaces/chain.h
20@@ -7,6 +7,7 @@
21 
22 #include <optional.h>               // For Optional and nullopt
23 #include <primitives/transaction.h> // For CTransactionRef
24+#include <sync.h> // For RecursiveMutex
25 
26 #include <memory>
27 #include <stddef.h>
28@@ -271,7 +272,7 @@ public:
29     virtual std::unique_ptr<Handler> handleRpc(const CRPCCommand& command) = 0;
30 
31     //! Notify handler of all memory pool transactions.
32-    virtual void notifyMemoryPoolTransactions(Notifications& notifications) = 0;
33+    virtual void notifyMemoryPoolTransactions(Notifications& notifications,RecursiveMutex&cs_wallet) = 0;
34 };
35 
36 //! Interface to let node manage chain clients (wallets, or maybe tools for
37diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
38index 554ee62547..d72201679a 100644
39--- a/src/wallet/wallet.cpp
40+++ b/src/wallet/wallet.cpp
41@@ -4404,7 +4404,7 @@ void CWallet::postInitProcess()
42     ReacceptWalletTransactions();
43 
44     // Update wallet transactions with current mempool transactions.
45-    m_chain.notifyMemoryPoolTransactions(*this);
46+    m_chain.notifyMemoryPoolTransactions(*this, cs_wallet);
47 }
48 
49 bool CWallet::BackupWallet(const std::string& strDest)

in src/interfaces/chain.h:274 in 12fb550b08 outdated

268@@ -269,6 +269,9 @@ class Chain
269     //! Register handler for RPC. Command is not copied, so reference
270     //! needs to remain valid until Handler is disconnected.
271     virtual std::unique_ptr<Handler> handleRpc(const CRPCCommand& command) = 0;
272+
273+    //! Notify handler of all memory pool transactions.
274+    virtual void notifyMemoryPoolTransactions(Notifications& notifications) = 0;

ryanofsky commented at 5:21 pm on March 25, 2019:

Could you call this notifyMempoolTransactions? I’d like to converge on mempool spelling rather than memPool or memoryPool. Currently we have:

so the only existing exception is the Chain::Lock method, which I want to go away anyway.

promag commented at 6:36 pm on March 25, 2019:

Will do.

promag force-pushed on Mar 25, 2019

promag commented at 8:06 pm on March 25, 2019: member

Updated, moved notifyMempoolTransactions to where the locks are held in the right order - ReacceptWalletTransactions. Needs squash though.

BTW, could rename ReacceptWalletTransactions to SyncWithChain or something like that.

Sjors commented at 3:45 pm on March 26, 2019: member

818b127 makes the new test pass on macOS. Don’t forget to rename the “wip” commit.

MarcoFalke commented at 4:41 pm on March 26, 2019: member

Please squash to make it ready for review

promag force-pushed on Mar 26, 2019

in src/wallet/wallet.cpp:1895 in fea282767f outdated

1889@@ -1890,6 +1890,9 @@ void CWallet::ReacceptWalletTransactions()
1890         CValidationState state;
1891         wtx.AcceptToMemoryPool(*locked_chain, state);
1892     }
1893+
1894+    // Update wallet transactions with current mempool transactions.
1895+    chain().notifyMempoolTransactions(*this);

jnewbery commented at 7:21 pm on March 26, 2019:

I don’t like this going in ReacceptWalletTransactions() for a couple of reasons:

The name ReacceptWalletTransactions() suggests to me that we’re reaccepting wallet transactions back into the mempool (which is what the function currently does). Adding functionality to fetch mempool txs back into the wallet changes the definition of this function so that it no longer matches the name.
ReacceptWalletTransactions() is called by various import* RPCs, which don’t need to refetch the mempool txs into the wallet.

I suggest moving this call into postInitProcessing() which is only called when a wallet is loaded.

promag commented at 0:39 am on March 27, 2019:

Fixed.

promag commented at 1:04 am on March 27, 2019:

I suggest moving this call into postInitProcessing() which is only called when a wallet is loaded.

This was attempted before but there was a locking order problem, and was for that reason that I’ve placed notifyMempoolTransactions here. I’ve included 0994e3121 to address that.

The important thing to ensure is #15652 (review).

in src/interfaces/chain.h:274 in fea282767f outdated

268@@ -269,6 +269,9 @@ class Chain
269     //! Register handler for RPC. Command is not copied, so reference
270     //! needs to remain valid until Handler is disconnected.
271     virtual std::unique_ptr<Handler> handleRpc(const CRPCCommand& command) = 0;
272+
273+    //! Notify handler of all memory pool transactions.
274+    virtual void notifyMempoolTransactions(Notifications& notifications) = 0;

jnewbery commented at 7:22 pm on March 26, 2019:

nit: I’d prefer the name getMempoolTransactions() or fetchMempoolTransactions(). Notify to me signifies that the wallet is subscribing and getting a notification, whereas this interface function is actually used by the wallet to actively fetch the mempool transactions.

promag commented at 0:40 am on March 27, 2019:

I’d prefer the name getMempoolTransactions() or fetchMempoolTransactions().

I think these are confusing since the function is void.

promag commented at 0:57 am on March 27, 2019:

Notify to me signifies that the wallet is subscribing and getting a notification

The “standard” for that is handleX, like handleNotifications.

ryanofsky commented at 4:15 pm on March 27, 2019:

re: #15652 (review)

Agree with John the name is not great. I think I’d probably call it pushMempoolNotifications.

More importantly, though, I think this needs to have a comment saying how it relates to handleNotifications. A good comment might be:

“Synchronously send TransactionAddedToMempool notifications about all current mempool entries to the specified handler. To avoid race conditions where out of date TransactionAddedFromMempool / TransactionRemovedFromMempool asynchronous notifications previously requested through handleNotifications would arrive during or after this call, this will block waiting for any previous asynchronous notifications to complete, and prevent new asynchronous notifications from being sent until this returns.”

I will say writing this makes me think the interface is overcomplicated. I guess I’d prefer my original suggestion of just sending TransactionAddedFromMempool notifications atomically in the same place the handler is registered: #15652 (review). Marco pointed out this would be wasteful in the case of createwallet calls #15652 (review), but the notifications could be optional if the waste is too much.

jnewbery commented at 4:22 pm on March 27, 2019:

requestMempoolTransactions()?

Agree with Russ about adding a comment.

promag commented at 4:39 pm on March 27, 2019:

just sending TransactionAddedFromMempool notifications atomically in the same place the handler is registered

I understand it is cleaner but it is weird to receive notifications before handleNotifications return. Thanks for the comment!

Eeny, meeny, miny, moe requestMempoolTransactions, after all we want to be notified of mempool transactions.

ryanofsky commented at 5:27 pm on March 27, 2019:

but it is weird to receive notifications before handleNotifications return

This is the current behavior and it isn’t weird. Notifications start to arrive when RegisterValidationInterface is called, and can happen before that function returns, much less before handleNotifications returns.

promag commented at 6:28 pm on March 27, 2019:

This is the current behavior

Currently cs_main and cs_wallet are locked when handleNotifications is called so that doesn’t happen, but otherwise you are right.

jnewbery commented at 7:23 pm on March 26, 2019: member

Great stuff @promag . Thanks for this, and for adding a test.

I have a couple of small suggestions inline, which I’ve implemented here: https://github.com/jnewbery/bitcoin/tree/pr15652.0

promag force-pushed on Mar 27, 2019

MarcoFalke commented at 1:31 pm on March 27, 2019: member

utACK 3bf22a78c2e9951a7e7febb20577f2b6ac178bfc

wallet: Move CWallet::ReacceptWalletTransactions locks to callers 0440481c6b

promag force-pushed on Mar 27, 2019

jnewbery commented at 5:24 pm on March 27, 2019: member

utACK 78ce8e91031ea8a87c55deeb060071d7dc26358e

ryanofsky commented at 5:36 pm on March 27, 2019: member

I think there’s probably still a race condition here because add/remove notifications scheduled before the request for mempool transactions could arrive after the request:

https://github.com/bitcoin/bitcoin/blob/656a15e5394d8d841619b5b186fa0f9c8be0322b/src/validationinterface.cpp#L155

https://github.com/bitcoin/bitcoin/blob/656a15e5394d8d841619b5b186fa0f9c8be0322b/src/validationinterface.cpp#L138

You might need to block waiting for previous notifications.

promag force-pushed on Mar 28, 2019

in src/interfaces/chain.cpp:370 in fae225a9f6 outdated

366@@ -367,6 +367,13 @@ class ChainImpl : public Chain
367     {
368         return MakeUnique<RpcHandlerImpl>(command);
369     }
370+    void requestMempoolTransactions(Notifications& notifications) override

ryanofsky commented at 11:52 am on March 28, 2019:

re: #15652 (comment)

You might need to block waiting for previous notifications.

One way to do this:

 0void requestMempoolTransactions(Notifications& notifications) override
 1{
 2    std::promise<void> promise;
 3    CallFunctionInValidationInterfaceQueue([&] {
 4        LOCK2(::cs_main, ::mempool.cs);
 5        for (const CTxMemPoolEntry& entry : ::mempool.mapTx) {
 6            notifications.TransactionAddedToMempool(entry.GetSharedTx());
 7        }
 8        promise.set_value();
 9    });
10    promise.get_future().wait();
11}

ryanofsky commented at 12:17 pm on March 28, 2019:

Previous suggestion in https://github.com/bitcoin/bitcoin/pull/15652/files#r269965497 isn’t exception safe. Also, cs_main needs to be released before calling this.

Probably better:

 0void requestMempoolTransactions(Notifications& notifications) override
 1{
 2    AssertLockNotHeld(::cs_main);
 3    AssertLockNotHeld(::mempool.cs);
 4    std::packaged_task<void()> task([&] {
 5        LOCK2(::cs_main, ::mempool.cs);
 6        for (const CTxMemPoolEntry& entry : ::mempool.mapTx) {
 7            notifications.TransactionAddedToMempool(entry.GetSharedTx());
 8        }
 9    });
10    CallFunctionInValidationInterfaceQueue([&] { task(); });
11    task.get_future().get();
12}

promag commented at 2:56 pm on March 28, 2019:

Thanks @ryanofsky!

promag commented at 3:07 pm on March 28, 2019:

Haven’t finished but it looks like there will be a lock order issue - cs_main -> mempool.cs -> cs_main -> cs_wallet.

ryanofsky commented at 3:30 pm on March 28, 2019:

re: #15652 (review)

Haven’t finished but it looks like there will be a lock order issue - cs_main -> mempool.cs -> cs_main -> cs_wallet.

That does seem like a problem. One way to fix it would be to release main and mempool locks before calling TransactionAddedToMempool, by pushing the transactions to a vector, releasing the locks and then looping over the vector.

Alternately if you changed the function to pushMempoolTransactions(std::function<void(std::vector<CTransactionRef> push_fn)) you could release the locks and then send the vector to the wallet directly. Both of these changes would be nice optimizations for #10102, since passing a vector once across the socket is more efficient than calling TransactionAddedToMempool repeatedly and requiring lots of roundtrip traffic.

ryanofsky commented at 3:29 pm on March 29, 2019:

re: #15652 (review)

Current code looks ok, but I think I was misguided in making this thread because previous code in d94847f20c750d88a0aefa572155d54f8e39c144 was also ok. I think:

I was right to point out there’s a race condition where normal mempool add/remove notifications triggered before the requestMempoolTransactions call could arrive out of order during and after the call
I was wrong to assume that the race condition would actually cause a problem. There is no problem if an unknown transaction is removed or an already added transaction is added again.
I was wrong to think that my proposed changes would actually prevent add/remove notifications from arriving out of order. In fact, they would only prevent older add/remove notifications from arriving during the requestMempoolTransactions call, not after the call.

So, I think if you want go back to the simpler implementation in d94847f20c750d88a0aefa572155d54f8e39c144 that would be fine.

Or if you did want to prevent obsolete add/remove notifications from arriving after the requestMempoolTransactions call in addition to during, you’d need acquire the locks before calling CallFunctionInValidationInterfaceQueue and also fill the vector at the same time:

 0void requestMempoolTransactions(Notifications& notifications) override
 1{
 2    std::vector<CTransactionRef> transactions;
 3    std::packaged_task<void()> task([&] {
 4        for (const CTransactionRef& tx : transactions) {
 5            notifications.TransactionAddedToMempool(tx);
 6        }
 7    });
 8    {
 9        LOCK2(::cs_main, ::mempool.cs);
10        for (const CTxMemPoolEntry& entry : ::mempool.mapTx) {
11            transactions.push_back(entry.GetSharedTx());
12        }
13        CallFunctionInValidationInterfaceQueue([&] { task(); });
14    }
15    AssertLockNotHeld(::cs_main);
16    AssertLockNotHeld(::mempool.cs);
17    task.get_future().get();
18}

promag commented at 5:03 pm on March 29, 2019:

I was wrong to assume that the race condition would actually cause a problem. There is no problem if an unknown transaction is removed or an already added transaction is added again.

Right, just unnecessary extra work, which is not an issue I think.

promag force-pushed on Mar 28, 2019

ryanofsky commented at 3:42 pm on March 28, 2019: member

Sorry for the churn and for keeping suggesting new code changes. If you want to go with a simple change that fixes the original bug and can be backported, but maybe is less efficient or has some obscure race conditions, that seems fine.

laanwj removed this from the milestone 0.17.2 on Mar 28, 2019

laanwj added this to the milestone 0.18.0 on Mar 28, 2019

promag force-pushed on Mar 28, 2019

promag commented at 9:38 am on March 29, 2019: member

@ryanofsky no problem at all! I think this is ready now.

ryanofsky approved

ryanofsky commented at 3:51 pm on March 29, 2019: member

utACK 3c3dfe8fde151070272f3fea70cd27150ffd1c03

MarcoFalke commented at 4:12 pm on March 29, 2019: member

If the branch that had d94847f in works without a future include and is correct, I’d prefer that

promag force-pushed on Mar 29, 2019

ryanofsky commented at 5:03 pm on March 29, 2019: member

If the branch that had d94847f in works without a future include and is correct, I’d prefer that

Branch is correct but the requestMempoolTransactions comment there doesn’t accurately describe what the function does. Comment could just be updated to warn that requestMempoolTransactions is not synchronized with handleNotifications, and obsolete handleNotifications notifications could arrive during and after notifications sent by requestMempoolTransactions (but that this is harmless because wallet will ignore notifications about removed transactions it doesn’t know about or added transactions that are already added).

MarcoFalke commented at 7:51 pm on March 29, 2019: member

re-utACK 78ce8e91031ea8a87c55deeb060071d7dc26358e

Only change since my last review is renaming a method and adding a comment. (Haven’t read the comment)

jnewbery commented at 8:23 pm on March 29, 2019: member

utACK 78ce8e91031ea8a87c55deeb060071d7dc26358e modulo the comment change that Russ requested.

MarcoFalke commented at 9:37 pm on March 29, 2019: member

Tested that 78ce8e91031ea8a87c55deeb060071d7dc26358e fails the test when run without compiling the bitcoind changes.

in src/interfaces/chain.h:273 in d94847f20c outdated

268@@ -269,6 +269,16 @@ class Chain
269     //! Register handler for RPC. Command is not copied, so reference
270     //! needs to remain valid until Handler is disconnected.
271     virtual std::unique_ptr<Handler> handleRpc(const CRPCCommand& command) = 0;
272+
273+    //! Synchronously send TransactionAddedToMempool notifications about all

ryanofsky commented at 0:39 am on March 30, 2019:

In commit “interfaces: Add Chain::requestMempoolTransactions” (d94847f20c750d88a0aefa572155d54f8e39c144)

Current comment isn’t really accurate. Maybe suggest:

0//! Synchronously send TransactionAddedToMempool notifications about all
1//! current mempool transactions to the specified handler and return after
2//! the last one is sent. These notifications aren't coordinated with async
3//! notifications sent by handleNotifications, so out of date async
4//! notifications from handleNotifications can arrive during and after
5//! synchronous notifications from requestMempoolTransactions. Clients need
6//! to be prepared to handle this by ignoring notifications about unknown
7//! removed transactions and already added new transactions.

ryanofsky approved

ryanofsky commented at 1:03 am on March 30, 2019: member

utACK 78ce8e91031ea8a87c55deeb060071d7dc26358e. Lot of things I don’t like about this PR but this seems fine as a fix, and thanks for considering all my suggestions.

In my opinion the nearest-best fix was https://github.com/bitcoin/bitcoin/commits/3c3dfe8fde151070272f3fea70cd27150ffd1c03, because it didn’t require new postInitProcess / ReacceptWalletTransactions locking added in this version of the PR. The new locking is more complicated than it first appears, because it relies on locking cs_main recursively, and may be deadlock-prone because it sometimes is and sometimes isn’t holding the cs_main lock when sending TransactionAdded notifications. (I would prefer to never be holding cs_main when sending any notification, which would have been the case after #15632 with the other version of this PR.)

In case anyone wants to clean this up in the future, I think the ideal solution would be to combine https://github.com/bitcoin/bitcoin/commits/3c3dfe8fde151070272f3fea70cd27150ffd1c03 with perfectly ordered cs_main free notifications implemented in #15652 (review), and also dropping the requestMempoolTransactions function entirely, just replacing it with an optional new mempool parameter to handleNotifications (optional callback function taking a std::vector<CTransactionRef> argument).

promag force-pushed on Mar 31, 2019

interfaces: Add Chain::requestMempoolTransactions 57908a739c

wallet: Update transactions with current mempool after load 2ebf650b2e

qa: Check unconfirmed balance after loadwallet 4bf1b1cefa

promag force-pushed on Mar 31, 2019

ryanofsky approved

ryanofsky commented at 3:53 pm on April 1, 2019: member

utACK 4bf1b1cefa9723bf2cfa8b1a938757abc99bb17b. Only change is code comment in second commit.

MarcoFalke commented at 4:33 pm on April 1, 2019: member

re-utACK 4bf1b1cefa

Only the comment changed, didn’t look at either the old or new comment.

jnewbery commented at 6:58 pm on April 1, 2019: member

utACK 4bf1b1cefa9723bf2cfa8b1a938757abc99bb17b

MarcoFalke merged this on Apr 1, 2019

MarcoFalke closed this on Apr 1, 2019

MarcoFalke referenced this in commit 5a2a9b5b06 on Apr 1, 2019

ryanofsky commented at 7:47 pm on April 1, 2019: member

Merge commit 5a2a9b5b0603c1206e4419ffd0fd0d4939813fc2 seems to be missing an ack: #15652#pullrequestreview-221199549

MarcoFalke referenced this in commit ebf65666c2 on Apr 1, 2019

promag deleted the branch on Apr 1, 2019

MarcoFalke commented at 8:33 pm on April 1, 2019: member

This is an upstream issue on GitHub, I believe. Will follow up with some details shortly

MarcoFalke referenced this in commit ed0498af28 on Apr 1, 2019

MarcoFalke referenced this in commit 59716ec395 on Apr 1, 2019

MarcoFalke referenced this in commit 95faffed26 on Apr 1, 2019

MarcoFalke commented at 9:09 pm on April 1, 2019: member

Comments:

https://api.github.com/repos/bitcoin/bitcoin/issues/15652/comments

Review summary comments:

https://api.github.com/repos/bitcoin/bitcoin/pulls/15652/reviews

They are showing my “Concept ACK” above, but for some reason not your utACK…

MarcoFalke commented at 10:23 pm on April 1, 2019: member

Will follow up with GitHub support

fanquake removed the label Needs backport on Apr 2, 2019

fanquake commented at 12:05 pm on April 2, 2019: member

Backported in #15691.

HashUnlimited referenced this in commit 5a4448013c on Apr 19, 2019

HashUnlimited referenced this in commit 5c7b220940 on Apr 19, 2019

HashUnlimited referenced this in commit 9de16b771b on Apr 19, 2019

HashUnlimited referenced this in commit 724b6b619d on Apr 19, 2019

ryanofsky referenced this in commit de030e6258 on May 12, 2020

ryanofsky referenced this in commit d1ae53126b on May 14, 2020

deadalnix referenced this in commit dc54947740 on May 25, 2020

deadalnix referenced this in commit aed5f059b9 on May 25, 2020

deadalnix referenced this in commit b1b46a3606 on May 25, 2020

deadalnix referenced this in commit 51aed4714a on May 25, 2020

ryanofsky referenced this in commit 9ffa362248 on Jun 1, 2020

ryanofsky referenced this in commit 44db893458 on Jun 1, 2020

ryanofsky referenced this in commit 57108199fc on Jun 5, 2020

ryanofsky referenced this in commit 26c1c69b8a on Jun 5, 2020

ryanofsky referenced this in commit fce30e67a1 on Jun 5, 2020

ryanofsky referenced this in commit 9c3e8fc6ca on Jun 5, 2020

ryanofsky referenced this in commit ac5d2bc6a6 on Jul 10, 2020

ryanofsky referenced this in commit ae7850a71d on Jul 11, 2020

ryanofsky referenced this in commit efd102cf37 on Jul 11, 2020

ryanofsky referenced this in commit 6dee0e4ea2 on Jul 13, 2020

ryanofsky referenced this in commit ad835cbcd6 on Jul 14, 2020

ryanofsky referenced this in commit 4c8d4c0919 on Aug 7, 2020

ryanofsky referenced this in commit 68cd2f23e7 on Aug 7, 2020

ryanofsky referenced this in commit 9eab04972b on Aug 17, 2020

ryanofsky referenced this in commit 7b2acad460 on Aug 17, 2020

ryanofsky referenced this in commit 450f4600cd on Sep 2, 2020

ryanofsky referenced this in commit 072e1d1756 on Sep 2, 2020

ryanofsky referenced this in commit a308c4abb9 on Sep 25, 2020

ryanofsky referenced this in commit 5328fe9623 on Sep 25, 2020

ryanofsky referenced this in commit 8aa64ef7f5 on Apr 11, 2021

ryanofsky referenced this in commit 10b1d5a6d5 on Apr 11, 2021

ryanofsky referenced this in commit d4fbbfb311 on May 19, 2021

ryanofsky referenced this in commit d36335f643 on May 19, 2021

ryanofsky referenced this in commit 6a37052140 on Jun 14, 2021

ryanofsky referenced this in commit 09b282d92e on Jun 14, 2021

ryanofsky referenced this in commit 4c2532cf31 on Jun 14, 2021

ryanofsky referenced this in commit cfe2009ff8 on Jun 15, 2021

ryanofsky referenced this in commit de6fa30a81 on Aug 19, 2021

ryanofsky referenced this in commit 591dd974dc on Aug 19, 2021

ryanofsky referenced this in commit aa80ff5876 on Sep 3, 2021

ryanofsky referenced this in commit 793f403611 on Sep 3, 2021

ryanofsky referenced this in commit a4fefa3ca5 on Oct 5, 2021

ryanofsky referenced this in commit 74821eda66 on Oct 5, 2021

ryanofsky referenced this in commit 99802facb4 on Oct 13, 2021

ryanofsky referenced this in commit 24dfff610e on Oct 13, 2021

ryanofsky referenced this in commit f8297e93e3 on Nov 29, 2021

ryanofsky referenced this in commit 2b898a0456 on Nov 29, 2021

ryanofsky referenced this in commit a26a270d1a on Nov 29, 2021

kittywhiskers referenced this in commit fd4c1f6d85 on Dec 4, 2021

kittywhiskers referenced this in commit 9a42e037a5 on Dec 12, 2021

kittywhiskers referenced this in commit 78c8cec3cb on Dec 13, 2021

kittywhiskers referenced this in commit d4681249b6 on Dec 13, 2021

kittywhiskers referenced this in commit 7cf77367da on Dec 13, 2021

DrahtBot locked this on Dec 16, 2021

wallet: Update transactions with current mempool after load #15652

Conflicts