wallet: importdescriptors update existing #19651

S3RK · 2020-08-03T11:47:04Z

Rationale: allow updating existing descriptors with importdescriptors command.

Currently if you run same importdescriptors command twice with a descriptor containing private key you will get very confusing error — Missing required fields. What happens is that Wallet tries to write imported private key to the disk, but it exists already so we get DB_KEYEXIST (-30995) from BerkelyDB. Please note, that we set DB_NOOVERWRITE (I guess not to lose some keys accidentally). The exception is caught in catch (...) in rpcdump.cpp with a generic error.

With this PR if a descriptor is already present than we will update its activeness, internalness, label, range and next_index.
For the range only expansion is allowed (range start can only decrease, range end increase).

elichai · 2020-08-03T12:09:26Z

Could we change BerkeleyBatch::WriteKey to return an enum describing the error? that will be inconsistent with the current API though.
but it will allow us to print a good error for the user, as he should probably know he already have that descriptor

achow101 · 2020-08-03T16:07:31Z

I think we should go with option 2: To update the existing spkman. The keys should either already exist or are being imported at that time. I don't think it's good to add keys in memory and not write them to disk as this could potentially result in lost keys.

S3RK · 2020-08-06T08:45:22Z

Could we change BerkeleyBatch::WriteKey to return an enum describing the error? that will be inconsistent with the current API though.
but it will allow us to print a good error for the user, as he should probably know he already have that descriptor

@elichai I believe it's better to process the command rather than just show better error message, because it allows to modify some meta data: for example descriptor's range. However I added a trivial commit to make error message less misleading.

I think we should go with option 2: To update the existing spkman. The keys should either already exist or are being imported at that time. I don't think it's good to add keys in memory and not write them to disk as this could potentially result in lost keys.

@achow101 Indeed, this aligns with my intuition as well. I changed the implementation and it looks cleaner to me. Hope I got the update semantics correct for the DescriptorScriptPubKeyMan. Could you please check?

DrahtBot · 2020-08-06T13:36:22Z

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

Conflicts

No conflicts as of last run.

achow101

The commit adding the test should be after or be the same commit as the one adding the fix. This way all commits pass tests.

It would be nice if the tests checked the update behavior (e.g. new ranges, changed active-ness, etc.) but we can do that in a followup.

src/wallet/scriptpubkeyman.cpp

S3RK · 2020-08-17T11:19:33Z

The commit adding the test should be after or be the same commit as the one adding the fix. This way all commits pass tests.

It would be nice if the tests checked the update behavior (e.g. new ranges, changed active-ness, etc.) but we can do that in a followup.

@achow101 I squashed the test and the fix commits into one and extracted new function CanUpdateToWalletDescriptor to avoid code duplication. I also added some tests to verify various update scenarios: label change, range change, next_index, internal flag change, descriptor activation. A minor change were required in DescriptorScriptPubKeyMan::SetCache to make the tests pass.

Please note, descriptor deactivation is not working right now so there is no test for it. I'm currently working on it and will do as a separate PR.

S3RK · 2020-08-19T10:52:27Z

Rebased and removed 51ea4c6 as it was already added to master

src/wallet/scriptpubkeyman.cpp

achow101 · 2020-08-21T16:04:59Z

src/wallet/wallet.cpp

    auto spk_man = m_spk_managers.at(id).get();
    spk_man->SetInternal(internal);
    spk_mans[type] = spk_man;

