MASP fee payment #3393

grarco · 2024-06-07T17:40:52Z

Describe your changes

Closes #2597.

Introduces fee payment via the MASP. If a transaction can pay fees via a transparent transfer from the wrapper signer's balance than the protocol proceeds like that. Otherwise, we look to see if the first transaction of the batch is a masp transfer and after its changes the transparent fee payment is possible (i.e. we check that the tx unshields enough funds to the fee payer).

Masp fee payment in the SDK is supported for the transactions for which it makes sense: shielded, unshielding and ibc transactions (when shielded action and only on the source chain, where the initial unshield happens).

To limit the DOS changes of an attacker, if masp fee payment is required than we set a custom gas limit (protocol param) to avoid attackers reducing the tps by requesting large gas limit: if fees are successfully paid we bring back the original gas limit set but the user.

Modification have been done to the way we construct masp transaction to account for multiple sources/targets which is required for fee shielded fee payment. Also, to pay fees, the SDK tries to collect the funds in this order:

From changes
From unused notes of the transaction's sources
From the optional, user provided spend notes

There are a few of things that could be improved but are not part of this PR (since I thought it's already quite large). I'm going to open issues for these:

the disposable-gas-payer is still a Tx arg but we should instead pull it out of there and move it to the args of the specific transactions for which the SDK supports masp fee payment
The new Changes type used when constructing masp transactions could probably be a U128Sum instead of a I128Sum
Currently we only support a single additional spending key for gas payment in the CLI but we should allow for multiples. To do that we need to add a new ArgMultiOpt arg which we are currently lacking

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

#3356 (diffs for review: https://github.com/anoma/namada/pull/3393/files/1e98403560a297f56f38ff67397a4bb9182068aa..e1b5857845bafa1be9f4ee4500b5544b56bf926f)

Checklist before merging to `draft`

I have added a changelog
Git history is in acceptable state

codecov · 2024-06-07T19:12:31Z

Codecov Report

Attention: Patch coverage is 25.86393% with 1373 lines in your changes missing coverage. Please review.

Project coverage is 53.50%. Comparing base (879a326) to head (e1b5857).
Report is 4 commits behind head on main.

Files	Patch %	Lines
crates/sdk/src/masp.rs	0.00%	572 Missing ⚠️
crates/sdk/src/tx.rs	0.00%	293 Missing ⚠️
crates/sdk/src/signing.rs	0.00%	134 Missing ⚠️
crates/apps_lib/src/cli.rs	0.00%	110 Missing ⚠️
crates/namada/src/ledger/protocol/mod.rs	78.13%	68 Missing ⚠️
crates/light_sdk/src/transaction/transfer.rs	0.00%	32 Missing ⚠️
crates/node/src/bench_utils.rs	0.00%	31 Missing ⚠️
crates/token/src/lib.rs	0.00%	26 Missing ⚠️
crates/sdk/src/lib.rs	0.00%	24 Missing ⚠️
crates/state/src/wl_state.rs	58.82%	21 Missing ⚠️
... and 12 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3393      +/-   ##
==========================================
- Coverage   53.92%   53.50%   -0.42%     
==========================================
  Files         317      317              
  Lines      107575   108596    +1021     
==========================================
+ Hits        58011    58108      +97     
- Misses      49564    50488     +924

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

batconjurer

The masp.rs file in the sdk crate is too damn large. Can we split out all tx building functionality (of which there is now a lot) into a separate file?

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

batconjurer · 2024-06-14T10:19:02Z

crates/sdk/src/lib.rs

@@ -1130,7 +1135,7 @@ pub mod testing {
            let tx_data = match masp_tx_type {
                MaspTxType::Shielded => {
                    tx.add_code_from_hash(code_hash, Some(TX_SHIELDED_TRANSFER_WASM.to_owned()));
-                    let data = ShieldedTransfer { section_hash: shielded_section_hash };
+                    let data = ShieldedTransfer { fee_unshield: transfers.0.first().map(|transfer| UnshieldingTransferData { target: transfer.target.to_owned(), token: transfer.token.to_owned(), amount: transfer.amount }), section_hash: shielded_section_hash };


how the hell did this get past cargo fmt?

batconjurer · 2024-06-14T10:22:22Z

crates/sdk/src/tx.rs

+    .unwrap_or_default();
+    let total_fee = checked!(fee_amount.amount() * u64::from(args.gas_limit))?;
+
+    Ok(match total_fee.checked_sub(balance) {


nit: I think it's clearer to check total_fee > balance and then calculate the diff afterwards

crates/tests/src/integration/masp.rs

batconjurer · 2024-06-14T10:31:39Z

crates/sdk/src/masp.rs

+            )
+            .expect("Change is guaranteed to be non-negative");
+            changes = changes
+                .map(|prev| prev + change_amt.clone())


curious that unchecked arithmetic is ok here

we don't have the strict clippy checks in the sdk :/ - ideally, we shouldn't be implementing any logic here

imho we should move the sdk masp mod into the shielded_token crate

It's actually checked if I recall correctly

grarco · 2024-06-14T13:22:48Z

The masp.rs file in the sdk crate is too damn large. Can we split out all tx building functionality (of which there is now a lot) into a separate file?

I agree but can we do that in a separate PR that does only that?

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

cwgoes

Re-reviewed the core changes.

brentstone · 2024-06-25T05:46:13Z

What's the status of this PR? Is it rdy for review and just waiting for some?

* grarco/masp-fee-payment: Removes fallback logic when failed fee payment Renames misleading gas limit variable Removes useless write-log commit in fee payment Fixes typo Fixes masp amounts conversion Fixes broken docs Reuses token transfer Fixes typo Panics in fee payment if balance read fails Changelog #3393 Adds missing gas spending key arg to ibc tx Masp fee payment for shielded actions Fixes masp tx generation and integration tests Updates shielded wasm code to handle fee unshielding Removes unused denominate function Adds support for masp fee payment in sdk Refactors the write log api Different gas cost for storage deletes Removes write log precommit and leverages the batch log Adds integration tests for masp fee payment Refactors batch execution in case of masp fee payment Skips the execution of the first inner tx when masp fee payment Renames fee payment gas limit parameter Returns `BatchedTxResult` from masp fee payment `check_fees` drop the storage changes in case of failure `check_fees` checks masp fee payment Reworks masp fee payment to correctly handle errors. Misc refactors Passes the correct tx index to masp fee payment check Introduces masp fee payment

* origin/grarco/masp-fee-payment: Removes fallback logic when failed fee payment Renames misleading gas limit variable Removes useless write-log commit in fee payment Fixes typo Fixes masp amounts conversion Fixes broken docs Reuses token transfer Fixes typo Panics in fee payment if balance read fails Changelog #3393 Adds missing gas spending key arg to ibc tx Masp fee payment for shielded actions Fixes masp tx generation and integration tests Updates shielded wasm code to handle fee unshielding Removes unused denominate function Adds support for masp fee payment in sdk Refactors the write log api Different gas cost for storage deletes Removes write log precommit and leverages the batch log Adds integration tests for masp fee payment Refactors batch execution in case of masp fee payment Skips the execution of the first inner tx when masp fee payment Renames fee payment gas limit parameter Returns `BatchedTxResult` from masp fee payment `check_fees` drop the storage changes in case of failure `check_fees` checks masp fee payment Reworks masp fee payment to correctly handle errors. Misc refactors Passes the correct tx index to masp fee payment check Introduces masp fee payment

grarco added 25 commits June 6, 2024 11:12

Vectorizes transparent transfers

e9f5a46

Vectorizes masp transfers

36e215b

Fixes benchmarks

3eff3b4

Fixes signature generation for vectorized transfers

453b211

Check no vectorized transfers in cli

4a94e0e

Misc improvements to signing for vectorized transfers

9d6c19d

Avoids reloading shielded context

561e4e8

Fmt

3a77b4a

Changelog #3356

1e98403

Introduces masp fee payment

69980be

Passes the correct tx index to masp fee payment check

40401f4

Reworks masp fee payment to correctly handle errors. Misc refactors

bece3de

check_fees checks masp fee payment

3d9ccc4

check_fees drop the storage changes in case of failure

25dbf81

Returns BatchedTxResult from masp fee payment

d5aafd5

Renames fee payment gas limit parameter

606a0f0

Skips the execution of the first inner tx when masp fee payment

2f072b0

Refactors batch execution in case of masp fee payment

ba943ef

Adds integration tests for masp fee payment

07522e2

Removes write log precommit and leverages the batch log

5a1b578

Different gas cost for storage deletes

0e2b488

Refactors the write log api

92aafe7

Adds support for masp fee payment in sdk

120ad8e

Removes unused denominate function

584d2eb

Updates shielded wasm code to handle fee unshielding

9f390a7

grarco mentioned this pull request Jun 7, 2024

Draft #3377

Closed

Fixes masp tx generation and integration tests

fb0df69

grarco force-pushed the grarco/masp-fee-payment branch from b467bcd to fb0df69 Compare June 11, 2024 10:30

Masp fee payment for shielded actions

217afe3

batconjurer requested changes Jun 14, 2024

View reviewed changes

grarco added 4 commits June 14, 2024 16:38

Panics in fee payment if balance read fails

9698bb7

Fixes typo

70f40a6

Reuses token transfer

4f89335

Fixes broken docs

ed74656

grarco added the breaking: protocol label Jun 14, 2024

Fixes masp amounts conversion

6026c42

cwgoes reviewed Jun 17, 2024

View reviewed changes

Fixes typo

dc0316c

This was referenced Jun 18, 2024

Support multiple shielded gas payer in client #3423

Open

Make Changes unsigned #3425

Open

Removes useless write-log commit in fee payment

9e4d4c9

grarco requested review from cwgoes and tzemanovic June 21, 2024 16:08

cwgoes reviewed Jun 24, 2024

View reviewed changes

crates/namada/src/ledger/protocol/mod.rs Show resolved Hide resolved

cwgoes reviewed Jun 24, 2024

View reviewed changes

Renames misleading gas limit variable

13b4bcd

grarco mentioned this pull request Jun 25, 2024

Early sapling balance check #3439

Merged

2 tasks

Removes fallback logic when failed fee payment

e1b5857

grarco added the ready for draft label Jun 25, 2024

grarco mentioned this pull request Jul 1, 2024

Murisi/generalize vectorized transfers #3446

Closed

2 tasks

brentstone added the merged to draft label Jul 1, 2024

yito88 mentioned this pull request Jul 2, 2024

Draft+3459+3463+hermes #3470

Closed

2 tasks

brentstone added a commit that referenced this pull request Jul 4, 2024

fixup! Merge branch 'origin/grarco/masp-fee-payment' (#3393)

65cdd3e

brentstone merged commit bde8a9a into main Jul 5, 2024
10 of 19 checks passed

brentstone deleted the grarco/masp-fee-payment branch July 5, 2024 21:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MASP fee payment #3393

MASP fee payment #3393

grarco commented Jun 7, 2024 •

edited

Loading

codecov bot commented Jun 7, 2024 •

edited

Loading

batconjurer left a comment

batconjurer Jun 14, 2024

batconjurer Jun 14, 2024

batconjurer Jun 14, 2024

tzemanovic Jun 14, 2024

tzemanovic Jun 14, 2024

grarco Jun 14, 2024

grarco commented Jun 14, 2024

cwgoes left a comment

brentstone commented Jun 25, 2024

MASP fee payment #3393

MASP fee payment #3393

Conversation

grarco commented Jun 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 Jun 7, 2024 • edited Loading

Codecov Report

batconjurer left a comment

Choose a reason for hiding this comment

batconjurer Jun 14, 2024

Choose a reason for hiding this comment

batconjurer Jun 14, 2024

Choose a reason for hiding this comment

batconjurer Jun 14, 2024

Choose a reason for hiding this comment

tzemanovic Jun 14, 2024

Choose a reason for hiding this comment

tzemanovic Jun 14, 2024

Choose a reason for hiding this comment

grarco Jun 14, 2024

Choose a reason for hiding this comment

grarco commented Jun 14, 2024

cwgoes left a comment

Choose a reason for hiding this comment

brentstone commented Jun 25, 2024

grarco commented Jun 7, 2024 •

edited

Loading

Checklist before merging to `draft`

codecov bot commented Jun 7, 2024 •

edited

Loading