Wallet passive startup #15719

ryanofsky · 2019-04-01T21:35:48Z

Move wallet startup code closer to a simple model where the wallet attaches to the chain with a single chain.handleNotifications() call, and just starts passively receiving blocks and mempool notifications from the last update point, instead having to actively rescan blocks and request a mempool snapshot, and deal with the tip changing, and deal with early or stale notifications.

Also, stop locking the cs_wallet mutex and registering for validationinterface notifications before the rescan. This was new behavior since 6a72f26 #16426 and is not ideal because it stops other wallets and rpcs and the gui from receiving new notifications until after the scan completes.

This change is a half-step towards implementing multiwallet scans (#11756), since it provides needed locator and birthday timestamp information to the Chain interface, and it rationalizes locking and event ordering in the startup code. The second half of implementing parallel rescans requires moving the ScanForWalletTransactions implementation (which this PR does not touch) from the wallet to the node.

DrahtBot · 2019-04-01T22:01:09Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#23581 (Move BlockManager to node/blockstorage by MarcoFalke)
#23417 (wallet, spkm: Move key management from DescriptorScriptPubKeyMan to wallet level KeyManager by achow101)
#22341 (rpc: add getxpub by Sjors)
#21603 (log: Mitigate disk filling attacks by rate limiting LogPrintf by dergoegge)
#20205 (wallet: Properly support a wallet id by achow101)
#18554 (wallet: ensure wallet files are not reused across chains by rodentrabies)
#16673 (Relog configuration args on debug.log rotation by LarryRuane)

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.

promag · 2019-04-01T22:09:58Z

How will snapshot_fn work with IPC?

ryanofsky · 2019-04-02T00:37:50Z

re: #15719 (comment)

How will snapshot_fn work with IPC?

No different than other callback arguments. In #10102, std::function arguments are passed as objects with a single call() method:

https://github.com/ryanofsky/bitcoin/blob/3440513a45e94a5f1f0c67ab7409439afbbef673/src/interfaces/capnp/messages.capnp#L144-L154

https://github.com/ryanofsky/bitcoin/blob/3440513a45e94a5f1f0c67ab7409439afbbef673/src/interfaces/capnp/messages.capnp#L236-L239

src/interfaces/chain.cpp

promag · 2019-04-02T12:24:57Z

@ryanofsky ok thanks, will look into that.

promag · 2019-04-02T13:05:07Z

src/interfaces/chain.cpp

+        std::vector<CTransactionRef> snapshot;
+        std::packaged_task<std::unique_ptr<Handler>()> task([&] {
+            if (snapshot_fn) snapshot_fn(std::move(snapshot));
+            return MakeUnique<NotificationsHandlerImpl>(*this, notifications);


Mempool can change after the snapshot but before registering the interface and so those changes will be missed.

re: #15719 (comment)

Mempool can change after the snapshot but before registering the interface and so those changes will be missed.

This isn't true, and the comment below, "Hold locks while scheduling the task so notifications about added and removed transactions after the snapshot arrive after the snapshot" is specifically referring to this case.

Let me know if I should make it more clear, but as long as the locks are held, transactions can't be added / removed by other threads, and as long as CallFunctionInValidationInterfaceQueue is called before transactions are added/removed, the handler will be registered before the queued notifications signal it.

But the queue can be non empty when CallFunctionInValidationInterfaceQueue is called?

re: #15719 (comment)

But the queue can be non empty when CallFunctionInValidationInterfaceQueue is called?

I think you are referring to a different problem now (problem of old notifications being received when they shouldn't be, which is different from the problem of new notifications not being received when they should be). The problem of old notifications being received when they shouldn't be is what happens without this PR in the current code:

bitcoin/src/interfaces/chain.h

Lines 273 to 281 in 2c364fd

//! Synchronously send TransactionAddedToMempool notifications about all

//! current mempool transactions to the specified handler and return after

//! the last one is sent. These notifications aren't coordinated with async

//! notifications sent by handleNotifications, so out of date async

//! notifications from handleNotifications can arrive during and after

//! synchronous notifications from requestMempoolTransactions. Clients need

//! to be prepared to handle this by ignoring notifications about unknown

//! removed transactions and already added new transactions.

virtual void requestMempoolTransactions(Notifications& notifications) = 0;

The problem is fixed by this PR by not constructing NotificationsHandlerImpl until after snapshot_fn is called. The comment there "to first send the snapshot before enabling subsequent notifications" is about this.

AddToProcessQueue tricked me, what really goes to that queue are notifications.

promag · 2019-04-02T17:12:10Z

src/interfaces/chain.cpp

+        std::vector<CTransactionRef> snapshot;
+        std::packaged_task<std::unique_ptr<Handler>()> task([&] {
+            if (snapshot_fn) snapshot_fn(std::move(snapshot));
+            return MakeUnique<NotificationsHandlerImpl>(*this, notifications);


AddToProcessQueue tricked me, what really goes to that queue are notifications.

promag · 2019-04-02T17:16:02Z

src/interfaces/chain.cpp

+
+        // Snapshot mempool transactions, then schedule the task to execute.
+        // Hold locks while scheduling the task so notifications about added and
+        // removed transactions after the snapshot arrive after the snapshot.


also "arrive after task() is called" right?

re: #15719 (comment)

also "arrive after task() is called" right?

Yes task() is what sends the snapshot, so that's what this is referring to. Replaced "transactions after the snapshot arrive after the snapshot" with "transactions after taking the snapshot arrive after the snapshot callback in task()" to reference it and be more specific.

promag · 2019-04-02T20:07:37Z

Should we assert cs_main is held in the relevant validation interface notifications?

ryanofsky · 2019-04-02T20:24:25Z

re: #15719 (comment)

Should we assert cs_main is held in the relevant validation interface notifications?

cs_main can be acquired inside notification callbacks, if necessary, but it is not held when these notifications happen. This PR and #15632 disentangle notifications and locking, so locks are never held when notifications are received (right now they are inconsistently held in some cases but not others).

Removing locking from notifications is a step toward eventually removing Chain::Lock completely, and having the wallet just asynchronously process notifications without being able to hold on to cs_main.

promag · 2019-04-03T13:35:00Z

Sorry for not being clear. I mean that cs_main is locked when GetMainSignals().TransactionAddedToMempool() is called, and it is important that doesn't change, by adding:

@@ -152,6 +152,8 @@ void CMainSignals::UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockInd
 }

 void CMainSignals::TransactionAddedToMempool(const CTransactionRef &ptx) {
+    AssertLockHeld(::cs_main);
+    AssertLockHeld(::mempool.cs);
     m_internals->m_schedulerClient.AddToProcessQueue([ptx, this] {
         m_internals->TransactionAddedToMempool(ptx);
     });

jnewbery · 2019-04-11T21:00:12Z

Concept ACK

Mempool transactions are sent in a single IPC call instead of in a loop calling an IPC method repeatedly.

Do we know what this does to memory usage if there's a very large mempool (either at startup when starting one or multiple wallets, or when loading a wallet dynamically during runtime)?

ryanofsky · 2019-04-11T21:49:12Z

Do we know what this does to memory usage if there's a very large mempool (either at startup when starting one or multiple wallets, or when loading a wallet dynamically during runtime)?

With this PR by itself, a new temporary vector with one pointer per mempool transaction is created when a wallet is loaded. It's something to consider, but probably not likely to cause problems.

With this PR combined with #10102 and running bitcoin-node with a wallet loading in a different process, this will use significantly more memory, because serializing the vector serializes all the transactions at once in memory before sending them. This might be about as expensive as a getrawmempool call. If this is a problem, the snapshot_fn callback could be tweaked to send the transactions in batches. I guess I wouldn't want to try to optimize this prematurely, though.

Since I did write this PR "Improves performance over IPC" in the description, the increased memory usage might factor into that. But to be sure, the motivation for this PR is mainly to make notifications show up cleanly and in order on the client side.

src/wallet/wallet.cpp

jnewbery · 2019-04-12T14:36:43Z

src/wallet/wallet.cpp

@@ -4213,6 +4215,17 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain,
    // Try to top up keypool. No-op if the wallet is locked.
    walletInstance->TopUpKeyPool();

+    // Register with the validation interface. Skip requesting mempool transactions if wallet is empty.


I think moving the sync-from-mempool function to above the initial rescan breaks the (undocumented) expectation that the wallet is notified of transactions in dependency order. In fact, this was slightly broken by #15652, but this PR makes it worse.

Prior to dynamically-loaded wallets, the wallet would always receive a transaction after it had received all of that transaction's ancestors. That wallet relies on that expectation:

CWallet::IsFromMe() will only match on a transaction if its parent is already in the wallet.

nTimeSmart should be monotonically increasing for transaction descendency. If transactions arrive in the wrong order then this is no longer true, and they'll be shown out-of-order in the GUI.

Since #10740, a wallet could receive mempool transactions out-of-order (since a newly loaded wallet would not be notified of transactions in the mempool, but could be notified of their descendant transactions when they arrive). #15652 improved the situtation by notifying the wallet of all mempool transactions, but didn't sort them first, so a wallet could still be notified of the mempool txs in the wrong order. This PR potentially makes things worse by notifying the wallet of mempool txs before the blocks which could contain the mempool txs' ancestors.

I think to fix this, we need to:

move the sync-from-mempool to after the rescan call, while cs_main is still held

sort the mempool txs by using mempool.infoAll() before notifying the wallet.

We should also document that the wallet expects to see transactions in order!

I also wonder if there could brokenness in the case where a reorg happens during a rescanblockchain or importmulti RPC call (these rescans are different because cs_main isn't held the whole time).

re: #15719 (comment)

I think to fix this, we need to:

move the sync-from-mempool to after the rescan call, while cs_main is still held

sort the mempool txs by using mempool.infoAll() before notifying the wallet.

We should also document that the wallet expects to see transactions in order!

Moving sync is done. Rescanning happens before syncing from mempool in the current version of this PR. Sorting mempool isn't done yet. So this will still be as broken as it has been since #15652, but not more broken. There's already a lot going on here, so I think I would want to address this in a different PR. It seems pretty easy to fix independently of this PR, but writing a test case could be a challenge

maflcko · 2019-08-16T13:56:25Z

@ryanofsky Are you still working on this? If no, then please close it.

ryanofsky · 2019-08-19T15:35:29Z

@ryanofsky Are you still working on this? If no, then please close it.

Thanks for the reminder. I looked into this again and I think the WIP tag still applies. I had been indecisive about involving rescan in this this change after John pointed out it was necessary, since I was unsure if it was better to incrementally change the rescan code (first synchronize rescan with notifications, then consolidate rescans across wallets https://gist.github.com/ariard/89f9bcc3a7ab9576fc6d15d251032cfa), or to just do both things in a single PR.

But now I think it should be possible to synchronize rescan with notifications in a way that doesn't change existing code too much and should also make ariard's proposed change easier.

maflcko · 2021-10-27T10:09:50Z

test/sanitizer_suppressions/tsan

+#
+# Uses of std::cout are guaranteed thread safe by the c++ standard
+# https://stackoverflow.com/questions/50322790/thread-safety-and-piping-to-streams
+race:std::__1::ios_base::width


Can remove this after #23370

Allow retrieving locator from Chain findBlock methods, used in upcoming commit by the wallet to get the locator for the chain tip on startup.

Prevents log callbacks from deadlocking if they call code containing log statements. In most cases it will not make sense to call logging code from a log hook, because it could trigger and endless chain of log messages. But if a hook is conditioned on the content of messages, like the DebugLogHelper hook, it can make sense and be useful. The new functionality is used in the CreateWalletFromFile test in an upcoming commit to be able to create transactions and blocks at specific points during test execution and check for race conditions.

Make AttachChain just responsible for syncing to the chain, moving error handling and chain assignment to the caller before more syncing functionality is moved in the next commit. Also, start using the AttachChain method in tests instead of trying attach in a more partial way using Chain::handleNotifications.

Move wallet startup code closer to a simple model where the wallet attaches to the chain with a single chain.handleNotifications() call, and just starts passively receiving blocks and mempool notifications from the last update point, instead having to actively rescan blocks and request a mempool snapshot, and deal with the tip changing, and deal with early or stale notifications. Also, stop locking the cs_wallet mutex and registering for validationinterface notifications before the rescan. This was new behavior since 6a72f26 bitcoin#16426 and is not ideal because it stops other wallets and rpcs and the gui from receiving new notifications until after the scan completes. This change is a half-step towards implementing multiwallet parallel scans (bitcoin#11756), since it provides needed locator and birthday timestamp information to the Chain interface, and it rationalizes locking and event ordering in the startup code. The second half of implementing parallel rescans requires moving the ScanForWalletTransactions implementation (which this PR does not touch) from the wallet to the node.

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

DrahtBot · 2021-12-03T08:40:22Z

🐙 This pull request conflicts with the target branch and needs rebase.

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}

practicalswift reviewed Apr 2, 2019

View reviewed changes

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

laanwj added the Mempool label Apr 2, 2019

promag reviewed Apr 2, 2019

View reviewed changes

ryanofsky force-pushed the pr/pool branch from 10f92ce to 5f593ab Compare April 2, 2019 15:09

promag reviewed Apr 2, 2019

View reviewed changes

ryanofsky force-pushed the pr/pool branch from 5f593ab to 977bf7b Compare April 2, 2019 18:10

DrahtBot added the Needs rebase label Apr 9, 2019

ryanofsky force-pushed the pr/pool branch from 977bf7b to 2ef48b7 Compare April 10, 2019 01:10

DrahtBot removed the Needs rebase label Apr 10, 2019

ryanofsky force-pushed the pr/pool branch from 2ef48b7 to 0ebc74e Compare April 11, 2019 10:03

ryanofsky commented Apr 12, 2019

View reviewed changes

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

jnewbery reviewed Apr 12, 2019

View reviewed changes

ryanofsky changed the title ~~Drop Chain::requestMempoolTransactions method~~ WIP: Drop Chain::requestMempoolTransactions method Apr 17, 2019

DrahtBot added the Needs rebase label Apr 17, 2019

ryanofsky mentioned this pull request May 30, 2019

gui: Enable console line edit on setClientModel #16122

Merged

ryanofsky mentioned this pull request Jan 22, 2020

wallet: Remove calls to Chain::Lock methods #17954

Merged

ryanofsky force-pushed the pr/pool branch 2 times, most recently from 532cc1b to a075d47 Compare January 27, 2020 22:37

ryanofsky changed the title ~~WIP: Drop Chain::requestMempoolTransactions method~~ Drop Chain::requestMempoolTransactions method Jan 27, 2020

DrahtBot mentioned this pull request Sep 30, 2021

rpc, wallet: Add listaddresses RPC #23019

Closed

DrahtBot added the Needs rebase label Sep 30, 2021

ryanofsky force-pushed the pr/pool branch from 793f403 to 74821ed Compare October 5, 2021 17:36

DrahtBot removed the Needs rebase label Oct 5, 2021

DrahtBot added the Needs rebase label Oct 13, 2021

ryanofsky force-pushed the pr/pool branch from 74821ed to 24dfff6 Compare October 13, 2021 12:36

DrahtBot removed the Needs rebase label Oct 13, 2021

ryanofsky mentioned this pull request Oct 27, 2021

CI failure: ThreadSanitizer: data race /usr/lib/llvm-12/bin/../include/c++/v1/ios:523:12 in std::__1::ios_base::width() const #23366

Open

maflcko reviewed Oct 27, 2021

View reviewed changes

This was referenced Nov 3, 2021

wallet, spkm: Move key management from DescriptorScriptPubKeyMan to wallet level KeyManager #23417

Closed

rpc: add path to gethdkey #22341

Closed

This was referenced Nov 16, 2021

policy: Treat taproot as always active #23512

Merged

rpc, wallet: addhdseed, infer seed when importing descriptor with xpub #23544

Closed

DrahtBot mentioned this pull request Nov 24, 2021

Move BlockManager to node/blockstorage #23581

Merged

DrahtBot added the Needs rebase label Nov 25, 2021

ryanofsky added 5 commits November 29, 2021 11:20

lib: Add FoundBlock locator support

a1c9b87

Allow retrieving locator from Chain findBlock methods, used in upcoming commit by the wallet to get the locator for the chain tip on startup.

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 force-pushed the pr/pool branch from 24dfff6 to 2b898a0 Compare November 29, 2021 21:11

DrahtBot removed the Needs rebase label Nov 29, 2021

DrahtBot added the Needs rebase label Dec 3, 2021

ryanofsky mentioned this pull request Dec 9, 2021

Make rescans faster #23727

Closed

4 tasks

ryanofsky mentioned this pull request Feb 1, 2022

indexes: Stop using node internal types and locking cs_main, improve sync logic #24230

Draft

ryanofsky mentioned this pull request Mar 7, 2022

Automatic wallet rescan skipped after abort #24487

Closed

maxim85200 approved these changes Mar 13, 2022

View reviewed changes

achow101 closed this Oct 12, 2022

bitcoin locked and limited conversation to collaborators Oct 12, 2023

	//! Synchronously send TransactionAddedToMempool notifications about all
	//! current mempool transactions to the specified handler and return after
	//! the last one is sent. These notifications aren't coordinated with async
	//! notifications sent by handleNotifications, so out of date async
	//! notifications from handleNotifications can arrive during and after
	//! synchronous notifications from requestMempoolTransactions. Clients need
	//! to be prepared to handle this by ignoring notifications about unknown
	//! removed transactions and already added new transactions.
	virtual void requestMempoolTransactions(Notifications& notifications) = 0;

Wallet passive startup #15719

Wallet passive startup #15719

Uh oh!

Conversation

ryanofsky commented Apr 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

promag commented Apr 1, 2019

Uh oh!

ryanofsky commented Apr 2, 2019

Uh oh!

Uh oh!

promag commented Apr 2, 2019

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!

promag commented Apr 2, 2019

Uh oh!

ryanofsky commented Apr 2, 2019

Uh oh!

promag commented Apr 3, 2019

Uh oh!

jnewbery commented Apr 11, 2019

Uh oh!

ryanofsky commented Apr 11, 2019

Uh oh!

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!

maflcko commented Aug 16, 2019

Uh oh!

ryanofsky commented Aug 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Dec 3, 2021

Uh oh!

Uh oh!

ryanofsky commented Apr 1, 2019 •

edited

Loading

DrahtBot commented Apr 1, 2019 •

edited

Loading