mobilecoind: add support for mixed transactions, including those with SCIs in them #3214

cbeck88 · 2023-03-09T17:04:55Z

This adds a new endpoint to mobliecoind called generate_mixed_transaction.

Unlike the generate_transaction endpoint, it does not assume that every outlay has the token id of the fee. Instead the request has a fee_token_id and each outlay is now an OutlayV2 which has a token id parameter.

Outlay is used both in the requests and responses for generate_transaction, so what I decided to do is extend Outlay to OutlayV2, which has a token id parameter, and TxProposal now contains OutlayV2. That protobuf extension was backwards compatible. All the historical requests, though, still use Outlay.

Clients that only know about Outlay will still deserialize TxProposal correctly, ignoring the new field. Clients that know about OutlayV2 will be able to see the token id of each outlay.

The generate_transaction request type is not changed, it still uses the old outlay type. This is because I think it would be ambiguous -- old clients expect to set that field using the token_id parameter which governs the fee. If the server would default that to zero, it could change the meaning of requests coming from old clients.

The generate_mixed_transaction request type uses OutlayV2 as the parameter.

Let me know if you see a problem with this compatibility strategy, happy to revisit it.

This PR increased in scope from what I originally planned -- to implement this stuff correctly, we need to be able to track balance changes coming from SCIs across several tokens, and then aggregate all those changes across the whole transaction to figure out the sizes of change outputs. So I added some functionality to SignedContingentInput to help with this.

Additionally, I found myself doing a lot of manual u64 and u128 arithmetic which is a bit subtle. In earlier PRs Eran had suggested that we should figure out a way to DRY all that up, but I didn't see a good way and it was only used in two places at the time. This time around, now that I have to do all that yet again, I decided to make such a way to reuse code, which is now a small utility class U64Ratio.

I'm happy to try to split out the lower level changes into a separate PR that can be reviewed first, but I feel like it's helpful to see what I'm trying to accomplish in order to scope those changes.

In latest commits, we have also removed the FALLBACK_FEE constant, which was unexpected and thought to be potentially hazardous during testing of what happens if you try to pay a fee in eusd and you don't specify a fee. If the user doesn't specify a fee, and the blockchain doesn't give a minimum fee for a particular token, then an error is returned that this token cannot be used to pay fees.

mobilecoind/src/payments.rs

mobilecoind/src/service.rs

cbeck88 · 2023-03-12T00:59:44Z

transaction/extra/src/signed_contingent_input.rs

