Require timestamps for importmulti keys #9682

pull ryanofsky wants to merge 3 commits into bitcoin:master from ryanofsky:pr/multinow changing 4 files +77 −13
  1. ryanofsky commented at 9:42 pm on February 3, 2017: member

    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

  2. ryanofsky force-pushed on Feb 3, 2017
  3. TheBlueMatt commented at 9:53 pm on February 3, 2017: member
    Needs 0.14 tag.
  4. in src/wallet/rpcdump.cpp: in e6fabf6780 outdated 
    983@@ -970,13 +984,16 @@ UniValue importmulti(const JSONRPCRequest& mainRequest)
984             "  [     (array of json objects)\n"
985             "    {\n"
986             "      \"scriptPubKey\": \"<script>\" | { \"address\":\"<address>\" }, (string / json, required) Type of scriptPubKey (string for script, json for address)\n"
987+            "      \"timestamp\": 1454686740 | \"now\"                       , (integer / string) Integer timestamp, or the string \"now\" to substitute the current synced\n"
    achow101 commented at 10:34 pm on February 3, 2017:
    You should say whether it is required or optional
    jonasschnelli commented at 7:19 pm on February 5, 2017:
    If we are at this, why do we have the number 1454686740 here? Can’t we just say <timestamp>?
    ryanofsky commented at 3:41 pm on February 6, 2017:
    Updated in af06509f9c0bb92231ab41fd5fd9e89106b65b0e. Personally I think "timestamp": 1454686740 is more informative than "timestamp": <timestamp>, but made the requested change anyway.
  5. fanquake added the label RPC/REST/ZMQ on Feb 4, 2017
  6. fanquake added this to the milestone 0.14.0 on Feb 4, 2017
  7. gmaxwell commented at 10:35 pm on February 4, 2017: contributor
    Concept ACK but people with more opinions (and knowledge about json rpc norms) should comment on the type of the timestamp argument being variable.
  8. jonasschnelli commented at 7:22 pm on February 5, 2017: contributor

    Concept ACK, will test.

    I liked the idea from @sipa where he mentioned that “unknown” as timestamp for avoiding rescans seems to be the better choice (instead of “now”). “Now” would imply that the key was generated during import which is (almost) impossible.

  9. MarcoFalke commented at 7:51 pm on February 5, 2017: member
    sipa proposed timestamp: "unused" instead of “now”. #9491 (comment)
  10. laanwj commented at 11:00 am on February 6, 2017: member

    Concept ACK

    Concept ACK but people with more opinions (and knowledge about json rpc norms) should comment on the type of the timestamp argument being variable.

    It’s hard to wrap in bindings for statically typed languages - could be worked around with an extra flag that specifies how to handle the timestamp.

  11. ryanofsky referenced this in commit af06509f9c on Feb 6, 2017
  12. ryanofsky commented at 4:31 pm on February 6, 2017: member
    Added RPC test to verify that this is correctly interpreting the “now” value and saving the current time as the key creation time in 2cd875a53ed4ea7bde36f943603949f30bdcbc30.
  13. ryanofsky force-pushed on Feb 6, 2017
  14. ryanofsky commented at 4:41 pm on February 6, 2017: member
    Squashed 2cd875a53ed4ea7bde36f943603949f30bdcbc30 -> c07ebb6efced42cb9e4ed8bce387288ae705dca8 (multinow.3 -> multinow.4)
  15. in src/wallet/rpcdump.cpp: in c07ebb6efc outdated 
    1031@@ -1015,6 +1032,12 @@ UniValue importmulti(const JSONRPCRequest& mainRequest)
