wallet: Update transactions with current mempool after load by promag · Pull Request #15652 · bitcoin/bitcoin

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: 3cb4fb15bf0f97a9b40388254d8dc4c7c99868e8

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

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

Github-Pull: bitcoin#15652 Rebased-From: 4bf1b1c

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

…ks to callers Summary: bitcoin/bitcoin@0440481 --- Partial backport of Core [[bitcoin/bitcoin#15652 | PR15652]] Test Plan: ninja check Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D6245

Summary: bitcoin/bitcoin@57908a7 --- Depends on D6245 Partial backport of Core [[bitcoin/bitcoin#15652 | PR15652]] Test Plan: ninja Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D6246

…er load Summary: bitcoin/bitcoin@2ebf650 --- Depends on D6246 Partial backport of Core [[bitcoin/bitcoin#15652 | PR15652]] Test Plan: ninja check Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D6247

Summary: bitcoin/bitcoin@4bf1b1c --- Depends on D6247 Concludes backport of Core [[bitcoin/bitcoin#15652 | PR15652]] Test Plan: ./test/functional/test_runner.py wallet_balance Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D6248

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: 3cb4fb15bf0f97a9b40388254d8dc4c7c99868e8

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 Outdated 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 Outdated Show resolved Hide resolved

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

HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this pull request Apr 19, 2019

qa: Check unconfirmed balance after loadwallet

724b6b6

Github-Pull: bitcoin#15652 Rebased-From: 4bf1b1c

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request May 12, 2020

refactor: Move cs_wallet to ReacceptWalletTransactions

de030e6

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 14, 2020

refactor: Move cs_wallet to ReacceptWalletTransactions

d1ae531

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 1, 2020

refactor: Move cs_wallet to ReacceptWalletTransactions

9ffa362

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 1, 2020

refactor: Move cs_wallet to ReacceptWalletTransactions

44db893

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 5, 2020

refactor: Move cs_wallet to ReacceptWalletTransactions

5710819

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 5, 2020

refactor: Move cs_wallet to ReacceptWalletTransactions

26c1c69

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 5, 2020

refactor: Move cs_wallet to ReacceptWalletTransactions

fce30e6

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 5, 2020

refactor: Move cs_wallet to ReacceptWalletTransactions

9c3e8fc

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 Jul 10, 2020

refactor: Move cs_wallet to ReacceptWalletTransactions

ac5d2bc

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 Jul 11, 2020

refactor: Move cs_wallet to ReacceptWalletTransactions

ae7850a

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 Jul 11, 2020

refactor: Move cs_wallet to ReacceptWalletTransactions

efd102c

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 Jul 13, 2020

refactor: Move cs_wallet to ReacceptWalletTransactions

6dee0e4

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 Jul 14, 2020

refactor: Move cs_wallet to ReacceptWalletTransactions

ad835cb

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 7, 2020

refactor: Move cs_wallet to ReacceptWalletTransactions

4c8d4c0

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 7, 2020

refactor: Move cs_wallet to ReacceptWalletTransactions

68cd2f2

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 17, 2020

refactor: Move cs_wallet to ReacceptWalletTransactions

9eab049

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

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

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