@@ -82,11 +83,16 @@ impl SignedContingentInput {
    /// Note: This does check any other rules like tombstone block, or
    /// confirm proofs of membership, which are normally added only when this
    /// is incorporated into a transaction
-    pub fn validate(&self) -> Result<(), SignedContingentInputError> {
+    pub fn validate(&self) -> Result<SignedContingentInputAmounts, SignedContingentInputError> {


I decided to change the validate function so that it returns an object which contains lists of all the amounts of things that it unmasked during validation.

This makes it less complicated to find all those amounts after validating the SCI, and avoids the overhead of unmasking the TxOut's again and again.

Nice, I think this can be useful in the DEQS

cbeck88 · 2023-03-12T01:01:08Z

transaction/extra/src/signed_contingent_input.rs

    ZeroPartialFillChange,
+    /// Partial fill output amount was zero
+    ZeroPartialFillOutput,


this is not currently an error from the consensus network's point of view, but if we change input rules validation to use U64Ratio, then it probably will be.

I think that's okay, there's not any interesting use-case for having zero-value partial fill outputs.

It just happens to work today without an error because I wrote the code in a pretty low-level way that didn't require constructing U64Ratio.

this reduces code duplication, and the fact that tests are still passing convinces me that the input selection and change calculation stuff in build_mixed_transaction is likely all correct, since this puts way more test coverage on that function -- all the APIs are now going through build_mixed_transaction

mobilecoind/src/service.rs

eranrund

Very nice work, got some small comments but over all looks great.

mobilecoind/api/proto/mobilecoind_api.proto

mobilecoind/api/src/lib.rs

mobilecoind/src/payments.rs

mobilecoind/src/service.rs

eranrund · 2023-03-13T17:50:11Z

transaction/extra/src/signed_contingent_input.rs

@@ -82,11 +83,16 @@ impl SignedContingentInput {
    /// Note: This does check any other rules like tombstone block, or
    /// confirm proofs of membership, which are normally added only when this
    /// is incorporated into a transaction
-    pub fn validate(&self) -> Result<(), SignedContingentInputError> {
+    pub fn validate(&self) -> Result<SignedContingentInputAmounts, SignedContingentInputError> {


Nice, I think this can be useful in the DEQS

mobilecoind/src/service.rs

…tests this addresses some Eran comments, we think after review and testing that the fallback fee constant is kind of dangerous now that we have a multitoken ecosystem

cbeck88 · 2023-03-14T00:05:05Z

mobilecoind/src/payments.rs

-                candidate_block_version,
-                last_block_info.network_block_version,
-            );
+                .ok_or_else(|| Error::TxBuild("Token cannot be used to pay fees".into()))?;


this is what we do now instead of the FALLBACK_FEE

it's possible that we should give this error if the token id is not in the last_block_info, even if the user provided a fee value

❓ for the fee here my understanding is that if the request doesn't have a sufficient fee the transaction will eventually get rejected by consensus.
What I'm not sure about is if the user provides a larger fee than required. I'm guessing that consensus will gladly take that full fee value??..

yes, in the scenario that there is network congestion, users have to pay higher fees to get their transactions through

Yes, it will happily take a larger fee. Which is why this FALLBACK_FEE was problematic - users might end up unexpectedly paying higher fees than necessary

mobilecoind/src/payments.rs

mobilecoind/src/service.rs

they aren't in the request, they are also visible in the Tx and so they don't need to be duplicated

nick-mobilecoin

The main blocker is making sure the proto changes are correct and that falling back to MOB is what we want to do if these protos happen to talk to an older API.

mobilecoind-json/src/data_types.rs

mobilecoind/api/proto/mobilecoind_api.proto

mobilecoind/src/payments.rs

util/u64-ratio/src/lib.rs

transaction/extra/src/signed_contingent_input.rs

mobilecoind/api/proto/mobilecoind_api.proto

mobilecoind/src/payments.rs

cbeck88 · 2023-03-14T20:14:14Z

The main blocker is making sure the proto changes are correct and that falling back to MOB is what we want to do if these protos happen to talk to an older API.

What I've tried to do is, not change any of the request types of the older APIs -- falling back to MOB is a hazard, so that's why there's an OutlayV2 and we didn't just add token_id to all Outlay.

Only new APIs use OutlayV2 in requests, and in the response type TxProposal, I think it's safe to use OutlayV2, which can be deserialized by old clients correctly as an Outlay.

(I understand your concern now that, old TxProposal could be deserialized by a new client to have a bunch of spurious zeroes, but I also think that is going to be okay, because I don't think old TxProposals actually get passed to new clients in any actual workflow using mobilecoind.)

Co-authored-by: Nick Santana <[email protected]>

… SCIs in them (#3214) * add support for mixed transactions, including those with SCIs in them * make build_transaction call to build_mixed_transaction this reduces code duplication, and the fact that tests are still passing convinces me that the input selection and change calculation stuff in build_mixed_transaction is likely all correct, since this puts way more test coverage on that function -- all the APIs are now going through build_mixed_transaction * add test coverage * fixup api docs about fees * cleanup stuff about Outlay vs OutlayV2 comparisons * add some error handling for if change overflows a u64 * improve test coverage around counterparty change output * test SCIs in the response * remove fallback fee, use a better fee-map for mobilecoind tests, fix tests this addresses some Eran comments, we think after review and testing that the fallback fee constant is kind of dangerous now that we have a multitoken ecosystem * clippy * clear out sci proofs when adding sci records to TxProposal they aren't in the request, they are also visible in the Tx and so they don't need to be duplicated * Update mobilecoind/api/proto/mobilecoind_api.proto Co-authored-by: Nick Santana <[email protected]> * Update transaction/extra/src/signed_contingent_input.rs Co-authored-by: Nick Santana <[email protected]> * Update mobilecoind/api/proto/mobilecoind_api.proto Co-authored-by: Nick Santana <[email protected]> * update many code comments, per review * update more code comments, add more tests to util-u64-ratio --------- Co-authored-by: Nick Santana <[email protected]>

cbeck88 mentioned this pull request Mar 9, 2023

Mobilecoind scis #3212

Merged

Base automatically changed from mobilecoind-scis to release/v4.1 March 11, 2023 04:15

cbeck88 force-pushed the mobilecoind-mixed-transactions branch 2 times, most recently from 752a29d to 13cdb77 Compare March 12, 2023 00:45

cbeck88 commented Mar 12, 2023

View reviewed changes

mobilecoind/src/payments.rs Show resolved Hide resolved

cbeck88 commented Mar 12, 2023

View reviewed changes

mobilecoind/src/service.rs Show resolved Hide resolved

cbeck88 commented Mar 12, 2023

View reviewed changes

cbeck88 changed the title ~~add support for mixed transactions, including those with SCIs in them~~ mobilecoind: add support for mixed transactions, including those with SCIs in them Mar 12, 2023

cbeck88 force-pushed the mobilecoind-mixed-transactions branch 2 times, most recently from 82b04fa to 47bd256 Compare March 12, 2023 06:00

add support for mixed transactions, including those with SCIs in them

25c4626

cbeck88 force-pushed the mobilecoind-mixed-transactions branch from 47bd256 to 25c4626 Compare March 12, 2023 17:15

cbeck88 commented Mar 13, 2023

View reviewed changes

mobilecoind/src/service.rs Show resolved Hide resolved

add test coverage

da3e548

cbeck88 requested review from eranrund, nick-mobilecoin, jcape, wjuan-mob and a team March 13, 2023 06:45

cbeck88 marked this pull request as ready for review March 13, 2023 06:46

cbeck88 self-assigned this Mar 13, 2023

cbeck88 mentioned this pull request Mar 13, 2023

make transaction core input rules validation use U64Ratio #3224

Merged

cbeck88 added the ! v4.1.0 Blocker for v4.1 label Mar 13, 2023

cbeck88 mentioned this pull request Mar 13, 2023

enable code-reuse for partial fill fraction calculations #3223

Closed

eranrund suggested changes Mar 13, 2023

View reviewed changes

cbeck88 added 2 commits March 13, 2023 13:03

fixup api docs about fees

c077a08

cleanup stuff about Outlay vs OutlayV2 comparisons

76d856e

cbeck88 mentioned this pull request Mar 13, 2023

Improve transaction builder APIs around SCIs #3230

Open

cbeck88 added 2 commits March 13, 2023 13:31

add some error handling for if change overflows a u64

d3a2620

improve test coverage around counterparty change output

f3566ab

cbeck88 commented Mar 13, 2023

View reviewed changes

mobilecoind/src/service.rs Outdated Show resolved Hide resolved

test SCIs in the response

655dd10

cbeck88 added the mobilecoind The MobileCoinD application label Mar 13, 2023

remove fallback fee, use a better fee-map for mobilecoind tests, fix …

3f3914e

…tests this addresses some Eran comments, we think after review and testing that the fallback fee constant is kind of dangerous now that we have a multitoken ecosystem

cbeck88 requested a review from eranrund March 13, 2023 23:20

clippy

116c7a1

cbeck88 commented Mar 14, 2023

View reviewed changes

mobilecoind/src/payments.rs Show resolved Hide resolved

cbeck88 added the enhancement New feature or request label Mar 14, 2023

cbeck88 commented Mar 14, 2023

View reviewed changes

mobilecoind/src/service.rs Outdated Show resolved Hide resolved

clear out sci proofs when adding sci records to TxProposal

606697f

they aren't in the request, they are also visible in the Tx and so they don't need to be duplicated

nick-mobilecoin approved these changes Mar 14, 2023

View reviewed changes

wjuan-mob reviewed Mar 14, 2023

View reviewed changes

mobilecoind/api/proto/mobilecoind_api.proto Show resolved Hide resolved

mobilecoind/api/proto/mobilecoind_api.proto Show resolved Hide resolved

mobilecoind/src/payments.rs Show resolved Hide resolved

eranrund reviewed Mar 14, 2023

View reviewed changes

mobilecoind/src/payments.rs Show resolved Hide resolved

eranrund approved these changes Mar 14, 2023

View reviewed changes

wjuan-mob approved these changes Mar 14, 2023

View reviewed changes

cbeck88 and others added 5 commits March 14, 2023 14:16

Update mobilecoind/api/proto/mobilecoind_api.proto

cbeea6f

Co-authored-by: Nick Santana <[email protected]>

Update transaction/extra/src/signed_contingent_input.rs

8489d15

Co-authored-by: Nick Santana <[email protected]>

Update mobilecoind/api/proto/mobilecoind_api.proto

503d24e

Co-authored-by: Nick Santana <[email protected]>

update many code comments, per review

c246084

update more code comments, add more tests to util-u64-ratio

5da13d1

cbeck88 enabled auto-merge (squash) March 14, 2023 21:22

cbeck88 merged commit ced867a into release/v4.1 Mar 14, 2023

cbeck88 deleted the mobilecoind-mixed-transactions branch March 14, 2023 21:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mobilecoind: add support for mixed transactions, including those with SCIs in them #3214

mobilecoind: add support for mixed transactions, including those with SCIs in them #3214

cbeck88 commented Mar 9, 2023 •

edited

Loading

cbeck88 Mar 12, 2023 •

edited

Loading

eranrund Mar 13, 2023

cbeck88 Mar 12, 2023 •

edited

Loading

eranrund left a comment

eranrund Mar 13, 2023

cbeck88 Mar 14, 2023

nick-mobilecoin Mar 14, 2023

cbeck88 Mar 14, 2023

eranrund Mar 14, 2023

nick-mobilecoin left a comment

cbeck88 commented Mar 14, 2023 •

edited

Loading

mobilecoind: add support for mixed transactions, including those with SCIs in them #3214

mobilecoind: add support for mixed transactions, including those with SCIs in them #3214

Conversation

cbeck88 commented Mar 9, 2023 • edited Loading

cbeck88 Mar 12, 2023 • edited Loading

Choose a reason for hiding this comment

eranrund Mar 13, 2023

Choose a reason for hiding this comment

cbeck88 Mar 12, 2023 • edited Loading

Choose a reason for hiding this comment

eranrund left a comment

Choose a reason for hiding this comment

eranrund Mar 13, 2023

Choose a reason for hiding this comment

cbeck88 Mar 14, 2023

Choose a reason for hiding this comment

nick-mobilecoin Mar 14, 2023

Choose a reason for hiding this comment

cbeck88 Mar 14, 2023

Choose a reason for hiding this comment

eranrund Mar 14, 2023

Choose a reason for hiding this comment

nick-mobilecoin left a comment

Choose a reason for hiding this comment

cbeck88 commented Mar 14, 2023 • edited Loading

cbeck88 commented Mar 9, 2023 •

edited

Loading

cbeck88 Mar 12, 2023 •

edited

Loading

cbeck88 Mar 12, 2023 •

edited

Loading

cbeck88 commented Mar 14, 2023 •

edited

Loading