search

0xPolygonMiden / miden-client

Client library that facilitates interaction with the Miden rollup
MIT License
32 stars 28 forks source link

feat: `NoteWithProof` follow ups #485

Open tomyrd opened 1 month ago

tomyrd commented 1 month ago
  1. We should make sure that if we import a note with proof, we either verify that it exists immediately or during the next sync.
  2. Handle cases where the imported proof is invalid.

_Based on the post by @bobbinth in https://github.com/0xPolygonMiden/miden-client/pull/449#discussion_r1722095139_

tomyrd commented 1 month ago

I have some questions regarding this:

  1. Should these two validations be made at the same time? i.e verifying the note's existance and the proof's validity at the same time
  2. To verify if a note exists, should we use the GetNoteById endpoint? If so, then there wouldn't be a significant difference between NoteFile::NoteWithProof and NoteFile::NoteId
bobbinth commented 1 month ago

To verify if a note exists, should we use the GetNoteById endpoint? If so, then there wouldn't be a significant difference between NoteFile::NoteWithProof and NoteFile::NoteId

We could do this, but this could be more expensive than needed. This is because if we have the note inclusion proof, we only need to get the block inclusion proof (i.e., block header + MMR proof) to check if the note is in the chain. This is also good for privacy as we'd be asking the network for a block rather than a specific note.

Should these two validations be made at the same time? i.e verifying the note's existance and the proof's validity at the same time

Not sure I fully understand the question, but I think we can preform all validations at sync time. Basically, I'm imagining that we have a set of notes with note inclusion proofs but without the associated block inclusion proofs in the store. After we've synced to the chain tip, we'd get a list of these blocks and will request the node to send us MMR proofs for these blocks (via the GetBlockHeaderByNumber endpoint).

Once we get the headers and the associated proofs, we'd insert them into the store and also check if the note inclusion proofs match the block headers we got (e.g., we can verify the Merkle paths against the note_root of the relevant block header). If there is a mismatch, we should maker a note as "invalid" somehow and probably include it in the SyncSummary.

btw, I think we probably should refactor SyncSummary to contain the actual IDs of new notes, new nullifiers etc. (instead of just counts).

tomyrd commented 1 month ago

we should maker a note as "invalid" somehow and probably include it in the SyncSummary

What should be done with these invalid notes. Keep them tracked in the client but separated from the rest? (like what happens with ignored notes)

bobbinth commented 1 month ago

What should be done with these invalid notes. Keep them tracked in the client but separated from the rest? (like what happens with ignored notes)

Yes - but also I'm thinking we should allow users to delete such notes (via a separate function). More generally though, I wonder if we should refactor the input_notes table again to make sure we handle all possible note states holistically.

igamigo commented 1 month ago

btw, I think we probably should refactor SyncSummary to contain the actual IDs of new notes, new nullifiers etc. (instead of just counts).

Created #512 for this