Coming from #29073.
Applied ryanofsky suggested changes on #29073 (comment) with few modifications coming from #18338 (comment).
The only point I did not tackle from #18338 (comment) is:
- Move log print and flush out of ReleaseWallet into CWallet destructor
Because it would mean every CWallet object would flush data to disk during destruction. Which is not necessary for wallet tool utilities and unit tests.
Releases wallet shared pointers prior to doing the
final settings update and prevent GUI races trying
to access a wallet that is no longer loaded.
And update function's documentation.
Multiple threads could try to delete the wallet at the same time.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | ryanofsky, ismaelsadeeq, achow101 |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
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.
226@@ -227,10 +227,10 @@ static std::set<std::string> g_loading_wallet_set GUARDED_BY(g_loading_wallet_mu
227 static std::set<std::string> g_unloading_wallet_set GUARDED_BY(g_wallet_release_mutex);
228
229 // Custom deleter for shared_ptr<CWallet>.
230-static void ReleaseWallet(CWallet* wallet)
231+static void FlushAndDelete(CWallet* wallet)
232 {
233 const std::string name = wallet->GetName();
234- wallet->WalletLogPrintf("Releasing wallet\n");
235+ wallet->WalletLogPrintf("Releasing wallet %s..\n", name);
nit: should this change be in its own commit? then you can use scripted diff for the rename.
Updated. Scripted both changes within the same commit.
226@@ -227,10 +227,10 @@ static std::set<std::string> g_loading_wallet_set GUARDED_BY(g_loading_wallet_mu
227 static std::set<std::string> g_unloading_wallet_set GUARDED_BY(g_wallet_release_mutex);
228
229 // Custom deleter for shared_ptr<CWallet>.
230-static void ReleaseWallet(CWallet* wallet)
231+static void FlushAndDelete(CWallet* wallet)
In commit “wallet: rename ReleaseWallet to FlushAndDelete” (7a73b0b7d5e690235bcefca9b82f01442b37ad5e)
Not important, but maybe this should be called FlushAndDeleteWallet to be consistent with WaitForDeleteWallet.
To better describe the function's behavior.
And add wallet name to logprint.
226@@ -227,10 +227,10 @@ static std::set<std::string> g_loading_wallet_set GUARDED_BY(g_loading_wallet_mu
227 static std::set<std::string> g_unloading_wallet_set GUARDED_BY(g_wallet_release_mutex);
228
229 // Custom deleter for shared_ptr<CWallet>.
230-static void ReleaseWallet(CWallet* wallet)
231+static void FlushAndDeleteWallet(CWallet* wallet)
232 {
233 const std::string name = wallet->GetName();
234- wallet->WalletLogPrintf("Releasing wallet\n");
235+ wallet->WalletLogPrintf("Releasing wallet %s..\n", name);
re: #30659 (review)
Can you explain why a four-line diff requires a five-line scripted diff? Seems easier to just have a normal diff when the script is longer than the diff.
Agree that a manual change is fine here, but IMO unless a scripted diff is badly written or fragile (e.g. hardcoding line numbers), I’d always prefer it over a manual change because it:
IMO unless a scripted diff is badly written or fragile (e.g. hardcoding line numbers), I’d always prefer it over a manual change because it:
I understand why scripted-diffs are useful. However, you forgot to mention the risks and downsides:
Also, the diff contained a statement, which looks like it shouldn’t be part of it, because it applies to one line of code. (sed -i 's/WalletLogPrintf("Releasing wallet\\n"/WalletLogPrintf("Releasing wallet %s..\\n", name/g' $(git grep -l '"Releasing wallet\\n"' ./src/wallet/wallet.cpp)). I don’t see any benefit in putting that into a scripted-diff, which is why I mentioned it (and also left my comment on this line).
Obviously the rest of the script was fine, but personally just calling a single time git grep $old_value is faster than to type ./test/lint/commit-script-check.sh HEAD~$N..HEAD (or rely on the CI).
