make create_transaction() read only. attempt 3. #162
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously a write-lock was held across `create_transaction()` which includes a lengthy (potentially minutes) call to `prove`. This would be unacceptably bad for concurrency. This commit makes create_transaction() take &self instead of &mut self. Only a single mutation was being performed inside add_change() and this is now moved into `send()` RPC after `create_transaction()` completes. The write-lock acquisition is like-wise moved, so it should only be held very briefly while adding a record to wallet's `expected_utxos`.
Refactors create_transaction() to enable the caller to specify which output utxo shold use onchain global-announcement vs offchain expected-utxo. A method is provided to automatically determine this based on if it appears the utxo is destined for our own wallet or not. Also, create_transaction() now returns (unblinded) TransactionData which includes a list of all ExpectedUtxo. It is caller's responsibility to inform the wallet, which mutates state. It can be said that we now: * support off-chain notifications for the general case of all utxo going to our wallet instead of only the sub-case of change address. * support specifying if each utxo will use on or off-chain notification except for the change address, which is always off-chain. Changes: wallet_state: * add get_known_spending_keys(). placeholder to centralize logic * add is_wallet_utxo() models/state/mod.rs: * add struct TransactionData * move UtxoReceiverData into utxo_receiver.rs * use ExpectedUtxo in place of ChangeUtxoData * create_transaction() returns (Transaction, TransactionData) * add doc-comment, example for create_transaction() * receiver_data param to create_transaction can now specify if each utxo uses Onchain or Offchain notification * add add_expected_utxos_to_wallet() * remove redundant create_transaction_with_timestamp() rpc_server.rs: * add send_to_many() endpoint, which is called by send() * use UtxoReceiverData::auto() to determine if recipient(s) should be notified Onchain or Offchain * remove pause/resume of miner. doesn't seem necessary. * simplify wallet notificaiton
Addresses Neptune-Crypto#122 Enables caller of create_transaction() to choose whether the Change UTXO will use OnChain or OffChain notification. Note that caller could already do this since the last commit by manually creating the Change UTXO so that sum(inputs) == sum(outputs). Therefore, this change only makes it a bit easier, and is more consistent in that the caller can easily control the notification behavior of all outputs. The default behavior is still OffChain notification and doc-comments and example usage recommend this for normal usage. Also, this fine level of control is not available in the RPC send and send_to_many endpoints. Only in the rust API. Changes: * add change_notify_method arg to create_transaction() * update tests to use extra arg. * rename UtxoReceiverData --> UtxoReceiver * rename TransactionData --> TransactionDetails * fix: backwards logic in UtxoRecipient::auto() * improve doc-comment for create_transaction() * add ChangeNotifyMethod enum * add some missing doc-comments
adds: * test_utxoreceiver_auto_off_chain() * test_utxoreceiver_auto_on_chain
UtxoReceiver::receiver_preimage -> receiver_privacy_digest
Changes: * remove redundant receive_privacy_digest param from UtxoReceiver::auto() * RpcServer::send_to_many(): flush DBs before notifying main * RpcServer::send_to_many(): log error if noiifying main fails * test_rpc_server(): spawn a dummy main to listen to RpcServer notifications * add test: send_to_many_test()
Usage: neptune-cli send-to-many <OUTPUTS>... <FEE> Arguments: <OUTPUTS>... format: address:amount address:amount ... <FEE> ---- Also simplifies the success output of Send comand to just "Send completed" instead of verbosely including the many-line address and amount.
Changes enums UtxoNotifyMethod and ChangeNotifyMethod in anticipation
of the day that we implement Symmetric Key notifications for UTXO
that can be claimed by our own wallet, such as Change outputs.
pub enum UtxoNotifyMethod {
OnChain(PublicAnnouncement),
OffChain,
}
becomes:
pub enum UtxoNotifyMethod {
OnChainPubKey(PublicAnnouncement),
OnChainSymmetricKey,
OffChain,
}
This was referenced Jul 9, 2024
aszepieniec
approved these changes
Jul 11, 2024
|
I went ahead and merged this. @Sword-Smith if you have any concerns, pls let me know. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Addresses #122. Obsoletes #159.
This is my 3rd attempt to refactor create_transaction(). Perhaps 3rd try is the charm.
For original background motivation and discussion, see previous attempts: #136 and #159.
Some goals of this attempt are:
Changes since #159:
highlights / key changes:
rpc_server:
global_state:
next_unused_generation_spending_key() takes &mut self.
wallet:
matching SpendingKeyType with each input. (fixes prev TODO)
transaction:
other:
Explanations of things that may not be obvious:
preparing for derived keys. Presently some functions call
WalletSecret::nth_generation_spending_key(&self, counter: u16), passing 0. I've now added::next_unused_generation_spending_key(&mut self), which presently always uses index 0 but in the future will increment a stateful counter. The difficulty is that it must take &mut self, so it cannot be called from within GlobalState methods that take &self, such as create_transaction(). So create_transaction() now requires the caller to provide the next unused spending key.let me know if there's something else. I put a lot of code comments, but let me know if anything is unclear.