This pull request fully replace #18279 it fix 3 issues with wallet
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
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.
Simplify patch, make more fuzzing test
0#!/bin/bash
1while [ 1 ]
2do
3 src/bitcoin-cli -testnet loadwallet test
4 src/bitcoin-cli -testnet unloadwallet test3
5 src/bitcoin-cli -testnet loadwallet test2
6 src/bitcoin-cli -testnet unloadwallet test
7 src/bitcoin-cli -testnet loadwallet test3
8 src/bitcoin-cli -testnet unloadwallet test2
9done
208+
209+ auto shouldProcessQueue = [this]() {
210 LOCK(m_cs_callbacks_pending);
211- should_continue = !m_callbacks_pending.empty();
212- }
213+ return !m_callbacks_pending.empty() || m_are_callbacks_running;
Why’s this adding || m_are_callbacks_running? In the case where m_callbacks_pending is empty and m_are_callbacks_running is true, won’t ProcessQueue return immediately and the loop below be a busy loop consuming 100% of CPU?
Also could use while (WITH_LOCK(m_cs_callbacks_pending, return ...)) { instead of a lambda
Why’s this adding || m_are_callbacks_running? In the case where m_callbacks_pending is empty and m_are_callbacks_running is true, won’t ProcessQueue return immediately and the loop below be a busy loop consuming 100% of CPU?
It’s same without m_are_callbacks_running i.e. while loop consume a lot of CPU. The change is to ensure notification thread is finish processing callback.
Why’s this adding || m_are_callbacks_running? In the case where m_callbacks_pending is empty and m_are_callbacks_running is true, won’t ProcessQueue return immediately and the loop below be a busy loop consuming 100% of CPU?
Oh, you’re right. The current code is can busy-loop when the queue non-empty and running is true, though AreThreadsServicingQueue check makes this less likely to happen. I still don’t understand why you now want to busy loop in the case where the queue is empty and running is true. Again prefer just using shared_ptr to avoid use-after-free bugs instead of doing more complicated things with the scheduler.
AreThreadsServicingQueue is not needed at all. Its purpose is obsolete in the context.
134@@ -137,6 +135,8 @@ void UnloadWallet(std::shared_ptr<CWallet>&& wallet)
135 // Notify the unload intent so that all remaining shared pointers are
136 // released.
137 wallet->NotifyUnload();
138+ wallet->BlockUntilSyncedToCurrentChain();
139+ wallet->m_chain_notifications_handler.reset();
m_pscheduler->AreThreadsServicingQueue() is true, which it always is, except during shutdown. So a notification could still be in progress and raw wallet pointer still in use after the UnregisterValidationInterface call and handler.reset() call and wallet.reset() call. Adding the BlockUntilSyncedToCurrentChain right before probably makes this much less unlikely, but not impossible. #18338 is be a more direct, race-free fix for the issue, because it gets rid of the raw pointer and uses shared_ptr for all wallet references.
CValidationInterface instance right after UnregisterValidationInterface - it’s wrong because of boost::signals2 threading model.
Oh, just saw this PR has expanded since I looked at it previously. I think some of the uses of shared_ptr / weak_ptr here like the new ones added in CWalletTx do not make sense. shared_ptr / weak_ptr only make sense when lifetime of the reference doesn’t have a definite scope. For cases like CWalletTx where the wallet reference can’t outlive the wallet, CWallet& makes more sense than weak_ptr<CWallet>. (Also, though in that specific case, it would be preferable to stop storing the wallet references entirely since they are redundant and no CWalletTx method is ever called without already having a reference to the wallet).
It would probably make sense to break this PR up into multiple commits so individual changes here can be understood and reviewed more easily
shared_ptr / weak_ptr only make sense when lifetime of the reference doesn’t have a definite scope.
I agree, in plus it defines no ownership. I agree, that reference makes much more sense here, too
Are you interested in this patch or you prefer to be closed? The changes are:
Interestingly this change seems like a superset of #18742 by @MarcoFalke and #18791 by @promag and #18592 by @brakmic but it precedes all these other prs!
For the #18742 / #18791 overlap, I think there’s a question about where it makes sense to use the shared pointer validationinterface callbacks, and where it makes sense to use the non-shared ones. Maybe it would be good to do what this PR does and use shared pointers everywhere but there are possible concerns about overhead and making shutdown not deterministic. Some discussion #18791 (comment)
For the #18592 overlap, I think #18592 might be taking a better approach. It’s generally good to replace raw pointers with smart pointers or references. But a lot of instances here look like they should be references instead of shared_ptrs, and there’s also an Optional<const CWallet&> usage which I think is just recreating a type of a raw pointer. It seems like the pointer changes here might be going too far in some cases.
Probably we should focus on #18742 and #18791 and #18592 for now, but see if there’s anything this PR does better or isn’t covered by the smaller prs.
Signed-off-by: Anthony Fieroni <bvbfan@abv.bg>
Signed-off-by: Anthony Fieroni <bvbfan@abv.bg>
Signed-off-by: Anthony Fieroni <bvbfan@abv.bg>
🐙 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”.