1032     LOCK2(cs_main, pwalletMain->cs_wallet);
1033     EnsureWalletIsUnlocked();
1034 
1035+    // Verify all timestamps are present before importing any keys.
1036+    const int64_t now = chainActive.Tip() ? chainActive.Tip()->GetBlockTime() : 0;
    TheBlueMatt commented at 3:30 pm on February 7, 2017:
    Can we use the MTP here?
    ryanofsky commented at 4:10 pm on February 7, 2017:
    Switched to MTP in dc402a4444d09263b74483a7d8ed904325dd89f6
  16. TheBlueMatt commented at 3:41 pm on February 7, 2017: member
    utACK c07ebb6efced42cb9e4ed8bce387288ae705dca8 if we switch to using MTP for the “now” timestamp. Did not review tests.
  17. ryanofsky commented at 4:11 pm on February 7, 2017: member
    Now using MTP in dc402a4444d09263b74483a7d8ed904325dd89f6
  18. TheBlueMatt commented at 4:18 pm on February 8, 2017: member
    utACK dc402a4444d09263b74483a7d8ed904325dd89f6
  19. laanwj commented at 6:34 am on February 9, 2017: member

    I liked the idea from @sipa where he mentioned that “unknown” as timestamp for avoiding rescans seems to be the better choice (instead of “now”). “Now” would imply that the key was generated during import which is (almost) impossible. sipa proposed timestamp: "unused" instead of “now”.

    A “unused” flag makes sense, however that is not a property of the timestamp.

  20. kallewoof commented at 8:16 am on February 9, 2017: member
    Timestamp = unused sounds strange to me as well. Timestamp = now makes more sense.
  21. in src/rpc/misc.cpp: in dc402a4444 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\" : <timestamp>,      (number, optional) The creation time of the key if available\n"
    kallewoof commented at 8:19 am on February 9, 2017:
    Nit: Non-string arguments tend to be represented as simply name, not <name>, e.g. https://github.com/bitcoin/bitcoin/blame/master/src/rpc/blockchain.cpp#L649
    ryanofsky commented at 10:36 am on February 9, 2017:
    Fixed in 38c01b530da85850bf466ff3cf0cbd228232e729
  22. in src/wallet/rpcdump.cpp: in dc402a4444 outdated 
    639@@ -640,7 +640,8 @@ UniValue dumpwallet(const JSONRPCRequest& request)
640 }
641 
642 
643-UniValue processImport(const UniValue& data) {
644+UniValue processImport(const UniValue& data, int64_t timestamp)
    kallewoof commented at 8:20 am on February 9, 2017:
    Nit: const int64_t timestamp Nit 2 / coding standard: Functions in Bitcoin generally start with a capital letter, i.e. ProcessImport here. (Exception being the RPC commands, which match the CLI command names.) I know you didn’t name this one, but it coincides with the new function below (getImportTimestamp). Renaming this one should not be a big issue, as it is used in one place only.
    ryanofsky commented at 10:36 am on February 9, 2017:
    Fixed in 6524d401485a8dbd9881e6d69b3f542bb6598d6b
  23. in src/wallet/rpcdump.cpp: in dc402a4444 outdated 
    957@@ -958,6 +958,20 @@ UniValue processImport(const UniValue& data) {
958     }
959 }
960 
961+int64_t getImportTimestamp(const UniValue& data, int64_t now)
    kallewoof commented at 8:22 am on February 9, 2017:
    Nit / coding standard / repeat: GetImportTimestamp (see above)
    ryanofsky commented at 10:37 am on February 9, 2017:
    Fixed in 6524d401485a8dbd9881e6d69b3f542bb6598d6b
  24. in src/wallet/rpcdump.cpp: in dc402a4444 outdated 
    983@@ -970,13 +984,16 @@ UniValue importmulti(const JSONRPCRequest& mainRequest)
984             "  [     (array of json objects)\n"
985             "    {\n"
986             "      \"scriptPubKey\": \"<script>\" | { \"address\":\"<address>\" }, (string / json, required) Type of scriptPubKey (string for script, json for address)\n"
987+            "      \"timestamp\": <timestamp> | \"now\"                      , (integer / string, required) Integer timestamp, or the string \"now\" to substitute the current synced\n"
    kallewoof commented at 8:23 am on February 9, 2017:
    Nit / repeat: <timestamp> -> timestamp (see above comment)
    ryanofsky commented at 10:37 am on February 9, 2017:
    Fixed in 6524d401485a8dbd9881e6d69b3f542bb6598d6b
  25. in src/wallet/rpcdump.cpp: in dc402a4444 outdated 
    1031@@ -1015,6 +1032,12 @@ UniValue importmulti(const JSONRPCRequest& mainRequest)
1032     LOCK2(cs_main, pwalletMain->cs_wallet);
1033     EnsureWalletIsUnlocked();
1034 
1035+    // Verify all timestamps are present before importing any keys.
1036+    const int64_t now = chainActive.Tip() ? chainActive.Tip()->GetMedianTimePast() : 0;
    kallewoof commented at 8:25 am on February 9, 2017:
    I don’t know if 0 is a good default for now, but it doesn’t seem harmful either. I wonder if the function shouldn’t simply throw a runtime_exception if !chainActive.Tip() saying to sync up first..
    ryanofsky commented at 10:39 am on February 9, 2017:
    Would be ok with that change, but prefer the current behavior of not aborting just to give a notification that the chain is not synced when the outcome is safe either way.
    kallewoof commented at 11:52 am on February 9, 2017:
    OK, yeah I think a notification would be useful.
  26. in src/wallet/rpcdump.cpp: in dc402a4444 outdated 
    983@@ -970,13 +984,16 @@ UniValue importmulti(const JSONRPCRequest& mainRequest)