+    if (spk_mans_other[type] == spk_man) {
+        spk_mans_other[type] = nullptr;


Why? I don't think it is necessarily wrong for a ScriptPubKeyMan to serve both internal and external addresses. We do this in the legacy wallet. Also, it seems like this should break the legacy wallet.

Two reasons:

If we don't do that, than semantics of importdescriptors changes. What I'm trying to achieve with this pull request is to make the command idempotent and allow for updates. In case when we don't remove specified SPK from previous map in the wallet (m_internal_spk_managers/m_external_spk_managers) than it's not really possible for a user to change internal flag. When she would try to do so, we would just register same descriptor in the second map. And it's not what I would expect.

This is required to keep consistency when we move descriptor from internal to external and vice versa. DescriptorScriptPubKeyMan has m_internal flag. It begs for some bugs if this flag is inconsistent with the descriptor placements in the wallet. For example it will affects KeypoolCountExternalKeys() and as a consequence the reporting of internal/external key pool sizes.

While this indeed goes against how legacy wallet operates it looks like it doesn't break it. The reason being is that legacy wallet is configured by SetupLegacyScriptPubKeyMan and don't use LoadActiveScriptPubKeyMan. I tried to verify by using a wallet from 0.16.3 and calling getnewaddress and getrawchagneaddress and both worked returning a key from the same hdseedid. Anyway, let me think how I can improve it. I'm also open from your suggestions.

P.S. I also looked at the possibility of removing m_internal but that doesn't seems possible due to the public interface of SPK. My motivation was to completely illuminate possible inconsistency between wallet and SPK in terms of internal flag. Do you think this is an idea worth exploring?

While legacy wallets do use SetupLegacyScriptPubKeyMan, they also have externalspk and internalspk entries which are handled by LoadActiveScriptPubKeyMan. So it should break legacy wallets, and I'm not sure why it doesn't.

I would prefer that the changing of the activeness and internalness is handled by another function which should also have a comment about why it should remain separate from LoadActiveScriptPubKeyMan. As it is now, I think it makes the loading code more fragile and difficult to understand.

I analyzed the code and couldn't find any uses of LoadActiveScriptPubKeyMan for legacy wallets. The function is only called when 1) we create new wallet, 2) call importdescriptors command or 3) when loading a wallet from disk. But if I'm not mistaken there is no active SPKs on disk for legacy wallets. That makes me believe this function is only relevant for descriptor wallets. So I put a guard at the top to throw an exception when we call it for legacy wallet. And all wallet functional tests pass on my laptop for now. I'm happy to reproduce any scenario in which this code should break legacy wallet.

Hmm, SetupLegacyScriptPubKeyMan adds the LegacySPKM to the maps that track internal/external, but doesn't write them to disk. I thought it wrote them to disk and now I'm wondering whether not doing so is a bug. But since it doesn't, that's why this works.

cead7ed ISTM this change should have test coverage (more so given the discussion) to spec the behavior and provide regression coverage. Edit: Ah, if I understand correctly some test coverage is added in the next commit b66b26a.

This is covered in the test section under "Check we can change descriptor internal flag"

S3RK · 2020-08-27T11:17:26Z

Added wallet: deactivate descriptor commit from #19774

src/wallet/scriptpubkeyman.cpp

src/wallet/wallet.cpp

S3RK · 2020-08-30T10:27:31Z

@achow101 thanks for your time and review. I pushed changes as fixup commits for easier incremental review, when you believe the code is ready to be merged I'll squash them into original commits.

achow101 · 2020-09-03T16:54:23Z

ACK, please squash.

S3RK · 2021-05-10T19:10:54Z

This should also fix #21716

achow101 · 2021-06-03T20:52:19Z

re-ACK 5d96704

meshcollider

Concept ACK + light code review, I'll finish reviewing soon

meshcollider · 2021-06-10T05:05:33Z

src/wallet/scriptpubkeyman.cpp

+    m_wallet_descriptor = descriptor;
+}
+
+bool DescriptorScriptPubKeyMan::CanUpdateToWalletDescriptor(const WalletDescriptor& descriptor, std::string& error)


I feel like the name of this function is confusing and could be improved.

I've spent some time, but couldn't come up with anything better. Any suggestions?

I think a start could be to drop To from the name, so that it matches UpdateWalletDescriptor <-> CanUpdateWalletDescriptor

jonatack · 2021-06-24T09:30:40Z

I'll try to review this later today.

jonatack

Concept/Approach ACK 5d96704 reviewed, debug-built and ran unit tests and wallet_importdescriptors.py on each commit. I don't know this part of the codebase well; the code and behavioral changes seem ok but my concern is if there is test coverage of all the changes in behavior.

src/wallet/scriptpubkeyman.cpp

src/wallet/wallet.cpp

src/wallet/rpcdump.cpp

jonatack · 2021-06-24T13:02:48Z

src/wallet/wallet.cpp

    auto spk_man = m_spk_managers.at(id).get();
    spk_man->SetInternal(internal);
    spk_mans[type] = spk_man;

