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

MASP validity predicate #56

Closed
Tracked by #2020
juped opened this issue Jan 26, 2022 · 5 comments
Closed
Tracked by #2020

MASP validity predicate #56

juped opened this issue Jan 26, 2022 · 5 comments

Comments

@juped
Copy link
Member

juped commented Jan 26, 2022

Depends on #54, #55.

Desired abilities:

  • stores notes/nullifiers
  • verifies (proof, notes nullifiers) sets from transactions
  • exposes readable Merkle tree roots for clients in its storage
  • holds assets (just balances for now?)

┆Issue is synchronized with this Asana task by Unito

@juped juped added the MASP label Jan 26, 2022
@sync-by-unito sync-by-unito bot closed this as completed Feb 3, 2022
@juped juped reopened this Feb 3, 2022
@tzemanovic tzemanovic transferred this issue from anoma/anoma Jul 7, 2022
@cwgoes
Copy link
Collaborator

cwgoes commented Jan 12, 2023

@murisi this is done, right?

@murisi
Copy link
Collaborator

murisi commented Jan 22, 2023

@cwgoes If I'm not mistaken, I believe @juped has completed these tasks. But I would need to review the validity predicate code to ascertain whether the validation is sufficient, i.e. to ensure that the VP does not in any circumstances accept any transactions that should be rejected.

@juped
Copy link
Member Author

juped commented Jan 23, 2023

yes; but we were discussing porting it to native, which would be so much less flaky

@murisi
Copy link
Collaborator

murisi commented Jan 23, 2023

@juped Ah, I see. Great! Regardless of whether or not we port, I see a potential issue at https://github.com/anoma/namada/blob/main/wasm/wasm_source/src/vp_masp.rs#L75 . If the client somehow manages to construct a Transaction with shielded_tx.value_balance equal to zero (and hence having no components), the VP might crash in the process of unwrapping.

More generally to prevent potential exploits, we should probably try (whether here or in the port) to enforce the 4 conditions at https://github.com/anoma/namada/blob/main/documentation/specs/src/masp/ledger-integration.md#boundary-conditions . They serve to ensure that the outer Transfer object is consistent with the inner Transaction in terms of the numbers and fields of the transparent inputs/outputs. I think we haven't really had an issue here yet because the CLI client has been well behaved so far.

@murisi
Copy link
Collaborator

murisi commented May 15, 2023

yes; but we were discussing porting it to native, which would be so much less flaky

@juped I'm not sure what the status of this is, but I've identified some potential further issues in the MASP VP code as it currently stands: #1373 .

@cwgoes cwgoes closed this as completed Feb 2, 2024
@github-project-automation github-project-automation bot moved this from Todo to Tested in Devnet in Namada-Old Feb 2, 2024
phy-chain pushed a commit to phy-chain/namada that referenced this issue Mar 1, 2024
* Manually copied over the changes from feat/47_staking_gov_pgf, as there
  had been a big refactor and this could not be merged automatically
* Had to add containers as we have `Appcomponents__ContentContainer`
  which is not display: flex and in the account views things would break
  in the current form, if this was to be changed to flex. Should be
  refactored though
* Deleted left over files from merge
* creating Table component
* navigation and main components in Staking view
* anoma#55 Staking and Governance State (anoma#56)
* Initial files for staking and governance state
* created types in Redux
* moving fake data and table configurations away from a file next to the component
* validator data through action and Redux
* changed the way how to pass callbacks to table rows
* removed console logs and added comments to indicate upcoming functionality
* put back the placeholder view elements to new routes
* Fixes based on PR feedback
* Changed naming on PR#54 feedback
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Tested in Devnet
Development

No branches or pull requests

4 participants