feat: import `NoteFile` in client #375

tomyrd · 2024-06-04T15:05:20Z

closes #371

This PR implements the required logic to handle each type of imported NoteFile. Broadly, here's how the client handles each type:

NoteId: This notes are fetched from the node via GetNotesById (only if public) with it's metadata and inclusion proof. If the note was already tracked then its updated with the node information.
NoteDetails+Option<Tag>: The concept of "ignored" notes is introduced. If the note imported via NoteDetails has a tag that was already being tracked then it behaves like a normal Expected imported note, updated after sync. If the tag is not tracked (or is None) then the note is "ignored", this means it's not updated with the rest of the notes via sync. These notes are updated "manually" by fetching the metadata+inclusion proof from the node via GetNotesById. Notes can have their ignored status removed if the tag that was imported with them starts being tracked.
NoteWithProof: These notes are imported as committed notes without fetching the mmr data. This means that they are committed but they can't be consumed. This required changing the sync so that it updates mmr information for committed notes that don't have it.

A more in depth description of each case can be found in this comment and in the issue discussion.

igamigo

Looking good! I left some comments, some related to code structure. Most of them don't need to be tackled now and can be left for a follow-up issue.

src/cli/export.rs

src/cli/import.rs

src/client/notes.rs

src/client/sync.rs

src/client/notes.rs

mFragaBA

