Murisi+grarco/multi tx masp vp rebased #3186

murisi · 2024-05-07T09:45:16Z

Describe your changes

Tried to increase the readability, strength, and generality of the MASP validity predicate whilst decreasing its verbosity. The changes are as follows:

Transaction inputs and outputs that exceed pre-balances and post-balances respectively are no longer allowed.
Now use the Sapling value balance in order to verify that movement in and out of the shielded pool is correct
Now handle all AssetType decoding in one place before starting to check the transparent bundles
The VP should now support Transactions and unrelated Transfers touching the same addresses/tokens
Tried to remove the code that gave IBC minting transactions special treatment now that Sapling value balance is used
Reduced dependency on the multitoken VP by directly checking the correctness of MASP VP balance changes
Now check that all Transactions and the balancing transaction are sufficiently authorized; prevents malleability

What would still need to be handled/fixed if this code turns out to be relevant:

Add integration tests. Like checking that an unshielding Transaction to Bertha is embeddable inside a Transfer to Christel only if Bertha has signed the entire transaction (in addition to the usual MASP checks).

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

Namada 0.34.0
anoma/masp#79

Checklist before merging to `draft`

I have added a changelog
Git history is in acceptable state

… sapling bundle anymore

…oves unchecked operation.

…t party.

codecov · 2024-05-07T10:10:36Z

Codecov Report

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

Project coverage is 59.32%. Comparing base (9d4de02) to head (7531eed).
Report is 263 commits behind head on main.

Files	Patch %	Lines
crates/namada/src/ledger/native_vp/masp.rs	0.00%	343 Missing ⚠️
crates/namada/src/ledger/native_vp/mod.rs	0.00%	102 Missing ⚠️
crates/sdk/src/masp.rs	0.00%	20 Missing ⚠️
...ledger/native_vp/ethereum_bridge/bridge_pool_vp.rs	0.00%	18 Missing ⚠️
crates/trans_token/src/storage_key.rs	0.00%	14 Missing ⚠️
crates/core/src/token.rs	0.00%	12 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3186      +/-   ##
==========================================
- Coverage   59.40%   59.32%   -0.09%     
==========================================
  Files         298      298              
  Lines       92326    92435     +109     
==========================================
- Hits        54849    54839      -10     
- Misses      37477    37596     +119

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

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

batconjurer · 2024-05-08T12:46:02Z

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

+            let epoched_asset_type =
+                encode_asset_type(token.clone(), *denom, *digit, Some(epoch))
+                    .wrap_err("unable to create asset type")?;
+            if conversion_state.assets.contains_key(&epoched_asset_type) {


shouldn't this check also be present in the function below?

This check is here to prevent an unepoched asset from entering the shielded pool in the case where its underlying token address is participating in the shielded rewards program. But it is possible for an unepoched asset to enter the shielded pool and then later have its underlying token address participate in the shielded pool. In this case we need to allow users to withdraw these assets even though their epoched equivalents are in the conversion state. This was my reasoning for why this check is not repeated in the context of the transparent output. But please let me know if I'm missing something!

sug0

mostly code quality related comments

crates/core/src/token.rs

sug0 · 2024-05-10T08:43:24Z

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

+impl From<Uint> for SignedAmount {
+    fn from(lo: Uint) -> Self {
+        let mut arr = [0u64; Self::N_WORDS];
+        arr[..4].copy_from_slice(&lo.0);
+        Self(SignedAmountInt(arr))
+    }
+}
+
+impl From<Amount> for SignedAmount {
+    fn from(lo: Amount) -> Self {
+        let mut arr = [0u64; Self::N_WORDS];
+        arr[..4].copy_from_slice(&lo.raw_amount().0);
+        Self(SignedAmountInt(arr))
+    }
+}


add some unit tests to check these conversions hold

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

sug0 · 2024-05-10T08:49:19Z

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

+impl TryInto<Amount> for SignedAmount {
+    type Error = std::io::Error;
+
+    fn try_into(self) -> Result<Amount, Self::Error> {
+        if self.0.0[Self::N_WORDS - 1] == 0 {
+            Ok(Amount::from_uint(
+                Uint([self.0.0[0], self.0.0[1], self.0.0[2], self.0.0[3]]),
+                0,
+            )
+            .unwrap())
+        } else {
+            Err(std::io::Error::new(
+                std::io::ErrorKind::InvalidInput,
+                "Integer overflow when casting to Amount",
+            ))
+        }
+    }
+}


add a conversion test with failures included

sug0 · 2024-05-10T09:22:15Z

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

+construct_uint! {
+    #[derive(
+        BorshSerialize,
+        BorshDeserialize,
+        BorshSchema,
+    )]
+
+    struct SignedAmountInt(5);
+}
+
+/// A positive or negative amount
+#[derive(
+    Copy,
+    Clone,
+    Eq,
+    PartialEq,
+    BorshSerialize,
+    BorshDeserialize,
+    BorshSchema,
+    Default,
+)]
+pub struct SignedAmount(SignedAmountInt);


btw, I think we can replace I256 with this type, renaming it to something like I320. as it stands, signed arithmetic in core/src/uint.rs is prone to loss of precision.

I would move this to core

Thanks. I've renamed SignedAmount to I320 and moved it into the core file core/src/uint.rs in #3264. Probably we can replace usage of I256 with I320 in a separate PR.

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

grarco · 2024-05-13T11:51:25Z

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

