wallet: avoid double keypool TopUp() call on descriptor wallets #25526
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, 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. |
There was a problem hiding this comment.
ACK 616a54b.
Looks good!
I verified that TopUp calls and type checking are not duplicated anymore.
Side note, can't these two be merged in a single if?
bitcoin/src/wallet/scriptpubkeyman.cpp
Lines 1688 to 1697 in 616a54b
616a54b to
cba0cb9
Compare
|
Thanks for the feedback! Rebased on master, conflict solved. Ready to go.
Good eye @aureleoules. |
Move TopUp() responsibility from the wallet class to each scriptpubkeyman. So each spkm can decide to call it or not after perform the basic checks for the new destination request. Reason: We were calling it twice in the following flows for descriptor wallets: A) CWallet::GetNewDestination: 1) Calls spk_man->TopUp() 2) Calls spk_man->GetNewDestination() --> which, after the basic script checks, calls TopUp() again. B) CWallet::GetReservedDestination: 1) Calls spk_man->TopUp() 2) Calls spk_man->GetReservedDestination() --> which calls to GetNewDestination (which calls to TopUp again).
cba0cb9 to
bfb9b94
Compare
|
rebased, conflicts solved. |
|
re-ACK bfb9b94. |
| } else { | ||
| throw std::runtime_error(std::string(__func__) + ": Types are inconsistent. Stored type does not match type of newly generated address"); | ||
| if (!ExtractDestination(scripts_temp[0], dest)) { | ||
| return util::Error{_("Error: Cannot extract destination from the generated scriptpubkey")}; // shouldn't happen |
There was a problem hiding this comment.
nit: To be consistent with other internal errors that should never happen (e.g. "types are inconsistent" above), could also throw std::runtime_error here:
| return util::Error{_("Error: Cannot extract destination from the generated scriptpubkey")}; // shouldn't happen | |
| throw std::runtime_error(std::string(__func__) + "Cannot extract destination from the generated scriptpubkey"); // shouldn't happen |
There was a problem hiding this comment.
Did this change on purpose:
As we are not catching the runtime error at the GUI level. If this happens, the process will crash abruptly. No message, nor error description for the user.
With this change, the user will (1) receive the proper error message on a dialog, and (2) be able to continue using the node and wallet (up to a certain point).
Plus, sometimes abrupt shutdowns can corrupt the db. So, safer to not crash abruptly if can be avoided.
There was a problem hiding this comment.
Following that logic, we should either
- get rid of literally all runtime error throws that could happen in the GUI (alone in ./src/wallet there are 75 instances of those, according to
$ git grep "throw std::runtime_error" ./src/wallet | wc -l), or - simply catch runtime errors in the GUI, with a clear error message with dialog to the user and a clean shutdown to avoid db corruption
The second option seems to be the more obvious solution to me, rather than avoiding std::runtime_error (which seems a perfect candidate for internal errors that should never happen) at all.
(Of course, this is potential material for follow-ups and not directly related to this PR)
|
ACK bfb9b94 |
…criptor wallets bfb9b94 wallet: remove duplicate descriptor type check in GetNewDestination (furszy) 76b982a wallet: remove unused `nAccountingEntryNumber` field (furszy) 599ff5a wallet: avoid double TopUp() calls on descriptor wallets (furszy) Pull request description: Found it while was digging over a `getnewaddress` timeout on the functional test suite. ### Context: We are calling `TopUp()` twice in the following flows for descriptor wallets: A) `CWallet::GetNewDestination`: 1) Calls spk_man->TopUp() 2) Calls spk_man->GetNewDestination() --> which, after the basic script checks, calls TopUp() again. B) `CWallet::GetReservedDestination`: 1) Calls spk_man->TopUp() 2) Calls spk_man->GetReservedDestination() --> which calls to GetNewDestination (which calls to TopUp again). ### Changes: Move `TopUp()` responsibility from the wallet class to each scriptpubkeyman. So each spkm can decide to call it or not after perform the basic checks for the new destination request. Aside from that, remove the unused `nAccountingEntryNumber` wallet field. And a duplicated descriptor type check in `GetNewDestination` ACKs for top commit: aureleoules: re-ACK bfb9b94. achow101: ACK bfb9b94 theStack: Code-review ACK bfb9b94 Tree-SHA512: 3ab73f37729e50d6c6a4434f676855bc1fb404619d63c03e5b06ce61c292c09c59d64cb1aa3bd9277b06f26988956991d62c90f9d835884f41ed500b43a12058
Found it while was digging over a
getnewaddresstimeout on the functional test suite.Context:
We are calling
TopUp()twice in the following flows for descriptor wallets:A)
CWallet::GetNewDestination:B)
CWallet::GetReservedDestination:Changes:
Move
TopUp()responsibility from the wallet class to each scriptpubkeyman.So each spkm can decide to call it or not after perform the basic checks
for the new destination request.
Aside from that, remove the unused
nAccountingEntryNumberwallet field. And a duplicated descriptor type check inGetNewDestination