WIP: Allow wallet key import RPCs to track TxOut amounts on -prune nodes (on top of #9306) #9137

pull ryanofsky wants to merge 2 commits into bitcoin:master from ryanofsky:import-pruned changing 10 files +234 −38
  1. ryanofsky commented at 9:10 pm on November 11, 2016: member

    Allow importprivkey, importaddress, importpubkey, and importmulti RPCs to be called with rescan=True option on nodes with pruned blockchains (started with -prune option). Instead of returning errors, the RPCs will now look for transactions spending to the key in the UTXO database, and display them in the wallet.

    Because the UTXO database doesn’t store full transaction information (it discards TxIns) this change introduces a new “fIncomplete” CWalletTx member to distinguish these from normal wallet transactions.

    This change makes the -prune mode usable with wallets that need to import keys after the initial sync.

    Fixes #8497.

  2. fanquake added the label Wallet on Nov 12, 2016
  3. in src/primitives/transaction.cpp: in 3fc4535171 outdated 
    61@@ -62,9 +62,14 @@ uint256 CMutableTransaction::GetHash() const
62     return SerializeHash(*this, SER_GETHASH, SERIALIZE_TRANSACTION_NO_WITNESS);
63 }
64 
65-void CTransaction::UpdateHash() const
66+void CTransaction::SetHash(const uint256 &tx_hash)
67 {
68-    *const_cast<uint256*>(&hash) = SerializeHash(*this, SER_GETHASH, SERIALIZE_TRANSACTION_NO_WITNESS);
69+    *const_cast<uint256*>(&hash) = tx_hash;
    jonasschnelli commented at 8:31 am on November 12, 2016:
    Hmm… this would result in not longer coupling the hash with the data-structure. This can result in difficult states for future changes. Would a new primitive (maybe CIncompleteTransaction or similar) make more sense?
    ryanofsky commented at 12:39 pm on November 12, 2016:

    I don’t know if @sipa’s suggestion of making a new CTransaction constructor that accepts a hash would sufficiently address your concern. (It would not eliminate the risk that some code could make a seemingly harmless UpdateHash() call on the wallet transaction and overwrite the correct hash).

    Personally, the way I would go about this would be to change CWalletTx not to inherit from CMerkleTx, and instead just contain either a variant<CMerkleTx, CCoins> or an optional<CMerkleTx> member. I can try this in a separate PR (since would create a lot of unrelated code churn in the wallet) if you are amenable to the idea.

  4. in src/wallet/wallet.cpp: in 3fc4535171 outdated 
    1506@@ -1506,6 +1507,29 @@ int CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, bool fUpdate)
