MASP vp multiple txs #2690

grarco · 2024-02-21T19:17:08Z

Describe your changes

Closes #2595.

Modifies the MASP vp to allow multiple transfers in a single transaction. To achieve this, some constraints had to be removes (like the checks on the presence/absence of the transparent bundle, transparent inputs and outputs). Since we cannot expect a singles source/target anymore some changes were made, most importantly:

the checks on the sapling bundle have been modified to always run and to not expect the presence of shielded inputs
the checks on the transparent bundle have been modified to run against against all the changed balance keys instead of expecting a specific source/target

No changes to the SDK have been done in this PR because, at least for the moment, we still rely on the Transfer object to recover the Transaction, and so other tx_data types won't work, leading to an impossibility to effectively take advantage of these vp updates.

Indicate on which release or other PRs this topic is based on

#2621

Checklist before merging to `draft`

I have added a changelog
Git history is in acceptable state

codecov · 2024-02-21T19:43:23Z

Codecov Report

Attention: Patch coverage is 0% with 325 lines in your changes are missing coverage. Please review.

Project coverage is 53.93%. Comparing base (2535c9c) to head (ac55fc4).

Files	Patch %	Lines
crates/namada/src/ledger/native_vp/masp.rs	0.00%	275 Missing ⚠️
crates/namada/src/ledger/native_vp/mod.rs	0.00%	31 Missing ⚠️
crates/trans_token/src/storage_key.rs	0.00%	10 Missing ⚠️
crates/sdk/src/masp.rs	0.00%	9 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2690      +/-   ##
==========================================
- Coverage   53.95%   53.93%   -0.03%     
==========================================
  Files         308      308              
  Lines      100018   100054      +36     
==========================================
- Hits        53967    53963       -4     
- Misses      46051    46091      +40

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

grarco · 2024-02-22T16:24:37Z

crates/namada/src/ledger/native_vp/masp.rs

