Restore wallet txs after fast sync

kostyantyn commented 5 years ago

Resolves #786

To be able to restore wallet transactions, before syncing the node must be configured appropriately:

node must import the private key
node must generate addresses up to(including) the last one that was used.
(Alternatively), we can pre-generate few K addresses (as the rescan does)

Pre-generating addresses are required as since the snapshot doesn't contain the full blockchain history, node can't recover all its addresses.

After the sync, we have two limitations:

since the snapshot doesn't contain inputs, txs that end up in the wallet, won't have inputs too. It shouldn't affect the wallet but will require us to tune the GUI and properly visualize it. Alternatively, we can request full transaction from the peers on demand.
CMerkleTx has a field nIndex which contains the position of the transaction in the block. We also don't have this field and it's set to 0.

Created an issue to tackle the above limitations https://github.com/dtr-org/unit-e/issues/896

Signed-off-by: Kostiantyn Stepaniuk kostia@thirdhash.com

scravy commented 5 years ago

This is spaghetti.

snapshot/messages.cpp is part of libunite_server
primitives/transaction.cpp is part of libunite_consensus
Introduces circular dependencies all over the place.

That this thing even links is pure chance.

Circular dependencies such as this make the build fragile.

kostyantyn commented 5 years ago

@scravy we can link it because we have a forward declaration in the primitives/transaction.h and include snapshot/messages.h in primitives/transaction.cpp.

We have for example esperanza/finalization logic in libunite_server and they also use code from libunite_server. How would you expect to split the logic?

scravy commented 5 years ago

I ran bitcoins circular-dependencies.py on this branch (README):

