Nit fixes for PR 14565 #15032
pull meshcollider wants to merge 2 commits into bitcoin:master from meshcollider:201812_pr_14565_nits changing 4 files +41 −19-
meshcollider commented at 12:13 pm on December 24, 2018: contributorFixup the few final nits from #14565
-
meshcollider added the label Wallet on Dec 24, 2018
-
meshcollider added the label RPC/REST/ZMQ on Dec 24, 2018
-
DrahtBot commented at 12:49 pm on December 24, 2018: member
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Conflicts
Reviewers, this pull request conflicts with the following ones:
- #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.
-
in src/wallet/rpcdump.cpp:1004 in 97d086f5e7 outdated
1000@@ -1000,7 +1001,7 @@ static UniValue ProcessImport(CWallet * const pwallet, const UniValue& data, con 1001 const auto& str = keys[i].get_str(); 1002 CKey key = DecodeSecret(str); 1003 if (!key.IsValid()) { 1004- throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid private key encoding"); 1005+ throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid private key encoding at index " + i);
practicalswift commented at 11:12 am on December 26, 2018:This looks like very dangerous to me :-)
promag commented at 4:42 pm on December 26, 2018:@practicalswift why?
meshcollider commented at 4:07 am on December 28, 2018:Oops, thinking too pythonically. Fixed thanks :)
promag commented at 11:08 am on January 2, 2019:God 🤦♂️in src/wallet/rpcdump.cpp:970 in 97d086f5e7 outdated
918@@ -923,6 +919,11 @@ static std::string RecurseImportData(const CScript& script, ImportData& import_d 919 } 920 } 921 922+/** 923+ * ProcessImport is called once for each request within an importmulti call. 924+ * All input data is parsed and validated first. Then scripts, pubkeys and keys are imported. 925+ * This function doesn't throw - all errors are caught and returned in the error field. 926+ */ 927 static UniValue ProcessImport(CWallet * const pwallet, const UniValue& data, const int64_t timestamp) EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet)
promag commented at 4:52 pm on December 26, 2018:Could add noexcept specifier?practicalswift commented at 11:37 am on December 27, 2018: contributorAs a precaution this PR should probably be marked[wip]
until the security issue it introduces (see #15032 (review)) has been corrected?meshcollider commented at 5:00 am on December 28, 2018: contributorFixed the bug mentioned abovein src/wallet/rpcdump.cpp:1052 in adee4eed8c outdated
1000@@ -1000,7 +1001,7 @@ static UniValue ProcessImport(CWallet * const pwallet, const UniValue& data, con 1001 const auto& str = keys[i].get_str(); 1002 CKey key = DecodeSecret(str); 1003 if (!key.IsValid()) { 1004- throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid private key encoding"); 1005+ throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Invalid private key encoding at index %zu", i));
MarcoFalke commented at 11:31 am on December 28, 2018:Could add (or extend) a test for this?laanwj commented at 1:29 pm on January 2, 2019: memberutACK adee4eed8cf4a45f4095c653073c27509a931d2e
Agree it would be nice to have a test for “Invalid private key encoding at index…” as there was a near-screwup there.
in src/wallet/wallet.h:849 in adee4eed8c outdated
840@@ -841,6 +841,8 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface 841 CPubKey GenerateNewKey(WalletBatch& batch, bool internal = false) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); 842 //! Adds a key to the store, and saves it to disk. 843 bool AddKeyPubKey(const CKey& key, const CPubKey &pubkey) override EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
ryanofsky commented at 8:06 pm on January 2, 2019:Could make this method private and move next to AddWatchOnly:
in src/wallet/rpcdump.cpp:614 in adee4eed8c outdated
610@@ -614,11 +611,10 @@ UniValue importwallet(const JSONRPCRequest& request) 611 } 612 } 613 pwallet->WalletLogPrintf("Importing %s...\n", EncodeDestination(keyid)); 614- if (!pwallet->AddKeyPubKey(key, pubkey)) { 615+ if (!pwallet->AddKeyPubKey(key, pubkey, nTime)) {
ryanofsky commented at 8:11 pm on January 2, 2019:I think this is actually a bugfix, because previously
mapKeyMetadata
got updated too late, afterCWallet::AddKeyPubKeyWithDB
andCWallet::AddCryptedKey
already used the previous value:https://github.com/bitcoin/bitcoin/blob/fb52d0684e0f03bca5d7ddcf3fc5b0657859ac2c/src/wallet/wallet.cpp#L242 https://github.com/bitcoin/bitcoin/blob/fb52d0684e0f03bca5d7ddcf3fc5b0657859ac2c/src/wallet/wallet.cpp#L267
in src/wallet/rpcdump.cpp:620 in adee4eed8c outdated
617 continue; 618 } 619- pwallet->mapKeyMetadata[keyid].nCreateTime = nTime; 620 if (fLabel) 621 pwallet->SetAddressBook(keyid, strLabel, "receive"); 622 nTimeBegin = std::min(nTimeBegin, nTime);
ryanofsky commented at 8:25 pm on January 2, 2019:I think this line no longer does anything and can be dropped. IfnTime
is less thannTimeBegin
, then theUpdateTimeFirstKey
call insideAddKeyPubKey
will have already updated the minimum time.
jnewbery commented at 4:13 pm on January 31, 2019:nTimeBegin
is also used below in theRescanWallet()
call, so I think it needs to be set to the min value here.
ryanofsky commented at 8:00 pm on February 4, 2019:re: #15032 (review)
nTimeBegin is also used below in the RescanWallet() call, so I think it needs to be set to the min value here.
You’re right. Not sure how I missed the RescanWallet call.
ryanofsky approvedryanofsky commented at 8:31 pm on January 2, 2019: memberutACK adee4eed8cf4a45f4095c653073c27509a931d2ein test/functional/wallet_importmulti.py:56 in adee4eed8c outdated
127+ if warnings is None: 128+ warnings = [] 129 if 'warnings' in result[0]: 130- observed_warnings = result[0]['warnings'] 131- assert_equal("\n".join(sorted(warnings)), "\n".join(sorted(observed_warnings))) 132+ observed_warnings = result[0]['warnings']
benthecarman commented at 8:08 am on January 6, 2019:Nit: there was an extra space added to the beginning of this line
meshcollider commented at 5:52 pm on January 6, 2019:Indeed, that was deliberate :)DrahtBot added the label Needs rebase on Jan 9, 2019in test/functional/wallet_importmulti.py:351 in adee4eed8c outdated
427 self.test_importmulti({"scriptPubKey": {"address": address}, 428 "timestamp": "now", 429 "pubkeys": [wrong_key]}, 430 True, 431- warnings=["Importing as non-solvable: some required keys are missing. If this is intentional, don't provide any keys, pubkeys, witnessscript, or redeemscript.", "Some private keys are missing, outputs will be considered watchonly. If this is intentional, specify the watchonly flag."]) 432+ warnings=["Importing as non-solvable: some required keys are missing: " + bytes_to_hex_str(pkh) + ". If this is intentional, don't provide any keys, pubkeys, witnessscript, or redeemscript.", "Some private keys are missing, outputs will be considered watchonly. If this is intentional, specify the watchonly flag."])
jnewbery commented at 7:43 pm on January 14, 2019:nit (this isn’t introduced here, but since this PR is for nit-clearing…): this line is very long. Consider:
- building a warnings variable above and passing it to this function
- adding constants for the common error strings to avoid repetition
- using
.format()
string formating instead of string concatenation.
jnewbery commented at 7:44 pm on January 14, 2019: memberThanks for doing this @MeshCollider . Lightly tested ACK adee4eed8cf4a45f4095c653073c27509a931d2e.
I have one further nit inline that you could take or leave.
achow101 commented at 10:40 pm on January 15, 2019: memberutACK adee4eed8cf4a45f4095c653073c27509a931d2e
Needs rebase.
DrahtBot removed the label Needs rebase on Jan 16, 2019Sjors commented at 4:14 pm on January 16, 2019: memberTravis linter fails, probably due to too modern Python syntaxtest/functional/wallet_importmulti.py:395:23: F821 undefined name 'hex_str_to_bytes'
MarcoFalke commented at 4:48 pm on January 16, 2019: memberFailure when running the test:
02019-01-16T16:31:55.887000Z TestFramework (ERROR): Unexpected exception caught during testing 1Traceback (most recent call last): 2 File "/home/travis/build/bitcoin/bitcoin/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 173, in main 3 self.run_test() 4 File "/home/travis/build/bitcoin/bitcoin/build/bitcoin-i686-pc-linux-gnu/test/functional/wallet_importmulti.py", line 343, in run_test 5 pkh = hash160(hex_str_to_bytes(key.pubkey)) 6NameError: name 'hash160' is not defined
meshcollider commented at 8:27 pm on January 16, 2019: contributorAh that looks like its just the import statements being moved around in the importmulti test cleanup when I rebased this on master.laanwj commented at 1:18 pm on January 31, 2019: memberWhat is the status of this? Github made sufficiently much of a mess out of the comments that I can’t follow if all the remarks have been addressed or not!Sjors commented at 3:58 pm on January 31, 2019: memberutACK aee171b @ryanofsky’s comment could use more eyes (is this actually fixing a bug or not? if so, maybe rename that commit)
There’s a Python formatting nit by @jnewbery that’s still valid (though not worth holding back merging a PR that itself fixes nits).
jnewbery commented at 9:42 pm on January 31, 2019: memberI thought that this might have introduced a behaviour change in
CWallet::DeriveNewSeed()
which callsAddKeyPubKey()
without a timestamp.CWallet::DeriveNewSeed()
now causesUpdateTimeFirstKey()
to be called which setsnTimeFirstKey
to min(current time, existingnTimeFirstKey
) (since themapKeyMetadata
is updated here: https://github.com/bitcoin/bitcoin/blob/252fd15addf1cd061c3b6258ad916df7deeecf83/src/wallet/wallet.cpp#L1381).After digging into this a bit more, I think
nTimeFirstKey
is only read when the wallet is being loaded from file here:It’s set in
LoadKeyMetadata()
,LoadScriptMetadata()
, andGenerateNewKey()
(throughUpdateTimeFirstKey()
) before then, but any time thatnTimeFirstKey
is set after wallet load is unnecessary, since it doesn’t get read again, and isn’t persisted to disk. Notably, the calls in the rpc methods toUpdateTimeFirstKey()
that are removed in this PR didn’t actually achieve anything.I think
nTimeFirstKey
should be removed as a wallet global entirely. I think-rescan
as a wallet command-line argument can also be removed since we now have therescan()
rpc method (removing-rescan
has been suggested here: #7061 (comment) and here: #13044 (comment)). If we want to retain the ability to rescan from the wallet’s birthday (ie the earliest key birthday in the wallet), then there should be a functionGetWalletBirthday()
to iterate through the wallet keys and return the wallet’s birthday. Those things can be done in a future PRs.That’s a long way of saying that this PR may change when
UpdateTimeFirstKey()
is called, but that I don’t think it has an functional impact. I just wanted to avoid other reviewers from falling down the same rabbit hole.utACK aee171b1c2f7201cb0780ce9049e322157506e6c
DrahtBot added the label Needs rebase on Feb 1, 2019ryanofsky approvedNits from PR 14565 291130c939meshcollider commented at 6:52 am on February 5, 2019: contributorRebased after #15235DrahtBot removed the label Needs rebase on Feb 5, 2019Make AddKeyPubKey accept timestamp for metadata 6e51d1d6dbin src/wallet/rpcdump.cpp:651 in 6e51d1d6db
647@@ -651,7 +648,8 @@ UniValue importwallet(const JSONRPCRequest& request) 648 continue; 649 } 650 pwallet->WalletLogPrintf("Importing %s...\n", EncodeDestination(keyid)); 651- if (!pwallet->AddKeyPubKey(key, pubkey)) { 652+ nTimeBegin = std::min(nTimeBegin, time);
jnewbery commented at 4:26 pm on February 5, 2019:I think the update to
nTimeBegin
here:https://github.com/bitcoin/bitcoin/pull/15032/files#diff-522490d83dce5375d423b23886e4125eR659
is better. We don’t want to update
nTimeBegin
if theAddKeyPubKey()
fails.I also think you can remove the update to the key metadata here:
https://github.com/bitcoin/bitcoin/pull/15032/files#diff-522490d83dce5375d423b23886e4125eR656
since
AddKeyPubKey()
will now do that for you.Finally, I think:
https://github.com/bitcoin/bitcoin/pull/15032/files#diff-522490d83dce5375d423b23886e4125eR683
is unnecessary for the reasons described in #15306, but that can be left for a future PR.
jnewbery commented at 4:30 pm on February 5, 2019: memberRebase looks generally good, but one comment inline about tidying this up a bit more.DrahtBot commented at 10:04 pm on February 7, 2019: memberDrahtBot added the label Needs rebase on Feb 7, 2019ryanofsky approvedfanquake commented at 9:44 am on June 17, 2019: member@meshcollider Will you have a chance to follow up here soon? Otherwise maybe we can label up for grabs and see if anyone else wants to pick it up.fanquake added the label Up for grabs on Aug 14, 2019fanquake commented at 8:14 am on August 14, 2019: memberThis has idled for a while, needs a rebase and the last comments addressed. Closing with “Up for grabs”.fanquake closed this on Aug 14, 2019
laanwj removed the label Needs rebase on Oct 24, 2019DrahtBot locked this on Dec 16, 2021
github-metadata-mirror
This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-21 15:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me