Skip to content
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

Merged
merged 2 commits into from Dec 24, 2018

Conversation

@sipa
Copy link
Member

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

@sipa sipa force-pushed the sipa:201810_refactor_importmulti branch Oct 25, 2018

@fanquake

This comment has been minimized.

Copy link
Member

commented Oct 25, 2018

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

This comment has been minimized.

Copy link
Member

commented Oct 25, 2018

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

@sipa

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

Copy link
Contributor

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.

@Sjors
Copy link
Member

left a comment

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?

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

@sipa sipa force-pushed the sipa:201810_refactor_importmulti branch Oct 26, 2018

@sipa

This comment has been minimized.

Copy link
Member Author

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.

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

@sipa sipa force-pushed the sipa:201810_refactor_importmulti branch Oct 31, 2018

@meshcollider

This comment has been minimized.

Copy link
Member

commented Nov 3, 2018

utACK 33855fe

@instagibbs
Copy link
Member

left a comment

looking pretty good so far

Show resolved Hide resolved src/wallet/rpcdump.cpp Outdated
Show resolved Hide resolved src/wallet/rpcdump.cpp Outdated
Show resolved Hide resolved src/wallet/rpcdump.cpp Outdated
CRIPEMD160().Write(fullid.begin(), fullid.size()).Finalize(id.begin());
auto subscript = std::move(data.witnessscript);
if (!subscript) return "missing witnessscript";
if (CScriptID(*subscript) != id) return "witnessScript does not match the scriptPubKey or redeemScript";

This comment has been minimized.

Copy link
@instagibbs

instagibbs Nov 5, 2018

Member

note for self: check if we're checking this error

CScriptID id = CScriptID(uint160(solverdata[0]));
auto subscript = std::move(data.redeemscript);
if (!subscript) return "missing redeemscript";
if (CScriptID(*subscript) != id) return "redeemScript does not match the scriptPubKey";

This comment has been minimized.

Copy link
@instagibbs

instagibbs Nov 5, 2018

Member

note for self: check if we're checking this error

Show resolved Hide resolved src/wallet/rpcdump.cpp Outdated
Show resolved Hide resolved src/wallet/rpcdump.cpp Outdated
Show resolved Hide resolved src/wallet/rpcdump.cpp Outdated
Show resolved Hide resolved src/wallet/rpcdump.cpp Outdated
@instagibbs

This comment has been minimized.

Copy link
Member

commented Nov 5, 2018

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

@instagibbs

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

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 sipa:201810_refactor_importmulti branch Nov 9, 2018

@sipa

This comment has been minimized.

Copy link
Member Author

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

Show resolved Hide resolved src/wallet/rpcdump.cpp Outdated
@jnewbery

This comment has been minimized.

Copy link
Member

commented Dec 6, 2018

I've started refactoring the wallet_importmulti.py into this style. It's a WIP here: https://github.com/jnewbery/bitcoin/tree/importmulti_tests . I hope to continue working on it this week.

This work is (mostly) complete. Intermediate commits could probably do with some tidy-up, which I intend to do later today.

PR here: #14886

@jnewbery jnewbery added this to Blockers in High-priority for review Dec 6, 2018

@jnewbery

This comment has been minimized.

Copy link
Member

commented Dec 6, 2018

I've started writing tests for this PR: https://github.com/jnewbery/bitcoin/tree/pr14565.tests

It's certainly not complete, but it makes a start by:

  • testing the return object from importmulti for the right warnings return value.
  • calling getaddressinfo for the different address variants when privkeys are imported.

To continue, we should:

  • restructure the test so in order, it's testing:
    • P2PKH (scriptPubKey and address)
    • P2WPKH (scriptPubKey and address)
    • P2SH-P2WPKH (scriptPubKey and address)
    • bare multisig (scriptPubKey only)
    • P2SH (scriptPubKey and address)
    • P2WSH (scriptPubKey and address)
    • P2SH-P2WSH (scriptPubKey and address)
  • increase coverage, so that all warnings and errors are hit.

I'm unlikely to be able to spend time on the tests for the next few days. Anyone is free to take the branch and continue building on it.

