MASP nullifier uniqueness #2240

grarco · 2023-11-30T15:32:10Z

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`

I have added a changelog
Git history is in acceptable state

murisi

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?

shared/src/ledger/native_vp/masp.rs

murisi · 2023-12-01T07:55:56Z

shared/src/ledger/native_vp/ibc/context.rs

@@ -237,6 +239,19 @@ where
        );
        self.write(&current_tx_key, record.serialize_to_vec())?;
        self.write(&head_tx_key, (current_tx_idx + 1).serialize_to_vec())?;
+        for description in shielded


I see a bit of duplication in how we update the nullifier set in the transaction codes. But maybe getting rid of it is easier said than done and can be deferred to a later PR?

I've tried to improve the thing with a shared function now

grarco · 2023-12-01T11:54:32Z

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

murisi

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?

shared/src/ledger/native_vp/masp.rs

murisi · 2023-12-04T06:36:57Z

shared/src/ledger/native_vp/ibc/context.rs

@@ -237,6 +238,7 @@ where
        );
        self.write(&current_tx_key, record.serialize_to_vec())?;


I think this is a bug? We call serialize_to_vec on (Epoch, BlockHeight, TxIndex, Transfer, Transaction) to obtain a Vec<u8>. And then StorageWrite::write itself calls serialize_to_vec again on the Vec<u8> argument it receives producing another Vec<u8>, but this time prefixed with 4 bytes indicating the length of record.serialize_to_vec(). We should probably change this line to self.write(&current_tx_key, record)?; so that we remain compatible with how we write to the same key in /tx_prelude/src/token.rs. No?

murisi · 2023-12-04T06:43:16Z

shared/src/ledger/native_vp/ibc/context.rs

@@ -237,6 +238,7 @@ where
        );
        self.write(&current_tx_key, record.serialize_to_vec())?;
        self.write(&head_tx_key, (current_tx_idx + 1).serialize_to_vec())?;


Same serialize_to_vec issue as above?

murisi · 2023-12-04T07:01:50Z

tx_prelude/src/token.rs

@@ -60,6 +61,7 @@ pub fn handle_masp_tx(
    );
    ctx.write(&current_tx_key, record)?;
    ctx.write(&head_tx_key, current_tx_idx + 1)?;
+    masp_utils::reveal_nullifiers(ctx, shielded)?;


This deduplication will do. But note that the contexts in which masp_utils::reveal_nullifiers is called are essentially identical. So I am wondering if there is a way to reuse handle_masp_tx wholesale? Like can't we move this whole handle_masp_tx function into core/src/ledger/masp_utils.rs and call it like namada_core::ledger::masp_utils::handle_masp_tx(ctx, transfer, shielded) here and namada_core::ledger::masp_utils::handle_masp_tx(self, shielded.transfer, shielded.masp_tx) from shared/src/vm/host_env.rs and shared/src/ledger/native_vp/ibc/context.rs?

grarco · 2023-12-04T13:03:12Z

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

murisi

Thanks for updating again. Looks good to me!

* origin/grarco/nullifier-uniqueness: Refactors `handle_masp_tx` Reduce code duplication to reveal masp nullifiers Fixes masp vp nullifier validation Changelog #2240 Removes wrong comment Masp VP checks uniqueness of nullifiers Updates masp tx to reveal nullifiers

grarco added 4 commits November 30, 2023 11:24

Updates masp tx to reveal nullifiers

d8b5114

Masp VP checks uniqueness of nullifiers

9f855dc

Removes wrong comment

5cc290c

Changelog #2240

9ed6d52

grarco marked this pull request as ready for review November 30, 2023 18:25

grarco requested a review from murisi November 30, 2023 18:25

murisi requested a review from juped December 1, 2023 06:23

murisi requested changes Dec 1, 2023

View reviewed changes

grarco added 2 commits December 1, 2023 12:05

Fixes masp vp nullifier validation

f1156c1

Reduce code duplication to reveal masp nullifiers

144536f

murisi previously approved these changes Dec 4, 2023

View reviewed changes

grarco dismissed murisi’s stale review via 65d031a December 4, 2023 11:45

Refactors handle_masp_tx

39e00c5

grarco force-pushed the grarco/nullifier-uniqueness branch from 65d031a to 39e00c5 Compare December 4, 2023 12:03

grarco requested a review from murisi December 4, 2023 12:59

murisi approved these changes Dec 4, 2023

View reviewed changes

grarco added the ready for draft label Dec 4, 2023

This was referenced Dec 4, 2023

Draft 0.28 #2203

Closed

Spend descriptions validation #2244

Merged

tzemanovic mentioned this pull request Dec 7, 2023

Draft #2252

Closed

grarco added the merged to draft label Dec 11, 2023

tzemanovic added a commit that referenced this pull request Dec 12, 2023

fixup! Merge branch 'grarco/nullifier-uniqueness' (#2240)

bd4374b

brentstone added a commit that referenced this pull request Dec 29, 2023

fixup! Merge branch 'grarco/nullifier-uniqueness' (#2240)

67c94f2

brentstone merged commit 48d5e32 into main Dec 29, 2023
12 of 14 checks passed

brentstone deleted the grarco/nullifier-uniqueness branch December 29, 2023 19:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MASP nullifier uniqueness #2240

MASP nullifier uniqueness #2240

grarco commented Nov 30, 2023 •

edited

Loading

murisi left a comment

murisi Dec 1, 2023

grarco Dec 1, 2023

grarco commented Dec 1, 2023

murisi left a comment

murisi Dec 4, 2023

murisi Dec 4, 2023

murisi Dec 4, 2023

grarco commented Dec 4, 2023 •

edited

Loading

murisi left a comment

		@@ -237,6 +238,7 @@ where
		);
		self.write(&current_tx_key, record.serialize_to_vec())?;

MASP nullifier uniqueness #2240

MASP nullifier uniqueness #2240

Conversation

grarco commented Nov 30, 2023 • edited Loading

Describe your changes

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

Checklist before merging to draft

murisi left a comment

Choose a reason for hiding this comment

murisi Dec 1, 2023

Choose a reason for hiding this comment

grarco Dec 1, 2023

Choose a reason for hiding this comment

grarco commented Dec 1, 2023

murisi left a comment

Choose a reason for hiding this comment

murisi Dec 4, 2023

Choose a reason for hiding this comment

murisi Dec 4, 2023

Choose a reason for hiding this comment

murisi Dec 4, 2023

Choose a reason for hiding this comment

grarco commented Dec 4, 2023 • edited Loading

murisi left a comment

Choose a reason for hiding this comment

grarco commented Nov 30, 2023 •

edited

Loading

Checklist before merging to `draft`

grarco commented Dec 4, 2023 •

edited

Loading