Add support for on-chain notes to `block-producer` #303

phklive · 2024-04-05T21:39:48Z

In this PR I propose to update the block-proposer to enable addition of on-chain notes to the store through apply_block api

Closes: #141

+ add dabase to gitignore

phklive · 2024-04-06T10:58:30Z

@hackaugusto as we discussed note and tx validation together a few days ago I have the feeling that it is a full other PR in of itself. Should we open another issue to address that thoroughly?

phklive · 2024-04-06T11:00:54Z

@bobbinth Indeed I wasn't fully getting the extent of the trouble we could get into when using the Miden client as a dep for faucet or end-to-end testing. Here is an example where not having a dep on the client to send ProvenTransactions to the Block-Producer for testing could be very useful.

That should be something to address: #291

igamigo · 2024-04-06T21:43:08Z

block-producer/src/batch_builder/batch.rs

+            let created_notes: Vec<NoteCreated> = txs
+                .iter()
+                .flat_map(|tx| {
+                    tx.output_notes().iter().map(|note| {
+                        if let Some(note_with_details) = tx.get_output_note_details(&note.note_id())
+                        {
+                            NoteCreated {
+                                batch_index: 0,
+                                note_index: 0,
+                                note_id: Some(note.note_id().into()),
+                                sender: Some(note.metadata().sender().into()),
+                                tag: note.metadata().tag().into(),
+                                details: Some(note_with_details.to_bytes()),
+                            }
+                        } else {
+                            NoteCreated {
+                                batch_index: 0,
+                                note_index: 0,
+                                note_id: Some(note.note_id().into()),
+                                sender: Some(note.metadata().sender().into()),
+                                tag: note.metadata().tag().into(),
+                                details: None,
+                            }
+                        }
+                    })
+                })
+                .collect();
+


This will likely have to change after 0xPolygonMiden/miden-base#572. Maybe it's best to point at that branch for now to not have to redo this soonish.

This implementation missing note_index field within the batch.
I would write something like this:

