wallet: importdescriptors update existing

S3RK commented at 11:47 am on August 3, 2020: contributor

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

fanquake added the label Wallet on Aug 3, 2020

elichai commented at 12:09 pm on August 3, 2020: contributor

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 commented at 4:07 pm on August 3, 2020: member

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 force-pushed on Aug 6, 2020

S3RK commented at 8:45 am on August 6, 2020: contributor

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 commented at 1:36 pm on August 6, 2020: contributor

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

Conflicts

No conflicts as of last run.

in src/wallet/scriptpubkeyman.cpp:2264 in d95d9b7060 outdated

2259+{
2260+    LOCK(cs_desc_man);
2261+    if (!HasWalletDescriptor(descriptor)) {
2262+        throw std::runtime_error(std::string(__func__) + ": can only update matching descriptor");
2263+    }
2264+    if (descriptor.range_start > m_wallet_descriptor.range_start) {

achow101 commented at 5:03 pm on August 6, 2020:

We also need to check that new range_end >= old range_end.

achow101 commented at 5:11 pm on August 6, 2020: member

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.

fanquake added the label Waiting for author on Aug 14, 2020

S3RK force-pushed on Aug 17, 2020

S3RK commented at 11:19 am on August 17, 2020: contributor

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 force-pushed on Aug 18, 2020

DrahtBot added the label Needs rebase on Aug 18, 2020

fanquake removed the label Waiting for author on Aug 18, 2020

S3RK force-pushed on Aug 19, 2020

S3RK commented at 10:52 am on August 19, 2020: contributor

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

DrahtBot removed the label Needs rebase on Aug 19, 2020

S3RK requested review from achow101 on Aug 21, 2020

in src/wallet/scriptpubkeyman.cpp:2186 in c82033b415 outdated

2182@@ -2183,7 +2183,7 @@ void DescriptorScriptPubKeyMan::SetCache(const DescriptorCache& cache)
2183         FlatSigningProvider out_keys;
2184         std::vector<CScript> scripts_temp;
2185         if (!m_wallet_descriptor.descriptor->ExpandFromCache(i, m_wallet_descriptor.cache, scripts_temp, out_keys)) {
2186-            throw std::runtime_error("Error: Unable to expand wallet descriptor from cache");
2187+            continue;

achow101 commented at 3:58 pm on August 21, 2020:

Why? Usually being unable to ExpandFromCache is a bad thing as it means the cache is not the size we are expecting.

S3RK commented at 7:42 am on August 22, 2020:

Otherwise we are not able to reuse cache when we extend the range for a descriptor. The test will fail at test/functional/wallet_importdescriptors.py:240. We will try to reuse existing cache and apply it to the new range of a bigger size.

I can’t see any downsides to just skipping over missing parts in provided cache. The blanks will be filled with TopUp() ad hoc.

If you have a strong feeling about this, we can skip cache reuse whatsoever when we update descriptor and expand from the descriptor itself and not from cache.

achow101 commented at 4:44 pm on August 22, 2020:

I believe this is unsafe when the wallet is encrypted and the descriptor uses hardened derivation. Such DescriptorScriptPubKeyMans are entirely reliant on the cache when the wallet is locked and I think there should be an expectation that updating a such a descriptor should make the specified range available, i.e. the cache is filled.

I think it’s fine to regenerate the cache from scratch when updating a descriptor like this. It would be better if just the missing entries could be added, but that might be harder to do.

S3RK commented at 11:07 am on August 27, 2020:

Done. The cache is now regenerated on import.

in src/wallet/scriptpubkeyman.cpp:2312 in a2d7106a5b outdated

2253@@ -2254,3 +2254,41 @@ const std::vector<CScript> DescriptorScriptPubKeyMan::GetScriptPubKeys() const
2254     }
2255     return script_pub_keys;
2256 }
2257+
2258+void DescriptorScriptPubKeyMan::UpdateWalletDescriptor(WalletDescriptor& descriptor)

achow101 commented at 4:01 pm on August 21, 2020:

What if a descriptor adds private keys that weren’t there before?

S3RK commented at 7:32 am on August 22, 2020:

I thought about it and concluded that shouldn’t be possible. Here is how I reasoned about it.

If private keys are disabled in the wallet we don’t have to worry at all, afaik it’s not possible to change the flag on existing wallet.
Now, if keys are enabled and we add new keys I believe the descriptor would be a new one and won’t match any existing descriptor. We find a matching descriptor to be updated by using HasWalletDescriptor. So it’s only possible to update a wallet descriptor if and only if spring representation of the descriptor matches. I would say we can only update a meta-data of a descriptor: range, next_index, active, internal flags and so on. And if there are new private keys in the new descriptor that should lead to a different string representation with corresponding new public keys.

I’m new to the codebase, so might miss something. Let me know.

achow101 commented at 4:45 pm on August 22, 2020:

The only current possibility for new private keys is a multisig descriptor. It should be possible to import a multisig descriptor that has only some of the private keys, then update the descriptor with more of the private keys in the multisig.

S3RK commented at 11:07 am on August 27, 2020:

Thanks. Changed the code to properly handle this case and added a test for it as well.

in src/wallet/wallet.cpp:3168 in b34664c1f4 outdated

4482     auto spk_man = m_spk_managers.at(id).get();
4483     spk_man->SetInternal(internal);
4484     spk_mans[type] = spk_man;
4485 
4486+    if (spk_mans_other[type] == spk_man) {
4487+        spk_mans_other[type] = nullptr;

achow101 commented at 4:04 pm on August 21, 2020:

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.

S3RK commented at 8:54 am on August 22, 2020:

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?

achow101 commented at 4:57 pm on August 22, 2020:

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.

S3RK commented at 11:15 am on August 27, 2020:

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.

achow101 commented at 3:47 pm on August 27, 2020:

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.

jonatack commented at 1:02 pm on June 24, 2021:

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.

S3RK commented at 7:41 pm on June 28, 2021:

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

S3RK force-pushed on Aug 27, 2020

S3RK commented at 11:17 am on August 27, 2020: contributor

Added wallet: deactivate descriptor commit from #19774

S3RK renamed this:
~~wallet: allow import same descriptor twice~~
wallet: importdescriptors update existing
on Aug 27, 2020

in src/wallet/scriptpubkeyman.cpp:1839 in 745c2a860f outdated

1835@@ -1836,6 +1836,10 @@ void DescriptorScriptPubKeyMan::MarkUnusedAddresses(const CScript& script)
1836 void DescriptorScriptPubKeyMan::AddDescriptorKey(const CKey& key, const CPubKey &pubkey)
1837 {
1838     LOCK(cs_desc_man);
1839+    if (m_map_keys.find(pubkey.GetID()) != m_map_keys.end()) {

achow101 commented at 4:50 pm on August 27, 2020:

I think it would be better to have this inside of AddDescriptorKeyWithDB. Additionally, it needs to check m_map_crypted_keys otherwise this won’t work when the wallet is encrypted.

S3RK commented at 9:48 am on August 28, 2020:

Done

in src/wallet/wallet.cpp:4500 in 39e1c5a6c2 outdated

4493@@ -4494,6 +4494,22 @@ void CWallet::LoadActiveScriptPubKeyMan(uint256 id, OutputType type, bool intern
4494     NotifyCanGetAddressesChanged();
4495 }
4496 
4497+void CWallet::DeactivateScriptPubKeyMan(uint256 id, OutputType type, bool internal)
4498+{
4499+    auto& spk_mans = internal ? m_internal_spk_managers : m_external_spk_managers;
4500+    if (spk_mans[type] && spk_mans[type]->GetID() == id) {

achow101 commented at 4:54 pm on August 27, 2020:

I think using [] for access here could be problematic if the type isn’t already in the map (I think that can happen). I would prefer using find here and then if it works, checking that the returned object is not nullptr.

S3RK commented at 9:43 am on August 28, 2020:

I’m not a cpp expert, but the docs say in case when there is no such key a default value will be inserted.

std::map<Key,T,Compare,Allocator>::operator[] Returns a reference to the value that is mapped to a key equivalent to key, performing an insertion if such key does not already exist.

So I believe it’s safe in this particular case.

I still prefer spk_mans[type] over spk_mans.find(type) != spk_mans.end() && spk_mans[type] != nullptr but if that’s going against the norms for the project I can change to whatever is considered standard way.

achow101 commented at 3:56 pm on August 28, 2020:

Right, but the point is that we don’t want a default value inserted.

It might be easier to just use GetScriptPubKeyMan here.

S3RK commented at 10:23 am on August 30, 2020:

Done

S3RK commented at 10:27 am on August 30, 2020: contributor

@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 commented at 4:54 pm on September 3, 2020: member

ACK, please squash.

S3RK force-pushed on Sep 4, 2020

S3RK requested review from achow101 on Sep 9, 2020

achow101 commented at 9:12 pm on September 9, 2020: member

ACK 908e18e79847155144111bafa2e6e5dbd5c6511c

DrahtBot added the label Needs rebase on Nov 9, 2020

S3RK force-pushed on Dec 3, 2020

DrahtBot removed the label Needs rebase on Dec 3, 2020

achow101 approved

achow101 commented at 7:33 pm on December 24, 2020: member

re-ACK e06c123bbcb3be5b7cea4a9480af48c8b54daa83

DrahtBot added the label Needs rebase on Feb 18, 2021

S3RK force-pushed on Apr 9, 2021

DrahtBot removed the label Needs rebase on Apr 9, 2021

DrahtBot added the label Needs rebase on May 3, 2021

S3RK force-pushed on May 3, 2021

DrahtBot removed the label Needs rebase on May 3, 2021

S3RK commented at 7:10 pm on May 10, 2021: contributor

This should also fix #21716

achow101 commented at 8:52 pm on June 3, 2021: member

re-ACK 5d9670462219b8c753516bfbdc5c7fa23046cd07

meshcollider added this to the milestone 22.0 on Jun 3, 2021

in src/wallet/scriptpubkeyman.cpp:2326 in fed9eef48d outdated

2299+    m_map_script_pub_keys.clear();
2300+    m_max_cached_index = -1;
2301+    m_wallet_descriptor = descriptor;
2302+}
2303+
2304+bool DescriptorScriptPubKeyMan::CanUpdateToWalletDescriptor(const WalletDescriptor& descriptor, std::string& error)

meshcollider commented at 5:05 am on June 10, 2021:

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

S3RK commented at 7:38 pm on June 28, 2021:

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

meshcollider commented at 11:59 pm on June 28, 2021:

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

meshcollider commented at 5:12 am on June 10, 2021: contributor

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

jonatack commented at 9:30 am on June 24, 2021: contributor

I’ll try to review this later today.

in src/wallet/scriptpubkeyman.cpp:2312 in 5d96704622 outdated

2307+    if (!HasWalletDescriptor(descriptor)) {
2308+        error = "can only update matching descriptor";
2309+        return false;
2310+    }
2311+    // Use inclusive range for error
2312+    auto range = strprintf("current range = [%d,%d]", m_wallet_descriptor.range_start, m_wallet_descriptor.range_end-1);

jonatack commented at 12:30 pm on June 24, 2021:

nit, clang format

0    auto range = strprintf("current range = [%d,%d]", m_wallet_descriptor.range_start, m_wallet_descriptor.range_end - 1);

fjahr commented at 11:47 am on June 26, 2021:

nit: I prefer to not create helper variables outside of error handling that are only used inside of error handling so I would inline this. I also think this code be reorganized slightly to make the error cumulative message, i.e. if both errors are hit the error message can include both messages about range_start and range_end.

S3RK commented at 7:39 pm on June 28, 2021:

Restructured the code

in src/wallet/wallet.cpp:4629 in 5d96704622 outdated

4624@@ -4625,12 +4625,38 @@ void CWallet::AddActiveScriptPubKeyMan(uint256 id, OutputType type, bool interna
4625 
4626 void CWallet::LoadActiveScriptPubKeyMan(uint256 id, OutputType type, bool internal)
4627 {
4628+    if (!IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) {
4629+        throw std::runtime_error("Cannot LoadActiveScriptPubKeyMan to a non-descriptor wallet\n");

jonatack commented at 12:56 pm on June 24, 2021:

cead7ed This doesn’t seem to be covered by any tests?

fjahr commented at 11:27 am on June 26, 2021:

Is the newline needed here? Doesn’t seem to be the standard.

S3RK commented at 7:40 pm on June 28, 2021:

This is just a guard and should be never hit unless there is a logical error in the code. Replaced with Assert and added a comment

in src/wallet/rpcdump.cpp:1578 in 5d96704622 outdated

1576-    } catch (...) {
1577+    } catch (const std::runtime_error& e) {
1578         result.pushKV("success", UniValue(false));
1579-
1580-        result.pushKV("error", JSONRPCError(RPC_MISC_ERROR, "Missing required fields"));
1581+        result.pushKV("error", JSONRPCError(RPC_MISC_ERROR, e.what()));

jonatack commented at 12:57 pm on June 24, 2021:

c90f0279 can you extend the test coverage to show the descriptive errors and spec the change?

S3RK commented at 7:40 pm on June 28, 2021:

Hm… thanks for pointing to that. Now looked back at that piece, I decided it’s better to just drop it.

in src/wallet/walletdb.cpp:214 in 5d96704622 outdated

208@@ -209,6 +209,12 @@ bool WalletBatch::WriteActiveScriptPubKeyMan(uint8_t type, const uint256& id, bo
209     return WriteIC(make_pair(key, type), id);
210 }
211 
212+bool WalletBatch::EraseActiveScriptPubKeyMan(uint8_t type, bool internal)
213+{
214+    std::string key = internal ? DBKeys::ACTIVEINTERNALSPK : DBKeys::ACTIVEEXTERNALSPK;

jonatack commented at 1:14 pm on June 24, 2021:

5d96704 style nit

0    const std::string key{internal ? DBKeys::ACTIVEINTERNALSPK : DBKeys::ACTIVEEXTERNALSPK};

S3RK commented at 7:41 pm on June 28, 2021:

done

in src/wallet/wallet.cpp:3181 in 5d96704622 outdated

4648+    auto spk_man = GetScriptPubKeyMan(type, internal);
4649+    if (spk_man != nullptr && spk_man->GetID() == id) {
4650+        WalletLogPrintf("Deactivate spkMan: id = %s, type = %d, internal = %d\n", id.ToString(), static_cast<int>(type), static_cast<int>(internal));
4651+        WalletBatch batch(GetDatabase());
4652+        if (!batch.EraseActiveScriptPubKeyMan(static_cast<uint8_t>(type), internal)) {
4653+            throw std::runtime_error(std::string(__func__) + ": erasing active ScriptPubKeyMan id failed");

jonatack commented at 1:18 pm on June 24, 2021:

5d96704 test coverage?

S3RK commented at 7:42 pm on June 28, 2021:

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

in src/wallet/rpcdump.cpp:1590 in 5d96704622 outdated

1562@@ -1564,16 +1563,19 @@ static UniValue ProcessDescriptorImport(CWallet& wallet, const UniValue& data, c
1563             } else {
1564                 wallet.AddActiveScriptPubKeyMan(spk_manager->GetID(), *w_desc.descriptor->GetOutputType(), internal);
1565             }
1566+        } else {
1567+            if (w_desc.descriptor->GetOutputType()) {
1568+                wallet.DeactivateScriptPubKeyMan(spk_manager->GetID(), *w_desc.descriptor->GetOutputType(), internal);
1569+            }

jonatack commented at 1:21 pm on June 24, 2021:

5d9670462219b8c753516bfbdc5c7fa23046cd07 does this have test coverage?

S3RK commented at 7:42 pm on June 28, 2021:

This is covered in the section “Check can deactivate active descriptor”. You can try deleting this lines and the test will fail

jonatack commented at 1:29 pm on June 24, 2021: contributor

Concept/Approach ACK 5d9670462219b8c753516bfbdc5c7fa23046cd07 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.

in src/wallet/rpcdump.cpp:1570 in c90f027940 outdated

1545@@ -1546,7 +1546,7 @@ static UniValue ProcessDescriptorImport(CWallet& wallet, const UniValue& data, c
1546         auto existing_spk_manager = wallet.GetDescriptorScriptPubKeyMan(w_desc);
1547         if (existing_spk_manager) {
1548             if (!existing_spk_manager->CanUpdateToWalletDescriptor(w_desc, error)) {
1549-                throw JSONRPCError(RPC_INVALID_PARAMS, error);
1550+                throw JSONRPCError(RPC_INVALID_PARAMETER, error);

fjahr commented at 11:24 am on June 26, 2021:

This could just be fixed in the previous commit where this line is added

S3RK commented at 7:43 pm on June 28, 2021:

Done

fjahr commented at 11:52 am on June 26, 2021: contributor

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.

wallet: allow to import same descriptor twice bf68ebc1cd

wallet: don't mute exceptions in importdescriptors f1b7db1474

S3RK force-pushed on Jun 28, 2021

wallet: maintain SPK consistency on internal flag change 586f1d53d6

test: wallet importdescriptors update existing 6737d9655b

wallet: deactivate descriptor 3efaf83c75

S3RK force-pushed on Jun 28, 2021

achow101 commented at 10:41 pm on June 28, 2021: member

re-ACK 3efaf83c75cd8dc2fa084537b8ed6715fb58c04d

meshcollider commented at 0:10 am on June 29, 2021: contributor

Code review ACK 3efaf83c75cd8dc2fa084537b8ed6715fb58c04d

in test/functional/wallet_importdescriptors.py:275 in 3efaf83c75

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

jonatack commented at 2:35 pm on June 29, 2021:

6737d965 suggestion, separate the salient from the redundant

0-        self.test_importdesc({**range_request, "range": [5, 10]}, wallet=wpriv, success=False,
1-                             error_code=-8, error_message='new range must include current range = [0,20]')
2-        self.test_importdesc({**range_request, "range": [0, 10]}, wallet=wpriv, success=False,
3-                             error_code=-8, error_message='new range must include current range = [0,20]')
4-        self.test_importdesc({**range_request, "range": [5, 20]}, wallet=wpriv, success=False,
5-                             error_code=-8, error_message='new range must include current range = [0,20]')
6+        for r in [[5, 10], [0, 10], [5, 20]]:
7+            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]")

in test/functional/wallet_importdescriptors.py:93 in 3efaf83c75

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

jonatack commented at 2:39 pm on June 29, 2021:

bf68ebc formatting nit

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

or

0-        import_request = {"desc": descsum_create("pkh(" + key.pubkey + ")"),
1-                 "timestamp": "now",
2-                 "label": "Descriptor import test"}
3+        import_request = {
4+            "desc": descsum_create("pkh(" + key.pubkey + ")"),
5+            "timestamp": "now",
6+            "label": "Descriptor import test",
7+        }

in test/functional/wallet_importdescriptors.py:268 in 3efaf83c75

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

jonatack commented at 2:40 pm on June 29, 2021:

6737d96 readability nit

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

jonatack commented at 2:45 pm on June 29, 2021: contributor

Light ACK 3efaf83c75cd8dc2fa084537b8ed6715fb58c04d 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.

fanquake requested review from Sjors on Jun 30, 2021

fanquake requested review from instagibbs on Jul 1, 2021

fanquake merged this on Jul 1, 2021

fanquake closed this on Jul 1, 2021

sidhujag referenced this in commit a57e9dc15b on Jul 1, 2021

gwillen referenced this in commit 3c24e4c1b5 on Jun 1, 2022

Fabcien referenced this in commit 33d78b3c55 on Jul 5, 2023

bitcoin locked this on Aug 17, 2023

wallet: importdescriptors update existing #19651

Conflicts