984             "  [     (array of json objects)\n"
985             "    {\n"
986             "      \"scriptPubKey\": \"<script>\" | { \"address\":\"<address>\" }, (string / json, required) Type of scriptPubKey (string for script, json for address)\n"
987+            "      \"timestamp\": <timestamp> | \"now\"                      , (integer / string, required) Integer timestamp, or the string \"now\" to substitute the current synced\n"
988+            "                                                              blockchain time. The timestamp of the oldest key will determine how far back blockchain rescans\n"
989+            "                                                              for missing wallet transactions need to begin. \"now\" can be specified to bypass scanning, for\n"
990+            "                                                              keys which are known to never have been used, and 0 can be specified to scan the entire blockchain.\n"
    kallewoof commented at 8:38 am on February 9, 2017:
    Language: “The timestamp of the oldest key will determine how far back blockchain rescans need to begin for finding missing wallet transactions.” maybe?
    ryanofsky commented at 10:37 am on February 9, 2017:
    Fixed in 6524d401485a8dbd9881e6d69b3f542bb6598d6b
  27. kallewoof approved
  28. kallewoof commented at 8:41 am on February 9, 2017: member
    utACK, did not review tests
  29. ryanofsky referenced this in commit 38c01b530d on Feb 9, 2017
  30. ryanofsky referenced this in commit 6524d40148 on Feb 9, 2017
  31. ryanofsky force-pushed on Feb 9, 2017
  32. ryanofsky commented at 11:38 am on February 9, 2017: member
    Squashed 6524d401485a8dbd9881e6d69b3f542bb6598d6b -> 715f05d9987b6b9d8c2737d40a7113dd0680cfc4 (multinow.6 -> multinow.7)
  33. jonasschnelli approved
  34. jonasschnelli commented at 10:42 am on February 10, 2017: contributor
    Tested ACK 715f05d9987b6b9d8c2737d40a7113dd0680cfc4
  35. 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
  36. 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
  37. Use MTP for importmulti "now" timestamps 266a8114cb
  38. ryanofsky force-pushed on Feb 10, 2017
  39. ryanofsky commented at 8:48 pm on February 10, 2017: member

    Rebased 715f05d9987b6b9d8c2737d40a7113dd0680cfc4 -> 266a8114cbe2a87a6c84d7690a7716a18d782c56 (multinow.7 -> multinow.8) following the #9227 merge.

    This PR didn’t conflict with #9227, but I’m about to rebase #9108 on top of this, and #9108 did conflict with #9227.

  40. morcos commented at 7:47 pm on February 13, 2017: member
    utACK 266a811
  41. laanwj merged this on Feb 14, 2017
  42. laanwj closed this on Feb 14, 2017
  43. laanwj referenced this in commit edc9e63c57 on Feb 14, 2017
  44. laanwj referenced this in commit d8e8b06bd0 on Feb 15, 2017
  45. codablock referenced this in commit 8805c4ea62 on Jan 31, 2018
  46. codablock referenced this in commit e99b716e86 on Jan 31, 2018
  47. codablock referenced this in commit c0cc8630d0 on Jan 31, 2018
  48. codablock referenced this in commit 1186bd3519 on Jan 31, 2018
  49. codablock referenced this in commit 49d1322cdc on Jan 31, 2018
  50. codablock referenced this in commit de17fd766b on Jan 31, 2018
  51. codablock referenced this in commit 6f86725d00 on Feb 1, 2018
  52. codablock referenced this in commit 43f697866e on Feb 1, 2018
  53. HashUnlimited referenced this in commit 69f2891f67 on Feb 27, 2018
  54. HashUnlimited referenced this in commit c69f3be35e on Feb 27, 2018
  55. andvgal referenced this in commit 34931ae3aa on Jan 6, 2019
  56. andvgal referenced this in commit 165188b133 on Jan 6, 2019
  57. CryptoCentric referenced this in commit 37707c6285 on Feb 28, 2019
  58. CryptoCentric referenced this in commit b0a82fc804 on Feb 28, 2019
  59. CryptoCentric referenced this in commit be53fbb93c on Mar 2, 2019
  60. CryptoCentric referenced this in commit 45ba8c2f70 on Mar 2, 2019
  61. DrahtBot locked this on Sep 8, 2021


ryanofsky TheBlueMatt achow101 jonasschnelli gmaxwell MarcoFalke laanwj kallewoof morcos

Labels
RPC/REST/ZMQ

Milestone
0.14.0

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-05 00:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me