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

ryanofsky · 2016-11-11T21:10:03Z

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.

jonasschnelli · 2016-11-12T08:31:34Z

src/primitives/transaction.cpp

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?

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.

jonasschnelli · 2016-11-12T08:35:19Z

src/wallet/wallet.cpp

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.

Sounds good, will change.

jonasschnelli · 2016-11-12T08:47:29Z

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

sipa · 2016-11-12T10:32:14Z

~~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?

luke-jr · 2016-11-12T11:08:32Z

src/wallet/rpcdump.cpp

Shouldn't there be some way to scan for UTXOs on non-pruned nodes?

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?

luke-jr · 2016-11-12T11:10:28Z

src/wallet/wallet.cpp

These errors leave much to be desired...

ryanofsky · 2016-11-12T13:05:15Z

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

jonasschnelli · 2016-11-12T13:07:18Z

@ryanofsky: I think the GUI stuff can be done in a separate PR after this has been merged.

ryanofsky · 2016-11-12T13:12:11Z

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.

sipa · 2016-11-12T18:03:03Z

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.

luke-jr · 2016-11-13T20:28:38Z

src/wallet/wallet.cpp

Won't this miss unconfirmed TXOs?

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.

ryanofsky · 2016-11-29T21:48:07Z

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:

Maybe add test to confirm IsFinal works after an incomplete transaction output is spent. (jonasschnelli comment)
Maybe add visual attribution for incomplete transactions in gui (jonasschnelli comment)
Maybe add incomplete status to TxToJSON (jonasschnelli comment)
Maybe allow UTXO rescanning on non -prune nodes (luke-jr comment)
Maybe add ability for wallet to scan mempool for when importing keys (luke-jr comment)

ryanofsky · 2016-11-30T00:11:47Z

Added test code to import-rescan.py to make sure spends of incomplete transactions are picked up by the wallet and finalized.

ryanofsky · 2016-12-05T17:05:56Z

Updated to fix conflicts after #8580 merge.

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 bitcoin#9152, which adds a sweepprivkeys RPC, and bitcoin#9137, which improves handling of imported keys for nodes with pruning enabled.)

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 bitcoin#8497.

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 bitcoin#9152, which adds a sweepprivkeys RPC, and bitcoin#9137, which improves handling of imported keys for nodes with pruning enabled.)

gmaxwell · 2016-12-12T23:12:00Z

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")

ryanofsky · 2016-12-13T18:45:59Z

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.

ryanofsky · 2016-12-13T18:51:12Z

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.

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 bitcoin#9152, which adds a sweepprivkeys RPC, and bitcoin#9137, which improves handling of imported keys for nodes with pruning enabled.)

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 bitcoin#9152, which adds a sweepprivkeys RPC, and bitcoin#9137, which improves handling of imported keys for nodes with pruning enabled.)

ryanofsky · 2017-06-12T19:02:24Z

Haven't worked on this in a while. Will close and reopen if I make more more progress.

sangaman · 2017-08-23T05:40:37Z

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.

ryanofsky · 2017-08-24T21:41:00Z

@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.

fanquake added the Wallet label Nov 12, 2016

jonasschnelli reviewed Nov 12, 2016

View reviewed changes

luke-jr reviewed Nov 12, 2016

View reviewed changes

src/wallet/wallet.cpp Outdated

Copy link

Member

luke-jr Nov 12, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These errors leave much to be desired...

luke-jr reviewed Nov 13, 2016

View reviewed changes

ryanofsky force-pushed the import-pruned branch 2 times, most recently from 37a31b2 to a5b980f Compare November 21, 2016 21:13

ryanofsky force-pushed the import-pruned branch 3 times, most recently from daa3954 to 4c84290 Compare November 29, 2016 21:20

ryanofsky changed the title ~~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 Nov 29, 2016

ryanofsky force-pushed the import-pruned branch from 4c84290 to 454c314 Compare November 30, 2016 00:07

ryanofsky force-pushed the import-pruned branch from 454c314 to 4139b8f Compare December 5, 2016 17:01

ryanofsky added 2 commits December 9, 2016 09:50

ryanofsky force-pushed the import-pruned branch from 4139b8f to b26a64c Compare December 9, 2016 15:08

ryanofsky mentioned this pull request Dec 9, 2016

Make CCoinsViewCache::Cursor() return latest data #9306

Closed

ryanofsky force-pushed the import-pruned branch 2 times, most recently from efc18dd to 9a3a749 Compare December 12, 2016 15:45

ryanofsky mentioned this pull request Dec 12, 2016

[qa] Add test for rescan feature of wallet key import RPCs #9331

Merged

ryanofsky force-pushed the import-pruned branch from 9a3a749 to 63d11c8 Compare December 13, 2016 12:29

ryanofsky force-pushed the import-pruned branch 2 times, most recently from 408e3e7 to a63ec0e Compare December 15, 2016 20:06

ryanofsky changed the title ~~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) Dec 15, 2016

ryanofsky changed the title ~~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) Feb 6, 2017

ryanofsky mentioned this pull request Feb 15, 2017

Use 2 hour grace period for key timestamps in importmulti rescans #9761

Merged

ryanofsky closed this Jun 12, 2017

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

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

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

Uh oh!

Conversation

ryanofsky commented Nov 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Nov 12, 2016

Uh oh!

sipa commented Nov 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Nov 12, 2016

Uh oh!

jonasschnelli commented Nov 12, 2016

Uh oh!

ryanofsky commented Nov 12, 2016

Uh oh!

sipa commented Nov 12, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Nov 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Nov 30, 2016

Uh oh!

ryanofsky commented Dec 5, 2016

Uh oh!

gmaxwell commented Dec 12, 2016

Uh oh!

ryanofsky commented Dec 13, 2016

Uh oh!

ryanofsky commented Dec 13, 2016

Uh oh!

ryanofsky commented Jun 12, 2017

Uh oh!

sangaman commented Aug 23, 2017

Uh oh!

ryanofsky commented Aug 24, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ryanofsky commented Nov 11, 2016 •

edited

Loading

sipa commented Nov 12, 2016 •

edited

Loading

ryanofsky commented Nov 29, 2016 •

edited

Loading