1507     return ret;
1508 }
1509 
1510+void CWallet::ScanForWalletUTXOs()
1511+{
1512+    for (std::unique_ptr<CCoinsViewCursor> pcursor(pcoinsTip->Cursor());
    jonasschnelli commented at 8:35 am on November 12, 2016:
    Would there be a better way to keep the wallet separated from the UTXO set? During wallet creation (init.cpp) it could pass-in a lambda that would take care about finding the corresponding CCoins.
    ryanofsky commented at 12:40 pm on November 12, 2016:
    Sounds good, will change.
    luke-jr commented at 8:28 pm on November 13, 2016:
    Won’t this miss unconfirmed TXOs?
    ryanofsky commented at 10:14 pm on November 14, 2016:

    This doesn’t deal with unconfirmed TXOs, but neither does ScanForWalletTransactions, which this is just intended to be a lightweight fallback for.

    I don’t know the wallet code well enough to know how it does deal with unconfirmed TXOs, but I expect that if there are issues tracking them, they would be orthogonal to this change.

  5. jonasschnelli commented at 8:47 am on November 12, 2016: contributor

    Conceptual questions:

    • I guess the new importmulti call should also be covered in this change?
    • Idea: We could deprecate the other import calls in favor of importmulti (which has more features and makes generally more sense).
    • Does spending those incomplete transactions work well? IMO some checks (isFinal()) do read the nSequence value of the (now missing) inputs?

    Things that would need to be done: -> Visual attribution in the GUI -> the fIncomplete status should probably be added to the RPC TxToJSON (or similar) function -> Add tests

  6. sipa commented at 10:32 am on November 12, 2016: member

    NACK when done this way. If you need an incomplete transaction, use CMutableTransaction instead of hacking that functionality around CTransaction.

    Thinking more about the above, this is not useful advice. I said it in response to seeing the suggestion of a CIncompleteTransaction, assuming we were calling SetHash after it became complete.

    What we want here is a hash that does travel with the transaction, and is complete. I don’t think it needs to break CTransaction’s immutability, though. What about a constructor to CTransaction that takes a CMutableTransaction and a hash?

  7. in src/wallet/rpcdump.cpp: in 3fc4535171 outdated 
    394@@ -398,7 +395,10 @@ UniValue importpubkey(const JSONRPCRequest& request)
395 
396     if (fRescan)
397     {
398-        pwalletMain->ScanForWalletTransactions(chainActive.Genesis(), true);
399+        if (fPruneMode)
400+          pwalletMain->ScanForWalletUTXOs();
401+        else
402+          pwalletMain->ScanForWalletTransactions(chainActive.Genesis(), true);
    luke-jr commented at 11:08 am on November 12, 2016:
    Shouldn’t there be some way to scan for UTXOs on non-pruned nodes?
    ryanofsky commented at 12:52 pm on November 12, 2016:
    I could definitely add it as another option to the RPC. I guess the benefit to the RPC caller would be that if they’re importing a key which they already know is unspent, calling ScanForWalletUTXOs would be faster than calling ScanForWalletTransactions?
  8. in src/wallet/wallet.cpp: in 3fc4535171 outdated 
    1515+        uint256 hash;
1516+        CCoins coins;
1517+        if (!pcursor->GetKey(hash))
1518+            LogPrintf("GetKey failed");
1519+        else if (!pcursor->GetValue(coins))
1520+            LogPrintf("GetValue failed");
    luke-jr commented at 11:10 am on November 12, 2016:
    These errors leave much to be desired…
  9. ryanofsky commented at 1:05 pm on November 12, 2016: member
    Thanks @jonasschnelli, I am planning to get to the various todos you mentioned. The one I might like to know more about is “Visual attribution in the GUI.” Do you have any thoughts on what this should look like? And what would be the reasons for making the distinction? E.g. are there particular wallet features or RPC calls that won’t work with incomplete transaction data? (Asking because I actually haven’t used the wallet UI very much myself yet.)
  10. jonasschnelli commented at 1:07 pm on November 12, 2016: contributor
    @ryanofsky: I think the GUI stuff can be done in a separate PR after this has been merged.
  11. ryanofsky commented at 1:12 pm on November 12, 2016: member
    Thanks @jonasschnelli, that makes sense. I still would appreciate knowing any thoughts you may have on the GUI implementation either here or in the original issue #8497.
  12. sipa commented at 6:03 pm on November 12, 2016: member
    I’m working on a series of changes to the serialization code, the result of which will be that CTransaction is fully immutable, so UpdateHash will go away.
  13. ryanofsky force-pushed on Nov 14, 2016
  14. ryanofsky force-pushed on Nov 21, 2016
  15. ryanofsky force-pushed on Nov 29, 2016
  16. ryanofsky force-pushed on Nov 29, 2016
  17. ryanofsky force-pushed on Nov 29, 2016
  18. ryanofsky renamed this:
    WIP: Allow wallet key import RPCs to track TxOut amounts on -prune nodes
    Allow wallet key import RPCs to track TxOut amounts on -prune nodes
    on Nov 29, 2016
  19. ryanofsky commented at 9:48 pm on November 29, 2016: member

    This is better tested now and ready to be reviewed and merged. There are a few extra things that could be done based on comments above, but they don’t necessarily have to be part of this PR:

  20. ryanofsky force-pushed on Nov 30, 2016
  21. ryanofsky commented at 0:11 am on November 30, 2016: member
    Added test code to import-rescan.py to make sure spends of incomplete transactions are picked up by the wallet and finalized.
  22. ryanofsky force-pushed on Dec 5, 2016
  23. ryanofsky commented at 5:05 pm on December 5, 2016: member
    Updated to fix conflicts after #8580 merge.
  24. Make CCoinsViewCache::Cursor() return latest data 
    Change CCoinsViewCache::Cursor() method to return a cursor yielding the latest
CCoins entries, instead of just previous entries prior to the last cache flush.

The CCoinsViewCache::Cursor method is not currently used. This change just
enables new features that rely on scanning the UXTO set to work correctly (for
example https://github.com/bitcoin/bitcoin/pull/9152, which adds a
sweepprivkeys RPC, and https://github.com/bitcoin/bitcoin/pull/9137, which
improves handling of imported keys for nodes with pruning enabled.)
    59a77a6ac5
  25. Allow wallet key import RPCs to track TxOut amounts on -prune nodes 
    Allow importprivkey, importaddress, importpubkey, and importmulti RPCs to be
called with rescan=True option on nodes with pruned blockchains (started with
-prune option). Instead of returning errors, the RPCs will now look for
transactions spending to the key in the UTXO database, and display them in the
wallet.

Because the UTXO database doesn't store full transaction information (it
discards TxIns) this change introduces a new "fIncomplete" CWalletTx member to
distinguish these from normal wallet transactions.

This change makes the -prune mode usable with wallets that need to import keys
after the initial sync.

Fixes #8497.
    a63ec0e103
  26. ryanofsky referenced this in commit 6494980802 on Dec 9, 2016
  27. ryanofsky force-pushed on Dec 9, 2016
  28. ryanofsky force-pushed on Dec 12, 2016
  29. ryanofsky force-pushed on Dec 12, 2016
  30. gmaxwell commented at 11:12 pm on December 12, 2016: contributor
    rescan=true and importing pruned things are not the same though. Rescan makes sure you get the historical transactions and a utxo scan doesn’t. I think we may want to track a global state for a wallet that indicates if it believes its history is complete or not, and report that. (e.g. in the GUI there could be a bar at the top of the transaction list “some historical transactions may be missing”)
  31. ryanofsky force-pushed on Dec 13, 2016
  32. ryanofsky commented at 6:45 pm on December 13, 2016: member
    Having a gui indication of an incomplete historical transaction list seems like a good idea with this change, but it would probably also be a good idea without this change. (The transaction list will be incomplete now if you import with a key without rescanning, or if you use the removeprunedfunds API.) The goal of this change is to make possible to see and spend balances after importing keys on -prune nodes as described in #8497.
  33. ryanofsky commented at 6:51 pm on December 13, 2016: member
    I do think it would be a good idea not overload to the meaning of rescan=true, though. Maybe rescan could be changed from a bool to a string argument where rescan="history" would scan the blockchain and rescan="utxo" would scan the utxo set. rescan=true could still be kept for backwards compatibility.
  34. ryanofsky force-pushed on Dec 15, 2016
  35. ryanofsky force-pushed on Dec 15, 2016
  36. ryanofsky renamed this:
    Allow wallet key import RPCs to track TxOut amounts on -prune nodes
    Allow wallet key import RPCs to track TxOut amounts on -prune nodes (on top of #9306)
    on Dec 15, 2016
  37. ryanofsky referenced this in commit ef97df2109 on Dec 19, 2016
  38. ryanofsky referenced this in commit 96a1834f53 on Jan 2, 2017
  39. ryanofsky referenced this in commit 2ca75bccf6 on Jan 2, 2017
  40. ryanofsky renamed this:
    Allow wallet key import RPCs to track TxOut amounts on -prune nodes (on top of #9306)
    WIP: Allow wallet key import RPCs to track TxOut amounts on -prune nodes (on top of #9306)
    on Feb 6, 2017
  41. ryanofsky referenced this in commit d58b56d98d on Jun 2, 2017
  42. ryanofsky referenced this in commit 095c2e7e78 on Jun 12, 2017
  43. ryanofsky commented at 7:02 pm on June 12, 2017: member
    Haven’t worked on this in a while. Will close and reopen if I make more more progress.
  44. ryanofsky closed this on Jun 12, 2017
  45. sangaman commented at 5:40 am on August 23, 2017: contributor

    This is a feature I’d love to see added. I’ve been following the progress in this and related issues and PRs. Regarding the most recent comments, I think it would make sense to leave rescan=true as is, and have rescan=utxo only check an address or key against the utxo set and not do a full rescan for historical transactions.

    I’m new to contributing to this project but let me know if I can help with testing or anything for this PR. Or, @ryanofsky, if you don’t feel like you’ll be revisiting this issue in the foreseeable future maybe I can try to pick up where you left off. Thanks.

  46. ryanofsky commented at 9:41 pm on August 24, 2017: member
    @sangaman, it’d be great if you want to pick this up. As mentioned in my last update I haven’t worked on this on a while and don’t have plans to continue at the moment.
  47. DrahtBot locked this on Sep 8, 2021


ryanofsky jonasschnelli luke-jr sipa gmaxwell sangaman

Labels
Wallet

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