Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Embed masp transactions inside fee payment #2597

Closed
Tracked by #2530 ...
grarco opened this issue Feb 12, 2024 · 6 comments · Fixed by #3393
Closed
Tracked by #2530 ...

Embed masp transactions inside fee payment #2597

grarco opened this issue Feb 12, 2024 · 6 comments · Fixed by #3393
Assignees
Labels
client MASP pre-mainnet Must happen before mainnet. SDK

Comments

@grarco
Copy link
Collaborator

grarco commented Feb 12, 2024

The masp client has some intrinsics limitations when it comes to producing transactions which derives from previous txs' output descriptions for which it has not queried a node. More specifically, given the missing notes coming from other txs not produced by this very wallet, it's impossible to construct a merkle tree matching one of those seen by the protocol: the result is that (almost) all txs built as such will be rejected because of an invalid commitment anchor.

To get around this, for shielding and unshielding operations (the ones affected by this), as proposed by @murisi, we could embed them directly inside the fee unshielding Transaction object, so that the client can use the same, confirmed notes to produce a single transaction, avoiding the issue altogether.

To achieve this, we need to modify the client to produce a single Transaction object in such cases and attach an empty tx as the inner to mock a no-op. We could also support another custom tx as the inner to allow running multiple transactions (the ones embedded in the fee payment and the actual inner).

For safety reasons this depends on #2592 so that the fee unshielding operation cannot be exploited to perform free transactions.
It also depends on #2595 and #2596 for the unshielding case to support different tokens between the actual fee unshielding and the proper unshielding txs and also to avoid an extra transparent transfer from the fee payer to the actual recipient.

Finally, we need to review the logic in run_fee_unshielding in terms of error management to see if needs to be changed to support this feature.

@cwgoes
Copy link
Collaborator

cwgoes commented Apr 24, 2024

@grarco This will change the transaction format, right? Only for MASP transactions or also for all transactions?

@grarco
Copy link
Collaborator Author

grarco commented Apr 24, 2024

Haven't thought too much about it but since we'll need to unshield to more than one recipient this should require modifying the structure of WrapperTx, which would affect all transactions

@cwgoes
Copy link
Collaborator

cwgoes commented Apr 25, 2024

@grarco Would it be possible to do a special case analysis of an inner transaction with an output from a MASP transfer that pays the fee? Would that be simpler or more complicated? Would that allow us to not break wrapper transactions?

@cwgoes
Copy link
Collaborator

cwgoes commented Apr 26, 2024

New logic discussed 2024.04.26:

  • When we analyze for "sufficient fee or not", we should first look at the wrapper, and in the special case where the wrapper fee paying address doesn't have enough balance, check if the inner contains a valid unshield to that address. Reject immediately if the inner does not contain an unshield.

@cwgoes
Copy link
Collaborator

cwgoes commented Apr 29, 2024

Note with batching: first inner tx in a batch must pay for the fees.

@cwgoes
Copy link
Collaborator

cwgoes commented May 14, 2024

Order of operations here:

  1. Remove fee unshielding bit in wrapper tx (Remove wrapper fee unshielding #3217)
  2. Vectorize/refactor the transfer struct & inner tx (@grarco is working on this)
  3. Add in support for the first inner tx unshielding a fee (@grarco will work on this)

This was referenced May 24, 2024
@grarco grarco mentioned this issue Jun 4, 2024
2 tasks
@brentstone brentstone mentioned this issue Jun 6, 2024
@grarco grarco mentioned this issue Jun 7, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client MASP pre-mainnet Must happen before mainnet. SDK
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants