Fix wallet unload race condition

promag commented at 1:37 am on March 13, 2020: member

This PR consists in two fixes. The first fixes a concurrency issues with boost::signals2. The second fixes a wallet model destruction while it’s being used.

From boost signal documentation at https://www.boost.org/doc/libs/1_72_0/doc/html/signals2/thread-safety.html:

When a signal is invoked by calling signal::operator(), the invocation first acquires a lock on the signal’s mutex. Then it obtains a handle to the signal’s slot list and combiner. Next it releases the signal’s mutex, before invoking the combiner to iterate through the slot list.

This means that UnregisterValidationInterface doesn’t prevent more calls to that interface. The fix consists in capturing the shared_ptr<CValidationInterface> in each internal slot.

The GUI bug is fixed by using a Qt::QueuedConnection in the WalletModel::unload connection.

fanquake added the label Wallet on Mar 13, 2020

DrahtBot commented at 3:54 am on March 13, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#18469 (Build: Move wallet RPCs to their own libbitcoin_walletrpcs module by luke-jr)
#18354 (Protect wallet by using shared pointers by bvbfan)
#15761 (Replace -upgradewallet startup option with upgradewallet RPC by achow101)
#10102 ([experimental] Multiprocess bitcoin by ryanofsky)

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.

ryanofsky commented at 5:42 am on March 13, 2020: member

I wrote a test for new register/unregister functions that could be added here: d65b4874fda47aad2e412f0e0cac27e8eafb3838 (branch)

bvbfan commented at 1:31 pm on March 13, 2020: contributor

unregister from notifications earlier in UnloadWallet instead of ReleaseWallet, and use a new RegisterSharedValidationInterface function to prevent the CValidationInterface shared_ptr from being deleted until the last notification is actually finished.

Does not fix the root issue, which is notification thread hold mutex (no matter when notification is disconnected if it’s the last instance of the wallet) then goes in Release wallet, flush, then deleting mutex while it’s hold is SIGSEGV. Unit test does not do anything to ensure that.

ryanofsky commented at 2:19 pm on March 13, 2020: member

Does not fix the root issue, which is notification thread hold mutex (no matter when notification is disconnected if it’s the last instance of the wallet) then goes in Release wallet, flush, then deleting mutex while it’s hold is SIGSEGV. Unit test does not do anything to ensure that.

Can you give instructions to reproduce the issue or post a backtrace of the SIGSEGV? ReleaseWallet should not be called during a notification because ReleaseWallet is only called when the shared pointer reference count reaches 0. The unit test verifies that the reference count doesn’t reach 0 during a notification until after the notification callback method returns, by checking that the destructor is called only after the callback returns.

bvbfan commented at 7:24 am on March 14, 2020: contributor

0#!/bin/bash
1while [ 1 ]
2do
3    src/bitcoin-cli -testnet loadwallet test
4    src/bitcoin-cli -testnet unloadwallet test
5done

Running on testnet from scratch in parallel with this script it should not crash. Whole point of EmptyQueue was to not have notification holding wallet mutex while we delete it.

promag commented at 0:17 am on March 16, 2020: member

@bvbfan that script crashes bitcoin-qt -regtest -server -printtoconsole.

Still investigating but looking at call stack it’s caused by queued connections “in flight” (like WalletController::walletAdded(WalletModel*)) while the wallet model is deleted in WalletController::removeAndDeleteWallet.

bvbfan commented at 12:18 pm on March 16, 2020: contributor

It looks like another issue, wallet model holds a unique_ptr to interface wallet which in other hand have a shared_ptr to actual wallet. Since UnloadWallet actually deletes wallet (it gets it from wallet map) unless wallet model outlive wallet. First make sure it works like that bitcoind -testnet (it’s testnet because every few seconds new block is connected and notification is activated)

ryanofsky commented at 3:59 pm on March 16, 2020: member

First make sure it works like that bitcoind -testnet (it’s testnet because every few seconds new block is connected and notification is activated)