+    if (spk_mans_other[type] == spk_man) {
+        spk_mans_other[type] = nullptr;


cead7ed ISTM this change should have test coverage (more so given the discussion) to spec the behavior and provide regression coverage. Edit: Ah, if I understand correctly some test coverage is added in the next commit b66b26a.

src/wallet/walletdb.cpp

jonatack · 2021-06-24T13:18:50Z

src/wallet/wallet.cpp

+        WalletLogPrintf("Deactivate spkMan: id = %s, type = %d, internal = %d\n", id.ToString(), static_cast<int>(type), static_cast<int>(internal));
+        WalletBatch batch(GetDatabase());
+        if (!batch.EraseActiveScriptPubKeyMan(static_cast<uint8_t>(type), internal)) {
+            throw std::runtime_error(std::string(__func__) + ": erasing active ScriptPubKeyMan id failed");


5d96704 test coverage?

Idk how to cover this. One need to introduce a low-level database error to hit this line. I'm not sure it's worth it

src/wallet/rpcdump.cpp

fjahr

Concept ACK

My comments are not that important but I agree with @meshcollider that CanUpdateToWalletDescriptor could be named better and with @jonatack that test coverage should be improved.

src/wallet/rpcdump.cpp

src/wallet/wallet.cpp

src/wallet/scriptpubkeyman.cpp

achow101 · 2021-06-28T22:41:46Z

re-ACK 3efaf83

meshcollider

Code review ACK 3efaf83

meshcollider · 2021-06-28T23:59:32Z

src/wallet/scriptpubkeyman.cpp

+    m_wallet_descriptor = descriptor;
+}
+
+bool DescriptorScriptPubKeyMan::CanUpdateToWalletDescriptor(const WalletDescriptor& descriptor, std::string& error)


I think a start could be to drop To from the name, so that it matches UpdateWalletDescriptor <-> CanUpdateWalletDescriptor

jonatack

Light ACK 3efaf83 per git range-diff a000cb0 5d96704 3efaf83 and as a sanity check, re-debug-built on debian with gcc 10.2.1 and clang 11, ran wallet_importdescriptors.py

Some touch-ups if you update.

jonatack · 2021-06-29T14:35:02Z

test/functional/wallet_importdescriptors.py

+        self.test_importdesc({**range_request, "range": [0, 10]}, wallet=wpriv, success=False,
+                             error_code=-8, error_message='new range must include current range = [0,20]')
+        self.test_importdesc({**range_request, "range": [5, 20]}, wallet=wpriv, success=False,
+                             error_code=-8, error_message='new range must include current range = [0,20]')


6737d96 suggestion, separate the salient from the redundant

- self.test_importdesc({**range_request, "range": [5, 10]}, wallet=wpriv, success=False, - error_code=-8, error_message='new range must include current range = [0,20]') - self.test_importdesc({**range_request, "range": [0, 10]}, wallet=wpriv, success=False, - error_code=-8, error_message='new range must include current range = [0,20]') - self.test_importdesc({**range_request, "range": [5, 20]}, wallet=wpriv, success=False, - error_code=-8, error_message='new range must include current range = [0,20]') + for r in [[5, 10], [0, 10], [5, 20]]: + self.test_importdesc(req={**range_request, "range": r}, wallet=wpriv, success=False, error_code=-8, error_message="new range must include current range = [0,20]")

jonatack · 2021-06-29T14:39:42Z

test/functional/wallet_importdescriptors.py

-                             success=True)
+        import_request = {"desc": descsum_create("pkh(" + key.pubkey + ")"),
+                 "timestamp": "now",
+                 "label": "Descriptor import test"}


bf68ebc formatting nit

import_request = {"desc": descsum_create("pkh(" + key.pubkey + ")"), - "timestamp": "now", - "label": "Descriptor import test"} + "timestamp": "now", + "label": "Descriptor import test"}

or

- import_request = {"desc": descsum_create("pkh(" + key.pubkey + ")"), - "timestamp": "now", - "label": "Descriptor import test"} + import_request = { + "desc": descsum_create("pkh(" + key.pubkey + ")"), + "timestamp": "now", + "label": "Descriptor import test", + }

jonatack · 2021-06-29T14:40:54Z

test/functional/wallet_importdescriptors.py

+        assert_equal(wpriv.getwalletinfo()['keypoolsize'], 21)
+        # Can keep range the same
+        self.test_importdesc({**range_request, "range": [0, 20]}, wallet=wpriv, success=True)
+        assert_equal(wpriv.getwalletinfo()['keypoolsize'], 21)


6737d96 readability nit

