namada icon indicating copy to clipboard operation
namada copied to clipboard

MASP nullifier uniqueness

Open grarco opened this issue 1 year ago • 2 comments

Describe your changes

Addresses #1373.

Implements a nullifier set to prevent double spending in masp transactions. Updates the masp VP to check that the transactions writes the correct nullifiers and that these haven't already been revealed.

NOTE: testing this with an integration or e2e test is impossible cause we cannot force the submission of the tx if the builder fails to construct the shielded Transaction. We'd need to test this with a unit test (which I think would be the correct way regardless of the force issue) but at the moment we don't have the tools to write unit tests for the masp vp (at least to my knowledge)

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

v0.27.0

Checklist before merging to draft

  • [x] I have added a changelog
  • [x] Git history is in acceptable state

grarco avatar Nov 30 '23 15:11 grarco

Do you know why the unit and integration tests are failing in the CI? They seem to be timing out for some reason... Do they pass on your machine? Have you tried regenerating the MASP test fixtures maybe?

Both unit and integration tests pass on my machine without the need to rebuild the proofs

grarco avatar Dec 01 '23 11:12 grarco

Nice. Could you please just check out the serialize_to_vec issue in the comments to see if there is anything to it? And if you get the chance (but it's not strictly necessary), could you please try seeing if moving handle_masp_tx from tx_prelude/src/token.rs into core/src/ledger/masp_utils.rs is trivial or not?

Ok, I've moved the entire handle_masp_tx to the masp_utils file. This has also solved the double serialization issue (which I believe like you it was a bug). To be able to share this function also with the IBC context I had to remove the call to insert_verifier(masp_addr) from the function (otherwise I would have needed to implement TxEnv on the IBC context), which shouldn't be a problem because the tx writes some masp keys anyway so those should already trigger the vp.

I had to keep a wrapper inside the IbcStorageContext trait cause otherwise we would have needed to implement StorageRead for IbcActions which I think might be more work

grarco avatar Dec 04 '23 13:12 grarco