@Sjors Sjors referenced this pull request Dec 10, 2018

Open

[WIP] External signer support (e.g. hardware wallet) #14912

0 of 2 tasks complete

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Dec 11, 2018

Merge bitcoin#14886: [tests] Refactor importmulti tests
ee3b21d [tests] Add docstring for wallet_importmulti.py (John Newbery)
fbdba40 [tests] add test_address method to wallet_import.py (John Newbery)
fd3a02c [tests] add test_importmulti method to wallet_import.py (John Newbery)
08a4a0f [tests] add get_multisig function to wallet_importmulti.py (John Newbery)
7c99614 [tests] add get_key function to wallet_importmulti.py (John Newbery)
e5a8ea8 [tests] tidy up imports in wallet_importmulti.py (John Newbery)
cb41ade [tests] fix flake8 warnings in wallet_importmulti.py (John Newbery)

Pull request description:

  bitcoin#14565 needs test coverage. This PR refactors wallet_importmulti.py to the following pattern:

  1. Add `get_key()` and `get_multisig()` methods, which generate keys on node0 and return the priv/pubkeys and all scriptPubKey and address variants.
  2. Add `test_importmulti()` method, which takes an importmulti request, sends it to node1 and tests against success and error codes/messages.
  3. Add `test_address()` method, which takes an address, sends it as a getaddressinfo request to node1 and tests the values returned.

  This does not add any specific testing for bitcoin#14565, but makes it very straightforward to add that testing: `test_importmulti()` can be easily updated to test for returned warnings, and `test_address()` can be called multiple times against the different address variants for a singlesig/multisig.

Tree-SHA512: e0ae9d3436f0b4eec4f6b9bdc0f02aef49c5a16bbac319fd47b2cfcaf01d01780d7b296280e8760686a57fac63275eec09e2959d8aaeceae1b406d8eff768435

@sipa sipa force-pushed the sipa:201810_refactor_importmulti branch from 8a079e8 Dec 12, 2018

@DrahtBot DrahtBot removed the Needs rebase label Dec 12, 2018

@sipa

This comment has been minimized.

Copy link
Member Author

commented Dec 12, 2018

I've addressed many of the comments above, rebased on top of the new test changes from #14886, and made a few behavior changes as well:

  • Instead of sticking to a "what used to be an error and is still not supported remains an error, other things are warnings", as that resulted in pretty inconsistent behavior. I think I've changed things to be more consistently only errorring for actual failures. I still think we should do a deprecatedrpc thing to turn all warnings into errors, preferably in the same release, but maybe not in this PR.
  • Added a warning for specifying "watchonly" while providing all private keys.
  • Removed the special case error for redeemscript-for-P2WSH, while all other mismatches of this type are just warnings that result in non-solvability.
  • More explanations in warning messages.
@Sjors

Sjors approved these changes Dec 12, 2018

Copy link
Member

left a comment

utACK d6f2b34, but I suggest waiting for #14491 (descriptor support) rebase, as an extra check that we didn't miss anything here.

deprecatedrpc thing to turn all warnings into errors

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.

Show resolved Hide resolved src/wallet/rpcdump.cpp Outdated
Show resolved Hide resolved src/wallet/rpcdump.cpp
Show resolved Hide resolved src/wallet/rpcdump.cpp
Show resolved Hide resolved src/wallet/rpcdump.cpp
@promag
Copy link
Member

left a comment

PR description is misleading, especially:

It makes sure no changes to the wallet are made when an error in the input exists.

should state that it's a per request check.

Show resolved Hide resolved doc/release-notes-14565.md Outdated

// Analyse the provided scriptPubKey, determining which keys and which redeem scripts from the ImportData struct are needed to spend it, and mark them as used.
// Returns an error string, or the empty string for success.
static std::string RecurseImportData(const CScript& script, ImportData& import_data, const ScriptContext script_ctx)

This comment has been minimized.

Copy link
@promag

promag Dec 12, 2018

Member

IMO could do better than empty string as success — despite the fact that it works.

Alternatives:

  • could return std::pair<bool, std::string>
  • could have return bool and add argument std::string& error

This comment has been minimized.

Copy link
@sipa

