Note importing process follow-ups

[igamigo]

Some things to fix came up when testing #375:

There are some minor issues to revisit but I think overall it's working as expected and we can tackle them in a follow up PR:

• note should show whether it is ignored or not (easier for the user of the CLI to track)
• if I export a committed note as partial, then I consume it and I import it in another client, the note goes from Expected > => Committed but never gets marked as consumed
  - also, every sync says:
  2024-06-28T18:57:40.207197Z WARN miden_client::client::sync: Current partial MMR already contains the requested data State synced to block 167 New public notes: 0 Tracked notes updated: 1 Tracked notes consumed: 0 Tracked accounts updated: 0 Commited transactions: 0
  - ... but no notes got actually updated
  - this only happens for partial notes, since full notes are never ignored and id notes can't be imported (if they're private)
• I tried exporting a public committed note with export type id, then consumed the note, then imported it in another client and it failed.

Originally posted by @mFragaBA in #375 (review)

However, some of this might depend on how we rework the note importing workflow.

[mFragaBA]

• if I export a committed note as partial, then I consume it and I import it in another client, the note goes from Expected > => Committed but never gets marked as consumed

The main reason for this is that currently we can sync before importing the note and we might miss the nullifier update (the SyncStateResponse contains the nullifiers between request.block_num + 1 and response.block_header.block_num), so once we sync and then import the note we can't go back. This issue will happen for both ignored and non ignored notes.

I don't think there's a good way to solve this issue right now. We could:

• if an ignored note gets committed during an ignored notes sync, we could do a sync request to the node between the block the note was included in and the current chain tip (we wouldn't actually update the client state as a regular sync, we'd just update the nullifiers) and if the nullifier is there, we'd change the note status to consumed
  • The issue would still persist for non ignored notes though.
• when importing NoteFile::NoteDetails(...), check if the note is already committed (and set the status accordingly) and if so do the trick from above (and set the status accordingly).
  • however, we'll end up with a more complex algorithm, and the logic is scattered around multiple places. Proper documentation would be crucial
• If we were to have an endpoint to validate whether a list of notes have been consumed / nullifiers are present in the nullifier SMT, we could add a single step after both the normal sync and the ignored notes sync where we see if any of those got consumed.

cc @bobbinth

[bobbinth]

If we were to have an endpoint to validate whether a list of notes have been consumed / nullifiers are present in the nullifier SMT, we could add a single step after both the normal sync and the ignored notes sync where we see if any of those got consumed.

We do have a CheckNullifiers endpoint in the node - thought, it requires sending full nullifiers and returns back authentication paths for the nullifiers found in the node. So, this is a bit more heaving then needed and also reduced privacy. Maybe it is worth adding an endpoint to the node to return a list of nullifiers based on some prefix and block bounds. Could you create an issue for this in miden-node?

More generally, I think that maybe the root cause of the issues is that we are trying to do too much with the import note endpoint. Specifically, we try to cover 3 separate use cases:

