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
  1. meshcollider commented at 12:13 pm on December 24, 2018: contributor
    Fixup the few final nits from #14565
  2. meshcollider added the label Wallet on Dec 24, 2018
  3. meshcollider added the label RPC/REST/ZMQ on Dec 24, 2018
  4. 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.

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

    sipa commented at 12:44 pm on December 27, 2018:
    @promag You’re talking a pointer to a char array and adding an integer to it. If i is larger than the number of characters in the string, this results in reading in a buffer overflow.

    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 🤦‍♂️
  6. 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?
  7. practicalswift commented at 11:37 am on December 27, 2018: contributor
    As a precaution this PR should probably be marked [wip] until the security issue it introduces (see #15032 (review)) has been corrected?
  8. meshcollider commented at 5:00 am on December 28, 2018: contributor
    Fixed the bug mentioned above
  9. in 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?
  10. laanwj commented at 1:29 pm on January 2, 2019: member

    utACK adee4eed8cf4a45f4095c653073c27509a931d2e

    Agree it would be nice to have a test for “Invalid private key encoding at index…” as there was a near-screwup there.

  11. 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:
  12. 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, after CWallet::AddKeyPubKeyWithDB and CWallet::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

  13. 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. If nTime is less than nTimeBegin, then the UpdateTimeFirstKey call inside AddKeyPubKey will have already updated the minimum time.

    jnewbery commented at 4:13 pm on January 31, 2019:
    nTimeBegin is also used below in the RescanWallet() 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.

  14. ryanofsky approved
  15. ryanofsky commented at 8:31 pm on January 2, 2019: member
    utACK adee4eed8cf4a45f4095c653073c27509a931d2e
  16. in 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 :)
  17. DrahtBot added the label Needs rebase on Jan 9, 2019
  18. in 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.
  19. jnewbery commented at 7:44 pm on January 14, 2019: member

    Thanks for doing this @MeshCollider . Lightly tested ACK adee4eed8cf4a45f4095c653073c27509a931d2e.

    I have one further nit inline that you could take or leave.

  20. achow101 commented at 10:40 pm on January 15, 2019: member

    utACK adee4eed8cf4a45f4095c653073c27509a931d2e

    Needs rebase.

  21. DrahtBot removed the label Needs rebase on Jan 16, 2019
  22. Sjors commented at 4:14 pm on January 16, 2019: member
    Travis linter fails, probably due to too modern Python syntax test/functional/wallet_importmulti.py:395:23: F821 undefined name 'hex_str_to_bytes'
  23. MarcoFalke commented at 4:48 pm on January 16, 2019: member

    Failure 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
    
  24. meshcollider commented at 8:27 pm on January 16, 2019: contributor
    Ah that looks like its just the import statements being moved around in the importmulti test cleanup when I rebased this on master.
  25. laanwj commented at 1:18 pm on January 31, 2019: member
    What 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!
  26. Sjors commented at 3:58 pm on January 31, 2019: member

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

  27. jnewbery commented at 9:42 pm on January 31, 2019: member

    I thought that this might have introduced a behaviour change in CWallet::DeriveNewSeed() which calls AddKeyPubKey() without a timestamp. CWallet::DeriveNewSeed() now causes UpdateTimeFirstKey() to be called which sets nTimeFirstKey to min(current time, existing nTimeFirstKey) (since the mapKeyMetadata 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:

    https://github.com/bitcoin/bitcoin/blob/3b19d8e341a5234c3e41f59f7b3de8febfc51c21/src/wallet/wallet.cpp#L4217

    It’s set in LoadKeyMetadata(), LoadScriptMetadata(), and GenerateNewKey() (through UpdateTimeFirstKey()) before then, but any time that nTimeFirstKey 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 to UpdateTimeFirstKey() 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 the rescan() 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 function GetWalletBirthday() 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

  28. DrahtBot added the label Needs rebase on Feb 1, 2019
  29. ryanofsky approved
  30. ryanofsky commented at 8:10 pm on February 4, 2019: member
    utACK aee171b1c2f7201cb0780ce9049e322157506e6c. Only change since last review is rebase after #15108.
  31. Nits from PR 14565 291130c939
  32. meshcollider commented at 6:52 am on February 5, 2019: contributor
    Rebased after #15235
  33. DrahtBot removed the label Needs rebase on Feb 5, 2019
  34. Make AddKeyPubKey accept timestamp for metadata 6e51d1d6db
  35. in 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 the AddKeyPubKey() 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.

  36. jnewbery commented at 4:30 pm on February 5, 2019: member
    Rebase looks generally good, but one comment inline about tidying this up a bit more.
  37. DrahtBot commented at 10:04 pm on February 7, 2019: member
  38. DrahtBot added the label Needs rebase on Feb 7, 2019
  39. ryanofsky approved
  40. ryanofsky commented at 3:49 pm on March 11, 2019: member
    utACK 6e51d1d6dbf8d27a3efc1f17358830c7b3a771c3, but it would be good to address John’s comment. Changes since last review: rebase after #15235 and adding nTimeBegin assignment.
  41. fanquake 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.
  42. fanquake added the label Up for grabs on Aug 14, 2019
  43. fanquake commented at 8:14 am on August 14, 2019: member
    This has idled for a while, needs a rebase and the last comments addressed. Closing with “Up for grabs”.
  44. fanquake closed this on Aug 14, 2019

  45. laanwj removed the label Needs rebase on Oct 24, 2019
  46. DrahtBot 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