sipa Dec 13, 2018

Author Member

Both of these options result in uglier code, I think.

sipa added some commits Oct 25, 2018

Overhaul importmulti logic
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.

@sipa sipa force-pushed the sipa:201810_refactor_importmulti branch to eacff95 Dec 13, 2018

@sipa

This comment has been minimized.

Copy link
Member Author

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.

pwallet->UpdateTimeFirstKey(timestamp);
}
for (const auto& entry : pubkey_map) {
const CPubKey& pubkey = entry.second;
const CKeyID& id = entry.first;

This comment has been minimized.

Copy link
@meshcollider

meshcollider Dec 13, 2018

Member

Do we also need to update mapKeyMetadata here to include the timestamp?

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Dec 20, 2018

Contributor

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

This comment has been minimized.

Copy link
Member

commented Dec 13, 2018

re-utACK eacff95

"""Run importmulti and assert success"""
result = self.nodes[1].importmulti([req])
observed_warnings = []
if 'warnings' in result[0]:
observed_warnings = result[0]['warnings']

This comment has been minimized.

Copy link
@practicalswift

practicalswift Dec 15, 2018

Member

Indentation is not a multiple of four :-)

@@ -119,9 +119,13 @@ def get_multisig(self):
CScript([OP_HASH160, witness_script, OP_EQUAL]).hex(), # p2sh-p2wsh
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=[]):

This comment has been minimized.

Copy link
@practicalswift

practicalswift Dec 15, 2018

Member

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]

This comment has been minimized.

Copy link
@Sjors

Sjors Dec 15, 2018

Member

Yikes, I thought only Javascript behaved like that...

@jnewbery
Copy link
Member

left a comment

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

This comment has been minimized.

Copy link
@jnewbery

jnewbery Dec 20, 2018

Member

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,

This comment has been minimized.

Copy link
@jnewbery

jnewbery Dec 20, 2018

Member

nit: misaligned, please remove space

self.test_address(address,
solvable=True)
solvable=True,
ismine=False)

This comment has been minimized.

Copy link
@jnewbery

jnewbery Dec 20, 2018

Member

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");

This comment has been minimized.

Copy link
@jnewbery

jnewbery Dec 20, 2018

Member

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.

This comment has been minimized.

Copy link
@sipa

sipa Dec 20, 2018

Author Member

I really don't think that putting private keys in error messages is a good idea. They may get logged unintentionally, etc.

This comment has been minimized.

Copy link
@promag

promag Dec 20, 2018

Member

Absolutely agree with @sipa. Could show a couple chars only or the error message could say the index of the invalid entry?

This comment has been minimized.

Copy link
@jnewbery

jnewbery Dec 20, 2018

Member

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

This comment has been minimized.

Copy link
@jnewbery

jnewbery Dec 20, 2018

Member

remove word 'not'

// Import the address
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");

This comment has been minimized.

Copy link
@jnewbery

jnewbery Dec 20, 2018

Member

(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";

This comment has been minimized.

Copy link
@jnewbery

jnewbery Dec 20, 2018

Member

Error message could indicate which key, eg "key for pubkey hash is missing"

Show resolved Hide resolved src/wallet/rpcdump.cpp
default:
return "unrecognized script";
}
}

static UniValue ProcessImport(CWallet * const pwallet, const UniValue& data, const int64_t timestamp) EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet)

This comment has been minimized.

Copy link
@jnewbery

jnewbery Dec 20, 2018

Member

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

left a comment

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.

pwallet->UpdateTimeFirstKey(timestamp);
}
for (const auto& entry : pubkey_map) {
const CPubKey& pubkey = entry.second;
const CKeyID& id = entry.first;

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Dec 20, 2018

Contributor

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

This comment has been minimized.

Copy link
Member

commented Dec 24, 2018

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

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

meshcollider added a commit that referenced this pull request Dec 24, 2018

Merge #14565: Overhaul importmulti logic
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

@fanquake fanquake removed this from Blockers in High-priority for review Dec 24, 2018

laanwj added a commit that referenced this pull request Feb 7, 2019

Merge #14491: Allow descriptor imports with importmulti
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.