Skip to content

Conversation

@sipa
Copy link
Member

@sipa sipa commented Oct 25, 2018

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

@fanquake
Copy link
Member

Travis is sad about trailing whitespace:

This diff appears to have added new lines with trailing whitespace.
The following changes were suspected:
diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp
@@ -915,0 +969,10 @@ static UniValue ProcessImport(CWallet * const pwallet, const UniValue& data, con
+
^---- failure generated from test/lint/lint-whitespace.sh

@meshcollider
Copy link
Contributor

meshcollider commented Oct 25, 2018

Concept ACK, I think I prefer this over #14558 but will review both more in-depth first

@sipa
Copy link
Member Author

sipa commented Oct 25, 2018

I've also discovered when writing this that importmulti does not actually require that watchonly is set when no solvability is desired. I've kept the existing behavior for now, as it seems pretty invasive to people who may be relying on that, though I've added a TODO.

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.

@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 25, 2018

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #14918 (RPCHelpMan: Check default values are given at compile-time by MarcoFalke)
  • #14912 ([WIP] External signer support (e.g. hardware wallet) by Sjors)
  • #14491 (Allow descriptor imports with importmulti by MeshCollider)
  • #14075 (Import watch only pubkeys to the keypool if private keys are disabled by achow101)
  • #14021 (Import key origin data through descriptors in importmulti by achow101)

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.

Copy link
Member

@Sjors Sjors left a 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?

@sipa sipa force-pushed the 201810_refactor_importmulti branch from f96edc3 to 6341c59 Compare October 26, 2018 01:01
@sipa
Copy link
Member Author

sipa commented Oct 26, 2018

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.

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.

Do the existing tests cover the new sanity checks you added?

Not yet, will add those soon.

@meshcollider
Copy link
Contributor

utACK 33855fe

Copy link
Member

@instagibbs instagibbs left a 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

Copy link
Member

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

Copy link
Member

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

@instagibbs
Copy link
Member

instagibbs commented Nov 5, 2018

I noticed we have no getaddressinfo["ischange"] tests yet, and would be a good time to add those.

@instagibbs
Copy link
Member

instagibbs commented Nov 5, 2018

utACK 33855fec46dbc3a7cad76875ca2a7660fcc19e92 aside from the ischange test, I can't seem to convince myself we even honor the flag, even in master/0.17...

edit: #14662

@Sjors
Copy link
Member

Sjors commented Nov 8, 2018

@achow101 wrote in inline comment:

Instead of a std::set, could std::vector be used for these in order to preserve the order in which things will be imported? This is useful for #14075 where we want to have the order in which things are added to the wallet be the order that was specified in the import.

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

@sipa sipa force-pushed the 201810_refactor_importmulti branch from 33855fe to 6aab00f Compare November 9, 2018 03:23
@sipa
Copy link
Member Author

sipa commented Nov 9, 2018

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

@sipa
Copy link
Member Author

sipa commented Dec 13, 2018

@Sjors

Or just add that as an option? It seems like a useful feature, especially since we can't delete keys and don't have a dry-run option. Different PR is fine.

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;
Copy link
Contributor

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?

Copy link
Contributor

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

@Sjors
Copy link
Member

Sjors commented Dec 13, 2018

re-utACK eacff95

result = self.nodes[1].importmulti([req])
observed_warnings = []
if 'warnings' in result[0]:
observed_warnings = result[0]['warnings']
Copy link
Contributor

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=[]):
Copy link
Contributor

@practicalswift practicalswift Dec 15, 2018

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]

Copy link
Member

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

Copy link
Contributor

@jnewbery jnewbery left a 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)))
Copy link
Contributor

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,
Copy link
Contributor

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)
Copy link
Contributor

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");
Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Contributor

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?

Copy link
Contributor

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")
Copy link
Contributor

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");
Copy link
Contributor

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";
Copy link
Contributor

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)
Copy link
Contributor

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.

Copy link
Contributor

@ryanofsky ryanofsky left a 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;
Copy link
Contributor

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

@meshcollider
Copy link
Contributor

utACK eacff95

Agree with John, the final nits can be addressed in a followup

@meshcollider meshcollider merged commit eacff95 into bitcoin:master Dec 24, 2018
meshcollider added a commit that referenced this pull request Dec 24, 2018
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
laanwj added a commit that referenced this pull request Feb 7, 2019
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
maflcko pushed a commit that referenced this pull request Aug 28, 2019
…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
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 28, 2019
…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
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request May 19, 2020
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
monstrobishi pushed a commit to DeFiCh/ain that referenced this pull request Jul 30, 2020
…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
ShengguangXiao pushed a commit to DeFiCh/ain that referenced this pull request Aug 28, 2020
…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
kwvg added a commit to kwvg/dash that referenced this pull request Oct 28, 2021
pravblockc pushed a commit to pravblockc/dash that referenced this pull request Nov 18, 2021
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.