wallet: avoid double keypool TopUp() call on descriptor wallets #25526

pull furszy wants to merge 3 commits into bitcoin:master from furszy:2022_wallet_remove_extra_topup changing 3 files +10 −12

furszy commented at 6:58 PM on July 1, 2022: member
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
DrahtBot added the label Wallet on Jul 1, 2022
DrahtBot commented at 11:40 PM on July 1, 2022: contributor


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



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #25881 (wallet: remove unused DummySignTx and CKeyPool from GetReservedDestination by furszy)
- #24897 ([Draft / POC] Silent Payments by w0xlt)
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.
aureleoules commented at 11:39 AM on July 7, 2022: member

ACK 616a54becbdd6fdb63dbd74e6bf00b682ca308d0. 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? https://github.com/bitcoin/bitcoin/blob/616a54becbdd6fdb63dbd74e6bf00b682ca308d0/src/wallet/scriptpubkeyman.cpp#L1688-L1697
w0xlt approved
w0xlt commented at 1:47 PM on July 7, 2022: contributor

ACK https://github.com/bitcoin/bitcoin/pull/25526/commits/616a54becbdd6fdb63dbd74e6bf00b682ca308d0

It is better that only one base class (ScriptPubKeyMan) has the responsibility of calling TopUp().
DrahtBot added the label Needs rebase on Jul 12, 2022
furszy force-pushed on Jul 14, 2022
furszy commented at 7:58 PM on July 14, 2022: member

Thanks for the feedback!

Rebased on master, conflict solved. Ready to go.

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?

Good eye @aureleoules. Actually, the second error message isn't accurate, ExpandFromCache can only fail if the derivation fails or due some internal failure like a not matching xpub.
DrahtBot removed the label Needs rebase on Jul 14, 2022
DrahtBot added the label Needs rebase on Aug 10, 2022

wallet: avoid double TopUp() calls on descriptor wallets

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).

599ff5adfc

wallet: remove unused `nAccountingEntryNumber` field 76b982a4a5
wallet: remove duplicate descriptor type check in GetNewDestination bfb9b94ebe
furszy force-pushed on Aug 12, 2022
furszy commented at 3:47 PM on August 12, 2022: member

rebased, conflicts solved.
DrahtBot removed the label Needs rebase on Aug 12, 2022
furszy renamed this:
~~wallet: avoid double keypool TopUp() calls on descriptor wallets~~
wallet: avoid double keypool TopUp() call on descriptor wallets
on Aug 16, 2022
aureleoules commented at 8:28 AM on August 22, 2022: member

re-ACK bfb9b94ebefdb95ac7656836975b3d5afc428744.
in src/wallet/scriptpubkeyman.cpp:1698 in bfb9b94ebe
```
1698 | -        if (out_script_type && out_script_type == type) {
1699 | -            ExtractDestination(scripts_temp[0], dest);
1700 | -        } else {
1701 | -            throw std::runtime_error(std::string(__func__) + ": Types are inconsistent. Stored type does not match type of newly generated address");
1702 | +        if (!ExtractDestination(scripts_temp[0], dest)) {
1703 | +            return util::Error{_("Error: Cannot extract destination from the generated scriptpubkey")}; // shouldn't happen
```
theStack commented at 1:05 PM on September 19, 2022:

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:

throw std::runtime_error(std::string(__func__) + "Cannot extract destination from the generated scriptpubkey"); // shouldn't happen
furszy commented at 1:43 PM on September 19, 2022:

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.
theStack commented at 2:18 PM on September 19, 2022:

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)
theStack approved
theStack commented at 1:09 PM on September 19, 2022: contributor

Code-review ACK bfb9b94ebefdb95ac7656836975b3d5afc428744
achow101 commented at 3:23 PM on October 13, 2022: member

ACK bfb9b94ebefdb95ac7656836975b3d5afc428744
achow101 merged this on Oct 13, 2022
achow101 closed this on Oct 13, 2022
sidhujag referenced this in commit 9f2cc216eb on Oct 23, 2022
furszy deleted the branch on May 27, 2023
bitcoin locked this on May 26, 2024

Contributors

Labels

Linked (view graph)

#25881 wallet: remove unused DummySignTx and CKeyPool from GetReservedDestination

wallet: avoid double keypool TopUp() call on descriptor wallets #25526

Context:

Changes:

Conflicts