self.log.info("Verify we can only extend descriptor's range") range_request = {"desc": descsum_create(desc), "timestamp": "now", "range": [5, 10], 'active': True} + self.test_importdesc(range_request, wallet=wpriv, success=True) assert_equal(wpriv.getwalletinfo()['keypoolsize'], 6) + self.test_importdesc({**range_request, "range": [0, 10]}, wallet=wpriv, success=True) assert_equal(wpriv.getwalletinfo()['keypoolsize'], 11) + self.test_importdesc({**range_request, "range": [0, 20]}, wallet=wpriv, success=True) assert_equal(wpriv.getwalletinfo()['keypoolsize'], 21) + # Can keep range the same self.test_importdesc({**range_request, "range": [0, 20]}, wallet=wpriv, success=True) assert_equal(wpriv.getwalletinfo()['keypoolsize'], 21)

3efaf83 wallet: deactivate descriptor (S3RK) 6737d96 test: wallet importdescriptors update existing (S3RK) 586f1d5 wallet: maintain SPK consistency on internal flag change (S3RK) f1b7db1 wallet: don't mute exceptions in importdescriptors (S3RK) bf68ebc wallet: allow to import same descriptor twice (S3RK) Pull request description: Rationale: allow updating existing descriptors with `importdescriptors` command. Currently if you run same `importdescriptors` command twice with a descriptor containing private key you will get very confusing error — `Missing required fields`. What happens is that Wallet tries to write imported private key to the disk, but it exists already so we get `DB_KEYEXIST (-30995)` from BerkelyDB. Please note, that we set `DB_NOOVERWRITE` (I guess not to lose some keys accidentally). The exception is caught in `catch (...)` in rpcdump.cpp with a generic error. With this PR if a descriptor is already present than we will update its activeness, internalness, label, range and next_index. For the range only expansion is allowed (range start can only decrease, range end increase). ACKs for top commit: achow101: re-ACK 3efaf83 meshcollider: Code review ACK 3efaf83 jonatack: Light ACK 3efaf83 per `git range-diff a000cb0 5d96704 3efaf83` and as a sanity check, re-debug-built on debian with gcc 10.2.1 and clang 11, ran wallet_importdescriptors.py Tree-SHA512: 122c4b621d64ec8a3b625f3aed9f01a2b5cbaf2029ad0325b5ff38d67fff5cd35324335fabe2dd5169548b01b267c81be6ae0f5c834342f3d5f6eeed515c4843

Summary: Rationale: allow updating existing descriptors with `importdescriptors` command. Currently if you run same `importdescriptors` command twice with a descriptor containing private key you will get very confusing error — `Missing required fields`. What happens is that Wallet tries to write imported private key to the disk, but it exists already so we get `DB_KEYEXIST (-30995)` from BerkelyDB. Please note, that we set `DB_NOOVERWRITE` (I guess not to lose some keys accidentally). The exception is caught in `catch (...)` in rpcdump.cpp with a generic error. With this PR if a descriptor is already present than we will update its activeness, internalness, label, range and next_index. For the range only expansion is allowed (range start can only decrease, range end increase). > wallet: allow to import same descriptor twice > wallet: don't mute exceptions in importdescriptors > wallet: maintain SPK consistency on internal flag change > test: wallet importdescriptors update existing > wallet: deactivate descriptor This is a backport of [[bitcoin/bitcoin#19651 | core#19651]] Test Plan: `ninja all check-all` ``` $ ecashaddress convert mpA2Wh9dvZT7yfELq1UnrUmAoc5qCkMetg --prefix ecregtest ecregtest:qp0v86h53rc92hjrlpwzpjtdlgzsxu25svv6g40fpl ``` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D14207

fanquake added the Wallet label Aug 3, 2020

S3RK force-pushed the importdescriptors_twice branch from fcf3da0 to d8eb7b5 Compare August 6, 2020 08:40

This was referenced Aug 6, 2020

wallet: Migrate legacy wallets to descriptor wallets #19602

Merged

wallet: add parent_desc to getaddressinfo #19136

Merged

achow101 reviewed Aug 6, 2020

View reviewed changes

src/wallet/scriptpubkeyman.cpp Outdated Show resolved Hide resolved

fanquake added the Waiting for author label Aug 14, 2020

S3RK force-pushed the importdescriptors_twice branch from d8eb7b5 to 16dfa98 Compare August 17, 2020 11:11

S3RK force-pushed the importdescriptors_twice branch from 7b603d9 to 51ea4c6 Compare August 18, 2020 11:19

DrahtBot added the Needs rebase label Aug 18, 2020

