Use importmulti timestamp when importing watch only keys (on top of #9682) #9108

pull ryanofsky wants to merge 5 commits into bitcoin:master from ryanofsky:watchtime changing 8 files +206 −69
  1. ryanofsky commented at 10:05 PM on November 8, 2016: member

    (This is based on #9682 to avoid merge conflicts in importmulti.py. Only the two commits "Dedup nTimeFirstKey update logic" and "Use importmulti timestamp when importing watch only keys" pertain to this PR. The other three commits belong to #9682).

    This fixes issue #9034.

    When importing a watch-only address over importmulti with a specific timestamp, the wallet's nTimeFirstKey is currently set to 1. After this change, the provided timestamp will be used and stored as metadata associated with watch-only key. This can improve wallet performance because it can avoid the need to scan the entire blockchain for watch only addresses when timestamps are provided.

  2. fanquake added the label Wallet on Nov 9, 2016
  3. fanquake added the label RPC/REST/ZMQ on Nov 9, 2016
  4. fanquake commented at 5:37 AM on November 9, 2016: member

    The fundrawtransaction test failed on two linux builds, same error for both:

    .........
fundrawtransaction.py:
Initializing test directory /tmp/testz6xi8e_p/637
start_node: bitcoind started, waiting for RPC to come up
start_node: RPC succesfully started
start_node: bitcoind started, waiting for RPC to come up
start_node: RPC succesfully started
start_node: bitcoind started, waiting for RPC to come up
start_node: RPC succesfully started
start_node: bitcoind started, waiting for RPC to come up
start_node: RPC succesfully started
Mining blocks...
start_node: bitcoind started, waiting for RPC to come up
start_node: RPC succesfully started
start_node: bitcoind started, waiting for RPC to come up
start_node: RPC succesfully started
start_node: bitcoind started, waiting for RPC to come up
start_node: RPC succesfully started
start_node: bitcoind started, waiting for RPC to come up
start_node: RPC succesfully started
Stopping nodes
Cleaning up
Tests successful
stderr:
Warning: Error reading wallet.dat! All keys read correctly, but transaction data or address book entries might be missing or incorrect.
Pass: False, Duration: 41 s
  5. ryanofsky force-pushed on Nov 10, 2016
  6. ryanofsky force-pushed on Nov 10, 2016
  7. ryanofsky commented at 7:54 PM on November 10, 2016: member

    Fixed fundrawtransaction test, and added importmulti tests.

  8. ryanofsky renamed this:
    WIP: Use importmulti timestamp when importing watch only keys
    Use importmulti timestamp when importing watch only keys
    on Nov 10, 2016
  9. jonasschnelli commented at 10:13 AM on January 1, 2017: contributor

    Concept ACK. Plans to test this soon.

  10. laanwj added this to the milestone 0.14.0 on Jan 19, 2017
  11. gmaxwell commented at 8:28 PM on January 19, 2017: contributor

    utACK.

  12. in src/wallet/wallet.cpp:None in 6a52c4de03 outdated 
     218 | @@ -222,6 +219,17 @@ bool CWallet::LoadCryptedKey(const CPubKey &vchPubKey, const std::vector<unsigne
 219 |      return CCryptoKeyStore::AddCryptedKey(vchPubKey, vchCryptedSecret);
 220 |  }
 221 |  
 222 | +void CWallet::UpdateTimeFirstKey(int nCreateTime)
    jonasschnelli commented at 9:43 AM on January 20, 2017:

    shouldn't this be int64_t nCreateTime.

    ryanofsky commented at 7:05 PM on January 23, 2017:

    Good catch, fixed in 6891d5e1f24e74561aeba45c4752756a760c281d.

  13. in src/wallet/walletdb.cpp:None in 6a52c4de03 outdated 
     456 | @@ -454,20 +457,27 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue,
 457 |              }
 458 |              wss.fIsEncrypted = true;
 459 |          }
 460 | -        else if (strType == "keymeta")
 461 | +        else if (strType == "keymeta" || strType == "watchmeta")
 462 |          {
 463 | -            CPubKey vchPubKey;
 464 | -            ssKey >> vchPubKey;
 465 | +            uint160 keyId;
    jonasschnelli commented at 9:45 AM on January 20, 2017:

    Is there a reason for not using CKeyID here?

    ryanofsky commented at 6:45 PM on January 23, 2017:

    Again this can hold CScriptID values, and is changed to CTxDestination in 777b3db229e162c524d49ab7b448421f51bfa5ca.

  14. in src/wallet/wallet.cpp:None in 6a52c4de03 outdated 
    3261 | @@ -3247,7 +3262,7 @@ void CWallet::GetKeyBirthTimes(std::map<CKeyID, int64_t> &mapKeyBirth) const {
3262 |      mapKeyBirth.clear();
3263 |  
3264 |      // get birth times for keys with metadata
3265 | -    for (std::map<CKeyID, CKeyMetadata>::const_iterator it = mapKeyMetadata.begin(); it != mapKeyMetadata.end(); it++)
3266 | +    for (std::map<uint160, CKeyMetadata>::const_iterator it = mapKeyMetadata.begin(); it != mapKeyMetadata.end(); it++)
    jonasschnelli commented at 9:46 AM on January 20, 2017:

    I don't see the reason for changing this from CKeyID to uint160?

    ryanofsky commented at 5:41 PM on January 23, 2017:

    Changed from uint160 to CTxDestination in 777b3db229e162c524d49ab7b448421f51bfa5ca. The map isn't keyed by CKeyID anymore because it can also contain CScriptID entries for watch only keys.

  15. jonasschnelli changes_requested
  16. morcos commented at 9:20 PM on January 23, 2017: member

    utACK 6891d5e

  17. jtimon commented at 11:34 PM on January 23, 2017: contributor

    Concept ACK

  18. in src/rpc/misc.cpp:None in 6891d5e1f2 outdated 
     204 | @@ -204,10 +205,20 @@ UniValue validateaddress(const JSONRPCRequest& request)
 205 |          if (pwalletMain && pwalletMain->mapAddressBook.count(dest))
 206 |              ret.push_back(Pair("account", pwalletMain->mapAddressBook[dest].name));
 207 |          CKeyID keyID;
 208 | -        if (pwalletMain && address.GetKeyID(keyID) && pwalletMain->mapKeyMetadata.count(keyID) && !pwalletMain->mapKeyMetadata[keyID].hdKeypath.empty())
 209 | +        if (pwalletMain)
 210 |          {
 211 | -            ret.push_back(Pair("hdkeypath", pwalletMain->mapKeyMetadata[keyID].hdKeypath));
 212 | -            ret.push_back(Pair("hdmasterkeyid", pwalletMain->mapKeyMetadata[keyID].hdMasterKeyID.GetHex()));
 213 | +            auto it = pwalletMain->mapKeyMetadata.find(CScriptID(scriptPubKey));
    TheBlueMatt commented at 6:31 PM on January 25, 2017:

    Shouldn't you try address.GetKeyID(keyID) first before you give up and use the CScriptID? Otherwise you end up using the P2SH of the P2PH.

    ryanofsky commented at 4:29 PM on February 1, 2017:

    Changed lookup order in 1ea9543ac25cd8976430beab2f913d4875767e8b. I didn't understand what you mean by P2SH of the P2PH, but I think it does make sense to prefer regular key metadata over watchonly key metadata if both exist.

  19. in src/wallet/wallet.cpp:None in 6891d5e1f2 outdated 
     258 | @@ -251,11 +259,18 @@ bool CWallet::AddWatchOnly(const CScript &dest)
 259 |  {
    TheBlueMatt commented at 7:01 PM on January 25, 2017:

    I'd significantly prefer to not expose this version, making it explicit to callers that the key's nCreateTime is being set to 0 (which, after this PR, implies the wallet's nTimeFirstKey is set to 1).

    TheBlueMatt commented at 7:13 PM on January 25, 2017:

    The second two calls to AddWatchOnly in processImport probably could take the timestamp argument.

    TheBlueMatt commented at 4:04 PM on January 26, 2017:

    Grr, meant for this to be on AddWatchOnly at line https://github.com/bitcoin/bitcoin/pull/9108/files#diff-b2bb174788c7409b671c46ccc86034bdL251

    ryanofsky commented at 6:58 PM on February 2, 2017:

    Thanks, added the two missing timestamp arguments in f87008a8b0edc677db6ec16036f7e9940a3c904d, and expanded the test to verify the fix.

    I very much agree that it's not good to expose the AddWatchOnly version not taking a timestamp, so I made it private in 1b9969682e59e082855179e4c50fb1dd38227d67. I also experimented with getting rid of the overload by adding the timestamp to AddWatchOnly methods in the keystore superclasses, but this was a bigger change and made the keystore and walletdb code more confusing, so I went with this simpler fix.

  20. in src/wallet/walletdb.cpp:None in 6891d5e1f2 outdated 
     127 |  }
 128 |  
 129 |  bool CWalletDB::EraseWatchOnly(const CScript &dest)
 130 |  {
 131 |      nWalletDBUpdated++;
 132 | +    if (!Erase(std::make_pair(std::string("watchmeta"), *(const CScriptBase*)(&dest))))
    TheBlueMatt commented at 7:04 PM on January 25, 2017:

    I think we need to ignore this error - otherwise if a watchonly has been added in a previous version we will fail to erase here (as it might have no watchmeta)?

    ryanofsky commented at 4:29 PM on February 1, 2017:

    Having no watchmeta shouldn't be a problem. Erase still returns true if the key is not found. It just false if there is a database error.

  21. in src/wallet/walletdb.cpp:None in 6891d5e1f2 outdated 
     115 | @@ -116,15 +116,20 @@ bool CWalletDB::WriteCScript(const uint160& hash, const CScript& redeemScript)
 116 |      return Write(std::make_pair(std::string("cscript"), hash), *(const CScriptBase*)(&redeemScript), false);
 117 |  }
 118 |  
 119 | -bool CWalletDB::WriteWatchOnly(const CScript &dest)
 120 | +bool CWalletDB::WriteWatchOnly(const CScript &dest, const CKeyMetadata& keyMeta)
 121 |  {
 122 |      nWalletDBUpdated++;
 123 | +    if (!Write(std::make_pair(std::string("watchmeta"), *(const CScriptBase*)(&dest)),
 124 | +               keyMeta, false))
    TheBlueMatt commented at 7:11 PM on January 25, 2017:

    Maybe do overwrite here? Seems strange that we update in-wallet-state without updating on-disk state (and its probably a supported use-case to re-add something to wallet to set its date differently).

    ryanofsky commented at 4:29 PM on February 1, 2017:

    Enabled overwrite in 50474f1305693bf22a995f0841ee27dad545d01a. This was just a mistake copying the line of code from taken from CWalletDB::WriteKey.

  22. in src/wallet/walletdb.cpp:None in 99c1aa1695 outdated 
     462 | @@ -463,11 +463,6 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue,
 463 |              wss.nKeyMeta++;
 464 |  
 465 |              pwallet->LoadKeyMetadata(vchPubKey, keyMeta);
 466 | -
    jtimon commented at 1:56 AM on February 1, 2017:

    Missing this here?

    pwallet->UpdateTimeFirstKey(keyMeta.nCreateTime);

    EDIT: no, LoadKeyMetadata calls it.

  23. jtimon commented at 2:02 AM on February 1, 2017: contributor

    utACK individual commits: 99c1aa169592a71c24e7a2475c50ad7598eafb65 6891d5e1f24e74561aeba45c4752756a760c281d

    Can we squash already?

  24. ryanofsky force-pushed on Feb 1, 2017
  25. ryanofsky commented at 4:27 PM on February 1, 2017: member

    Squashed 6891d5e1f24e74561aeba45c4752756a760c281d -> 89011a3032f635b6536a476bfa48502c5dcc7a65, and will look into Matt's comments.

  26. ryanofsky referenced this in commit 1ea9543ac2 on Feb 2, 2017
  27. ryanofsky referenced this in commit 50474f1305 on Feb 2, 2017
  28. ryanofsky referenced this in commit f87008a8b0 on Feb 2, 2017
  29. ryanofsky force-pushed on Feb 2, 2017
  30. ryanofsky commented at 7:18 PM on February 2, 2017: member

    Squashed 1b9969682e59e082855179e4c50fb1dd38227d67 -> 3d6dbed2c3132d53650b86f09d42e339149d3210 (watchtime.3 -> watchtime.4)

  31. ryanofsky force-pushed on Feb 2, 2017
  32. ryanofsky commented at 7:39 PM on February 2, 2017: member

    Latest changes conflicted (trivially) with #9607 and #9121, so rebased 3d6dbed2c3132d53650b86f09d42e339149d3210 -> b78ec3cc3dbf5e1fc2dc105c609574101e462ae8 (watchtime.4 -> watchtime.5)

  33. in src/wallet/wallet.h:None in b78ec3cc3d outdated 
     609 | +    /**
 610 | +     * Private version of AddWatchOnly method which does not accept a timestamp.
 611 | +     * Because this is an inherited virtual method, it is accessible despite
 612 | +     * being marked private, but it is marked private anyway to encourage use of
 613 | +     * the other AddWatchOnly which accepts a timestamp and sets nTimeFirstKey
 614 | +     * more intelligently for more efficient rescans.
    TheBlueMatt commented at 9:45 PM on February 3, 2017:

    Ugh, I totally missed that its an override, thanks for catching that. Can you further document here that this method implies setting the first key time to 1?

    ryanofsky commented at 10:04 PM on February 3, 2017:

    Added in 5dbd05d8f7c4fc86d1296a200a00a834b61fb5c3

  34. TheBlueMatt commented at 9:51 PM on February 3, 2017: member

    utACK b78ec3cc3dbf5e1fc2dc105c609574101e462ae8 +/- wanting another comment, Didn't look too closely at tests.

  35. ryanofsky referenced this in commit 5dbd05d8f7 on Feb 3, 2017
  36. ryanofsky force-pushed on Feb 3, 2017
  37. ryanofsky commented at 10:08 PM on February 3, 2017: member

    Added comment in 5dbd05d8f7c4fc86d1296a200a00a834b61fb5c3.

    Squashed 5dbd05d8f7c4fc86d1296a200a00a834b61fb5c3 -> b8fca20399c3528e5c293509629907f6874575e3 (watchtime.6 -> watchtime.7)

  38. ryanofsky force-pushed on Feb 6, 2017
  39. ryanofsky commented at 6:11 PM on February 6, 2017: member

    Rebased b8fca20399c3528e5c293509629907f6874575e3 -> be820d8ad761b87ca61427ce8b3005b0f6db0d06 (watchtime.7 -> watchtime.8) to fix conflict with #9227.

  40. TheBlueMatt commented at 3:13 PM on February 7, 2017: member

    be820d8ad761b87ca61427ce8b3005b0f6db0d06 appears to be equivalent to my previous utACK (b78ec3cc3dbf5e1fc2dc105c609574101e462ae8) plus a fixup for the comments I requested.

  41. laanwj commented at 6:41 AM on February 9, 2017: member

    @jonasschnelli Looks like your suggestions have been carried out by @ryanofsky and the "changes requested" can be cancelled?

  42. in src/rpc/misc.cpp:None in be820d8ad7 outdated 
     166 | @@ -167,6 +167,7 @@ UniValue validateaddress(const JSONRPCRequest& request)
 167 |              "  \"pubkey\" : \"publickeyhex\",    (string) The hex value of the raw public key\n"
 168 |              "  \"iscompressed\" : true|false,  (boolean) If the address is compressed\n"
 169 |              "  \"account\" : \"account\"         (string) DEPRECATED. The account associated with the address, \"\" is the default account\n"
 170 | +            "  \"timestamp\" : \"int64\"         (number, optional) The creation time of the key if available\n"
    kallewoof commented at 7:37 AM on February 9, 2017:

    Nit: JSON represents integer values without quotes, so probably " \"timestamp\" : int64 (number, optional) The crea..."

    ryanofsky commented at 10:59 AM on February 9, 2017:

    Fixed in 6a56f6d77a4e0679ab88b354dea7143ec8539c4e

  43. in src/wallet/rpcdump.cpp:None in be820d8ad7 outdated 
     160 | @@ -161,7 +161,7 @@ void ImportScript(const CScript& script, const string& strLabel, bool isRedeemSc
 161 |  
 162 |      pwalletMain->MarkDirty();
 163 |  
 164 | -    if (!pwalletMain->HaveWatchOnly(script) && !pwalletMain->AddWatchOnly(script))
 165 | +    if (!pwalletMain->HaveWatchOnly(script) && !pwalletMain->AddWatchOnly(script, 0 /* nCreateTime */))
    kallewoof commented at 7:39 AM on February 9, 2017:

    Is there a reason why this is not simply defaulting to 0 in AddWatchOnly() which would leave this line untouched?

    ryanofsky commented at 10:59 AM on February 9, 2017:

    There was a lot of discussion about this above. We think it is safer to require the timestamp.

  44. in src/wallet/rpcdump.cpp:None in be820d8ad7 outdated 
     583 |  
 584 |      // sort time/key pairs
 585 |      std::vector<std::pair<int64_t, CKeyID> > vKeyBirth;
 586 | -    for (std::map<CKeyID, int64_t>::const_iterator it = mapKeyBirth.begin(); it != mapKeyBirth.end(); it++) {
 587 | -        vKeyBirth.push_back(std::make_pair(it->second, it->first));
 588 | +    for (auto it = mapKeyBirth.begin(); it != mapKeyBirth.end(); it++) {
    kallewoof commented at 7:44 AM on February 9, 2017:

    Nit:

    -    for (auto it = mapKeyBirth.begin(); it != mapKeyBirth.end(); it++) {
-        if (const CKeyID* keyID = boost::get<CKeyID>(&it->first)) {
-            vKeyBirth.push_back(std::make_pair(it->second, *keyID));
+    for (const auto& it : mapKeyBirth) {
+        if (const CKeyID* keyID = boost::get<CKeyID>(&it.first)) {
+            vKeyBirth.push_back(std::make_pair(it.second, *keyID));
    ryanofsky commented at 11:00 AM on February 9, 2017:

    Fixed in 6a56f6d77a4e0679ab88b354dea7143ec8539c4e

  45. in src/wallet/rpcdump.cpp:None in be820d8ad7 outdated 
     584 |      // sort time/key pairs
 585 |      std::vector<std::pair<int64_t, CKeyID> > vKeyBirth;
 586 | -    for (std::map<CKeyID, int64_t>::const_iterator it = mapKeyBirth.begin(); it != mapKeyBirth.end(); it++) {
 587 | -        vKeyBirth.push_back(std::make_pair(it->second, it->first));
 588 | +    for (auto it = mapKeyBirth.begin(); it != mapKeyBirth.end(); it++) {
 589 | +        if (const CKeyID* keyID = boost::get<CKeyID>(&it->first)) {
    kallewoof commented at 7:45 AM on February 9, 2017:

    Nit: worthwhile to add // set and test or similar as the single = might be mistaken for == by someone reading the code.

    ryanofsky commented at 11:00 AM on February 9, 2017:

    Fixed in 6a56f6d77a4e0679ab88b354dea7143ec8539c4e

  46. in src/wallet/wallet.cpp:None in be820d8ad7 outdated 
    3436 |      AssertLockHeld(cs_wallet); // mapKeyMetadata
3437 |      mapKeyBirth.clear();
3438 |  
3439 |      // get birth times for keys with metadata
3440 | -    for (std::map<CKeyID, CKeyMetadata>::const_iterator it = mapKeyMetadata.begin(); it != mapKeyMetadata.end(); it++)
3441 | +    for (auto it = mapKeyMetadata.begin(); it != mapKeyMetadata.end(); it++)
    kallewoof commented at 7:49 AM on February 9, 2017:

    Nit: as above, can be made to look a bit nicer with for (const auto& it : mapKeyMetadata) and s/it->/it./.

    ryanofsky commented at 11:00 AM on February 9, 2017:

    Fixed in 6a56f6d77a4e0679ab88b354dea7143ec8539c4e

  47. in src/wallet/wallet.h:None in be820d8ad7 outdated 
     762 | @@ -748,7 +763,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
 763 |      bool GetDestData(const CTxDestination &dest, const std::string &key, std::string *value) const;
 764 |  
 765 |      //! Adds a watch-only address to the store, and saves it to disk.
 766 | -    bool AddWatchOnly(const CScript &dest);
 767 | +    bool AddWatchOnly(const CScript &dest, int64_t nCreateTime);
    kallewoof commented at 7:50 AM on February 9, 2017:

    Nit: (see above) make 2nd argument optional, Nit 2: CScript& dest

    bool AddWatchOnly(const CScript& dest, int64_t nCreateTime = 0);
    ryanofsky commented at 11:00 AM on February 9, 2017:

    Nit 2 fixed in b3ba65488d575899c6df2a98f9dc6447c93ae22d

  48. in src/wallet/wallet.cpp:None in be820d8ad7 outdated 
     218 | @@ -222,6 +219,17 @@ bool CWallet::LoadCryptedKey(const CPubKey &vchPubKey, const std::vector<unsigne
 219 |      return CCryptoKeyStore::AddCryptedKey(vchPubKey, vchCryptedSecret);
 220 |  }
 221 |  
 222 | +void CWallet::UpdateTimeFirstKey(int64_t nCreateTime)
 223 | +{
 224 | +    AssertLockHeld(cs_wallet);
 225 | +    // Cannot determine birthday information, so set the wallet birthday to the
 226 | +    // beginning of time.
 227 | +    if (nCreateTime <= 1)
    kallewoof commented at 8:07 AM on February 9, 2017:

    Maybe I'm misreading the code, but the comment Cannot determine birthday information seems to refer to the if case below it being true; I would prefer comments being inside the block in this case. I would probably also include brackets after doing so as the comment makes the single-line conditional state less obvious.

    ryanofsky commented at 11:00 AM on February 9, 2017:

    Fixed in f95843a294dad786117bd40801c8bf40afcb8784

  49. kallewoof approved
  50. kallewoof commented at 8:09 AM on February 9, 2017: member

    Slight/weak ACK (some testing but not a lot)

  51. ryanofsky referenced this in commit f95843a294 on Feb 9, 2017
  52. ryanofsky referenced this in commit 6a56f6d77a on Feb 9, 2017
  53. ryanofsky referenced this in commit b3ba65488d on Feb 9, 2017
  54. ryanofsky force-pushed on Feb 9, 2017
  55. ryanofsky commented at 11:35 AM on February 9, 2017: member

    Squashed b3ba65488d575899c6df2a98f9dc6447c93ae22d -> f35ce5172bb9c6bec6bb636ea349104b559d36de (watchtime.10 -> watchtime.11)

  56. jonasschnelli approved
  57. jonasschnelli commented at 12:15 PM on February 9, 2017: contributor

    UtACK f35ce5172bb9c6bec6bb636ea349104b559d36de

  58. Require timestamps for importmulti keys
    Additionally, accept a "now" timestamp, to allow avoiding rescans for keys
which are known never to have been used.

Note that the behavior when "now" is specified is slightly different than the
previous behavior when no timestamp was specified at all. Previously, when no
timestamp was specified, it would avoid rescanning during the importmulti call,
but set the key's nCreateTime value to 1, which would not prevent future block
reads in later ScanForWalletTransactions calls. With this change, passing a
"now" timestamp will set the key's nCreateTime to the current block time
instead of 1.

Fixes #9491
    442887f27f
  59. Add test to check new importmulti "now" value
    Easiest way to test this was to expose the timestamp via the validateaddress
RPC (which was already looking up and returning key metadata).
    3cf991756c
  60. Use MTP for importmulti "now" timestamps 266a8114cb
  61. Dedup nTimeFirstKey update logic
    Also make nTimeFirstKey member variable private.

This is just a cleanup change, it doesn't change behavior in any significant
way.
    a58370e6a2
  62. Use importmulti timestamp when importing watch only keys
    When importing a watch-only address over importmulti with a specific timestamp,
the wallet's nTimeFirstKey is currently set to 1. After this change, the
provided timestamp will be used and stored as metadata associated with
watch-only key. This can improve wallet performance because it can avoid the
need to scan the entire blockchain for watch only addresses when timestamps are
provided.

Also adds timestamp to validateaddress return value (needed for tests).

Fixes #9034.
    a80f98b1c7
  63. ryanofsky force-pushed on Feb 10, 2017
  64. ryanofsky renamed this:
    Use importmulti timestamp when importing watch only keys
    Use importmulti timestamp when importing watch only keys (on top of #9682)
    on Feb 10, 2017
  65. ryanofsky commented at 9:18 PM on February 10, 2017: member

    Rebased on top of #9682 to avoid merge conflicts in the importmulti.py test. No other changes.

    f35ce5172bb9c6bec6bb636ea349104b559d36de -> a80f98b1c7a49432dc53d18d0fb51ac334de96be (watchtime.11 -> watchtime.12)

  66. jonasschnelli commented at 2:06 PM on February 14, 2017: contributor

    Not sure if this is a nit or the intended behaviour: If I re-import a watch only address (same address I have already imported), I get no notice that it was already present. This may be okay.

    Another option would be to reject the import with "already exists". But the current overwrite-behaviour allows to re-set the timestamp which can be handy.

  67. jonasschnelli commented at 2:20 PM on February 14, 2017: contributor

    Correction of my comment above. If you re-import a watch-only, it does not set the new timestamp but still response with "success". I think we should do this (fail when reimport a watch-only)...

    diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp
index 0a32259..401614c 100644
--- a/src/wallet/rpcdump.cpp
+++ b/src/wallet/rpcdump.cpp
@@ -922,9 +922,12 @@ UniValue ProcessImport(const UniValue& data, const int64_t timestamp)
                     throw JSONRPCError(RPC_WALLET_ERROR, "The wallet already contains the private key for this address or script");
                 }
 
-                pwalletMain->MarkDirty();
+                if (pwalletMain->HaveWatchOnly(script)) {
+                    throw JSONRPCError(RPC_WALLET_ERROR, "The wallet already contains this address or script");
+                }
 
-                if (!pwalletMain->HaveWatchOnly(script) && !pwalletMain->AddWatchOnly(script, timestamp)) {
+                pwalletMain->MarkDirty();
+                if (!pwalletMain->AddWatchOnly(script, timestamp)) {
                     throw JSONRPCError(RPC_WALLET_ERROR, "Error adding address to wallet");
                 }

    An alternative would be to overwrite the timestamp with the new one.

  68. ryanofsky commented at 3:14 PM on February 14, 2017: member

    Ok, I can add logic to error on attempts to import watch only keys that have already imported. Note that your fix above is incomplete because it only covers importmulti, not importaddress, importpubkey, etc.

    I'll do it in a different PR though, unless you think it needs to be here, because this is longstanding behavior not really related to saving timestamps.

  69. jonasschnelli commented at 4:12 PM on February 14, 2017: contributor

    Tested ACK of a58370e6a2d4dce50eefbcab5bde9f14facef8fc a80f98b1c7a49432dc53d18d0fb51ac334de96be on top of master (e87ce95fbdc6ca6ef822c978d98b2acba5948ee1). I agree with @ryanofsky that the overwrite-/already-exists-behavior can be fixed later.

  70. morcos commented at 5:48 PM on February 14, 2017: member

    re-utACK a80f98b

  71. sipa commented at 9:06 AM on February 15, 2017: member

    utACK a80f98b1c7a49432dc53d18d0fb51ac334de96be

  72. laanwj merged this on Feb 15, 2017
  73. laanwj closed this on Feb 15, 2017
  74. laanwj referenced this in commit d8e8b06bd0 on Feb 15, 2017
  75. ryanofsky referenced this in commit 0f25026b55 on Feb 15, 2017
  76. ryanofsky referenced this in commit 07afcd6379 on Feb 15, 2017
  77. ryanofsky referenced this in commit a496e16775 on Feb 21, 2017
  78. ryanofsky referenced this in commit 7759aa23d1 on Mar 3, 2017
  79. practicalswift referenced this in commit 03606b35e4 on Apr 27, 2017
  80. deadalnix referenced this in commit fe14ea286e on Jan 20, 2018
  81. codablock referenced this in commit e99b716e86 on Jan 31, 2018
  82. codablock referenced this in commit 1186bd3519 on Jan 31, 2018
  83. codablock referenced this in commit de17fd766b on Jan 31, 2018
  84. codablock referenced this in commit 43f697866e on Feb 1, 2018
  85. HashUnlimited referenced this in commit c69f3be35e on Feb 27, 2018
  86. HashUnlimited referenced this in commit c22bb21e07 on Feb 27, 2018
  87. HashUnlimited referenced this in commit 4d33acaaba on Feb 28, 2018
  88. lateminer referenced this in commit ada5ec795c on Jan 4, 2019
  89. andvgal referenced this in commit 165188b133 on Jan 6, 2019
  90. CryptoCentric referenced this in commit b0a82fc804 on Feb 28, 2019
  91. CryptoCentric referenced this in commit 45ba8c2f70 on Mar 2, 2019
  92. DrahtBot locked this on Sep 8, 2021
Contributors
ryanofskyfanquakejonasschnelligmaxwellmorcosjtimonTheBlueMattlaanwjkallewoofsipa
Labels
WalletRPC/REST/ZMQ

Milestone
0.14.0

Linked (view graph)
#9034 importmulti does not respect the given timestamp#9682 Require timestamps for importmulti keys#9771 Add missing cs_wallet lock that triggers new lock held assertion#9818 Save watch only key timestamps when reimporting keys
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: 2026-04-19 03:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me