When the wallet disconnects chain notifications, it is expecting no further notifications to execute, but this is not the case. This results in test failures such as in #34354. Instead of disconnecting the notifications and continuing shutdown, we should wait for the validation interface queue to be drained before the rest of wallet shutdown. This is achieved by adding an interfaces::Chain::waitForNotifications() function which calls SyncWithValidationInterfaceQueue().
Fixes #34354
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | furszy, stickies-v, rkrux, sedited |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
exportwatchonlywallet RPC to export a watchonly version of a wallet by achow101)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.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
1071 | @@ -1072,6 +1072,9 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati 1072 | //! Find the private key for the given key id from the wallet's descriptors, if available 1073 | //! Returns nullopt when no descriptor has the key or if the wallet is locked. 1074 | std::optional<CKey> GetKey(const CKeyID& keyid) const; 1075 | + 1076 | + //! Disconnect chain notifications and wait for validation interfrace queue to drain
typo nit
//! Disconnect chain notifications and wait for validation interface queue to drain
Done
25 | @@ -26,6 +26,9 @@ class Handler 26 | 27 | //! Disconnect the handler. 28 | virtual void disconnect() = 0; 29 | + 30 | + //! Block until all notifications have been handled 31 | + virtual void sync() = 0;
Adding a virtual sync method that's no-op for all but one of its implementations doesn't seem ideal. Would adding a Chain::waitForNotifications method be a workable alternative? E.g.:
<details> <summary>git diff on 563ec53d54</summary>
diff --git a/src/common/interfaces.cpp b/src/common/interfaces.cpp
index 1c1a6f3203..ffd85e6131 100644
--- a/src/common/interfaces.cpp
+++ b/src/common/interfaces.cpp
@@ -17,7 +17,6 @@ public:
explicit CleanupHandler(std::function<void()> cleanup) : m_cleanup(std::move(cleanup)) {}
~CleanupHandler() override { if (!m_cleanup) return; m_cleanup(); m_cleanup = nullptr; }
void disconnect() override { if (!m_cleanup) return; m_cleanup(); m_cleanup = nullptr; }
- void sync() override {}
std::function<void()> m_cleanup;
};
@@ -27,7 +26,6 @@ public:
explicit SignalHandler(boost::signals2::connection connection) : m_connection(std::move(connection)) {}
void disconnect() override { m_connection.disconnect(); }
- void sync() override {}
boost::signals2::scoped_connection m_connection;
};
diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h
index e6847b9b16..a0f288aee6 100644
--- a/src/interfaces/chain.h
+++ b/src/interfaces/chain.h
@@ -332,6 +332,9 @@ public:
//! chain tip.
virtual void waitForNotificationsIfTipChanged(const uint256& old_tip) = 0;
+ //! Wait for all pending notifications to be processed.
+ virtual void waitForNotifications() = 0;
+
//! Register handler for RPC. Command is not copied, so reference
//! needs to remain valid until Handler is disconnected.
virtual std::unique_ptr<Handler> handleRpc(const CRPCCommand& command) = 0;
diff --git a/src/interfaces/handler.h b/src/interfaces/handler.h
index 6b8484f80f..b3aebf7e14 100644
--- a/src/interfaces/handler.h
+++ b/src/interfaces/handler.h
@@ -26,9 +26,6 @@ public:
//! Disconnect the handler.
virtual void disconnect() = 0;
-
- //! Block until all notifications have been handled
- virtual void sync() = 0;
};
//! Return handler wrapping a boost signal connection.
diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp
index 3b2a726d10..2cc30ff519 100644
--- a/src/node/interfaces.cpp
+++ b/src/node/interfaces.cpp
@@ -499,10 +499,6 @@ public:
m_proxy.reset();
}
}
- void sync() override
- {
- m_signals.SyncWithValidationInterfaceQueue();
- }
ValidationSignals& m_signals;
std::shared_ptr<NotificationsProxy> m_proxy;
};
@@ -539,8 +535,6 @@ public:
::tableRPC.removeCommand(m_command.name, &m_command);
}
}
- void sync() final {}
-
~RpcHandlerImpl() override { disconnect(); }
CRPCCommand m_command;
@@ -790,6 +784,10 @@ public:
if (!old_tip.IsNull() && old_tip == WITH_LOCK(::cs_main, return chainman().ActiveChain().Tip()->GetBlockHash())) return;
validation_signals().SyncWithValidationInterfaceQueue();
}
+ void waitForNotifications() override
+ {
+ validation_signals().SyncWithValidationInterfaceQueue();
+ }
std::unique_ptr<Handler> handleRpc(const CRPCCommand& command) override
{
return std::make_unique<RpcHandlerImpl>(command);
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 9e918599d5..1e7ad3fc6b 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -4582,7 +4582,7 @@ void CWallet::DisconnectChainNotifications()
{
if (m_chain_notifications_handler) {
m_chain_notifications_handler->disconnect();
- m_chain_notifications_handler->sync();
+ chain().waitForNotifications();
m_chain_notifications_handler.reset();
}
}
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
index b90668e64c..e6823c2441 100644
--- a/src/wallet/wallet.h
+++ b/src/wallet/wallet.h
@@ -1073,7 +1073,7 @@ public:
//! Returns nullopt when no descriptor has the key or if the wallet is locked.
std::optional<CKey> GetKey(const CKeyID& keyid) const;
- //! Disconnect chain notifications and wait for validation interfrace queue to drain
+ //! Disconnect chain notifications and wait for validation interface queue to drain
void DisconnectChainNotifications();
};
</details>
Taken the suggestion
Concept ACK
The current approach has a downside during IBD, any wallet unload during IBD could stall for hours because notification will keep coming. This wouldn't be much a deal for an RPC thread but for the GUI, this would totally freeze the app.
What we can do instead, is to unregister the wallet from the validation interface and submit a blocking task to the interface, so that we only wait up until the pending notifications gets cleared, and nothing else. Then we continue shutdown etc.
I implemented it here https://github.com/furszy/bitcoin-core/commit/bf1abcd6d50374a461dad305f0243d3a613ce158. All yours (haven't tested it).
If we want more concurrency in the future, instead of waiting for the queue to drain, we can submit the remaining unload steps into the validation interface so they are executed only after the pending tasks get processed. I didn't do it because it would require further changes that don't think they are acceptable for the release freeze period.
submit a blocking task to the interface, so that we only wait up until the pending notifications gets cleared, and nothing else. Then we continue shutdown etc.
Isn't that what SyncWithValidationInterfaceQueue does? It also calls CallFunctionInValidationInterfaceQueue, but with std::promise, and then it wait()s on that future.
Isn't that what
SyncWithValidationInterfaceQueuedoes? It also callsCallFunctionInValidationInterfaceQueue, but withstd::promise, and then itwait()s on that future.
ha yeah.. nvm. Bad brain, dunno why I was completely convinced it was waiting until the queue was empty instead 🤷🏼♂️.
331 | @@ -332,6 +332,9 @@ class Chain 332 | //! chain tip. 333 | virtual void waitForNotificationsIfTipChanged(const uint256& old_tip) = 0; 334 | 335 | + //! Wait for all pending notifications to be processed
nano nit:
//! Wait for all pending notifications up to this point to be processed
Or with even less ambiguity: "Wait for notifications that were pending before this call to be processed."
I don't think this is a nano nit, important to be precise when describing interfaces. Also would prefer improving the handleNotifications docstring to highlight that there may be pending notifications when the handler is destroyed, referencing waitForNotifications. This might prevent similar issues in the future. For example:
//! Register handler for notifications. Some notifications are asynchronous
//! and may still execute after the handler is disconnected. Use
//! waitForNotifications() after the handler is disconnected to ensure all
//! pending notifications have been processed.
virtual std::unique_ptr<Handler> handleNotifications(std::shared_ptr<Notifications> notifications) = 0;
note: if no Notifications are registered, there can be no pending notifications, yet this will still block, so the docstring is still not entirely correct. But it's consistent with pre-existing waitForNotificationsIfTipChanged, and I'm not sure there's currenly a better approach without significant scope creep.
@stickies-v, one option could be to move the listener unregistration into the interface so it mirrors the registration process (handleNotifications). That way we keep symmetry between the two and can enforce the requirements internally. See how I did it here https://github.com/furszy/bitcoin-core/commit/bf1abcd6d50374a461dad305f0243d3a613ce158 (note: the latch part can be replaced for a SyncWithQueue if not empty calls).
Updated the comment as suggested
I think it would be better if disconnect() did the waiting, but that would be enlarging the scope of this PR to be more than I think is appropriate for a feature freeze fix. Since it would affect more than just the wallet, there may be other side effects that have to be considered as well.
Some notifications are asynchronous
Shouldn't majority of the notifications in the codebase be asynchronous? Considering that the queueing mechanism is used usually used for async stuff (not specific to core).
And I assume synchronous notifications are those that are called like how SyncWithValidationInterfaceQueue calls?
https://github.com/bitcoin/bitcoin/blob/9cad97f6cdf1bd660732cd10e844a6a7e0771ea0/src/validationinterface.cpp#L146-L155
ACK c010fe8a2f9ec0dcee09a872da8926e0ee5f91a4
1071 | @@ -1072,6 +1072,9 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati 1072 | //! Find the private key for the given key id from the wallet's descriptors, if available 1073 | //! Returns nullopt when no descriptor has the key or if the wallet is locked. 1074 | std::optional<CKey> GetKey(const CKeyID& keyid) const; 1075 | + 1076 | + //! Disconnect chain notifications and wait for validation interface queue to drain
nit: better adhere to interface boundaries/terminology
//! Disconnect chain notifications and wait for all notifications to be processed.
An alternative approach would be to always wait for notifications to be processed in disconnect(). Is my understanding that the reason we don't is because during shutdown, the scheduler may already have been stopped? If so, I think this would be helpful to document DisconnectChainNotifications better so this doesn't get refactored out in the future, it's quite subtle I think.
Done as suggested.
If so, I think this would be helpful to document
DisconnectChainNotificationsbetter so this doesn't get refactored out in the future, it's quite subtle I think.
It's minor, but I think this comment has not yet been addressed. If my understanding is correct, a suggestion could be:
<details> <summary>git diff on 98e8af4bb9</summary>
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 1e7ad3fc6b..4fa6203d6d 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -4580,6 +4580,9 @@ std::optional<WalletTXO> CWallet::GetTXO(const COutPoint& outpoint) const
void CWallet::DisconnectChainNotifications()
{
+ // waitForNotifications() is called separately from disconnect() rather than integrated into it,
+ // because Shutdown() may tear down the scheduler before notifications are disconnected, causing
+ // the disconnection to block.
if (m_chain_notifications_handler) {
m_chain_notifications_handler->disconnect();
chain().waitForNotifications();
</details>
crACK c010fe8a2f9ec0dcee09a872da8926e0ee5f91a4
PR description needs to be updated after the changed approach.
Waiting for the chain notifications to be processed makes the wallet shutdown more correct, but also slower. In most cases this seems harmless, but I did not test if there are edge cases (e.g. IBD, multiple wallets, a somehow clogged up/slow validation queue) where this can be problematic.
Co-Authored-By: stickies-v <stickies-v@protonmail.com>
When unloading a wallet, there may be unexecuted callbacks in the
validation interface queue that can still execute after we have
completed all of the other wallet shutdown tasks. Instead of letting
these run in the background, once the notifications are disconnected,
wait for the queue to drain before continuing with wallet shutdown.
ACK 98e8af4bb991fd8edeb15c0fb8afa66bff6b5cac
utACK 98e8af4bb991fd8edeb15c0fb8afa66bff6b5cac
crACK 98e8af4bb991fd8edeb15c0fb8afa66bff6b5cac
This makes the wallet unloading / shutdown operation more robust albeit slower (specially in case of multiple wallets loaded), which I don't mind. I'd prefer consistent & slow(er) operations over brittle & fast(er) ones.
I too feel that the waiting portion can be encapsulated within the disconnect function, but can be addressed later.
ACK 98e8af4bb991fd8edeb15c0fb8afa66bff6b5cac