#19833 (wallet: Avoid locking cs_wallet recursively by promag)
#19521 (Coinstats Index (without UTXO set hash) by fjahr)
#19425 (refactor: Get rid of more redundant chain methods by ryanofsky)
#18766 (Disable fee estimation in blocksonly mode (by removing the fee estimates global) by darosior)
#14053 (Add address-based index (attempt 4?) by marcinja)
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.
laanwj added the label
Bug
on Mar 16, 2020
bvbfan
commented at 1:46 pm on March 16, 2020:
contributor
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.
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.
ryanofsky
commented at 2:28 pm on March 16, 2020:
member
#18338 may still have some issues, but I think it is probably a better approach and I would suggest abandoning this.
bvbfan
commented at 6:49 am on March 17, 2020:
contributor
Use future instead of raw loop for waiting pending callbacks. It still much matters to me compared to #18338
promag changes_requested
promag
commented at 0:31 am on March 23, 2020:
member
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.
bvbfan
commented at 11:39 am on March 24, 2020:
contributor
@promag same as your approach with more aggressive refactor + missing virtual desctructors.
bvbfan renamed this:
Protect wallet from scheduler race conditions.
Protect wallet from by using shared pointer
on Mar 24, 2020
bvbfan renamed this:
Protect wallet from by using shared pointer
Protect wallet by using shared pointers
on Mar 24, 2020
ryanofsky
commented at 1:35 pm on March 25, 2020:
member
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!
ryanofsky
commented at 1:48 pm on March 25, 2020:
member
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
bvbfan
commented at 5:41 pm on March 25, 2020:
contributor
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
bvbfan
commented at 9:47 am on March 26, 2020:
contributor
Use Optional instead of weak_ptr, i can’t reproduce CI errors, all tests + functional ones passes in Linux x64.
DrahtBot added the label
Needs rebase
on Mar 26, 2020
DrahtBot removed the label
Needs rebase
on Mar 27, 2020
DrahtBot added the label
Needs rebase
on Mar 28, 2020
DrahtBot removed the label
Needs rebase
on Mar 30, 2020
DrahtBot added the label
Needs rebase
on Mar 31, 2020
MarcoFalke
commented at 2:08 pm on March 31, 2020:
member
Loosk like there are some cleanups from #18338 (comment) that can be addressed here
DrahtBot removed the label
Needs rebase
on Apr 1, 2020
DrahtBot added the label
Needs rebase
on Apr 3, 2020
DrahtBot removed the label
Needs rebase
on Apr 28, 2020
DrahtBot added the label
Needs rebase
on Apr 29, 2020
DrahtBot removed the label
Needs rebase
on May 1, 2020
DrahtBot added the label
Needs rebase
on May 1, 2020
DrahtBot removed the label
Needs rebase
on May 1, 2020
bvbfan
commented at 9:39 am on May 2, 2020:
contributor
Are you interested in this patch or you prefer to be closed?
The changes are:
Validation interface uses shared pointers only
Fix CValidationInterface and NetEventsInterface to use virtual destructors. Since there is no usage of creation and deletion via base pointer it does not have a problem but instead it’s not correct design.
RPC methods uses CWallet shared pointer only
CwalletTx and WalletRescanReserver uses optional reference to CWallet instead of pointer
Notification implementation uses weak pointer instead of shared one.
DrahtBot added the label
Needs rebase
on May 4, 2020
ryanofsky
commented at 1:55 am on May 7, 2020:
member
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.
DrahtBot removed the label
Needs rebase
on May 7, 2020
DrahtBot added the label
Needs rebase
on May 13, 2020
bvbfan
commented at 3:07 pm on May 15, 2020:
contributor
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.
DrahtBot removed the label
Needs rebase
on May 15, 2020
DrahtBot added the label
Needs rebase
on May 21, 2020
DrahtBot removed the label
Needs rebase
on May 22, 2020
DrahtBot added the label
Needs rebase
on May 23, 2020
DrahtBot removed the label
Needs rebase
on May 24, 2020
DrahtBot added the label
Needs rebase
on May 26, 2020
DrahtBot removed the label
Needs rebase
on May 26, 2020
bvbfan renamed this:
Protect wallet by using shared pointers
Use shared pointers only in validation interface
on May 28, 2020
DrahtBot added the label
Needs rebase
on May 28, 2020
DrahtBot removed the label
Needs rebase
on May 28, 2020
DrahtBot added the label
Needs rebase
on Jun 2, 2020
DrahtBot removed the label
Needs rebase
on Jun 4, 2020
DrahtBot added the label
Needs rebase
on Jun 5, 2020
DrahtBot removed the label
Needs rebase
on Jun 14, 2020
DrahtBot added the label
Needs rebase
on Jun 18, 2020
DrahtBot removed the label
Needs rebase
on Jun 20, 2020
DrahtBot added the label
Needs rebase
on Jun 21, 2020
DrahtBot removed the label
Needs rebase
on Jun 21, 2020
DrahtBot added the label
Needs rebase
on Jun 24, 2020
DrahtBot removed the label
Needs rebase
on Jun 28, 2020
DrahtBot added the label
Needs rebase
on Jul 3, 2020
DrahtBot removed the label
Needs rebase
on Jul 4, 2020
DrahtBot added the label
Needs rebase
on Jul 4, 2020
DrahtBot removed the label
Needs rebase
on Jul 4, 2020
DrahtBot added the label
Needs rebase
on Jul 12, 2020
DrahtBot removed the label
Needs rebase
on Jul 12, 2020
DrahtBot added the label
Needs rebase
on Jul 14, 2020
DrahtBot removed the label
Needs rebase
on Jul 14, 2020
DrahtBot added the label
Needs rebase
on Jul 30, 2020
DrahtBot removed the label
Needs rebase
on Jul 30, 2020
DrahtBot added the label
Needs rebase
on Aug 13, 2020
DrahtBot removed the label
Needs rebase
on Sep 12, 2020
DrahtBot added the label
Needs rebase
on Sep 15, 2020
DrahtBot removed the label
Needs rebase
on Sep 15, 2020
DrahtBot added the label
Needs rebase
on Sep 18, 2020
DrahtBot removed the label
Needs rebase
on Oct 2, 2020
promag
commented at 11:01 pm on October 6, 2020:
member
Is this still a bug fix? Do you think you can split in multiple commits?
bvbfan
commented at 6:29 am on October 7, 2020:
contributor
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.
Use shared pointer to validation interface
Signed-off-by: Anthony Fieroni <bvbfan@abv.bg>
9b97239276
Use weak reference to chain notification in validation interface proxy
Signed-off-by: Anthony Fieroni <bvbfan@abv.bg>
c45eb76e7a
Make interface classes desctructor virtual
Signed-off-by: Anthony Fieroni <bvbfan@abv.bg>
557b59670d
bvbfan
commented at 12:30 pm on October 7, 2020:
contributor
Refactor pwallet to wallet (wallet* to wallet shared_ptr) is removed.
DrahtBot added the label
Needs rebase
on Dec 1, 2020
DrahtBot
commented at 9:44 am on December 1, 2020:
member
🐙 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”.
MarcoFalke
commented at 12:01 pm on October 22, 2021:
member
Needs rebase if still relevant
bvbfan
commented at 12:19 pm on October 22, 2021:
contributor
I will investigate if it still needed, actually @ryanofsky does excellent work to well define shared interfaces ownership.
DrahtBot
commented at 10:49 am on February 22, 2022:
member
Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.
MarcoFalke
commented at 10:56 am on February 22, 2022:
member
Closing for now. Feel free to ask for this to be reopened or create a new pull request.
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2024-10-04 13:12 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me