$ ../contrib/devtools/circular-dependencies.py {*,*/*,*/*/*}.{h,cpp}
Circular dependency: blockchain/blockchain_genesis -> blockchain/blockchain_parameters -> blockchain/blockchain_genesis
Circular dependency: chain -> pow -> chain
Circular dependency: chainparamsbase -> util -> chainparamsbase
Circular dependency: coins -> snapshot/iterator -> coins
Circular dependency: coins -> snapshot/messages -> coins
Circular dependency: consensus/merkle -> primitives/block -> consensus/merkle
Circular dependency: esperanza/checks -> esperanza/finalizationstate -> esperanza/checks
Circular dependency: esperanza/checks -> txmempool -> esperanza/checks
Circular dependency: esperanza/checks -> validation -> esperanza/checks
Circular dependency: esperanza/finalizationstate -> validation -> esperanza/finalizationstate
Circular dependency: esperanza/walletextension -> wallet/wallet -> esperanza/walletextension
Circular dependency: finalization/state_processor -> finalization/state_repository -> finalization/state_processor
Circular dependency: httpserver -> init -> httpserver
Circular dependency: init -> net_processing -> init
Circular dependency: init -> rpc/server -> init
Circular dependency: init -> txdb -> init
Circular dependency: init -> validation -> init
Circular dependency: init -> validationinterface -> init
Circular dependency: injector -> validation -> injector
Circular dependency: keystore -> script/ismine -> keystore
Circular dependency: p2p/finalizer_commits_handler -> p2p/finalizer_commits_handler_impl -> p2p/finalizer_commits_handler
Circular dependency: policy/fees -> txmempool -> policy/fees
Circular dependency: policy/policy -> validation -> policy/policy
Circular dependency: primitives/transaction -> snapshot/messages -> primitives/transaction
Circular dependency: random -> util -> random
Circular dependency: snapshot/snapshot_index -> txdb -> snapshot/snapshot_index
Circular dependency: staking/validation_error -> staking/validation_result -> staking/validation_error
Circular dependency: sync -> util -> sync
Circular dependency: txmempool -> validation -> txmempool
Circular dependency: usbdevice/debugdevice -> usbdevice/usbdevice -> usbdevice/debugdevice
Circular dependency: usbdevice/ledgerdevice -> usbdevice/usbdevice -> usbdevice/ledgerdevice
Circular dependency: usbdevice/usbdevice -> wallet/wallet -> usbdevice/usbdevice
Circular dependency: validation -> validationinterface -> validation
Circular dependency: wallet/coincontrol -> wallet/wallet -> wallet/coincontrol
Circular dependency: wallet/fees -> wallet/wallet -> wallet/fees
Circular dependency: wallet/init -> wallet/wallet -> wallet/init
Circular dependency: wallet/rpcwallet -> wallet/wallet -> wallet/rpcwallet
Circular dependency: wallet/wallet -> wallet/walletdb -> wallet/wallet
Circular dependency: blockchain/blockchain_behavior -> blockchain/blockchain_custom_parameters -> blockchain/blockchain_genesis -> blockchain/blockchain_behavior
Circular dependency: blockchain/blockchain_rpc -> rpc/util -> injector -> blockchain/blockchain_rpc
Circular dependency: consensus/tx_verify -> esperanza/finalizationstate -> validation -> consensus/tx_verify
Circular dependency: esperanza/vote -> keystore -> script/script -> esperanza/vote
Circular dependency: httprpc -> httpserver -> init -> httprpc
Circular dependency: init -> injector -> settings -> init
Circular dependency: init -> wallet/init -> wallet/rpcwallet -> init
Circular dependency: injector -> staking/transactionpicker -> miner -> injector
Circular dependency: policy/fees -> policy/policy -> validation -> policy/fees
Circular dependency: policy/rbf -> txmempool -> validation -> policy/rbf
Circular dependency: txmempool -> validation -> validationinterface -> txmempool
Circular dependency: base58 -> blockchain/blockchain_behavior -> blockchain/blockchain_parameters -> settings -> base58
Circular dependency: chain -> primitives/block -> primitives/transaction -> snapshot/messages -> chain
Circular dependency: finalization/state_processor -> snapshot/creator -> snapshot/p2p_processing -> injector -> finalization/state_processor
Circular dependency: finalization/vote_recorder -> injector -> p2p/finalizer_commits_handler -> p2p/finalizer_commits_handler_impl -> finalization/vote_recorder
Circular dependency: httpserver -> init -> wallet/init -> wallet/rpcwallet -> httpserver
Circular dependency: injector -> p2p/finalizer_commits_handler -> p2p/finalizer_commits_handler_impl -> net_processing -> injector
Circular dependency: injector -> p2p/finalizer_commits_handler -> p2p/finalizer_commits_handler_impl -> snapshot/p2p_processing -> injector
Circular dependency: injector -> staking/transactionpicker -> miner -> txmempool -> injector
Circular dependency: consensus/tx_verify -> finalization/vote_recorder -> injector -> staking/transactionpicker -> miner -> consensus/tx_verify
Circular dependency: consensus/tx_verify -> finalization/vote_recorder -> injector -> p2p/finalizer_commits_handler -> p2p/finalizer_commits_handler_impl -> consensus/tx_verify
Circular dependency: esperanza/checks -> finalization/vote_recorder -> injector -> staking/transactionpicker -> miner -> esperanza/checks
Circular dependency: esperanza/checks -> finalization/vote_recorder -> injector -> p2p/finalizer_commits_handler -> p2p/finalizer_commits_handler_impl -> esperanza/checks
Circular dependency: esperanza/checks -> finalization/vote_recorder -> injector -> proposer/multiwallet -> wallet/wallet -> esperanza/checks
Circular dependency: consensus/tx_verify -> finalization/vote_recorder -> injector -> staking/transactionpicker -> miner -> txmempool -> consensus/tx_verify
Circular dependency: consensus/tx_verify -> finalization/vote_recorder -> injector -> proposer/multiwallet -> wallet/wallet -> wallet/walletdb -> consensus/tx_verify

scravy commented 5 years ago

Note that including and forward declarations are completely different from the linking process.

Luckily we do have a workshop on linking: https://github.com/dtr-org/workshops/tree/master/20180908-linker

kostyantyn commented 5 years ago

@scravy thanks! very nice! let's keep this discussion under https://github.com/dtr-org/unit-e/pull/885#discussion_r272251401 thread as it touches one particular part of the PR (not the whole one).

scravy commented 5 years ago

See https://github.com/dtr-org/unit-e/issues/896#issuecomment-480072231

scravy commented 5 years ago

I am having trouble with the design proposed in this pull request, to be frank I consider it a bit of a hack.

Circular Dependencies:

we have been creating circular dependencies rather carelessly and I would accept the point "why did we merge things like that in the past but now request changes on this one in particular?". First, to raise this issue, I opened one about it: https://github.com/dtr-org/unit-e/issues/898 . Second, I only realized these circular dependencies because of design choices in this pull request which I am not okay with.

Design:

I for one am not okay with the proposed changes, as they invalidate certain assumptions which (I for one) and others (upstream) have. I would be extremely cautious when changing the very basics like CTransaction, especially when it comes to their meaning/invariants. A CTransaction is a complete, immutable, transaction. This changes it to be a potentially incomplete transaction. I for one do not know what problems this may bring - either now or when syncing things from upstream. I am not willing to open that door.

I agree that it sucks to just be nagging without showing an alternative. Quite frankly, I was under the impression that we tagged this issue for 1.0 so I am a bit unwilling to put a lot of time into it right now, as I would need to think about this too. I did spend a bit of time now and right now I am thinking the following:

CMerkleTx has a transaction pointer. I for one would welcome it if we placed the changes there which would make CMerkleTx be okay with a nullptr as tx (= no transaction) and an additional IncompleteTransaction (or maybe this is not even needed but a UTXO from snapshot directly). CMerkleTx::GetHash() would then need to be changed to take into account Am I having an incomplete transaction or a proper transaction.

This would also enable us to understand that a CWalletTx is an incomplete transaction (something which would not be obvious with the currently proposed design).

As for the circular dependencies:

ScanForWalletTransactions for example is a method in the wallet. So are most things concerning the wallet. Maybe we could have a ImportUTXO function in a similar spirit. We are trying to keep changes to CWallet to a minimum, so maybe it would be good to put it into the WalletExtension, but I think it would be okay to attach it to CWallet directly as fast sync is pretty basic. This should get rid of the dependency introduced from server to wallet (the dependency should always only be wallet to server).

dtr-org / unit-e

Restore wallet txs after fast sync #885