refactor create_transaction, avoid holding lock across prove()

[dan-da]

Closes #122. Obsoletes #136

This PR builds on #136 which modifies GlobalState::create_transaction() to take &self instead of &mut self and makes the state update (for expected_utxo) responsibility of the caller. The primary motivation is to avoid holding a write lock across the call to prove() which may take a very long time.

This PR focuses on improving the flexibility and ergonomics of the create_transaction() API.

In particular, it improves on #136 in the following ways:

• handles the general case of notify wallet of any incoming utxos instead of the sub-case of notify wallet of incoming change utxo -- as discussed with @aszepieniec.
• enables the caller to individually specify that each output utxo use either on-chain or off-chain notification, and to easily default to off-chain for utxos that our wallet can claim and on-chain for all other utxos.
• create_transaction() now returns a tuple (Transaction, TransactionDetails) where the latter contains all the tx components, including ExpectedUtxos, which caller must use to inform the wallet.
• adds rpc endpoint send_to_many() which exposes multi-output functionality at the rpc level. The send() endpoint now just wraps send_to_many().
• adds a test case for send_to_many() RPC
• adds a mock MainLoop channel endpoint for send_to_many() to transmit to, which avoids an error case that was previously not logged and now triggered by the test case.
• adds UtxoNotifyMethod and ChangeNotifyMethod enums for selecting OnChainPubKey and OffChain notifications: aka PublicAnnouncement and ExpectedUtxo. It also has a variant OnChainSymetricKey in preparation for when we add symmetric-key notifications as alternative to OffChain.
• Simplifies creation of UtxoReceiver struct, which represents each output passed to create_transaction().

These changes are a bit experimental. I think they are a net overall improvement to the API, but am open to criticisms/suggestions.

I should point out that the ability to select onchain/offchain notification per output utxo is available at the level of the rust API, ie create_transaction(), but is not available in the RPC API, which just uses the defaults. So this functionality is not presently exposed to end-users.

One further change worth mentioning: this eliminates the pause/unpause miner steps inside send/send_to_many rpc calls. As I see it, a new block could arrive at any moment from a peer while we are creating/proving, so there is nothing special about mining in that regard. Maybe there's an argument to be made that we can have CPU contention and the Tx will take longer to prove(), however I don't think that is valid right now while mining is single-threaded. I am open to putting this back if it should prove necessary, but right now I don't see a need for the added complexity.

For purposes of reviewing, I would recommend to start with rpc_server.rs to see how usage has changed, then models/state/mod.rs and models/state/utxo_receiver.rs for most impl changes. Other files are mostly unit test adaptations.

• [x] TODO: I just realized I also should add send_to_many command to neptune-cli. That can happen in a follow-up commit. (done in 592ff16)

ALSO: I noticed that our wallet is always just using derivation index 0. I wrote a couple wallet functions that pretend this is not the case, ie is_wallet_utxo() and get_known_spending_keys(). This seems an area that needs attention, ie the wallet should keep track (state) of the last-used key index and increment each time a new wallet address is requested.

[aszepieniec]

The name ChangeNotifyMethod is a little confusing because:

• Non-change UTXOs can also have recipients that need to be notified. Is there a good reason to limit the struct's scope to exclude them?
• I don't foresee a world in which OnChainPubKey would be used for change. If it's going back to yourself, you don't need public key crypto and symmetric crypto suffices.

Maybe just NotifyMethod suffices? That's more of an invitation to elaborate than a suggestion.

[dan-da]

yeah, I'm not yet really satisfied with UtxoNotifyMethod and ChangeNotifyMethod as they both have the same variants.

The reason they are distinct is that UtxoNotifyMethod is used by the caller to actually provide PublicAnnouncement to create_transaction(). And that is done because create_transaction() deals with Utxo and PublicAnnouncement, not GenerationAddress. That keeps create_transaction() agnostic about the type of address used, which is good, I think. (and preserves prior behavior)

ChangeNotifyMethod is used only for telling create_transaction() how to notify for the change output. In this case, create_transaction() generates the PublicAnnouncement itself, using a SpendingKey. So that kind of breaks the above model of being address-type agnostic... but then that code pre-exists this PR. This is only necessary because create_transaction() creates the change utxo if sum(inputs) > sum(outputs). Perhaps a cleaner/better solution would be to return an error in this case, which forces the caller to create the correct change utxo itself. With that modification, there is nothing special about the change utxo, it is an output created by the caller like any other. I am leaning toward this solution.

The name ChangeNotifyMethod is a little confusing because:

yeah, we can get rid of that enum entirely with above proposal.

• Non-change UTXOs can also have recipients that need to be notified. Is there a good reason to limit the struct's scope to exclude them?

Those utxo are taken care of by the UtxoNotifyMethod enum. With above proposal, it would cover all utxo, including change.

• I don't foresee a world in which OnChainPubKey would be used for change. If it's going back to yourself, you don't need public key crypto and symmetric crypto suffices.

Fair point. Though it also applies to any output UTXO that can be claimed/spent by "my" wallet. So I think that create_transaction() should just throw an error in that case.

Does the above sound reasonable to you?

---

edit: I just reviewed the create_transaction() code again and found a complication. Presently the caller cannot construct a change utxo because they do not know the inputs and thus cannot calc total_spend which is needed to calc change.

Pursuing this approach further, the logical thing is to make the assemble_inputs_for_transaction() method public, so then the caller can obtain the inputs, calc spend and change, and pass it all into create_transaction().

Taking the idea further, the caller can also create the ExpectedUtxo(s) and create_transaction() does not need to return them, which makes that API cleaner. It is more work for the caller, but I think the end result will be cleaner more sensible APIs, given the constraints.

anyway, that's the path I'm exploring now....

[dan-da]

closing. obsoleted by #162.