Do not attempt to construct 0-amount TxOuts#58
Conversation
The test wallet creates UTXOs out of thin air during coin selection. We use these UTXOs as inputs in our protocol transactions. When verifying these transactions, the UTXOs that we created are assumed to be correct: that is, their previous outputs are not verified (which is why we're creating coins out of thin air). Therefore, when creating these UTXOs the value of the nonexistent previous outputs can be anything. We set it to zero to stress the fact that money is being created. Conversely, the explicit asset ID must be part of the list of nonexistent previous outputs in order to produce a valid surjection proof.
When moving away from blockchain-node based tests we unknowingly relaxed the requirements on our transactions, which was a problem. A transaction may correctly spend its inputs and still be invalid by virtue of being inflationary (or deflationary). We have now added checks to ensure that all transactions have correct assets, amounts and proofs.
| /// | ||
| /// The corresponding UTXOs are generated ad hoc and subsequently | ||
| /// stored in the internal state of the wallet. | ||
| pub fn coin_select(&mut self, amount: Amount, asset: AssetId) -> Result<Vec<Input>> { |
There was a problem hiding this comment.
Maybe this should be called mint instead then? :)
There was a problem hiding this comment.
I'm not sure about this one.
I think the fact that the coins didn't exist before the call is just an implementation detail and we really just care about selecting coins when calling this.
I only added the docstring because a previous version of this change introduced some other arguments which made the whole test API worth explaining. Without that I think the API doesn't need to be documented, but it can't hurt.
thomaseizinger
left a comment
thomaseizinger
left a comment
There was a problem hiding this comment.
Looks good!
If you find c34e568 too noisy, you can always extract functions for the shared code to reduce the duplication.
I'll pass this time because it's only duplicated once and a function which adds the outputs (if needed) to |
Change outputs are now only added to the loan transaction if necessary. This fixes bug #38. We therefore no longer need to always add a buffer to the amount we select coins for. The original loan protocol tests now use the least number of outputs possible (we still need to a collateral change output for the loan transaction because we don't know the transaction fee ahead of time. See: #57). To verify that we still allow the presence of change outputs (principal change outputs in particular), we've added the test `can_run_protocol_with_principal_change_outputs`.
luckysori
force-pushed
the
pesky-change-outputs
branch
from
c34e568 to
7433efd
Compare
2 participants
Fix #38.
On the way, I noticed that after replacing the blockchain-based testing with
elements-consensus(PR #34) we were no longer verifying that our transactions were not printing (or destroying) money. That's the motivation for b005a04.Please review patch by patch.