wallet: Update transactions with current mempool after load #15652

promag · 2019-03-23T10:11:07Z

promag · 2019-03-23T10:12:09Z

If it's the right fix then it needs:

update to functional test
backport to 0.17 and 0.18.

gmaxwell · 2019-03-23T10:15:22Z

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?

promag · 2019-03-23T10:21:27Z

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

laanwj · 2019-03-23T10:30:37Z

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 · 2019-03-23T10:38:07Z

Wallet is registered in CWallet::CreateWalletFromFile:

bitcoin/src/wallet/wallet.cpp

Line 4386 in 7b13c64

    
           walletInstance->m_chain_notifications_handler = chain.handleNotifications(*walletInstance);

which happens before CWallet::postInitProcess, so I think there's no race.

promag · 2019-03-23T10:43:36Z

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.

src/wallet/wallet.cpp

Github-Pull: bitcoin#15652 Rebased-From: 3cb4fb1

laanwj · 2019-03-23T10:59:26Z

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 · 2019-03-23T12:54:08Z

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

Also added a test.

maflcko · 2019-03-23T13:47:09Z

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

promag · 2019-03-23T16:57:44Z

@MarcoFalke fixed.

src/wallet/wallet.cpp

meshcollider · 2019-03-25T08:16:45Z

tested ACK 3e31d68, cannot reproduce #15591 with this fix

maflcko

Concept ACK

maflcko · 2019-03-25T13:43:09Z

src/wallet/wallet.cpp

