kaspanet / kaspad

Kaspad was the reference full node Kaspa implementation written in Go (golang), now rewritten in Rust: https://github.com/kaspanet/rusty-kaspa
ISC License
450 stars 232 forks source link

Transaction Tracking (Wallet) #1193

Open aspect opened 3 years ago

aspect commented 3 years ago

This issue tracks wallet subsystem feature integration

Please review and merge the implementation of getTransactionsByAddress(startingBlockHash:string, addresses:string[]) returning:

TransactionResponse{
  string lasBlockScanned;
  TransactionData[] transactions;
  RPCError error;
}

This method allows the caller to scan local Kaspad storage for the presence of transactions related to the set of supplied addresses.

startingBlockHash is used as a marker to resume the scan. If startingBlockHash is not supplied, the scan should occur from the first block available in the non-pruned local dataset. Otherwise, a client should preserve lastBlockScanned each time the function is used and supply it as startingBlockHash next time getAddressTransactions() is used.

This has not been tested in the wallet module pending resolution of https://github.com/kaspanet/kaspad/issues/1158

stasatdaglabs commented 3 years ago

@aspect cross-posting from Discord:

If I'm to understand correctly, this is required only for transaction history/transaction sync between wallets That is to say, a user working with a singular wallet (e.g. on their PC) will never require this functionality (the transactions will always be created within this wallet, so save them to local storage after their creation)

On the flipside, naively rescanning the DAG as was implemented here: https://github.com/kaspanet/kaspad/pull/1166/files#diff-2b9e050b3c7f6382842b570a51feced04687f3b05b83f44c540669c8d130f6f5R15 would be prohibitively expensive A user who shuts down her wallet for 24 hours would ask of her node to parse the scriptPubKey of all the transactions in 86400 blocks A more correct approach (as an example) would be to keep transactions in a separate, optional index in kaspad

Due to the above, it's best to handle this uncommon, not-trivially implementable case once initial wallet functionality has been completed.

aspect commented 3 years ago

@stasatdaglabs Just to quickly follow up on the discord discussion. I did not check the implementation before this and I agree the way this is implemented is heavy.

This is meant to address various edge cases and multi-wallet "interference" edge cases. The only reason I have suggested this is under the assumption that 1) performance impact can be avoided by not starting Kaspad before the operation is complete :) this is a good solution since if Kaspad is not connected to the network, there is no performance to impact :) 2) jokes aside - the idea is that this scan would occur 99.99% of the time during run-time not requiring any rescans, and if rescan is required, the idea is to apply it to the dataset that was received only in the last few seconds (hence the restart anchor in the form of lastBlockScanned).

One of the issues is that since the wallet is an external observer, there will be potential for very rare edge cases where transactions will go missing. While it is acceptable that transactions can go missing due to pruning, this shouldn't really be the case in other circumstances.

Here are a few edge cases to review:

These are really rare conditions, but they should be well understood nevertheless, especially the reuse of HD keys situation as it is quite common for user to re-use HD keys (I used to run daemons myself this way - one at the office, one at home... so if I was to run Kaspad this way, if the office wallet is not running while home wallet is issuing transactions, my office wallet, when powered up, will not see them).

In other words, if a set of transactions result in pruned UTXOs and the wallet is not running at this time, these transactions will be missing - the user will only see the wallet balance update (based on the existing UTXO set).

For the time being, in the era of wallets that are "required to be running to receive transactions" this is not really essential and can most certainly be deferred till later.

elichai commented 3 years ago

@aspect Is this still an open issue?