Thanks for clarifying this. I had tried a bunch of ways to reproduce the issue with -regtest and could never get it to happen. But with cf4cb28efcf80c018a7f070c671f43cd172dbd86 and -testnet I was able to get the deadlock to happen in just a few minutes.

I still haven’t been able to reproduce a SIGSEGV or any problem with this current PR 73bd96206851aa88db79b8efbc844700630980e1, though. So again if you have a stack trace it would help. Or it would help if you could describe in a more step-by-step way how you see the SIGSEGV happening

bee22193 approved

bvbfan commented at 5:06 pm on March 16, 2020: contributor

I still haven’t been able to reproduce a SIGSEGV or any problem with this current PR 73bd962, though. So again if you have a stack trace it would help. Or it would help if you could describe in a more step-by-step way how you see the SIGSEGV happening

Since UnloadWallet actively deletes wallet that’s may not happen since g_wallet_init_interface is global and it’s call UnloadWallet in its destructor.

ryanofsky commented at 5:32 pm on March 16, 2020: member

I still haven’t been able to reproduce a SIGSEGV or any problem with this current PR 73bd962, though. So again if you have a stack trace it would help. Or it would help if you could describe in a more step-by-step way how you see the SIGSEGV happening

Since UnloadWallet actively deletes wallet that’s may not happen since g_wallet_init_interface is global and it’s call UnloadWallet in its destructor.

What may not happen?

ryanofsky commented at 5:34 pm on March 16, 2020: member

What may not happen?

Oh, maybe you are referring to the bitcoin-qt bug. I haven’t tried to reproduce that yet.

ryanofsky commented at 6:56 pm on March 16, 2020: member

re: #18338 (comment)

@bvbfan that script crashes bitcoin-qt -regtest -server -printtoconsole.

Still investigating but looking at call stack it’s caused by queued connections “in flight” (like WalletController::walletAdded(WalletModel*)) while the wallet model is deleted in WalletController::removeAndDeleteWallet. @promag, I was able to reproduce this and I think it’s basically a separate issue from the deadlock that happens with bitcoind. I created a bug report #18362. Would move any discussion of that issue there

in src/wallet/wallet.cpp:137 in 73bd962068 outdated

132@@ -137,6 +133,9 @@ void UnloadWallet(std::shared_ptr<CWallet>&& wallet)
133     // Notify the unload intent so that all remaining shared pointers are
134     // released.
135     wallet->NotifyUnload();
136+
137+    wallet->m_chain_notifications_handler.reset();

bvbfan commented at 7:32 pm on March 16, 2020:

BlockUntilSyncedToCurrentChain is still needed, unless we can expect data loss

ryanofsky commented at 7:35 pm on March 16, 2020:

re: #18338 (review)

BlockUntilSyncedToCurrentChain is still needed, unless we can expect data loss

BlockUntilSyncedToCurrentChain can’t be called with locks held and it doesn’t make sense here. It makes sense at beginning of RPC calls to ensure consistency between calls. It doesn’t directly relate to writing or consolidating data.

bvbfan commented at 6:20 am on March 17, 2020:

Where lock is held? If here it’s, that’s nasty bug.

ryanofsky commented at 8:44 am on March 17, 2020:

Where lock is held? If here it’s, that’s nasty bug.

Right, I don’t think there is a nasty bug now. I think it is good to avoid nasty bugs by not calling BlockUntilSyncedToCurrentChain when it is not necessary.

in src/validationinterface.h:185 in 73bd962068 outdated