+        // Update wallet transactions with current mempool transactions.
+        auto locked_chain = chain.lock();
+        for (const CTransactionRef& tx : locked_chain->getMemoryPoolTransactions()) {
+            wallet->TransactionAddedToMempool(tx);


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.

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

re: #15652 (comment)

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:

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

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

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

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

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

re: #15652 (comment)

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.

src/wallet/wallet.cpp

DrahtBot · 2019-03-25T16:21:15Z

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.

src/interfaces/chain.cpp

src/interfaces/chain.h

src/interfaces/chain.cpp

This is just a code simplification and revert of 0440481 from bitcoin#15652. No behavior is changing.

fanquake added Wallet Needs backport labels Mar 23, 2019

fanquake added this to the 0.18.0 milestone Mar 23, 2019

promag mentioned this pull request Mar 23, 2019

Sporadic feature_segwit.py failure #15653

Closed

promag commented Mar 23, 2019

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

promag added a commit to promag/bitcoin that referenced this pull request Mar 23, 2019

wallet: Update transactions with current mempool after load

3e60cbd

Github-Pull: bitcoin#15652 Rebased-From: 3cb4fb1

promag force-pushed the 2019-03-fix-15591 branch from 363b383 to a617ab3 Compare March 23, 2019 12:51

maflcko modified the milestones: 0.18.0, 0.17.2 Mar 23, 2019

promag force-pushed the 2019-03-fix-15591 branch from a617ab3 to 3f70633 Compare March 23, 2019 14:20

maflcko reviewed Mar 23, 2019

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

promag force-pushed the 2019-03-fix-15591 branch from 3f70633 to 3e31d68 Compare March 23, 2019 23:06

maflcko reviewed Mar 25, 2019

View reviewed changes

promag force-pushed the 2019-03-fix-15591 branch from 3e31d68 to 12fb550 Compare March 25, 2019 16:03

maflcko reviewed Mar 25, 2019

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

maflcko reviewed Mar 25, 2019

View reviewed changes

src/interfaces/chain.cpp Show resolved Hide resolved

ryanofsky reviewed Mar 25, 2019

View reviewed changes

src/interfaces/chain.h Outdated Show resolved Hide resolved

src/interfaces/chain.cpp Show resolved Hide resolved

promag force-pushed the 2019-03-fix-15591 branch from 12fb550 to 863a26d Compare March 25, 2019 19:01

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Aug 17, 2020

refactor: Move cs_wallet to ReacceptWalletTransactions

7b2acad

This is just a code simplification and revert of 0440481 from bitcoin#15652. No behavior is changing.

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Sep 2, 2020

refactor: Move cs_wallet to ReacceptWalletTransactions

450f460

This is just a code simplification and revert of 0440481 from bitcoin#15652. No behavior is changing.

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Sep 2, 2020

refactor: Move cs_wallet to ReacceptWalletTransactions

072e1d1

This is just a code simplification and revert of 0440481 from bitcoin#15652. No behavior is changing.

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Sep 25, 2020

refactor: Move cs_wallet to ReacceptWalletTransactions

a308c4a

This is just a code simplification and revert of 0440481 from bitcoin#15652. No behavior is changing.

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Sep 25, 2020

refactor: Move cs_wallet to ReacceptWalletTransactions

5328fe9

This is just a code simplification and revert of 0440481 from bitcoin#15652. No behavior is changing.

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Apr 11, 2021

refactor: Move cs_wallet to ReacceptWalletTransactions

8aa64ef

This is just a code simplification and revert of 0440481 from bitcoin#15652. No behavior is changing.

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Apr 11, 2021

refactor: Move cs_wallet to ReacceptWalletTransactions

10b1d5a

This is just a code simplification and revert of 0440481 from bitcoin#15652. No behavior is changing.

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request May 19, 2021

refactor: Move cs_wallet to ReacceptWalletTransactions

d4fbbfb

This is just a code simplification and revert of 0440481 from bitcoin#15652. No behavior is changing.

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request May 19, 2021

refactor: Move cs_wallet to ReacceptWalletTransactions

d36335f

This is just a code simplification and revert of 0440481 from bitcoin#15652. No behavior is changing.

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Jun 14, 2021

refactor: Move cs_wallet to ReacceptWalletTransactions

6a37052

This is just a code simplification and revert of 0440481 from bitcoin#15652. No behavior is changing.

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Jun 14, 2021

refactor: Move cs_wallet to ReacceptWalletTransactions

09b282d

This is just a code simplification and revert of 0440481 from bitcoin#15652. No behavior is changing.

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Jun 14, 2021

refactor: Move cs_wallet to ReacceptWalletTransactions

4c2532c

This is just a code simplification and revert of 0440481 from bitcoin#15652. No behavior is changing.

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Jun 15, 2021

refactor: Move cs_wallet to ReacceptWalletTransactions

cfe2009

This is just a code simplification and revert of 0440481 from bitcoin#15652. No behavior is changing.

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Aug 19, 2021

refactor: Move cs_wallet to ReacceptWalletTransactions

de6fa30

This is just a code simplification and revert of 0440481 from bitcoin#15652. No behavior is changing.

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Aug 19, 2021

refactor: Move cs_wallet to ReacceptWalletTransactions

591dd97

This is just a code simplification and revert of 0440481 from bitcoin#15652. No behavior is changing.

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Sep 3, 2021

refactor: Move cs_wallet to ReacceptWalletTransactions

aa80ff5

This is just a code simplification and revert of 0440481 from bitcoin#15652. No behavior is changing.

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Sep 3, 2021

refactor: Move cs_wallet to ReacceptWalletTransactions

793f403

This is just a code simplification and revert of 0440481 from bitcoin#15652. No behavior is changing.

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Oct 5, 2021

refactor: Move cs_wallet to ReacceptWalletTransactions

a4fefa3

This is just a code simplification and revert of 0440481 from bitcoin#15652. No behavior is changing.

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Oct 5, 2021

refactor: Move cs_wallet to ReacceptWalletTransactions

74821ed

This is just a code simplification and revert of 0440481 from bitcoin#15652. No behavior is changing.

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Oct 13, 2021

refactor: Move cs_wallet to ReacceptWalletTransactions

99802fa

This is just a code simplification and revert of 0440481 from bitcoin#15652. No behavior is changing.

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Oct 13, 2021

refactor: Move cs_wallet to ReacceptWalletTransactions

24dfff6

This is just a code simplification and revert of 0440481 from bitcoin#15652. No behavior is changing.

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Nov 29, 2021

refactor: Move cs_wallet to ReacceptWalletTransactions

f8297e9

This is just a code simplification and revert of 0440481 from bitcoin#15652. No behavior is changing.

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Nov 29, 2021

refactor: Move cs_wallet to ReacceptWalletTransactions

2b898a0

This is just a code simplification and revert of 0440481 from bitcoin#15652. No behavior is changing.

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Nov 29, 2021

refactor: Move cs_wallet to ReacceptWalletTransactions

a26a270

This is just a code simplification and revert of 0440481 from bitcoin#15652. No behavior is changing.

kwvg added a commit to kwvg/dash that referenced this pull request Dec 4, 2021

merge bitcoin#15652: Update transactions with current mempool after load

fd4c1f6

kwvg added a commit to kwvg/dash that referenced this pull request Dec 12, 2021

merge bitcoin#15652: Update transactions with current mempool after load

9a42e03

kwvg added a commit to kwvg/dash that referenced this pull request Dec 13, 2021

merge bitcoin#15652: Update transactions with current mempool after load

78c8cec

kwvg added a commit to kwvg/dash that referenced this pull request Dec 13, 2021

merge bitcoin#15652: Update transactions with current mempool after load

d468124

kwvg added a commit to kwvg/dash that referenced this pull request Dec 13, 2021

merge bitcoin#15652: Update transactions with current mempool after load

7cf7736

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

wallet: Update transactions with current mempool after load #15652

wallet: Update transactions with current mempool after load #15652

Uh oh!

Conversation

promag commented Mar 23, 2019

Uh oh!

promag commented Mar 23, 2019

Uh oh!

gmaxwell commented Mar 23, 2019

Uh oh!

promag commented Mar 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Mar 23, 2019

Uh oh!

promag commented Mar 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Mar 23, 2019

Uh oh!

Uh oh!

laanwj commented Mar 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Mar 23, 2019

Uh oh!

maflcko commented Mar 23, 2019

Uh oh!

promag commented Mar 23, 2019

Uh oh!

Uh oh!

meshcollider commented Mar 25, 2019

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

DrahtBot commented Mar 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

promag commented Mar 23, 2019 •

edited

Loading

promag commented Mar 23, 2019 •

edited

Loading

laanwj commented Mar 23, 2019 •

edited

Loading

DrahtBot commented Mar 25, 2019 •

edited

Loading