-            token,
-            amount,
-        })
+            if let ShieldedActionOwner::Minted = counterpart {


I think this could be removed if we only validated the absolute value of the balance changes instead of the value and the sign. I'm not sure though if this is completely safe to do

Can I just clarify the following?: When receiving a simple IBC transfer, the Transaction will have a vin from the IBC account's transparent address and no vouts (because the amount will be in the shielded outputs)? And because of this, validate_transparent_bundle computes a negative delta? Is this the reason why we negate diff here?

To my understating when we receive a shielded action two things happen:

We mint the token amount in the #Multitoken/{token_addr}/balance/minted balance (this should be the case only if we are receiving a foreign token, if we are receiving back the native token than we should unescrow from #Multitoken/{token_addr}/balance/#Ibc)

The amount is shielded, thus increasing the transparent balance of the MASP, but this does not reduce the amount of the minted balance, so in this case the sum of the amounts does not collapse to 0, leading to the need to mock an opposite balance change for the minted balance to make the computation work

murisi

Nice, I like the way you have structured the MASP VP. I just think that perhaps some of the logic could maybe be simplified and made less verbose. I also think we should take advantage of Transaction::sapling_value_balance() in the MASP VP checks somehow since that value tells us how much is flowing into/out of the shielded pool for a given Transaction.

I've sketched some of my ideas here: https://github.com/anoma/namada/tree/murisi/multi-tx-masp-vp-experiments .

murisi · 2024-02-29T08:38:03Z

crates/namada/src/ledger/native_vp/masp.rs

+                let mut token_bundle_balances = BTreeMap::default();
+
+                // To help recognize asset types not in the conversion tree
+                let unepoched_tokens = unepoched_tokens(token, denom)?;


It might be easier to compute and accumulate all unepoched_tokens corresponding to ChangedBalance::masp's elements in the loop in validate_state_and_get_transfer_data. Doing that would remove this loop body's direct dependence on token, and hence potentially allow us to remove the for token in changed_balances.masp { loop in this function. Doing that might allows us to reduce the loop nesting in this function.

Actually we still depend on the token binding in two more places inside this loop:

When checking the conversion

When matching the transparent bundle agains the changed keys in storage

For the second point we could simply iterate on the entire token_bundle_balances but for the first point I suspect we'd need to iterate anyway, so we'll end up just moving the for loop to a different place instead of removing it

murisi · 2024-02-29T08:49:31Z

crates/namada/src/ledger/native_vp/masp.rs

+
+                    match delta_balance {
+                        Some(delta_balance)
+                            if delta_balance == &delta_bundle => {}


It seems that we check here that the transparent account balance changes recorded in the MASP transaction reflect the transparent account balance changes reflected in the keys. So if the key changes indicate that Bertha's balance has increased by +1 BTC, a transaction with a vin from Bertha of 5 BTC and a vout of 6 BTC TO Bertha would pass, right? Do we have any checks to prevent the VP from accepting a vin from Bertha of (5+x) BTC and a vout of (6+x) BTC (where x is large enough to exceed relevant source account balances)? If not, does anything in the client depend on the vins and vouts correctness (and not just the correctness of the delta)?

Ah good point. We don't have such a check, so it's possible for a transparent address to withdraw more than it owns if in the same transaction it also receives an amount great enough to compensate for this difference. To my understanding though, this is also the logic used in the multitoken vp. The client seems to be working only with the deltas implied by the transparent bundle (atm we use the changed balance keys only to determine who's the sender and who's the receiver but we can perhaps rework that)

murisi · 2024-02-29T08:51:30Z

crates/namada/src/ledger/native_vp/masp.rs

+
+                // Iterate over the delta balances described by the transparent
+                // bundle and verify that these match the actual modifications
+                // in storage NOTE: this effectively prevent the


Is this restriction (of not allowing the same addresses/tokens to be involved in other transparent transfers in the same tx) conceptually necessary? Or is it just an accidental consequence of our current design?

No this is an unfortunate consequence of this logic, I'd be very happy if we could lift this constraint

murisi · 2024-02-29T08:58:04Z

crates/namada/src/ledger/native_vp/masp.rs

-            token,
-            amount,
-        })
+            if let ShieldedActionOwner::Minted = counterpart {


Can I just clarify the following?: When receiving a simple IBC transfer, the Transaction will have a vin from the IBC account's transparent address and no vouts (because the amount will be in the shielded outputs)? And because of this, validate_transparent_bundle computes a negative delta? Is this the reason why we negate diff here?

murisi · 2024-02-29T09:00:19Z

crates/namada/src/ledger/native_vp/masp.rs

+                );
+                return Ok(false);
+            }
+        } else if !changed_balances.masp.is_empty() {


Is it possible for the MASP transparent balance to be incorrectly changed when transparent bundles are there? Like imagine a situation where a single Tx causes 5 BTC to be (legitimately) unshielded to Bertha (in the usual way with Transaction - Transfer pair) and simultaneously 10 ETH is transferred from MASP transparent address to Christel (with no corresponding vins or vouts). In this situation shielded_tx.transparent_bundle() would contain only BTC denominated vins and vouts, and changed_balances.masp would contain ETH and BTC. In this situation, which code prevents a positive change in changed_balances.other.get(ETH)?

The for loop at line 361 should iterate over all the masp changed balances and then checks the transparent bundle. If the bundle does not contain any data for ETH than token_bundle_balances should not be updated and therefore fail the test at line 579 where we check that the modifications implied by the transparent bundle effectively match to changes in storage. token_bundle_balances is instantiated for every token based on the modifications in the balance keys in storage so it should cover the case you illustrated

Correction, you're right, the token_bundle_balances would be empty in this case and so this is a bug 👍🏻

… sapling bundle anymore

…oves unchecked operation.

…l` multiple times

grarco · 2024-05-10T16:52:00Z

Closed in favor of #3186

* origin/murisi+grarco/multi-tx-masp-vp-rebased2: Enable governance to change MASP token map. Moved SignedAmount into core and removed use of unchecked arithmetic. Use try_fold to build the ChangedBalances object. Created a structure for representing conversion leafs. Factored the accumulation code out of verify_sapling_balancing_value. Updated the WASM binaries. Extend SignedAmount to hold MASP value balance. Use a MASP crate that guarantees ValueSums. Make MASP VP accept IBC transactions. Ensure that all implied transfers have been authorized by the relevant party. Split validate_transparent_bundle into 3 functions. Now check that the diff between pre and post is the value balance. Now decode AssetTypes separately. Unifies `DeltaBalance` and `SignedAmount` Fix transparent bundle validation bug Fixes balances check in masp vp to allow non-masp transfers Fixes balance owner when token is minted Changelog #2690 Refactors transparent bundle validation into a separate function. Removes unchecked operation. `valid_spend_descriptions_anchor` does not expect the presence of the sapling bundle anymore Refactors balances split Fixes unrecognized assets Refactors transparent bundle check to just compare maps Reworks masp vp to accept multiple transfers in a single tx

grarco mentioned this pull request Feb 21, 2024

MASP vp multiple txs #2689

Closed

2 tasks

grarco added the breaking-change label Feb 21, 2024

grarco requested a review from murisi February 21, 2024 19:35

grarco added a commit that referenced this pull request Feb 21, 2024

Changelog #2690

4629aba

grarco force-pushed the grarco/multi-tx-masp-vp branch from d7fe472 to 4629aba Compare February 21, 2024 19:55

grarco marked this pull request as ready for review February 21, 2024 19:57

grarco marked this pull request as draft February 22, 2024 09:05

grarco commented Feb 22, 2024

View reviewed changes

grarco added a commit that referenced this pull request Feb 23, 2024

Changelog #2690

ecf8b4e

grarco force-pushed the grarco/multi-tx-masp-vp branch from 0e2cc89 to 143fbae Compare February 23, 2024 11:31

grarco marked this pull request as ready for review February 23, 2024 13:28

murisi requested changes Feb 29, 2024

View reviewed changes

murisi mentioned this pull request Feb 29, 2024

Murisi/multi tx masp vp experiments #2779

Closed

2 tasks

grarco added 15 commits February 29, 2024 15:24

Run nullifier checks in masp vp regardless of tx type

659b057

Refactors and fix bundle checks in masp vp

2eed4dd

Changelog #2621

0f46fe4

Fixes sapling bundle writes check

35b0b4b

Reworks masp vp to accept multiple transfers in a single tx

83ed46b

Refactors transparent bundle check to just compare maps

2853541

Fixes unrecognized assets

4e0cf84

Refactors balances split

c6370e8

valid_spend_descriptions_anchor does not expect the presence of the…

304c889

… sapling bundle anymore

Refactors transparent bundle validation into a separate function. Rem…

c563bff

…oves unchecked operation.

Changelog #2690

adc06c7

Fixes balance owner when token is minted

71559f0

Fixes balances check in masp vp to allow non-masp transfers

ae2d0f7

Fix transparent bundle validation bug

2b7fc97

Unifies DeltaBalance and SignedAmount

01b7a8b

grarco force-pushed the grarco/multi-tx-masp-vp branch from 91405df to 01b7a8b Compare February 29, 2024 14:53

grarco added 3 commits February 29, 2024 19:30

Always get the opposite balance for the mint key in masp vp

7e0e9fc

Fixes bug for which amounts were accounted in the `transparent_tx_poo…

cb9b42d

…l` multiple times

Fixes empty transparent bundle validation

ac55fc4

grarco requested review from cwgoes and batconjurer March 19, 2024 14:27

batconjurer approved these changes Mar 21, 2024

View reviewed changes

murisi mentioned this pull request May 6, 2024

Murisi/multi tx masp vp experiments #3177

Closed

2 tasks

murisi pushed a commit that referenced this pull request May 7, 2024

Changelog #2690

14e8238

murisi mentioned this pull request May 7, 2024

Murisi+grarco/multi tx masp vp rebased #3186

Merged

2 tasks

grarco closed this May 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MASP vp multiple txs #2690

MASP vp multiple txs #2690

grarco commented Feb 21, 2024 •

edited

Loading

codecov bot commented Feb 21, 2024 •

edited

Loading

grarco Feb 22, 2024 •

edited

Loading

murisi Feb 29, 2024

grarco Mar 1, 2024 •

edited

Loading

murisi left a comment •

edited

Loading

murisi Feb 29, 2024

grarco Mar 2, 2024

murisi Feb 29, 2024 •

edited

Loading

grarco Mar 1, 2024

murisi Feb 29, 2024

grarco Mar 1, 2024

murisi Feb 29, 2024

murisi Feb 29, 2024

grarco Mar 1, 2024

grarco Mar 1, 2024

grarco commented May 10, 2024

MASP vp multiple txs #2690

MASP vp multiple txs #2690

Conversation

grarco commented Feb 21, 2024 • edited Loading

Describe your changes

Indicate on which release or other PRs this topic is based on

Checklist before merging to draft

codecov bot commented Feb 21, 2024 • edited Loading

Codecov Report

grarco Feb 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grarco Mar 1, 2024 • edited Loading

Choose a reason for hiding this comment

murisi left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

murisi Feb 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grarco commented May 10, 2024

grarco commented Feb 21, 2024 •

edited

Loading

Checklist before merging to `draft`

codecov bot commented Feb 21, 2024 •

edited

Loading

grarco Feb 22, 2024 •

edited

Loading

grarco Mar 1, 2024 •

edited

Loading

murisi left a comment •

edited

Loading

murisi Feb 29, 2024 •

edited

Loading