1. Importing notes which already have inclusions proofs. By itself, handling these notes should be pretty straight-forward:
   a. If the note is "in the past" as compared to where the client currently is, immediately request the block header with the corresponding MMR path. Also, check if the nullifier for this note exists. Here, we need to be careful not to check for exact nullifiers as it may allow correlating nullifiers with notes created in the block.
   b. If the note is "in the future", request the block header for this note after the next sync (assuming we don't get it naturally). Also, add the nullifier prefix to the list of prefixes to be requested during the next sync.
2. Importing notes by ID. Also and easy case to handle: we immediately make a request to the node and if we get the note back, we treat it in the same way as case 1 above. If the note does not exist yet, we error out.
3. Importing "expected" notes. I think this is the most complicated case to handle because: (1) we don't know if the note is in the past or in the future, and (2) we don't know how long we should "wait" for the note. Also, the strategies would be different for notes which match existing client tags vs. notes that don't (i.e., tracked vs. untracked notes).

I'm not entirely happy with how we handle case 3, but I also don't have great suggestions here. The issues I'm seeing:

• For tracked notes, it is not clear how to handle notes that are "in the past". Currently, we leave this up to the user, but I think there should be better strategy here.
• For untracked notes, we could handle them similar to note IDs (i.e., make an immediate request to the node), but if the note is not in the chain yet, not clear how long we should keep trying to get it. I think we also leave this up to the user, and similarly, I think there could be a better strategy here.

[igamigo]

I think one way of simplifying the nullifier flow could be to make the sync endpoint return nullifier data regardless of the [request.block_num, response.block_num] block range. I don't think there's much penalty for this and nullifiers would then be handled by the sync on all cases; although it could be more confusing than useful if we can just move that data to a different endpoint.

[bobbinth]

I think one way of simplifying the nullifier flow could be to make the sync endpoint return nullifier data regardless of the [request.block_num, response.block_num] block range. I don't think there's much penalty for this and nullifiers would then be handled by the sync on all cases; although it could be more confusing than useful if we can just move that data to a different endpoint.

I think this would introduce quite a bit of overhead:

• In the node, we'd need to search through the entire set of nullifiers to find the relevant ones. While the nullifiers will be indexed, the number of nullifiers will be pretty large so even that may become quite expensive. With the current approach the node could shard the database (e.g., each shard could contain 2 weeks worth of blocks) and then searching could be done only in relevant shards.
• The sync_state response would contain a lot of duplicate data. Basically, for every 16-bit nullifier prefix, we'd get a lot of the same nullifiers for every request. In fact, I'd think that the vast majority of the returned nullifiers would be the same across multiple requests.

[bobbinth]

Following up on #405 (comment), I think we should first try to solve case "3a" - i.e., tracked expected notes - as it is probably by far the more likely one to happen in practice.

When we import such a note, ideally, we'd like to know the following:

• Has this note already been recorded on-chain.
• Has this note already been consumed.

I think we have the following options for getting this info:

1. Make the requests to the node using the exact note ID and nullifier. This would guarantee that we get the most accurate info, but has pretty bad implications for privacy. Specifically:
   a. Using the exact note ID leaks info that the user is potentially interested in a specific note. Given that note sender info is public, this may allow linking sender and recipient.
   b. Using the exact note nullifier by itself does not leak much info (or at least not much additional info as nullifiers are sent to the node when users consume notes in transactions anyway) - but making a request by note ID and very close to it checking a nullifier for a given note will allow the node to link note IDs and nullifiers, and this should be avoided.
   c. Separately, now that I think of it, we could probably increase the nullifier prefix that we send to the network (e.g., during state sync etc.) quite a bit - e.g., could be 32 bits or more and it shouldn't impact privacy too much.
2. Re-sync the client starting at some point in the past. This would basically simulate the note being "in the future" for the client. Since the note is already tracked, we won't have to modify much for the state sync requests. The are two drawbacks of this approach:
   a. It is not clear from which point we should re-sync. Though, this could be a client-specific setting (e.g., re-sync last week, or last month, or last year).
   b. We may get a lot of extra unrelated data (i.e., the data we've already received about account updates and nullifiers).
3. Have some specialized endpoints for handling this situation. This would be similar to what we are discussing in [Feature]: Add an endpoint to check for nullifiers given block bounds and prefixes  miden-node#404.

To summarize some of the above:

• Making requests by exact note IDs is probably not a good idea and we should try to avoid this (this applies to private notes only).
• Making requests for exact nullifiers may be OK, but we should be careful not to correlate requests for notes with request for nullifies. In this context, it may make sense to use nullifier prefixes (16 bits for now, and maybe increasing later).

Thinking more about the approach 3 above, we could do something like this:

To check if a note has been consumed, use a specialized endpoint which would take a set of 16-bit nullifier prefixes and would return a list of nullifiers matching these prefixes regardless of when these nullifiers have been generated.

To check if a note has been already recorded on chain, we use a specialized endpoint which could like like a "light sync" (basically, state sync but only for notes). We could call this endpoint SyncNotes (or something like that). This endpoint would take a list of note tags, and would return a list of notes found in a given period. Then, in the client, we'll make the "look-back" period configurable (and default it to something like 1 month) - though, the configuration would probably be defined by the CLI rather than client.

[bobbinth]

One other option is to make sure that expected notes always come with some lower bound on when they could be added to the chain. For example, NoteFile could be defined like so:

pub enum NoteFile {
    NoteId(NoteId),
    NoteDetails { details: NoteDetails, after_block_num: u32, tag: Option<Tag> },
    NoteWithProof(Note, NoteInclusionProof),
}

The after_block_num field in the NoteDetails variant would specify the block number after which the note is expected to be added to the chain. Then, based on this field the client could either defer retrieving of the note to the SyncState (if after_block_num is greater than the last block the client is aware of) or do the SyncNotes starting with the after_block_num block.

One thing to keep in mind though: after_block_num is more like a "hint" since there is no guaranteed that the creator of the NoteFile fills it in correctly. As such, we should make sure that it is not set too far in the past (or at least show some kind of a warning if that's the case).

[tomyrd]

With the addition of the CheckNullifiersByPrefix endpoint the problem of "Has this note already been consumed?" is solved as we can ask for recorded nullifiers without giving away it's information. I've implemented a solution for the two easier cases.

For the NoteFile::NoteDetails the question of "Has this note already been recorded on-chain?" remains. I like the idea of the light sync (SyncNotes) that with the addition of the after_block_num field we can use to know if a note is committed. Once the endpoint is impelmented I can get to work with this.

Small question: Given that the after_block_num is more of a "hint". What should happen if it states that the note is from the future but it's actually is from the past? I think this case is a bit of an extreme but we should be aware of it.

This will make the import_note more complex that we originally anticipated, we should probably also look to tackle #393 at the same time and find a way to make it more legible and clear.

[bobbinth]

Given that the after_block_num is more of a "hint". What should happen if it states that the note is from the future but it's actually is from the past? I think this case is a bit of an extreme but we should be aware of it.

I think the other case here is that the not was committed in the past but not as far back as after_block_num suggested (e.g., client is currently at block 20, after_block_num = 10 but the note was committed in block 5).

I think this case could be handled separately. Specifically, we should probably do something about "stale" expected notes - i.e., notes which we have been expecting "for a while". For example, if an expected note didn't get committed within a week, we try to get it by ID. But this would be a separate issue (i.e., something about "handling of stale notes").

[igamigo]

The after_block_num field in the NoteDetails variant would specify the block number after which the note is expected to be added to the chain. Then, based on this field the client could either defer retrieving of the note to the SyncState (if after_block_num is greater than the last block the client is aware of) or do the SyncNotes starting with the after_block_num block.

You might have been referring to tracked notes here, but we can't always assume that if an expected note is in the future, the normal SyncState will be responsible of retrieving it, no? It depends entirely on whether the correct tags are requested.

[bobbinth]

You might have been referring to tracked notes here, but we can't always assume that if an expected note is in the future, the normal SyncState will be responsible of retrieving it, no? It depends entirely on whether the correct tags are requested.

Yes, this is specifically what I called the case 3a (tracked expected notes). I haven't thought through the case 3b (untracked expected notes) - but also these should be quite rare.

[tomyrd]

Importing NoteWithProof or NoteId notes which are already consumed is addressed in #449
Importing NoteDetails which is committed in the past is addressed in #472