This pull request fully replace #18279 it fix 3 issues with wallet
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
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
#!/bin/bash
while [ 1 ]
do
src/bitcoin-cli -testnet loadwallet test
src/bitcoin-cli -testnet unloadwallet test3
src/bitcoin-cli -testnet loadwallet test2
src/bitcoin-cli -testnet unloadwallet test
src/bitcoin-cli -testnet loadwallet test3
src/bitcoin-cli -testnet unloadwallet test2
done
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.
Got'cha, you're right. I'll investigate for solution.
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();
This is a partial fix for the bug, not a full fix. While calling BlockUntilSyncedToCurrentChain before reset makes it less likely the wallet pointer will be deleted while still in use, it's still possible for a blockconnected or other notification to be running when wallet.reset() is called below
As I understand it, the EmptyQueue call added there (in UnregisterValidationInterface) does nothing if 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.
Approach NACK. The problem is not in the scheduler, but in the invalid assumption that it's safe to delete a CValidationInterface instance right after UnregisterValidationInterface - it's wrong because of boost::signals2 threading model.
Concept ACK, though this is making a lot of changes in a single commit. The changes will probably get simpler if #18338 is merged first, and maybe they can be broken up into smaller commits. Could consider rebasing on top of #18338 of you don't want to wait for it to be merged. I'd also encourage you to review #18338!
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
Use Optional instead of weak_ptr, i can't reproduce CI errors, all tests + functional ones passes in Linux x64.
Loosk like there are some cleanups from #18338 (comment) that can be addressed here
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.
Revert optional ref. @ryanofsky none of those PRs are complete like this, i agree they have same approach. Using shared pointer is cheap enough to be pass by value.
Is this still a bug fix? Do you think you can split in multiple commits?
Hi @promag mostly it's not an issue, AFAIK. It has some side effects in Qt gui, it was fixed i think. Notification proxy can extend notification lifetime which isn't correct to me. As well as using shared pointers only to validation interface, missing virtual destructors to interface classes like NetEventsInterface (it's not a problem since it's not deleted through base pointer till now). I can split it to more commits if you're interested in this features.
Signed-off-by: Anthony Fieroni <bvbfan@abv.bg>
Signed-off-by: Anthony Fieroni <bvbfan@abv.bg>
Signed-off-by: Anthony Fieroni <bvbfan@abv.bg>
Refactor pwallet to wallet (wallet* to wallet shared_ptr) is removed.
<!--cf906140f33d8803c4a75a2196329ecb-->
🐙 This pull request conflicts with the target branch and needs rebase.
<sub>Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".</sub>
Needs rebase if still relevant
I will investigate if it still needed, actually @ryanofsky does excellent work to well define shared interfaces ownership.
<!--13523179cfe9479db18ec6c5d236f789-->There hasn't been much activity lately and the patch still needs rebase. What is the status here?
Closing for now. Feel free to ask for this to be reopened or create a new pull request.