This adds support for key origin information to the descriptor parser, and exposes the resulting key path information through FlatSigningProvider.
There is no observable functionality from this right now, except having the scantxoutset RPC accept descriptors that include key origin information.
Longer term this feature helps with a potential descriptors-based walletless PSBT updater, or for importing hardware wallet xpubs (once the wallet can import descriptors).
40 | @@ -41,7 +41,7 @@ struct PubkeyProvider 41 | virtual ~PubkeyProvider() = default; 42 | 43 | /** Derive a public key. */ 44 | - virtual bool GetPubKey(int pos, const SigningProvider& arg, CPubKey& out) const = 0; 45 | + virtual bool GetPubKey(int pos, const SigningProvider& arg, std::pair<CPubKey, KeyOriginInfo>& out) const = 0;
IMO could be ..., CPubKey& pubkey, KeyOriginInfo& origin). The code below is not very clear:
for (auto& entry : entries) {
pubkeys.push_back(entry.first);
out.origins.emplace(entry.first.GetID(), std::move(entry.second));
out.pubkeys.emplace(entry.first.GetID(), std::move(entry.first));
}
Agree, that's actually simpler. Done.
325 | } 326 | - for (const CPubKey& key : pubkeys) { 327 | - out.pubkeys.emplace(key.GetID(), std::move(key)); 328 | + std::vector<CPubKey> pubkeys; 329 | + pubkeys.reserve(entries.size()); 330 | + for (auto& entry : entries) {
Why 2 iterations? Couldn't this be:
std::vector<CPubKey> pubkeys;
pubkeys.reserve(m_providers.size());
for (const auto& p : m_providers) {
CPubKey pubkey;
KeyOriginInfo origin;
if (!p->GetPubKey(pos, arg, pubkey, origin)) return false;
pubkeys.push_back(pubkey);
out.origins.emplace(pubkey.GetID(), std::move(origin));
out.pubkeys.emplace(pubkey.GetID(), std::move(pubkey));
}
(considering the above suggestion)
I don't like interfaces that populate output argument when failure occurs.
Ah agree on that.
I also couldn't see why the code was written this way. Maybe add comment for entries like /* Temporary entries, to avoid modifying out in case of failure */.
utACK e11d7ea24b7c67793ab8e84b5ad05d233f43e7da
102 | @@ -100,9 +103,13 @@ void Check(const std::string& prv, const std::string& pub, int flags, const std:
103 | BOOST_CHECK_MESSAGE(SignSignature(Merge(keys_priv, script_provider), spks[n], spend, 0, 1, SIGHASH_ALL), prv);
104 | }
105 | }
Maybe add a comment:
// Check if origin information (master key fingerprint, derivation path) is present
It's actually not obvious to me how this test checks that an origin string passed into the test is actually stored correctly.
Added some comments.
146 | - Check("pk(xprv9uPDJpEQgRQfDcW7BkF7eTya6RPxXeJCqCJGHuCJ4GiRVLzkTXBAJMu2qaMWPrS7AANYqdq6vcBcBUdJCVVFceUvJFjaPdGZ2y9WACViL4L/0)", "pk(xpub68NZiKmJWnxxS6aaHmn81bvJeTESw724CRDs6HbuccFQN9Ku14VQrADWgqbhhTHBaohPX4CjNLf9fq9MYo6oDaPPLPxSb7gwQN3ih19Zm4Y/0)", DEFAULT, {{"210379e45b3cf75f9c5f9befd8e9506fb962f6a9d185ac87001ec44a8d3df8d4a9e3ac"}}); 147 | - Check("pkh(xprv9s21ZrQH143K31xYSDQpPDxsXRTUcvj2iNHm5NUtrGiGG5e2DtALGdso3pGz6ssrdK4PFmM8NSpSBHNqPqm55Qn3LqFtT2emdEXVYsCzC2U/2147483647'/0)", "pkh(xpub661MyMwAqRbcFW31YEwpkMuc5THy2PSt5bDMsktWQcFF8syAmRUapSCGu8ED9W6oDMSgv6Zz8idoc4a6mr8BDzTJY47LJhkJ8UB7WEGuduB/2147483647'/0)", HARDENED, {{"76a914ebdc90806a9c4356c1c88e42216611e1cb4c1c1788ac"}}); 148 | - Check("wpkh(xprv9vHkqa6EV4sPZHYqZznhT2NPtPCjKuDKGY38FBWLvgaDx45zo9WQRUT3dKYnjwih2yJD9mkrocEZXo1ex8G81dwSM1fwqWpWkeS3v86pgKt/1/2/*)", "wpkh(xpub69H7F5d8KSRgmmdJg2KhpAK8SR3DjMwAdkxj3ZuxV27CprR9LgpeyGmXUbC6wb7ERfvrnKZjXoUmmDznezpbZb7ap6r1D3tgFxHmwMkQTPH/1/2/*)", RANGE, {{"0014326b2249e3a25d5dc60935f044ee835d090ba859"},{"0014af0bd98abc2f2cae66e36896a39ffe2d32984fb7"},{"00141fa798efd1cbf95cebf912c031b8a4a6e9fb9f27"}}); 149 | - Check("sh(wpkh(xprv9s21ZrQH143K3QTDL4LXw2F7HEK3wJUD2nW2nRk4stbPy6cq3jPPqjiChkVvvNKmPGJxWUtg6LnF5kejMRNNU3TGtRBeJgk33yuGBxrMPHi/10/20/30/40/*'))", "sh(wpkh(xpub661MyMwAqRbcFtXgS5sYJABqqG9YLmC4Q1Rdap9gSE8NqtwybGhePY2gZ29ESFjqJoCu1Rupje8YtGqsefD265TMg7usUDFdp6W1EGMcet8/10/20/30/40/*'))", RANGE | HARDENED, {{"a9149a4d9901d6af519b2a23d4a2f51650fcba87ce7b87"},{"a914bed59fc0024fae941d6e20a3b44a109ae740129287"},{"a9148483aa1116eb9c05c482a72bada4b1db24af654387"}}); 150 | - Check("combo(xprvA2JDeKCSNNZky6uBCviVfJSKyQ1mDYahRjijr5idH2WwLsEd4Hsb2Tyh8RfQMuPh7f7RtyzTtdrbdqqsunu5Mm3wDvUAKRHSC34sJ7in334/*)", "combo(xpub6FHa3pjLCk84BayeJxFW2SP4XRrFd1JYnxeLeU8EqN3vDfZmbqBqaGJAyiLjTAwm6ZLRQUMv1ZACTj37sR62cfN7fe5JnJ7dh8zL4fiyLHV/*)", RANGE, {{"2102df12b7035bdac8e3bab862a3a83d06ea6b17b6753d52edecba9be46f5d09e076ac","76a914f90e3178ca25f2c808dc76624032d352fdbdfaf288ac","0014f90e3178ca25f2c808dc76624032d352fdbdfaf2","a91408f3ea8c68d4a7585bf9e8bda226723f70e445f087"},{"21032869a233c9adff9a994e4966e5b821fd5bac066da6c3112488dc52383b4a98ecac","76a914a8409d1b6dfb1ed2a3e8aa5e0ef2ff26b15b75b788ac","0014a8409d1b6dfb1ed2a3e8aa5e0ef2ff26b15b75b7","a91473e39884cb71ae4e5ac9739e9225026c99763e6687"}}); 151 | + Check("combo(01234567:xprvA1RpRA33e1JQ7ifknakTFpgNXPmW2YvmhqLQYMmrj4xJXXWYpDPS3xz7iAxn8L39njGVyuoseXzU6rcxFLJ8HFsTjSyQbLYnMpCqE2VbFWc)", "combo(01234567:xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL)", SIGNABLE, {{"2102d2b36900396c9282fa14628566582f206a5dd0bcc8d5e892611806cafb0301f0ac","76a91431a507b815593dfc51ffc7245ae7e5aee304246e88ac","001431a507b815593dfc51ffc7245ae7e5aee304246e","a9142aafb926eb247cb18240a7f4c07983ad1f37922687"}});
In BIP32 this test vector is m/0/2147483647'/1/2147483646' derived from master seed fffcf9f.... It would be better to use that for the origin info.
Meh.
This is not testing BIP32; it's just reusing the BIP32 tests because that was an easy place to find example xpubs/xprvs.
Concept ACK.
For me this would be easier to review when combined with a practical usage, like creating a PSBT with the origin info still in it. That could just be a WIP PR as it might involve quite a few changes, since descriptors currently are only used with scantxoutset.
42 | @@ -43,9 +43,12 @@ std::string MaybeUseHInsteadOfApostrophy(std::string ret) 43 | return ret; 44 | } 45 | 46 | -void Check(const std::string& prv, const std::string& pub, int flags, const std::vector<std::vector<std::string>>& scripts) 47 | +static const std::set<std::vector<uint32_t>> ONLY_EMPTY{{}};
warning: 'ONLY_EMPTY' is a static definition in anonymous namespace; static is redundant here [readability-static-definition-in-anonymous-namespace]
Fixed.
57 | @@ -58,10 +58,12 @@ struct FlatSigningProvider final : public SigningProvider 58 | { 59 | std::map<CScriptID, CScript> scripts; 60 | std::map<CKeyID, CPubKey> pubkeys; 61 | + std::map<CKeyID, KeyOriginInfo> origins; 62 | std::map<CKeyID, CKey> keys; 63 | 64 | bool GetCScript(const CScriptID& scriptid, CScript& script) const override; 65 | bool GetPubKey(const CKeyID& keyid, CPubKey& pubkey) const override; 66 | + bool GetKeyOrigin(const CKeyID& id, KeyOriginInfo& info) const override;
warning: function 'FlatSigningProvider::GetKeyOrigin' has a definition with different parameter names [readability-inconsistent-declaration-parameter-name]
Fixed.
259 | @@ -221,8 +260,10 @@ class SingleKeyDescriptor final : public Descriptor 260 | bool Expand(int pos, const SigningProvider& arg, std::vector<CScript>& output_scripts, FlatSigningProvider& out) const override 261 | { 262 | CPubKey key; 263 | - if (!m_provider->GetPubKey(pos, arg, key)) return false; 264 | + KeyOriginInfo info; 265 | + if (!m_provider->GetPubKey(pos, arg, key, info)) return false; 266 | output_scripts = std::vector<CScript>{m_script_fn(key)}; 267 | + out.origins.emplace(key.GetID(), std::move(info)); 268 | out.pubkeys.emplace(key.GetID(), std::move(key));
warning: std::move of the variable 'key' of the trivially-copyable type 'CPubKey' has no effect; remove std::move() [hicpp-move-const-arg]
Fixed.
330 | + std::vector<CPubKey> pubkeys; 331 | + pubkeys.reserve(entries.size()); 332 | + for (auto& entry : entries) { 333 | + pubkeys.push_back(entry.first); 334 | + out.origins.emplace(entry.first.GetID(), std::move(entry.second)); 335 | + out.pubkeys.emplace(entry.first.GetID(), std::move(entry.first));
warning: std::move of the expression of the trivially-copyable type 'CPubKey' has no effect; remove std::move() [performance-move-const-arg]
Fixed.
172 | @@ -139,6 +173,11 @@ class BIP32PubkeyProvider final : public PubkeyProvider 173 | assert(m_derive != DeriveType::HARDENED); 174 | out = key.pubkey; 175 | } 176 | + CKeyID keyid = m_extkey.pubkey.GetID(); 177 | + std::copy(keyid.begin(), keyid.begin() + 4, info.fingerprint); 178 | + info.path = m_path; 179 | + if (m_derive == DeriveType::UNHARDENED) info.path.push_back(pos);
2018-09-25 20:23:55 clang(pr=14150): script/descriptor.cpp:179:69: warning: implicit conversion changes signedness: 'int' to 'std::vector<unsigned int, std::allocator<unsigned int> >::value_type' (aka 'unsigned int') [-Wsign-conversion]
Fixed.
Rebased, and expanded documentation to include key origin information.
Was there a problem with the last rebase? It seems like some changes that were previously made, like #14150 (review), have been reverted.
Concept ACK, will wait for the rebase to be fixed before reviewing then
@ryanofsky Wow, indeed, thanks for pointing that out. I don't know how that happened.
I went through all comments again, and reapplied the fixed I made earlier where necessary. I've also expanded the documentation change a bit.
542 | + if (slash_split[0].size() != 8) return nullptr; 543 | + std::string fpr_hex = std::string(slash_split[0].begin(), slash_split[0].end()); 544 | + if (!IsHex(fpr_hex)) return nullptr; 545 | + auto fpr_bytes = ParseHex(fpr_hex); 546 | + KeyOriginInfo info; 547 | + std::copy(fpr_bytes.begin(), fpr_bytes.end(), info.fingerprint);
Maybe assert(fpr_bytes.size() == sizeof(info.fingerprint)) here. Or static_assert(sizeof(info.fingerprint) == 4). It's clear this is correct right now, but since this is copying into a raw array, you could imagine small changes that would lead this code to silently corrupt memory in the future.
Done, both.
321 | - CPubKey key; 322 | - if (!p->GetPubKey(pos, arg, key)) return false; 323 | - pubkeys.push_back(key); 324 | + std::pair<CPubKey, KeyOriginInfo> keyinfo; 325 | + if (!p->GetPubKey(pos, arg, keyinfo.first, keyinfo.second)) return false; 326 | + entries.emplace_back(std::move(keyinfo));
Could maybe simplify and avoid a copy with:
entries.emplace_back();
if (!p->GetPubKey(pos, arg, entries.back().first, entries.back().second)) return false;
Done.
61 | + KeyOriginInfo m_origin; 62 | + std::unique_ptr<PubkeyProvider> m_provider; 63 | + 64 | + std::string OriginString() const 65 | + { 66 | + return HexStr(m_origin.fingerprint, m_origin.fingerprint + 4) + FormatKeyPath(m_origin.path);
Ignore this comment if you just have a different style preference, but the hardcoded + 4's in this PR do not seem like the safest thing to me. (I also don't love the raw 0x80000000UL constants.)
Would you consider using HexStr(std::begin(m_origin.fingerprint), std::end(m_origin.fingerprint)) here? begin/end could also work in the std::copy calls.
Another option could be to add a KEY_FINGERPRINT_SIZE constant that replaces all the 4's everywhere and is used in KeyOriginInfo.
I've added std::begin/std::end in a number of places, and replaced the other 4s with sizeof(info.fingerprint). In C++17 we can use std::size(info.fingerprint) which would remain correct if the elements of the fingerprint weren't sizeof()==1, but that's too much work now.
33 | @@ -34,6 +34,7 @@ Output descriptors currently support: 34 | - `sh(wsh(multi(1,03f28773c2d975288bc7d1d205c3748651b075fbc6610e58cddeeddf8f19405aa8,03499fdf9e895e719cfd64e67f07d38e3226aa7b63678949e6e49b241a60e823e4,02d7924d4f7d43ea965a465ae3095ff41131e5946f3c85f79e44adbcf8e27e080e)))` represents a P2SH-P2WSH *1-of-3* multisig. 35 | - `pk(xpub661MyMwAqRbcFtXgS5sYJABqqG9YLmC4Q1Rdap9gSE8NqtwybGhePY2gZ29ESFjqJoCu1Rupje8YtGqsefD265TMg7usUDFdp6W1EGMcet8)` refers to a single P2PK output, using the public key part from the specified xpub. 36 | - `pkh(xpub68Gmy5EdvgibQVfPdqkBBCHxA5htiqg55crXYuXoQRKfDBFA1WEjWgP6LHhwBZeNK1VTsfTFUHCdrfp1bgwQ9xv5ski8PX9rL2dZXvgGDnw/1'/2)` refers to a single P2PKH output, using child key *1'/2* of the specified xpub. 37 | +- `pkh(d34db33f/44'/0'/0':xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL/1/*)` refers to a chain of P2PKH outputs, but additionally specifies that the specified xpub is a child of a master with fingerprint `d34db33f`, and derived using path `44'/0'/0'`.
This syntax is slightly weird to me because if you're looking at an expression like:
fingerprint/path1:xpub/path2
The fingerprint:path1 part only describes xpub, not xpub/path2, but there's no logical precedence of : and / operators that would lead me to see:
(fingerprint/path1:xpub)/path2
instead of:
(fingerprint/path1):(xpub/path2)
Maybe this is an unimportant aesthetic objection, but I'd almost rather there be an explicit origin function like:
origin(fingerprint/path1, xpub)/path2
or
origin(xpub, fingerprint/path1)/path2
that at least would group things together correctly.
537 | +{ 538 | + auto colon_split = Split(sp, ':'); 539 | + if (colon_split.size() > 2) return nullptr; 540 | + if (colon_split.size() == 1) return ParsePubkeyInner(colon_split[0], permit_uncompressed, out); 541 | + auto slash_split = Split(colon_split[0], '/'); 542 | + if (slash_split[0].size() != 8) return nullptr;
Could add CheckUnparsable checks for this condition and the !IsHex one below. Also maybe the !provider one.
Done.
utACK 3fdc704f06bb5618cb63c248b20447026509e00d. Left some comments below that aren't critical and you should feel free to ignore.
@ryanofsky Addressed all your comments except the syntax issue, which we should probably discuss at the PR level rather than buried inside a code comment.
I agree that fpr/path:key/path doesn't perfectly map with intuition about precedence of operation. On the other hand, I'd prefer to keep "key operations" as pure syntax, separated from "script operations" which are functions.
After thinking a bit more about it, how do you feel about fpr/path[key]/path? That way the entire expression maintains the order of path elements, and it just looks like you're "clarifying" the actual key somewhere inside the path itself, rather than as a separate thing prefixed onto it.
An alternative which is somewhat easier to parse I think is [fpr/path]key/path, which can still be read as "([fpr/path]key)/path".
utACK 71cbcd9da8b31cb7cbe19001df32c8e1c46a603b. Only changes since last review were suggested changes (replacing +4's, adding a few test cases and a comment).
I think I like both of the [fpr/path]key/path and fpr/path[key]/path syntaxes more than the current fpr/path:key/path syntax and more than my origin() suggestion, and I agree that the [fpr/path]key/path version is probably more understandable.
I might also propose a more generalized syntax for adding attributes to keys like:
key[origin=fpr/path, device=myhardwarewallet]/path
Which might have other uses like:
key[birthday=timestamp]
But this might be too heavyweight. Anyway, you've thought about this a lot more than I have and I think whatever syntax you like, including the current syntax, should be fine.
Longer term this feature helps with a potential descriptors-based walletless PSBT updater, or for importing hardware wallet xpubs (once the wallet can import descriptors).
Could you explain how these use-cases benefit from having origin information embedded inside descriptor expressions? It seems like it might be simpler if the descriptor language only described how things should be signed, and if key information was stored (or passed) in a separate a key id -> key metadata mapping.
@ryanofsky A PSBT updater needs to add key origin information to the PSBT file, or future signers may not be able to find the key to sign with.
I really think this information needs to be part of descriptors for them to be generally useful. Just listing the public keys is useless if those using the information don't know how to find the private key corresponding to said public key. In a way, that derivation information should from our point of view be treated as part of the public information about that key.
Another argument is so that you can 'specialize' a ranged descriptor to an individual one (like what may be desirable for #14503), without losing information. With origin support you can go from xpub/a/b/c/* to [fpr:a/b/c/i]pubkey when specializing for position i.
Switched to the [fpr/path]key/path notation, updating the parser, serializer, tests, and documentation.
As far as other attributes go, I'm not sure descriptors are the right place (this may be getting philosophical, though):
My preference is keeping those two as metadata on the descriptor (together with things like gap limit/range, whether the outputs are treated as change, ...). The same could be done with key origin information, but it feels more fundamental to not lose that information.
If there are generally useful properties of keys or path elements later we can still adopt such a syntax, though; it looks pretty readable.
532 | @@ -484,6 +533,28 @@ std::unique_ptr<PubkeyProvider> ParsePubkey(const Span<const char>& sp, bool per 533 | return MakeUnique<BIP32PubkeyProvider>(extpubkey, std::move(path), type); 534 | } 535 | 536 | +/** Parse a public key including origin information (if enabled). */ 537 | +std::unique_ptr<PubkeyProvider> ParsePubkey(const Span<const char>& sp, bool permit_uncompressed, FlatSigningProvider& out) 538 | +{ 539 | + auto colon_split = Split(sp, ']');
It would be good to rename colon_split, since colon is no longer used. Maybe origin_split.
Fixed.
62 | - - The usage of hardened derivation steps requires providing the private key. 63 | - - Instead of a `'`, the suffix `h` can be used to denote hardened derivation. 64 | +- Optionally, key origin information, consisting of: 65 | + - An open bracket `[` 66 | + - 8 hex characters for the fingerprint 67 | + - Followed by zero or more "/NUM" or "/NUM'" path elements to indicate unhardened or hardened derivation steps between the fingerprint and the key that follows
It might be more accurate to replace "key that follows" with "key or xpub/xprv root that follows" since the derivation steps are just for the root, not the full KEY expression.
Fixed.
Using backticks (`) rather than quotes around /NUM and /NUM' is better for clarity, like below
Also tiny nit, typo in commit message.
Fixed.
Will fix typo when squashing.
61 | - - Optionally followed by a single `/*` or `/*'` final step to denote all (direct) unhardened or hardened children. 62 | - - The usage of hardened derivation steps requires providing the private key. 63 | - - Instead of a `'`, the suffix `h` can be used to denote hardened derivation. 64 | +- Optionally, key origin information, consisting of: 65 | + - An open bracket `[` 66 | + - 8 hex characters for the fingerprint
Would it be clearer to replace "fingerprint" with "extended key identifier"? Could also link to https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki#key-identifiers if so.
It's specifically "The first 32 bits of the identifier are called the key fingerprint.", not the entire identifier.
I think "master fingerprint" is more specific if we're going to change this.
It isn't required to be the master fingerprint; just whatever key the device with the private key is on needs to derivate from. I guess most commonly that will be the master, but it could be a child key as well.
right, good point, I retract and ACK current language
I've expanded the comment slightly anyway.
utACK cfc812992da86d00406072fe8d3e7e2611c67882. Only change since last review is syntax update.
Thanks for the explanations. I can see why you wouldn't want to make birthdays a property of keys, and it's also very clear that a PBST API which takes descriptors will be a lot clumsier if key origin information has to be passed in a separate table, instead of just embedded in the descriptors.
concept ACK, if rebase is fixed I'll review further
if rebase is fixed I'll review further
I don't think there's a problem with current version of this PR (cfc812992da86d00406072fe8d3e7e2611c67882). The earlier comment #14150 (comment) was referring to 605367580b582a6e9510a06670cfd4898168cea5.
Oops, I reviewed 2 days ago and didn't refresh this page to see updates!
123 | +For example, when following BIP44, it would be useful to describe a 124 | +change chain directly as `xpub.../44'/0'/0'/1/i` where `xpub...` 125 | +corresponds with the master key `m`. Unfortunately, since there are 126 | +hardened derivation steps that follow the xpub, this descriptor does not 127 | +let you compute scripts without access to the corresponding private keys. 128 | +Instead, it should be written as `xpub.../1/*`, where xpub corresponds to
ultra-micro nit: you used i earlier in the paragraph, and now * to describe the chain.
Fixed, this was from a much earlier version.
81 | @@ -82,7 +82,7 @@ def run_test(self): 82 | assert_equal(self.nodes[0].scantxoutset("start", [ "combo(tprv8ZgxMBicQKsPd7Uf69XL1XwhmjHopUGep8GuEiJDZmbQz6o58LninorQAfcKZWARbtRtfnLcJ5MQ2AtHcQJCCRUcMRvmDUjyEmNUWwx8UbK/1/1/1)"])['total_amount'], Decimal("8.192")) 83 | assert_equal(self.nodes[0].scantxoutset("start", [ "combo(tprv8ZgxMBicQKsPd7Uf69XL1XwhmjHopUGep8GuEiJDZmbQz6o58LninorQAfcKZWARbtRtfnLcJ5MQ2AtHcQJCCRUcMRvmDUjyEmNUWwx8UbK/1/1/1500)"])['total_amount'], Decimal("16.384")) 84 | assert_equal(self.nodes[0].scantxoutset("start", [ "combo(tpubD6NzVbkrYhZ4WaWSyoBvQwbpLkojyoTZPRsgXELWz3Popb3qkjcJyJUGLnL4qHHoQvao8ESaAstxYSnhyswJ76uZPStJRJCTKvosUCJZL5B/1/1/0)"])['total_amount'], Decimal("4.096")) 85 | - assert_equal(self.nodes[0].scantxoutset("start", [ "combo(tpubD6NzVbkrYhZ4WaWSyoBvQwbpLkojyoTZPRsgXELWz3Popb3qkjcJyJUGLnL4qHHoQvao8ESaAstxYSnhyswJ76uZPStJRJCTKvosUCJZL5B/1/1/1)"])['total_amount'], Decimal("8.192")) 86 | + assert_equal(self.nodes[0].scantxoutset("start", [ "combo([abcdef88/1/2'/3/4h]tpubD6NzVbkrYhZ4WaWSyoBvQwbpLkojyoTZPRsgXELWz3Popb3qkjcJyJUGLnL4qHHoQvao8ESaAstxYSnhyswJ76uZPStJRJCTKvosUCJZL5B/1/1/1)"])['total_amount'], Decimal("8.192"))
Could there be a comment to point out what it's doing? As a reader of the test this is basically lost in the pile.
Not sure what you're asking. I've added 2 lines of comment on this block of tests.
Or is it specifically about this including of the key origin in the test? It has no effect, as for now key origins are not observable anyway. So this is just testing that you can in fact specify one, and nothing breaks.
I meant to a reader of the entire test itself, not the diff.
Ok, is what I added sufficient?
utACK
Addressed further comments in separate commits, will squash when ready.
re-ACK
320 | + // Construct temporary data in `entries`, to avoid producing output in case of failure. 321 | for (const auto& p : m_providers) { 322 | - CPubKey key; 323 | - if (!p->GetPubKey(pos, arg, key)) return false; 324 | - pubkeys.push_back(key); 325 | + std::pair<CPubKey, KeyOriginInfo> keyinfo;
I think you meant to use this below
Fixed; removed it.
utACK 91cff98264ba5c6a86ca5d18b2591f759cffe793, just documentation fixes and two variable cleanups
<!--e57a25ab6845829454e8d69fc972939a-->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.
Squashed fixups, no tree changes.
Rebased afa9224b14 -> 8afb166875