let created_notes: Vec<NoteCreated> = txs .iter() .flat_map(|tx| { tx.output_notes().iter().map(|note| { (note, tx.get_output_note_details(&note.note_id()) }) }) .enumerate() .map(|(note_index, (note, details))| NoteCreated { batch_index: 0, note_index: note_index as u32, note_id: Some(note.note_id().into()), sender: Some(note.metadata().sender().into()), tag: note.metadata().tag().into(), details: details.map(Note::to_bytes), }) .collect();

Thanks for the heads up, fixed the implementation.

Dominik1999 · 2024-04-10T08:09:28Z

Nice, only this is remaining, I suppose?

phklive · 2024-04-10T08:53:12Z

Nice, only this is remaining, I suppose?

Yes exactly, rebasing atm.

phklive · 2024-04-10T09:58:47Z

This PR has been rebased on continued here: #310

polydez · 2024-04-10T09:00:39Z

block-producer/src/batch_builder/batch.rs

@@ -26,9 +27,10 @@ pub struct TransactionBatch {
    id: BatchId,
    updated_accounts: BTreeMap<AccountId, AccountStates>,
    produced_nullifiers: Vec<Nullifier>,
-    created_notes_smt: BatchNoteTree,
    /// The notes stored `created_notes_smt`


It seems this comment is not in the right place now. And I think, it is confusing a bit, maybe it would be better to get rid of this comment at all?

That is also what I thought, I did not know if I had to remove it.

Removed.

polydez · 2024-04-10T09:20:41Z

block-producer/src/batch_builder/batch.rs

+            let created_notes: Vec<NoteCreated> = txs
+                .iter()
+                .flat_map(|tx| {
+                    tx.output_notes().iter().map(|note| {
+                        if let Some(note_with_details) = tx.get_output_note_details(&note.note_id())
+                        {
+                            NoteCreated {
+                                batch_index: 0,
+                                note_index: 0,
+                                note_id: Some(note.note_id().into()),
+                                sender: Some(note.metadata().sender().into()),
+                                tag: note.metadata().tag().into(),
+                                details: Some(note_with_details.to_bytes()),
+                            }
+                        } else {
+                            NoteCreated {
+                                batch_index: 0,
+                                note_index: 0,
+                                note_id: Some(note.note_id().into()),
+                                sender: Some(note.metadata().sender().into()),
+                                tag: note.metadata().tag().into(),
+                                details: None,
+                            }
+                        }
+                    })
+                })
+                .collect();
+


This implementation missing note_index field within the batch.
I would write something like this:

let created_notes: Vec<NoteCreated> = txs .iter() .flat_map(|tx| { tx.output_notes().iter().map(|note| { (note, tx.get_output_note_details(&note.note_id()) }) }) .enumerate() .map(|(note_index, (note, details))| NoteCreated { batch_index: 0, note_index: note_index as u32, note_id: Some(note.note_id().into()), sender: Some(note.metadata().sender().into()), tag: note.metadata().tag().into(), details: details.map(Note::to_bytes), }) .collect();

polydez · 2024-04-10T09:41:21Z

block-producer/src/batch_builder/batch.rs

-    created_notes: Vec<NoteEnvelope>,
+    created_notes_smt: BatchNoteTree,
+    created_notes: Vec<NoteCreated>,
+    created_note_envelopes: Vec<NoteEnvelope>,


Do we need these envelopes anywhere else?

polydez · 2024-04-10T09:51:01Z

block-producer/src/block_builder/mod.rs

@@ -79,14 +80,13 @@ where

        let updated_accounts: Vec<_> =
            batches.iter().flat_map(TransactionBatch::updated_accounts).collect();
-        let created_notes = batches
+        let created_notes: Vec<(usize, usize, NoteCreated)> = batches


We don't need these two additional usizes, because we already have batch_index and note_index fields in NoteCreated structure. The task is to fill them here. I would probably set note_index in the batch builder (as in my proposed piece of code) and batch_index here.

I think, the best solution would be to not to create NoteCreated in the batch builder, but just to add serialized details field in the TransactionBatch::created_note_envelopes vector:

created_note_envelopes_with_details: Vec<(NoteEnvelope, Option<Vec<u8>>)>,

And after that we would be able to produce NoteCreated here and init its batch_index and note_index fields during creation.

polydez · 2024-04-10T10:09:03Z

block-producer/src/block.rs

@@ -19,7 +19,7 @@ use crate::store::BlockInputsError;
 pub struct Block {
    pub header: BlockHeader,
    pub updated_accounts: Vec<UpdatedAccount>,
-    pub created_notes: Vec<(usize, usize, NoteEnvelope)>,
+    pub created_notes: Vec<(usize, usize, NoteCreated)>,


If we put NoteCreated here, we don't need to provide additionally batch_index and note_index here.

polydez · 2024-04-10T10:09:35Z

proto/src/domain/notes.rs

@@ -20,14 +18,15 @@ impl From<note::Note> for note::NoteSyncRecord {
 // NoteCreated
 // ================================================================================================

-impl From<(usize, usize, NoteEnvelope)> for note::NoteCreated {
-    fn from((batch_idx, note_idx, note): (usize, usize, NoteEnvelope)) -> Self {
+impl From<(usize, usize, NoteCreated)> for note::NoteCreated {


I'm not sure we need such conversion.

polydez · 2024-04-10T10:11:18Z

store/src/db/migrations.rs

@@ -24,6 +24,7 @@ pub static MIGRATIONS: Lazy<Migrations> = Lazy::new(|| {
            sender INTEGER NOT NULL,
            tag INTEGER NOT NULL,
            merkle_path BLOB NOT NULL,
+            details BLOB NOT NULL,


Please, consider making details nullable.

phklive and others added 27 commits April 4, 2024 11:11

Getting notes by id works

7c09397

lint and test

24c05ab

Improved comments + Added conversion from digest::Digest to NoteId

c52dafe

Added rpc validation for NoteId's

626cb54

Added documentation to Readmes

0d80b8b

Lint

0335339

Merge next + lint

1a4a829

Added details field to database

0c25238

Fixed ci problems

e0572ba

Merge branch 'phklive-get-notes-by-id' into phklive-db-onchain-note

9fd74e3

Added details to Note type

443795e

Added test for endpoint + refactored sql query + improved documentation

a6bd2ea

Merge branch 'phklive-get-notes-by-id' into phklive-db-onchain-note

8ce4963

Fixed lint

a32b312

Merge branch 'phklive-get-notes-by-id' into phklive-db-onchain-note

2b037a2

Fixed ci with test problem

895a7a2

lint

7907be9

Fix nit in migration

9daef27

+ add dabase to gitignore

Merge branch 'next' into phklive-db-onchain-note

fd34c43

Order rpc and store endpoints alphabitically

f9b4a0b

Change name to database to prevent gitignore problems

f0e7e1a

Change name to database to prevent gitignore problems

544473d

Merge branch 'next' into phklive-get-notes-by-id

3c5a9fd

Merge branch 'phklive-get-notes-by-id' into phklive-db-onchain-note

f4ec7e5

generated

5ff0403

Updated block + added details to NoteCreated instead of NoteEnvelope

c1fdaaa

node runs, able to produce blocks, modified apply_block

d36c140

phklive requested review from hackaugusto, igamigo and bobbinth April 6, 2024 00:18

phklive requested a review from polydez April 6, 2024 00:18

phklive marked this pull request as ready for review April 6, 2024 00:18

phklive changed the base branch from next to phklive-db-onchain-note April 6, 2024 21:19

phklive mentioned this pull request Apr 6, 2024

Add support for on-chain notes to the database #300

Merged

igamigo reviewed Apr 6, 2024

View reviewed changes

Improved documentation with gh comments

f0c0af1

Base automatically changed from phklive-db-onchain-note to next April 10, 2024 07:39

phklive closed this Apr 10, 2024

polydez requested changes Apr 10, 2024

View reviewed changes

bobbinth deleted the phklive-bp-support-onchain-notes branch April 29, 2024 07:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for on-chain notes to `block-producer` #303

Add support for on-chain notes to `block-producer` #303

phklive commented Apr 5, 2024

phklive commented Apr 6, 2024

phklive commented Apr 6, 2024

igamigo Apr 6, 2024

polydez Apr 10, 2024

phklive Apr 10, 2024

Dominik1999 commented Apr 10, 2024

phklive commented Apr 10, 2024

phklive commented Apr 10, 2024

polydez Apr 10, 2024

phklive Apr 10, 2024

polydez Apr 10, 2024

polydez Apr 10, 2024

polydez Apr 10, 2024

polydez Apr 10, 2024

polydez Apr 10, 2024

polydez Apr 10, 2024

Add support for on-chain notes to block-producer #303

Add support for on-chain notes to block-producer #303

Conversation

phklive commented Apr 5, 2024

phklive commented Apr 6, 2024

phklive commented Apr 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dominik1999 commented Apr 10, 2024

phklive commented Apr 10, 2024

phklive commented Apr 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add support for on-chain notes to `block-producer` #303

Add support for on-chain notes to `block-producer` #303