We should also test the flow for ignored notes (I think we have at least one test covering each tracked node variant but don't have any for ignored notes)
We may probably need a PR to update documentation in miden Base as well.

src/client/sync.rs

src/store/mod.rs

mFragaBA · 2024-06-25T17:42:10Z

src/store/mod.rs

    #[maybe_async]
-    fn insert_input_note(&self, note: &InputNoteRecord) -> Result<(), StoreError>;
+    fn insert_input_note(&self, note: InputNoteRecord) -> Result<(), StoreError>;
+
+    /// Updates the inclusion proof of the input note with the provided ID
+    fn update_note_inclusion_proof(
+        &self,
+        note_id: NoteId,
+        inclusion_proof: NoteInclusionProof,
+    ) -> Result<(), StoreError>;
+
+    /// Updates the metadata of the input note with the provided ID
+    fn update_note_metadata(
+        &self,
+        note_id: NoteId,
+        metadata: NoteMetadata,
+    ) -> Result<(), StoreError>;


I wouldn't do it in this PR, but it might be better to have a single update_note in the public API

At first I implemented it this way but I felt like we could end up with an invalid state. Maybe it's better if we join them as what was stated in this comment.

why would you end up with an invalid state?

For example, having an inclusion proof but with an "Expected" status

src/store/sqlite_store/mod.rs

src/store/sqlite_store/notes.rs

mFragaBA · 2024-06-25T17:58:02Z

src/store/sqlite_store/notes.rs

@@ -246,7 +253,7 @@ impl SqliteStore {
        Ok(notes)
    }

-    pub(crate) fn insert_input_note(&self, note: &InputNoteRecord) -> Result<(), StoreError> {
+    pub(crate) fn insert_input_note(&self, note: InputNoteRecord) -> Result<(), StoreError> {


why did we change from reference to owned?

I took the idea from the leftover notes PR. I feel like the insert_input_note should consume the InputNoteRecord when inserting. This is because internally, the note record is stored with some additional information that the original InputNoteRecord may not have so the user may end up using it instead of the note you get from get_input_note. Maybe it doesn't make too much sense, I could revert the decision if that's the case.

Yeah, I think it makes sense that this consumes the note. I think maybe originally the motivation was to avoid some cloning. I don't mind if we want to roll this back though, we might have to make more changes to the API to make this more consistent all the way.

I'm ok either way, but we should document that behavior then. We shouldn't expect that a user just because the input note gets consumed won't clone the note beforehand and use the outdated note.

At least I imagine the situation:

I call the function and use the note afterwards

I get an error

I clone and don't get the error anymore

I work with the outdated note

Also, I agree with the fact that we should ideally standardize a way to interact with the API.

src/store/sqlite_store/notes.rs

igamigo

LGTM! Left some comments and notes for other reviewers but overall I think we can merge this and tackle any remaining work in future PRs.

igamigo · 2024-06-27T16:53:56Z

src/client/sync.rs

+        let expected_notes = maybe_await!(self.store.get_input_notes(NoteFilter::Expected))?;

-        let note_tags: Vec<NoteTag> = [account_note_tags, stored_note_tags, uncommited_note_tags]
-            .concat()
-            .into_iter()
-            .collect::<BTreeSet<NoteTag>>()
-            .into_iter()
+        let uncommited_note_tags: Vec<NoteTag> = expected_notes
+            .iter()
+            .filter_map(|note| note.metadata().map(|metadata| metadata.tag()))


I think we could remove this, no?

As discussed offline, this might still apply in some cases. I thought that uncommitted note tags could fall under one of the two:

Ignored (tag is not on the persistent tag list): Would be updated separately

Not ignored: Since tag is on the persistent tag list, these lines would not be needed because the persistent tag list is sent to the node anyway

But there could be cases where this does not fully apply (like notes created by transactions that wouldn't be ignored, or a case where the tag is removed after importing a note) and these cases are not currently covered by any other flow. So for completeness, I think we can leave this for now (and also considering that this would very likely be reworked in the future). ccing @bobbinth just in case

igamigo · 2024-06-27T16:59:27Z

src/store/mod.rs

+    fn update_note_inclusion_proof(
+        &self,
+        note_id: NoteId,
+        inclusion_proof: NoteInclusionProof,
+    ) -> Result<(), StoreError>;
+
+    /// Updates the metadata of the input note with the provided ID
+    fn update_note_metadata(
+        &self,
+        note_id: NoteId,
+        metadata: NoteMetadata,
+    ) -> Result<(), StoreError>;


@bobbinth @dagarcia7 these are the new methods in the store. There are some alternatives to adding these functions:

Using insert_input_note() to insert or update notes instead of assuming it's always a new note.

Adding a single update_input_note() that replaces the data for a single note ID with a new one instead of having 2 different functions.

Personally I think the first one is best but I see the value in having separate functions with clearer intentions.

igamigo · 2024-06-27T17:01:39Z

src/store/mod.rs

@@ -92,9 +92,46 @@ pub trait Store {
        nullifiers
    }

+    /// Returns the notes that don't have their block header tracked
+    #[maybe_async]
+    fn get_notes_without_block_header(&self) -> Result<Vec<InputNoteRecord>, StoreError> {


Note for @bobbinth @dagarcia7: This is a new store function as well. It has a default implementation so it should not be much of a blocker. An alternative to this could be to create a loose function instead of this being a store method, but having it in the store makes it easier to optimize for each instance.

For @tomyrd: We should document this trait function to mention how the default implementation works as we do in other functions.

igamigo · 2024-06-27T17:03:23Z

src/store/sqlite_store/notes.rs

@@ -246,7 +253,7 @@ impl SqliteStore {
        Ok(notes)
    }

-    pub(crate) fn insert_input_note(&self, note: &InputNoteRecord) -> Result<(), StoreError> {
+    pub(crate) fn insert_input_note(&self, note: InputNoteRecord) -> Result<(), StoreError> {


Yeah, I think it makes sense that this consumes the note. I think maybe originally the motivation was to avoid some cloning. I don't mind if we want to roll this back though, we might have to make more changes to the API to make this more consistent all the way.

igamigo · 2024-06-27T17:10:27Z

src/cli/sync.rs

+#[derive(Debug, Parser, Clone)]
+#[clap(about = "Sync this client with the latest state of the Miden network.")]
+pub struct SyncCmd {
+    /// If enabled, the ignored notes will also be updated by fetching them directly from the node.
+    #[clap(short, long)]
+    update_ignored: bool,
+}


For @bobbinth: It was not entirely clear from the issue discussion (unless I missed something) whether we wanted something like this flag or not, but considering whether we are OK with the CLI being more opinionated I think it's fine.

I don't think this flag was specifically mentioned in the issue but I felt like maybe updating ignored notes with each sync maybe defeated the purpose. I can remove it and always call client.update_ignored_notes for the sync command if we want.

I think being more opinionated on the CLI side is fine - so, no issues with keeping this param.

docs/cli-reference.md

src/client/notes.rs

mFragaBA

LGTM!

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.

igamigo

LGTM! Created an issue for follow-ups but we should be good to merge for now.

* Update import in cli to use `NoteFile` * Update CHANGELOG * Update cli export to use `NoteFile` * Fix build errors with file tag * Rename `import_input_note` to `import_note` * Implement `From<InputNoteRecord>` for `NoteFile` * Change `import_note` parameter to `NoteFile` * Fix integration tests * Implement `metadata` for `NoteDetails` * Add `get_notes_without_block_header` method to store * Add `ignored` and `imported_tag` to `InputNoteRecord` * Update `import_note` with new functionality * Fix integration tests * Update `ignored` field on note tag add * Update `export` to accept type of note file * Update MMR data for committed notes on sync * Update ignored notes on sync * Fix after rebase * Update tracked note when importing `NoteFile::NoteId` * Remove `NoteFile` conversion from `InputNoteRecord` * Check ephemeral tags when importing * Fix rebase * Fix integration tests * Remove tag from export cli command * Remove `--no-verify` flag from import cli command * Remove `InputNoteRecord` conversion on cli export * Update doc comments for `import_note` * Change suggestions * Add `maybe_await` * Update note when importing `NoteId` even if private * Only update ignored notes if specified in `sync` command * Change suggestions * Update `cli-reference` * Change suggestions * Add tests for ignored notes * Improve doc for `get_notes_without_block_header` * Change suggestions * Fix authentication hash calls for note * Temporarily change `NODE_BRANCH` in Makefile * force CI reset --------- Co-authored-by: igamigo <[email protected]>

tomyrd changed the title ~~feat: import NoteFilein client~~ feat: import NoteFile in client Jun 4, 2024

tomyrd mentioned this pull request Jun 4, 2024

Implement NoteFile importing/exporting #371

Closed

tomyrd force-pushed the tomyrd-import-notefile branch 2 times, most recently from 9df2da3 to 3ef36d9 Compare June 6, 2024 18:32

mFragaBA force-pushed the mFragaBA-next-0.4 branch from 2e1c42f to 2286e8d Compare June 6, 2024 19:13

mFragaBA mentioned this pull request Jun 6, 2024

Remove client dependency from faucet 0xPolygonMiden/miden-node#368

Merged

mFragaBA force-pushed the mFragaBA-next-0.4 branch 4 times, most recently from 3680cef to cd11167 Compare June 11, 2024 02:04

tomyrd force-pushed the tomyrd-import-notefile branch 2 times, most recently from 4bfedc8 to 9769be1 Compare June 14, 2024 16:16

mFragaBA force-pushed the mFragaBA-next-0.4 branch 2 times, most recently from 215f26a to e4a2565 Compare June 14, 2024 20:27

Base automatically changed from mFragaBA-next-0.4 to next June 14, 2024 21:37

Dominik1999 mentioned this pull request Jun 17, 2024

Expose get_notes_by_id() to selectively get a note from a miden node #354

Closed

tomyrd force-pushed the tomyrd-import-notefile branch 3 times, most recently from 4d74c9d to 48defb2 Compare June 19, 2024 00:47

igamigo reviewed Jun 19, 2024

View reviewed changes

This was referenced Jun 24, 2024

refactor: Add conversions for NoteRecordDetails #392

Merged

Refactor import_note in Client #393

Closed

tomyrd added 8 commits June 24, 2024 12:50

Update import in cli to use NoteFile

42b197a

Update CHANGELOG

13baaf5

Update cli export to use NoteFile

12b5c64

Fix build errors with file tag

4341075

Rename import_input_note to import_note

db7db62

Implement From<InputNoteRecord> for NoteFile

9208610

Change import_note parameter to NoteFile

657ea67

Fix integration tests

576749a

tomyrd added 2 commits June 25, 2024 12:46

Change suggestions

f34090c

Update cli-reference

51d2a8b

tomyrd force-pushed the tomyrd-import-notefile branch from bbfe6ad to 51d2a8b Compare June 25, 2024 15:46

mFragaBA reviewed Jun 25, 2024

View reviewed changes

tomyrd added 2 commits June 26, 2024 12:02

Change suggestions

1e9b811

Add tests for ignored notes

1446b37

igamigo approved these changes Jun 27, 2024

View reviewed changes

igamigo reviewed Jun 27, 2024

View reviewed changes

tomyrd and others added 3 commits June 27, 2024 15:01

Improve doc for get_notes_without_block_header

85016c0

Merge branch 'next' into tomyrd-import-notefile

8bfc30a

Merge branch 'next' into tomyrd-import-notefile

cfa4e4d