+        // Ensure that every account for which balance has gone down has
+        // authorized this transaction
+        for (addr, pre) in changed_balances.pre {
+            if changed_balances.post[&addr] < pre && addr != masp_address_hash {


Could this check be done only if the balances decrease more than implied by the Transaction? As in, if we expect a balance to go down by 10 but we see a decrease of 15 we request a signature since this means that something else happened in parallel to the masp tx that we cannot verify in here. Otherwise, if the decrease matches (or is less than) the exepected value of Transaction we don't require/verify the signature. Could this also apply to validate_transparent_input?
This (if correct) could lead to less signatures needed on the transaction and less verifications.

I think you have a point here, though I'll have to think about the details some more. I'll try and sketch this change in a separate PR.

grarco and others added 15 commits May 7, 2024 09:10

Reworks masp vp to accept multiple transfers in a single tx

a007a32

Refactors transparent bundle check to just compare maps

b43be29

Fixes unrecognized assets

5887ee7

Refactors balances split

daf9788

valid_spend_descriptions_anchor does not expect the presence of the…

ad97698

… sapling bundle anymore

Refactors transparent bundle validation into a separate function. Rem…

c43bf92

…oves unchecked operation.

Changelog #2690

14e8238

Fixes balance owner when token is minted

66af13a

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

beb8e72

Fix transparent bundle validation bug

2687f30

Unifies DeltaBalance and SignedAmount

94b5efa

Now decode AssetTypes separately.

dcf6fd4

Now check that the diff between pre and post is the value balance.

6533037

Split validate_transparent_bundle into 3 functions.

56f7665

Ensure that all implied transfers have been authorized by the relevan…

01509e8

…t party.

Make MASP VP accept IBC transactions.

67ec3be

murisi force-pushed the murisi+grarco/multi-tx-masp-vp-rebased branch 2 times, most recently from 90d004a to 67ec3be Compare May 7, 2024 11:43

murisi requested review from grarco and batconjurer May 7, 2024 16:07

Use a MASP crate that guarantees ValueSums.

0380f7c

murisi force-pushed the murisi+grarco/multi-tx-masp-vp-rebased branch from 89a2e63 to 0380f7c Compare May 7, 2024 16:14

batconjurer reviewed May 8, 2024

View reviewed changes

crates/namada/src/ledger/native_vp/masp.rs Outdated Show resolved Hide resolved

batconjurer reviewed May 8, 2024

View reviewed changes

murisi force-pushed the murisi+grarco/multi-tx-masp-vp-rebased branch from 0ea6215 to 06f4da1 Compare May 8, 2024 16:40

murisi marked this pull request as ready for review May 8, 2024 16:45

murisi requested a review from sug0 May 8, 2024 16:46

murisi force-pushed the murisi+grarco/multi-tx-masp-vp-rebased branch 2 times, most recently from fd5e418 to 3938cde Compare May 8, 2024 17:48

murisi force-pushed the murisi+grarco/multi-tx-masp-vp-rebased branch from 3938cde to 29f0c60 Compare May 9, 2024 12:16

Extend SignedAmount to hold MASP value balance.

7531eed

murisi force-pushed the murisi+grarco/multi-tx-masp-vp-rebased branch from 29f0c60 to 7531eed Compare May 9, 2024 12:23

sug0 requested changes May 10, 2024

View reviewed changes

murisi mentioned this pull request May 10, 2024

Murisi/masp governance changes #3222

Closed

2 tasks

grarco mentioned this pull request May 10, 2024

MASP vp multiple txs #2690

Closed

2 tasks

grarco reviewed May 13, 2024

View reviewed changes

brentstone mentioned this pull request May 14, 2024

Draft - released as 0.36.0 #3213

Closed

tzemanovic merged commit 75b71b5 into main May 21, 2024
15 of 19 checks passed

tzemanovic deleted the murisi+grarco/multi-tx-masp-vp-rebased branch May 21, 2024 13:29

grarco mentioned this pull request May 24, 2024

Review sig check in masp vp #3311

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Murisi+grarco/multi tx masp vp rebased #3186

Murisi+grarco/multi tx masp vp rebased #3186

murisi commented May 7, 2024 •

edited

Loading

codecov bot commented May 7, 2024 •

edited

Loading

batconjurer May 8, 2024

murisi May 9, 2024

sug0 left a comment

sug0 May 10, 2024

sug0 May 10, 2024

sug0 May 10, 2024

murisi May 20, 2024 •

edited

Loading

grarco May 13, 2024 •

edited

Loading

murisi May 20, 2024 •

edited

Loading

Murisi+grarco/multi tx masp vp rebased #3186

Murisi+grarco/multi tx masp vp rebased #3186

Conversation

murisi commented May 7, 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 May 7, 2024 • edited Loading

Codecov Report

batconjurer May 8, 2024

Choose a reason for hiding this comment

murisi May 9, 2024

Choose a reason for hiding this comment

sug0 left a comment

Choose a reason for hiding this comment

sug0 May 10, 2024

Choose a reason for hiding this comment

sug0 May 10, 2024

Choose a reason for hiding this comment

sug0 May 10, 2024

Choose a reason for hiding this comment

murisi May 20, 2024 • edited Loading

Choose a reason for hiding this comment

grarco May 13, 2024 • edited Loading

Choose a reason for hiding this comment

murisi May 20, 2024 • edited Loading

Choose a reason for hiding this comment

murisi commented May 7, 2024 •

edited

Loading

Checklist before merging to `draft`

codecov bot commented May 7, 2024 •

edited

Loading

murisi May 20, 2024 •

edited

Loading

grarco May 13, 2024 •

edited

Loading

murisi May 20, 2024 •

edited

Loading