-
Notifications
You must be signed in to change notification settings - Fork 38.8k
Overhaul importmulti logic #14565
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Overhaul importmulti logic #14565
Conversation
fb1dcf9 to
f96edc3
Compare
|
Travis is sad about trailing whitespace: |
|
Concept ACK, I think I prefer this over #14558 but will review both more in-depth first |
|
EDIT: seems I misunderstand the original purpose; the "watchonly" is there to support importing something as watch-only without importing all the private keys (and is thus about spendability, not about solvability). That both matches the name better, and the existing (lack of) errors related to it. It also makes more sense, as there is no point in providing keys/script when no solvability is desired, while it makes perfect sense to include private keys when no spendability is desired. |
|
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. |
Sjors
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK. The new recursive ProcessSolvingImportStep and SolverContext are a lot more readable! I imagine that function lends itself to (future) unit tests better.
There's still a bunch of processing and checking happening outside of ProcessSolvingImportStep though, which is less easy to follow, but at least that code is now shorter.
Do the existing tests cover the new sanity checks you added?
f96edc3 to
6341c59
Compare
That's intentional. It's a recursive function to deal with analysing script specific solvability information. Anything that isn't specific to the script being imported isn't in there.
Not yet, will add those soon. |
6341c59 to
33855fe
Compare
|
utACK 33855fe |
instagibbs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking pretty good so far
src/wallet/rpcdump.cpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note for self: check if we're checking this error
src/wallet/rpcdump.cpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note for self: check if we're checking this error
|
I noticed we have no |
|
utACK 33855fec46dbc3a7cad76875ca2a7660fcc19e92 aside from the edit: #14662 |
|
@achow101 wrote in inline comment:
It's also better for recoverability if the wallet strives to use derivation paths in ascending order. Otherwise the user needs to remember what range they imported before. It would be also keep us a bit closer to honouring a BIP44-style gap limit (if the user generates more than 20 addresses without receiving coins on any of them, we'd still break that gap, but at least that's not the common case). |
33855fe to
6aab00f
Compare
|
I realized I misread what "watchonly" was supposed to mean; it means it's fine if the resulting address is not spendable (I was under the assumption it meant being fine if it's not solvable). I made some significant logic changes as a result to reflect the warning messages for that. All other comments are addressed, apart from the ordering of keys, and adding tests (which I will do soon). |
I'd really like to avoid that. There is no reason for that to be an option (which we may need to maintain forever), as the intended behavior is that these things are just errors, always. The only reason why they aren't is because we don't want to break existing software immediately. |
| } | ||
| for (const auto& entry : pubkey_map) { | ||
| const CPubKey& pubkey = entry.second; | ||
| const CKeyID& id = entry.first; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we also need to update mapKeyMetadata here to include the timestamp?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re: #14565 (comment)
Do we also need to update mapKeyMetadata here to include the timestamp?
I requested not doing this here: #14565 (comment). It shouldn't be necessary because timestamp is passed to AddWatchOnly below. In the future the mapKeyMetadata update above could also be removed, see #14565 (comment).
|
re-utACK eacff95 |
| result = self.nodes[1].importmulti([req]) | ||
| observed_warnings = [] | ||
| if 'warnings' in result[0]: | ||
| observed_warnings = result[0]['warnings'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation is not a multiple of four :-)
| script_to_p2sh_p2wsh(script_code)) # p2sh-p2wsh addr | ||
|
|
||
| def test_importmulti(self, req, success, error_code=None, error_message=None): | ||
| def test_importmulti(self, req, success, error_code=None, error_message=None, warnings=[]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest using warnings=None (see example below) instead to clarify that warnings is not meant to be remembered across calls.
Background:
>>> def test(i, i_arr=[]):
... i_arr.append(i)
... return i_arr
...
>>> test(1)
[1]
>>> test(2)
[1, 2]
>>> test(3)
[1, 2, 3]
Suggested alternative:
>>> def test(i, i_arr=None):
... if i_arr is None:
... i_arr=[]
... i_arr.append(i)
... return i_arr
...
>>> test(1)
[1]
>>> test(2)
[2]
>>> test(3)
[3]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yikes, I thought only Javascript behaved like that...
jnewbery
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bunch of comments, mostly around error messages. Please treat these all as nits. They shouldn't hold up merge and could be addressed in a follow-up PR.
This is certainly an improvement.
I've expanded the release notes here: jnewbery@0348531. Feel free to squash into your commit if you think it's an improvement.
utACK eacff95
| observed_warnings = [] | ||
| if 'warnings' in result[0]: | ||
| observed_warnings = result[0]['warnings'] | ||
| assert_equal("\n".join(sorted(warnings)), "\n".join(sorted(observed_warnings))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are the "\n".join adding anything here?
Presumably "\n".join(list1) == "\n".join(list2) ⇔ list1 == list2?
| False, | ||
| error_code=-5, | ||
| error_message='Key does not match address destination') | ||
| True, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: misaligned, please remove space
| self.test_address(address, | ||
| solvable=True) | ||
| solvable=True, | ||
| ismine=False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: test for watchonly?
| const auto& str = keys[i].get_str(); | ||
| CKey key = DecodeSecret(str); | ||
| if (!key.IsValid()) { | ||
| throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid private key encoding"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: should identify which private key was invalid in error message.
EDIT: same for all error messages below. importmulti can contain multiple scripts, addresses and keys. Any error messages should indicate which address/script/key was at fault.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really don't think that putting private keys in error messages is a good idea. They may get logged unintentionally, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely agree with @sipa. Could show a couple chars only or the error message could say the index of the invalid entry?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, very good point. I think Promag's suggestions are good.
In any case, this doesn't need to be included in this PR. A future PR to improve logging and test all failure modes would be nice.
|
|
||
| # Address + Public key + !Internal + Wrong pubkey | ||
| self.log.info("Should not import an address with a wrong public key") | ||
| self.log.info("Should not import an address with the wrong public key as non-solvable") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove word 'not'
| if (::IsMine(*pwallet, scriptpubkey_script) == ISMINE_SPENDABLE) { | ||
| // Check whether we have any work to do | ||
| if (::IsMine(*pwallet, script) & ISMINE_SPENDABLE) { | ||
| throw JSONRPCError(RPC_WALLET_ERROR, "The wallet already contains the private key for this address or script"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(unchanged by this PR)
This error message is slightly wrong, since ISMINE_SPENDABLE could mean that the wallet already contains private keys. I think it would be better to say that the address or script is already owned by the wallet.
| for (const auto& require_key : import_data.used_keys) { | ||
| if (!require_key.second) continue; // Not a required key | ||
| if (pubkey_map.count(require_key.first) == 0 && privkey_map.count(require_key.first) == 0) { | ||
| error = "some required keys are missing"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error message could indicate which key, eg "key for pubkey hash is missing"
| } | ||
| } | ||
|
|
||
| static UniValue ProcessImport(CWallet * const pwallet, const UniValue& data, const int64_t timestamp) EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a fairly involved function, so it'd be friendly to have a function-level comment:
- called once for each request within an importmulti call
- doesn't throw. All errors are caught and returned in the error field
- all input data is parsed and validated first. Then scripts, pubkeys and keys are imported.
ryanofsky
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK eacff95. Changes since last review: rebasing, warning about unused redeem scripts instead of erroring, changing "Key does not match address destination" error to "some required keys are missing", dropping warning about private keys accompanied by public keys, adding warning if watchonly is passed along with private keys, updating RPC documentation, adding key information to errors and warnings, changing internal used_keys representation, and renaming / moving some local variables.
| } | ||
| for (const auto& entry : pubkey_map) { | ||
| const CPubKey& pubkey = entry.second; | ||
| const CKeyID& id = entry.first; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re: #14565 (comment)
Do we also need to update mapKeyMetadata here to include the timestamp?
I requested not doing this here: #14565 (comment). It shouldn't be necessary because timestamp is passed to AddWatchOnly below. In the future the mapKeyMetadata update above could also be removed, see #14565 (comment).
|
utACK eacff95 Agree with John, the final nits can be addressed in a followup |
eacff95 Add release notes (Pieter Wuille) bdacbda Overhaul importmulti logic (Pieter Wuille) Pull request description: This is an alternative to #14558 (it will warn when fields are being ignored). In addition: * It makes sure no changes to the wallet are made when an error in the input exists. * It validates all arguments, and will fail if anything fails to parse. * Adds a whole bunch of sanity checks Tree-SHA512: fdee0b6aca8c643663f0bc295a7c1d69c1960951493b06abf32c58977f3e565f75918dbd0402dde36e508dc746c9310a968a0ebbacccc385a57ac2a68b49c1d0
b985e9c Add release notes for importmulti descriptor support (MeshCollider) fbb5e93 Add test for importing via descriptor (MeshCollider) 9f48053 [wallet] Allow descriptor imports with importmulti (MeshCollider) d2b381c [wallet] Refactor ProcessImport() to call ProcessImportLegacy() (John Newbery) 4cac0dd [wallet] Add ProcessImportLegacy() (John Newbery) a1b25e1 [wallet] Refactor ProcessImport() (John Newbery) Pull request description: ~~Based on #14454 #14565, last two commits only are for review.~~ Best reviewed with `?w=1` Allows a descriptor to be imported into the wallet using `importmulti` RPC. Start and end of range can be specified for ranged descriptors. The descriptor is implicitly converted to old structures on import. Also adds a simple test of a P2SH-P2WPKH address being imported as a descriptor. More tests to come, as well as release notes. Tree-SHA512: 160eb6fd574c4ae5b70e0109f7e5ccc95d9309138603408a1114ceb3c558065409c0d7afb66926bc8e1743c365a3b300c5f944ff18b2451acc0514fbeca1f2b3
…n mutable dict/list:s are used as default parameter values e4f4ea4 lint: Catch use of [] or {} as default parameter values in Python functions (practicalswift) 25dd867 Avoid using mutable default parameter values (practicalswift) Pull request description: Avoid common Python default parameter gotcha when mutable `dict`/`list`:s are used as default parameter values. Examples of this gotcha caught during review: * #16673 (comment) * #14565 (comment) Perhaps surprisingly this is how mutable list and dictionary default parameter values behave in Python: ``` >>> def f(i, j=[], k={}): ... j.append(i) ... k[i] = True ... return j, k ... >>> f(1) ([1], {1: True}) >>> f(1) ([1, 1], {1: True}) >>> f(2) ([1, 1, 2], {1: True, 2: True}) ``` In contrast to: ``` >>> def f(i, j=None, k=None): ... if j is None: ... j = [] ... if k is None: ... k = {} ... j.append(i) ... k[i] = True ... return j, k ... >>> f(1) ([1], {1: True}) >>> f(1) ([1], {1: True}) >>> f(2) ([2], {2: True}) ``` The latter is typically the intended behaviour. This PR fixes two instances of this and adds a check guarding against this gotcha going forward :-) ACKs for top commit: Sjors: Oh Python... ACK e4f4ea4. Testing tip: swap the two commits. Tree-SHA512: 56e14d24fc866211a20185c9fdb274ed046c3aed2dc0e07699e58b6f9fa3b79f6d0c880fb02d72b7fe5cc5eb7c0ff6da0ead33123344e1a872209370c2e49e3f
…cha when mutable dict/list:s are used as default parameter values e4f4ea4 lint: Catch use of [] or {} as default parameter values in Python functions (practicalswift) 25dd867 Avoid using mutable default parameter values (practicalswift) Pull request description: Avoid common Python default parameter gotcha when mutable `dict`/`list`:s are used as default parameter values. Examples of this gotcha caught during review: * bitcoin#16673 (comment) * bitcoin#14565 (comment) Perhaps surprisingly this is how mutable list and dictionary default parameter values behave in Python: ``` >>> def f(i, j=[], k={}): ... j.append(i) ... k[i] = True ... return j, k ... >>> f(1) ([1], {1: True}) >>> f(1) ([1, 1], {1: True}) >>> f(2) ([1, 1, 2], {1: True, 2: True}) ``` In contrast to: ``` >>> def f(i, j=None, k=None): ... if j is None: ... j = [] ... if k is None: ... k = {} ... j.append(i) ... k[i] = True ... return j, k ... >>> f(1) ([1], {1: True}) >>> f(1) ([1], {1: True}) >>> f(2) ([2], {2: True}) ``` The latter is typically the intended behaviour. This PR fixes two instances of this and adds a check guarding against this gotcha going forward :-) ACKs for top commit: Sjors: Oh Python... ACK e4f4ea4. Testing tip: swap the two commits. Tree-SHA512: 56e14d24fc866211a20185c9fdb274ed046c3aed2dc0e07699e58b6f9fa3b79f6d0c880fb02d72b7fe5cc5eb7c0ff6da0ead33123344e1a872209370c2e49e3f
Summary: This introduces various changes to the importmulti logic: * Instead of processing input and importing things at the same time, first process all input data and verify it, so no changes are made in case of an error. * Verify that no superfluous information is provided (no keys or scripts that don't contribute to solvability in particular). * Add way more sanity checks, by means of descending into all involved scripts. * Add release notes This is a backport of Core [[bitcoin/bitcoin#14565 | PR14565]] Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Subscribers: majcosta Differential Revision: https://reviews.bitcoinabc.org/D6140
…n mutable dict/list:s are used as default parameter values e4f4ea4 lint: Catch use of [] or {} as default parameter values in Python functions (practicalswift) 25dd867 Avoid using mutable default parameter values (practicalswift) Pull request description: Avoid common Python default parameter gotcha when mutable `dict`/`list`:s are used as default parameter values. Examples of this gotcha caught during review: * bitcoin/bitcoin#16673 (comment) * bitcoin/bitcoin#14565 (comment) Perhaps surprisingly this is how mutable list and dictionary default parameter values behave in Python: ``` >>> def f(i, j=[], k={}): ... j.append(i) ... k[i] = True ... return j, k ... >>> f(1) ([1], {1: True}) >>> f(1) ([1, 1], {1: True}) >>> f(2) ([1, 1, 2], {1: True, 2: True}) ``` In contrast to: ``` >>> def f(i, j=None, k=None): ... if j is None: ... j = [] ... if k is None: ... k = {} ... j.append(i) ... k[i] = True ... return j, k ... >>> f(1) ([1], {1: True}) >>> f(1) ([1], {1: True}) >>> f(2) ([2], {2: True}) ``` The latter is typically the intended behaviour. This PR fixes two instances of this and adds a check guarding against this gotcha going forward :-) ACKs for top commit: Sjors: Oh Python... ACK e4f4ea4. Testing tip: swap the two commits. Tree-SHA512: 56e14d24fc866211a20185c9fdb274ed046c3aed2dc0e07699e58b6f9fa3b79f6d0c880fb02d72b7fe5cc5eb7c0ff6da0ead33123344e1a872209370c2e49e3f
…n mutable dict/list:s are used as default parameter values 691cc30 lint: Catch use of [] or {} as default parameter values in Python functions (practicalswift) 5f48d0e Avoid using mutable default parameter values (practicalswift) Pull request description: Avoid common Python default parameter gotcha when mutable `dict`/`list`:s are used as default parameter values. Examples of this gotcha caught during review: * bitcoin/bitcoin#16673 (comment) * bitcoin/bitcoin#14565 (comment) Perhaps surprisingly this is how mutable list and dictionary default parameter values behave in Python: ``` >>> def f(i, j=[], k={}): ... j.append(i) ... k[i] = True ... return j, k ... >>> f(1) ([1], {1: True}) >>> f(1) ([1, 1], {1: True}) >>> f(2) ([1, 1, 2], {1: True, 2: True}) ``` In contrast to: ``` >>> def f(i, j=None, k=None): ... if j is None: ... j = [] ... if k is None: ... k = {} ... j.append(i) ... k[i] = True ... return j, k ... >>> f(1) ([1], {1: True}) >>> f(1) ([1], {1: True}) >>> f(2) ([2], {2: True}) ``` The latter is typically the intended behaviour. This PR fixes two instances of this and adds a check guarding against this gotcha going forward :-) ACKs for top commit: Sjors: Oh Python... ACK 691cc30. Testing tip: swap the two commits. Tree-SHA512: 56e14d24fc866211a20185c9fdb274ed046c3aed2dc0e07699e58b6f9fa3b79f6d0c880fb02d72b7fe5cc5eb7c0ff6da0ead33123344e1a872209370c2e49e3f
This is an alternative to #14558 (it will warn when fields are being ignored). In addition: