Addresses the boundary conditions for the Masp VP predicate #1104
Conversation
This commit adds most of the proper boundary checks from https://github.com/anoma/namada/blob/main/documentation/specs/src/masp/ledger-integration.md#boundary-conditions The ones not included are the following: 1. If the target address is not the masp, then the public key must be checked 2. If the source address is not the masp, then the asset type must be derived 3. If the source address is not the masp, then the value must be derived The first one is not satisifed as TxOut lacks a public key to check against, not only in code but also in the spec https://github.com/anoma/namada/blob/83e83315a5eaec302d08ed5db242d70625cfe45f/documentation/specs/src/masp/ledger-integration.md#transparent-output-format The second and third are not satisfied as TxIn in code lacks the values in the spec that can be used to satisfy them https://github.com/anoma/namada/blob/83e83315a5eaec302d08ed5db242d70625cfe45f/documentation/specs/src/masp/ledger-integration.md#transparent-input-format
|
pls update wasm |
|
This looks alright to me, but @murisi knows the code better - can you take a look? |
This commit implements checking the public key from the output transaction against the ripemd160 of the sha256 of the target address. Furhter this commit fixes the clippy warnings that appeared in the last commit
mariari
force-pushed
the
mariari/masp-boundary-conditionals
branch
from
dce6f9b to
7c5d387
Compare
| if transfer.source != masp() { | ||
| // Satisfies 1. | ||
| if shielded_tx.vin.len() != 1 { |
There was a problem hiding this comment.
Given that we no longer intend to check the amount and asset type of the transparent input, this condition is no longer necessary. The value balance checks will ensure that the user has indeed provided sufficient inputs.
| } | ||
|
|
||
| // Satisfies 2. | ||
| if !shielded_tx.vout.is_empty() { |
There was a problem hiding this comment.
This check enforces that transactions with a non-MASP source must have no transparent outputs. While this check can be done, the transaction would still be valid (i.e. leaving the shielded pool and transparent pool in a consistent state) without it. Perhaps we can remove this check to simplify the logic?
| if transfer.target != masp() { | ||
| // Satisfies 1. | ||
| if !shielded_tx.vin.is_empty() { |
There was a problem hiding this comment.
This check enforces that transactions with non-MASP targets mut not have any transparent inputs. While this check can be done, the transaction would still be valid (i.e. leaving the shielded pool and transparent pool in a consistent state) without it. Perhaps we can remove this check to simplify the logic?
| // Timestamp is derived to allow unshields for older tokens | ||
| let atype = | ||
| let atype: &AssetType = |
There was a problem hiding this comment.
This use of .components().next().unwrap() seems to be unsafe. What guarantees that shielded_tx.value_balance has at least one component? And when there are multiple components, what does the first extracted component mean semantically?
| // 1. One transparent input | ||
| // 2. Zero transparent output | ||
| // 3. the transparent transaction value pool's amount must equal the | ||
| // containing wrapper transaction's fee amount |
There was a problem hiding this comment.
Where do we check that condition (3) is met? It seems to me that there should be some mention of the wrapper transaction's fee somewhere in this code?
| // 1. One transparent input | ||
| // 2. Zero transparent output | ||
| // 3. the transparent transaction value pool's amount must equal the | ||
| // containing wrapper transaction's fee amount | ||
| if transfer.source != masp() { |
There was a problem hiding this comment.
Where do we check for the for the following condition?: If the source address is the MASP validity predicate, then no transparent inputs are permitted in the shielded transaction.
It seems to me that if the source and target are MASP, then both the conditional on line 105 and line 146 are skipped. This would allow for these sorts of transactions to have unlimited transparent inputs and outputs.
| // 2. One transparent output | ||
| // 3. Asset type must be properly derived | ||
| // 4. Value from the output must be the same as the containing transfer | ||
| // 5. Public key must be the hash of the target | ||
| if transfer.target != masp() { |
There was a problem hiding this comment.
Where do we check for the following condition?: If the target address is the MASP validity predicate, then no transparent outputs are permitted in the shielded transaction.
It seems to me that if the source and target are MASP, then both the conditional on line 105 and line 146 are skipped. This would allow for these sorts of transactions to have unlimited transparent inputs and outputs.
|
@cwgoes You mentioned that you found the specs at https://github.com/anoma/namada/blob/main/documentation/specs/src/masp/ledger-integration.md confusing and had (or were going to) rewritten some parts. Have you made any edits to the |
This PR implements the boundary conditions found in #56.
The following checks are not included
If the target address is not the masp, then the public key must be
checked
If the source address is not the masp, then the asset type must be
derived
If the source address is not the masp, then the value must be
derived
The first one is not satisifed as TxOut lacks a public key to check
against, not only in code but also in the spec
https://github.com/anoma/namada/blob/83e83315a5eaec302d08ed5db242d70625cfe45f/documentation/specs/src/masp/ledger-integration.md#transparent-output-format
The second and third are not satisfied as TxIn in code lacks the
values in the spec that can be used to satisfy them
https://github.com/anoma/namada/blob/83e83315a5eaec302d08ed5db242d70625cfe45f/documentation/specs/src/masp/ledger-integration.md#transparent-input-format
I suggest a followup pr once those values are in.