158@@ -151,7 +159,7 @@ class CMainSignals {
159 private:
160     std::unique_ptr<MainSignalsInstance> m_internals;
161 
162-    friend void ::RegisterValidationInterface(CValidationInterface*);
163+    friend void ::RegisterSharedValidationInterface(std::shared_ptr<CValidationInterface>);
164     friend void ::UnregisterValidationInterface(CValidationInterface*);

bvbfan commented at 7:33 pm on March 16, 2020:

Why not shared_ptr here?

ryanofsky commented at 7:37 pm on March 16, 2020:

re: #18338 (review)

Why not shared_ptr here?

This is an existing function the PR isn’t changing (and there’s no reason to change it)

bvbfan commented at 7:46 pm on March 16, 2020: contributor

In #18362 the bug is mostly horizontalHeader is nullptr (invalid pointer). Basically i did not like transfer ownership of wallet to the notification that’s not expected to me.

ryanofsky commented at 7:53 pm on March 16, 2020: member

]> Basically i did not like transfer ownership of wallet to the notification that’s not expected to me.

Sure, but if you are already using a shared_ptr to refer to an object some places, it makes sense to use it consistently. Combining shared pointers and raw pointers and asynchronous notifications is a recipe for bugs.

promag force-pushed on Mar 19, 2020

promag renamed this:
~~wip: Fix wallet unload race condition~~
Fix wallet unload race condition
on Mar 21, 2020

promag force-pushed on Mar 21, 2020

promag commented at 10:24 pm on March 21, 2020: member

I wrote a test for new register/unregister functions that could be added here: d65b487 (branch)

No longer WIP, added 2 comments in the first commit, and included the above test, thanks!

meshcollider added the label Bug on Mar 22, 2020

meshcollider added this to the "Bugfixes" column in a project

meshcollider added this to the milestone 0.20.0 on Mar 22, 2020

promag force-pushed on Mar 23, 2020

hebasto commented at 7:30 am on March 23, 2020: member

Reviewed and tested 179051c9e7835ba24baf623a2a37ba465792f1ad commit for now.

It indeed fixes #18362: Qt::QueuedConnection allows now to complete BitcoinGUI::addWallet() before the following BitcoinGUI::removeWallet() starts.

IIIC, both WalletController and WalletModel instances live in the GUI thread, therefore, the contexts for the created connections are the same. Why use different objects to specify the same event loop? I mean https://github.com/bitcoin/bitcoin/blob/179051c9e7835ba24baf623a2a37ba465792f1ad/src/qt/walletcontroller.cpp#L119 and https://github.com/bitcoin/bitcoin/blob/179051c9e7835ba24baf623a2a37ba465792f1ad/src/qt/walletcontroller.cpp#L123

hebasto commented at 8:03 am on March 23, 2020: member

@promag Why tests (d8102ce7c90378244fedbcf7e8cc81cb65d5ca0d) are dropped?

promag commented at 9:26 am on March 23, 2020: member

@promag Why tests (d8102ce) are dropped?

Two travis jobs are getting stuck, I’m trying to understand the problem.

bvbfan commented at 11:05 am on March 23, 2020: contributor

To be architectural correct notification should hold a weak pointer, then should extend its lifetime for a particular call.

hebasto commented at 2:18 pm on March 23, 2020: member

Tested 179051c9e7835ba24baf623a2a37ba465792f1ad on Linux Mint 19.3:

start bitcoin-qt without wallets
load a wallet via menu
close this wallet via menu
open it again:

Screenshot from 2020-03-23 16-17-50

in src/validationinterface.cpp:96 in f593902aff outdated

88@@ -87,6 +89,18 @@ void RegisterValidationInterface(CValidationInterface* pwalletIn) {
89     conns.NewPoWValidBlock = g_signals.m_internals->NewPoWValidBlock.connect(std::bind(&CValidationInterface::NewPoWValidBlock, pwalletIn, std::placeholders::_1, std::placeholders::_2));
90 }
91 
92+void RegisterValidationInterface(CValidationInterface* callbacks)
93+{
94+    // Create a shared_ptr with a no-op deleter - CValidationInterface lifecycle
95+    // is managed by the caller.
96+    RegisterSharedValidationInterface({callbacks, [](CValidationInterface*){}});

hebasto commented at 2:25 pm on March 23, 2020:

Approach -0

This makes reasoning about code behavior harder.

ryanofsky commented at 3:23 pm on March 23, 2020:

Approach -0

This makes reasoning about code behavior harder.

Harder than what? Harder than than the status quo? Or harder than an alternate solution?

This should clearly not be harder to reason about than the status quo. The status quo uses a combination of raw pointers and shared pointers and has a use-after-free bug as a result. Dropping raw pointers and using shared pointers consistently fixes the bug in the simplest way and makes it obvious and automatic that the pointer is only deleted when the last reference is deleted.

This approach is also not harder to reason about than the alternate approaches I’ve seen which have been to make the Unregister function blocking instead of nonblocking. Three PRs #18280 #18279 #18354 have tried that approach and encountered many bugs along the way. The approach is bad because you can no longer rely on reference counting to free the pointer after the last use, and instead have to manually think through every unload sequence and ensure that the new blocking behavior doesn’t lead to deadlocks and invalid pointer accesses. It’s also difficult because it changes the scheduler code in an invasive way compared to this PR which is just swapping out pointer type for another.

ryanofsky commented at 3:30 pm on March 23, 2020:

Approach -0

This makes reasoning about code behavior harder.

In case the comment is specifically about the RegisterValidationInterface function, this function and UnregisterValidationInterface could both be dropped in the future and replaced with RegisterShared functions. It wasn’t done here just to keep the PR smaller and avoid having to change indexing code.

hebasto commented at 11:52 am on March 29, 2020:

IIUC, std::shared_ptr with a no-op custom deleter (and without leveraging of use_count()) works the same as a raw pointer, no?

promag commented at 12:00 pm on March 29, 2020:

Right, that is intended for all validation interfaces except for wallets.

bvbfan commented at 3:00 pm on March 23, 2020: contributor

@hebasto about duplicate wallet error, if you wait a bit it should work, the problem is extended wallet lifetime due to shared ownership, it can be fixed if weak pointer is introduced, but i’m 100% sure.

promag commented at 3:12 pm on March 23, 2020: member

To be architectural correct notification should hold a weak pointer, then should extend its lifetime for a particular call.

The captured shared_ptr are only valid until the validation interface is unregistered and boost signal finishes calling all slots (if any) in the scheduler thread.

hebasto commented at 3:51 pm on March 23, 2020: member

@bvbfan

if you wait a bit it should work…

It keeps its state.

DrahtBot added the label Needs rebase on Mar 23, 2020

ryanofsky commented at 1:21 pm on March 25, 2020: member

Code review ACK 179051c9e7835ba24baf623a2a37ba465792f1ad. No changes since last review other than new comments and PR description

Needs rebase due to conflict, however.

Also, it would be good to mention in PR description that this fixes issues #16307 and #18362

ryanofsky approved

ryanofsky commented at 1:29 pm on March 25, 2020: member

I think this PR is good in its current form as a minimal bug fix. Future followups could also:

Unify RegisterSharedValidationInterface and RegisterValidationInterface in #18354 (requires changes to indexing code)
Add RegisterSharedValidationInterface unit test from #18338 (comment)

promag force-pushed on Mar 25, 2020

DrahtBot removed the label Needs rebase on Mar 25, 2020

hebasto commented at 7:07 pm on March 26, 2020: member

@promag could #18338 (comment) be addressed before merging ;)

promag commented at 10:13 am on March 27, 2020: member

@hebasto nice catch, unloading from the GUI was relying on deleting the wallet model to bring down the reference count to zero. With this PR this is no longer true because those validation callbacks are capturing the wallet shared pointer. Temporarily pushed 240cf29 to address this issue.

ryanofsky commented at 1:35 pm on March 27, 2020: member

@hebasto nice catch, unloading from the GUI was relying on deleting the wallet model to bring down the reference count to zero. With this PR this is no longer true because those validation callbacks are capturing the wallet shared pointer. Temporarily pushed 240cf29 to address this issue.

This is a good catch, and 240cf29205815f5f5221b0cbb6c8f5829e5a46ff looks like a good fix that stops notifications at a more appropriate place than the original fix.

promag force-pushed on Mar 27, 2020

promag commented at 2:07 pm on March 27, 2020: member

Changed fix to f80e957 and squashed to e66da18.

in src/wallet/wallet.cpp:67 in 87379413c2 outdated

61@@ -62,6 +62,8 @@ bool AddWallet(const std::shared_ptr<CWallet>& wallet)
62 
63 bool RemoveWallet(const std::shared_ptr<CWallet>& wallet)
64 {
65+    // Unregister with the validation interface which also drops shared ponters.
66+    wallet->m_chain_notifications_handler.reset();

ryanofsky commented at 2:59 pm on March 27, 2020:

In commit “gui: Handle WalletModel::unload asynchronous” (e66da188cbbd70f0e1c2f9050680d9af4fdc0c97)

Doesn’t make a big a difference in practice, but it might be a good idea to move assert(wallet) below before this, so it doesn’t look like this is dereferencing before checking for potential null

ryanofsky approved

ryanofsky commented at 3:02 pm on March 27, 2020: member

Code review ACK e66da188cbbd70f0e1c2f9050680d9af4fdc0c97. Changes since last review are moving handler reset earlier, and rebasing

Fix wallet unload race condition

Currently it's possible for ReleaseWallet to delete the CWallet pointer while
it is processing BlockConnected, etc chain notifications.

To fix this, unregister from notifications earlier in UnloadWallet instead of
ReleaseWallet, and use a new RegisterSharedValidationInterface function to
prevent the CValidationInterface shared_ptr from being deleted until the last
notification is actually finished.

ab31b9d6fe

gui: Handle WalletModel::unload asynchronous

This change prevents deleting a WalletModel instance while it's
being used.

41b0baf43c

promag force-pushed on Mar 27, 2020

ryanofsky commented at 3:24 pm on March 27, 2020: member

Some notes for future cleanup.

I think we can clean up shutdown sequence after this PR, since right now it’s confusing what responsibilities of different unloading functions are. I think I’d:

Move log print and flush out of ReleaseWallet into CWallet destructor
Rename ReleaseWallet to DeleteWallet since only thing it’s doing is deleting the wallet pointer
Move handler.reset and NotifyUnload calls to new CWallet::Unload method, and call that method from RemoveWallet, so GUI is notified as early as possible and CWallet data members aren’t being accessed from outside functions
Rename UnloadWallet to WaitForDeleteWallet since all it will be doing at that point is resetting the pointer and waiting for the gui and validation interfaces to let go their wallet references

ryanofsky approved

ryanofsky commented at 3:27 pm on March 27, 2020: member

Code review ACK 41b0baf43c243b64b394e774e336475a489cca2b. Only change is moving assert as suggested

in src/interfaces/chain.cpp:189 in ab31b9d6fe outdated

186+    explicit NotificationsHandlerImpl(std::shared_ptr<Chain::Notifications> notifications)
187+        : m_proxy(std::make_shared<NotificationsProxy>(std::move(notifications)))
188+    {
189+        RegisterSharedValidationInterface(m_proxy);
190+    }
191+    ~NotificationsHandlerImpl() override { disconnect(); }

hebasto commented at 3:13 pm on March 28, 2020:

nit: The destructor of the derived class should not be declared with override.

promag commented at 3:31 pm on March 29, 2020:

Done.

ryanofsky commented at 10:26 am on March 30, 2020:

re: #18338 (review)

nit: The destructor of the derived class should not be declared with override.

This is a strange suggestion and I would suggest reverting 17316b3a6eb41204824111ea3cfeb2e1b7d80b1b to 41b0baf43c243b64b394e774e336475a489cca2b to undo it. The point of override keyword is to get the compiler to enforce assumptions the derived class is making about the base class to avoid terrible bugs. In this case, override keyword makes it an error for the base class not to have a virtual destructor. This is important because if base destructor was made not virtual there would be a difficult to trace memory leak here with NotificationsHandlerImpl object getting freed but destructor never running and m_proxy member never being freed.

hebasto commented at 11:10 am on March 30, 2020:

A desctructor is not a regular member function. So I was confused with the following opinions:

hebasto commented at 11:31 am on March 30, 2020:

@ryanofsky

In this case, override keyword makes it an error for the base class not to have a virtual destructor.

I’ve verified this statement with the following diff:

 0--- a/src/interfaces/handler.h
 1+++ b/src/interfaces/handler.h
 2@@ -22,7 +22,7 @@ namespace interfaces {
 3 class Handler
 4 {
 5 public:
 6-    virtual ~Handler() {}
 7+    ~Handler() {}
 8 
 9     //! Disconnect the handler.
10     virtual void disconnect() = 0;

My compiler raises an error. You are correct. TIL. Thank you.

in src/interfaces/chain.cpp:156 in ab31b9d6fe outdated

165-            UnregisterValidationInterface(this);
166-        }
167-    }
168+    explicit NotificationsProxy(std::shared_ptr<Chain::Notifications> notifications)
169+        : m_notifications(std::move(notifications)) {}
170+    virtual ~NotificationsProxy() = default;

hebasto commented at 3:29 pm on March 28, 2020:

nit: Is the NotificationsProxy class intended for subclassing? If not, why its destructor is declared with virtual?

promag commented at 3:31 pm on March 29, 2020:

Removed.

ryanofsky commented at 10:28 am on March 30, 2020:

re: #18338 (review)

nit: Is the NotificationsProxy class intended for subclassing? If not, why its destructor is declared with virtual?

Would again suggest reverting 17316b3a6eb41204824111ea3cfeb2e1b7d80b1b to 41b0baf43c243b64b394e774e336475a489cca2b to undo this change. The point of declaring this virtual is to prevent memory leaks and incomplete destruction in case this is subclassed (whether intended or not). From https://isocpp.org/wiki/faq/virtual-functions#virtual-dtors:

Here’s a simplified rule of thumb that usually protects you and usually doesn’t cost you anything: make your destructor virtual if your class has any virtual functions.

This is an important rule to follow here and other places. Other alternatives would be to declare the class final or give it a protected nonvirtual destructor, but these are optimizations that are more fragile. Just having a virtual destructor when there are virtual functions is usually the simplest safe approach.

hebasto commented at 10:40 am on March 30, 2020:

Sorry for noise. I forgot that member functions defined with override are still virtual.

hebasto commented at 10:43 am on March 30, 2020:

@ryanofsky ibid:

Note: in a derived class, if your base class has a virtual destructor, your own destructor is automatically virtual.

ryanofsky commented at 10:48 am on March 30, 2020:

@ryanofsky ibid:

Note: in a derived class, if your base class has a virtual destructor, your own destructor is automatically virtual.

Yes, the problems happen when code changes over time. Without the override keyword somebody could make a change to the base class removing virtual and unknowingly cause memory leaks in the derived class. But with the override keyword, the compiler will show a “does not override” error building the derived class and not allow this.

hebasto commented at 11:04 am on March 30, 2020:

Yes, I understand that consistent usage of override in derived classes protects from bugs caused by changes in base classes over time.

But how virtual could help for a desctructor in a derived class, if:

in a derived class, if your base class has a virtual destructor, your own destructor is automatically virtual. You might need an explicitly defined destructor for other reasons, but there’s no need to redeclare a destructor simply to make sure it is virtual. No matter whether you declare it with the virtual keyword, declare it without the virtual keyword, or don’t declare it at all, it’s still virtual.

?

ryanofsky commented at 12:13 pm on March 30, 2020:

But how virtual could help for a desctructor in a derived class, if:

in a derived class, if your base class has a virtual destructor, your own destructor is automatically virtual. You might need an explicitly defined destructor for other reasons, but there’s no need to redeclare a destructor simply to make sure it is virtual. No matter whether you declare it with the virtual keyword, declare it without the virtual keyword, or don’t declare it at all, it’s still virtual.

?

None of that is applicable here. NotificationsProxy base class is CValidationInterface which does not have a virtual destructor. If it did have a virtual destructor, this could be declared with override instead of virtual, and registration would no longer require shared pointers with custom deleters. But these changes would increase size of the bugfix and are better saved for a followup PR. This PR is just trying to do the simple thing and

Declare virtual destructors where there are virtual functions
Use override instead of virtual where it prevents derived class code from being silently broken by future changes to the base class

hebasto commented at 12:55 pm on March 30, 2020:

Oh, I missed that: https://github.com/bitcoin/bitcoin/blob/41b0baf43c243b64b394e774e336475a489cca2b/src/validationinterface.h#L77-L83

in src/validationinterface.cpp:78 in ab31b9d6fe outdated

74@@ -75,8 +75,10 @@ CMainSignals& GetMainSignals()
75     return g_signals;
76 }
77 
78-void RegisterValidationInterface(CValidationInterface* pwalletIn) {
79-    ValidationInterfaceConnections& conns = g_signals.m_internals->m_connMainSignals[pwalletIn];
80+void RegisterSharedValidationInterface(std::shared_ptr<CValidationInterface> pwalletIn) {

hebasto commented at 4:06 pm on March 28, 2020:

nit: Mind identical parameter naming in the RegisterSharedValidationInterface declaration and definition: pwalletIn vs callbacks ?

promag commented at 3:30 pm on March 29, 2020:

Let’s do that in one of Russel suggestions and avoid unnecessary changes here.

hebasto approved

hebasto commented at 5:53 pm on March 28, 2020: member

ACK 41b0baf43c243b64b394e774e336475a489cca2b, tested on Linux Mint 19.3.

promag force-pushed on Mar 29, 2020

hebasto commented at 3:34 pm on March 29, 2020: member

re-ACK 17316b3a6eb41204824111ea3cfeb2e1b7d80b1b, only suggested changes about class destructors.

promag commented at 10:32 am on March 30, 2020: member

Reverted.

promag force-pushed on Mar 30, 2020

ryanofsky commented at 10:34 am on March 30, 2020: member

Code review ACK 17316b3a6eb41204824111ea3cfeb2e1b7d80b1b, but I did prefer 41b0baf43c243b64b394e774e336475a489cca2b with safe, intact destructors better. Either one is fine to merge though.

Besides the destructors other things on the future cleanup list are:

hebasto commented at 12:58 pm on March 30, 2020: member

Is my previous ACK valid if a commit is reverted to the ACKed one?

hebasto commented at 3:13 pm on March 30, 2020: member

@promag

@promag Why tests (d8102ce) are dropped?

Two travis jobs are getting stuck, I’m trying to understand the problem.

Any update about tests?

ryanofsky commented at 6:03 pm on March 30, 2020: member

Code review ACK 41b0baf43c243b64b394e774e336475a489cca2b (previously acked but just repeating for clarity).

This might be ready for merge. Lot of tweaking at the end but changes: f80e9574873c6091559ce5c4cba736d1b349319d -> e66da188cbbd70f0e1c2f9050680d9af4fdc0c97 41b0baf43c243b64b394e774e336475a489cca2b -> 17316b3a6eb41204824111ea3cfeb2e1b7d80b1b -> 41b0baf43c243b64b394e774e336475a489cca2b were all trivial.

This is a minimal bugfix and good candidate for merging before branching or for backporting.

laanwj merged this on Mar 31, 2020

laanwj closed this on Mar 31, 2020

promag deleted the branch on Mar 31, 2020

sidhujag referenced this in commit 578c6196d4 on Mar 31, 2020

fanquake removed this from the "Bugfixes" column in a project

MarkLTZ referenced this in commit 3ec6a5c547 on Apr 9, 2020

ryanofsky referenced this in commit 5741d750a0 on Apr 10, 2020

deadalnix referenced this in commit b49716c126 on Jun 18, 2020

DrahtBot locked this on Feb 15, 2022

Fix wallet unload race condition #18338

Conflicts