fanquake removed the Waiting for author label Aug 18, 2020

S3RK force-pushed the importdescriptors_twice branch from 51ea4c6 to f06b47e Compare August 19, 2020 10:51

DrahtBot removed the Needs rebase label Aug 19, 2020

S3RK requested a review from achow101 August 21, 2020 08:04

S3RK mentioned this pull request Aug 21, 2020

wallet: deactivate descriptor #19774

Closed

achow101 reviewed Aug 21, 2020

View reviewed changes

S3RK force-pushed the importdescriptors_twice branch from f06b47e to 39e1c5a Compare August 27, 2020 11:06

S3RK changed the title ~~wallet: allow import same descriptor twice~~ wallet: importdescriptors update existing Aug 27, 2020

achow101 reviewed Aug 27, 2020

View reviewed changes

src/wallet/scriptpubkeyman.cpp Outdated Show resolved Hide resolved

achow101 reviewed Aug 27, 2020

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Aug 29, 2020

wallet: Avoid locking cs_wallet recursively #19833

Merged

S3RK force-pushed the importdescriptors_twice branch from 2bc7a7c to 908e18e Compare September 4, 2020 10:40

DrahtBot mentioned this pull request May 7, 2021

wallet: remove GetScriptPubKeyMan spam #21880

Closed

meshcollider added this to the 22.0 milestone Jun 3, 2021

meshcollider reviewed Jun 10, 2021

View reviewed changes

jonatack reviewed Jun 24, 2021

View reviewed changes

fjahr reviewed Jun 26, 2021

View reviewed changes

src/wallet/rpcdump.cpp Show resolved Hide resolved

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

src/wallet/scriptpubkeyman.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Jun 26, 2021

'internal' loop's variable hides a function's parameter of the same name #22345

Closed

S3RK added 2 commits June 28, 2021 21:37

wallet: allow to import same descriptor twice

bf68ebc

wallet: don't mute exceptions in importdescriptors

f1b7db1

S3RK force-pushed the importdescriptors_twice branch from 5d96704 to d44c9e6 Compare June 28, 2021 19:38

S3RK added 3 commits June 28, 2021 21:44

wallet: maintain SPK consistency on internal flag change

586f1d5

test: wallet importdescriptors update existing

6737d96

wallet: deactivate descriptor

3efaf83

S3RK force-pushed the importdescriptors_twice branch from d44c9e6 to 3efaf83 Compare June 28, 2021 19:45

meshcollider reviewed Jun 29, 2021

View reviewed changes

jonatack reviewed Jun 29, 2021

View reviewed changes

fanquake requested review from Sjors and instagibbs June 30, 2021 01:34

fanquake merged commit 045bb06 into bitcoin:master Jul 1, 2021

This was referenced Jul 1, 2021

GetScriptPubKeyMan spamming the logs with irrelevant message #21716

Closed

wallet: do not spam about non-existent spk managers #22334

Merged

wallet, refactor: make DescriptorScriptPubKeyMan agnostic of internal flag #20191

Merged

S3RK mentioned this pull request Jul 1, 2021

wallet: erase spkmans rather than setting to nullptr #22379

Merged

gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022

Merge 045bb06 into merged_master (Bitcoin PR bitcoin/bitcoin#19651)

3c24e4c

bitcoin locked and limited conversation to collaborators Aug 17, 2023

wallet: importdescriptors update existing #19651

wallet: importdescriptors update existing #19651

Uh oh!

Conversation

S3RK commented Aug 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elichai commented Aug 3, 2020

Uh oh!

achow101 commented Aug 3, 2020

Uh oh!

S3RK commented Aug 6, 2020

Uh oh!

DrahtBot commented Aug 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

achow101 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

S3RK commented Aug 17, 2020

Uh oh!

S3RK commented Aug 19, 2020

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

S3RK commented Aug 27, 2020

Uh oh!

Uh oh!

Uh oh!

S3RK commented Aug 30, 2020

Uh oh!

achow101 commented Sep 3, 2020

Uh oh!

S3RK commented May 10, 2021

Uh oh!

achow101 commented Jun 3, 2021

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonatack commented Jun 24, 2021

Uh oh!

jonatack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

S3RK commented Aug 3, 2020 •

edited

Loading

DrahtBot commented Aug 6, 2020 •

edited

Loading

achow101 left a comment •

edited

Loading

jonatack left a comment •

edited

Loading