Extract notes via events issue #539 #572

hackaugusto · 2024-04-04T22:15:42Z

This PR is a little big, but mostly because of renaming of some of the existing structs.

Removed the ToEnvelope trait in favor of a simpler enum.
Rename Note to NoteConsumable. The goal is to make it a bit more clear how this differs from the other note types
- Split the recipient off to a NoteRecipient. The goal was to use this type to parse data from the advice map, in the event handler. Currently there isn't enough infrastructure to do that with traits, and because of time constraints I didn't add it. Nevertheless the separation aligns the struct type more closely to their semantics, and should make the code easier to follow, so I kept it around.
- Rename NoteAssets to NoteVault because we have been mixing the terms. I kinda regret this change, but rolling back seems like twice the work for no gain.
Changes the kernel code to emit an event, and an event handler to create the note
- The event handler checks the advice map, if there is data to create a consumable note in there, it is read. Otherwise a private note is created
- The above does not check the note's OnChain flag. That should probably be added with a follow up.
Adds serialization of miden code to and from Felts, so that is can be encoded and decoded to/from the advice map

hackaugusto · 2024-04-06T16:01:16Z

Cargo.toml

@@ -9,7 +9,7 @@ members = [

 [workspace.package]
 edition = "2021"
-rust-version = "1.75"
+rust-version = "1.77"


Bumped because split_first_chunk was stabilized on 1.77. This API is used together with u32::from_le_bytes to encode the NoteScript to be used with the advice_map.

hackaugusto · 2024-04-06T16:04:33Z

miden-lib/src/transaction/errors.rs

 }

 impl fmt::Display for TransactionKernelError {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        match self {
-            Self::InvalidStorageSlotIndex(index) => {
+            TransactionKernelError::InvalidStorageSlotIndex(index) => {


Self:: confused the rust-analyzer and broke the suggested actions. I have done similar changes in other places, where use TransactionKernelError::* caused issues (namely, autocompletion is gone, actions don't work)

hackaugusto · 2024-04-06T16:07:37Z

miden-tx/src/host/state_tracker.rs

-
-    /// Extracts information from the process state about the storage slot being updated and
-    /// records the latest value of this storage slot.
-    pub(super) fn on_account_storage_set_item<S: ProcessState>(


I put the handlers together, it makes it easier to follow the code.

hackaugusto · 2024-04-06T16:08:39Z

mock/src/mock/transaction.rs

@@ -53,17 +51,13 @@ pub fn mock_inputs_with_account_seed(
        },
    };

-    // mock notes


These comments didn't add context, the function being called is already named mock_notes.

hackaugusto · 2024-04-06T16:10:03Z

objects/src/notes/metadata.rs

-/// The metadata consists of:
-/// - sender is the ID of the account which created the note.
-/// - note_type defines how the note is to be stored (e.g., on-chain or off-chain).
-/// - tag is a value which can be used by the recipient(s) to identify notes intended for them.
-/// - aux is arbitrary user-defined value.


Moved the documentation to the fields. Having the comments along side thing they refer to makes it easier to update when changes are done.

There is one downside to this: the generated documentation will no longer include this info because the fields are private. So, one would be forced to look at the source code to understand what the metadata consists of. That was one of the main reasons why we originally put the description of fields into struct comments.

We could still keep the comments for each specific filed, but I think we also need similar detail in the struct comment.

I don't think that is a problem. If the attribute is private, the docs should not be shown because it is not part of the public API. The user has to rely on the public API, in this case the accessor methods (they have documentation).

hackaugusto · 2024-04-06T16:11:15Z

objects/src/notes/mod.rs

-    Hasher::merge(&[merge_script, inputs.commitment()])
-}
-
-impl Note {


Renamed to NoteConsumable and moved to the consumable module. For our current usage, Note is too generic, the new name should make a bit cleared that this structure is used to hold the details necessary to consume a note as an input.

hackaugusto · 2024-04-06T16:11:59Z

objects/src/notes/recipient.rs

+    serial_num: Word,
+    script: NoteScript,


This should probably be put into a NoteNullifier, but this PR was too big already.

hackaugusto · 2024-04-06T16:13:45Z

objects/src/transaction/outputs.rs

-/// When a note is produced in a transaction, the note's recipient, assets, and metadata must be
-/// known. However, other information about the note may or may not be know to the note's producer.
-#[derive(Debug, Clone, PartialEq, Eq)]
-pub struct OutputNote {


Remove in favor of an enum of NoteConsumable and NoteEnvelope, which correspond to public and private notes.

hackaugusto · 2024-04-06T16:14:25Z

objects/src/transaction/proven_tx.rs

-    output_notes: OutputNotes<NoteEnvelope>,
-
-    /// Optionally the output note's data, used to share the note with the network.
-    output_note_details: BTreeMap<NoteId, Note>,


This is no longer necessary, the enum gives better type safety since the data is not split into two places.

hackaugusto · 2024-04-06T16:15:06Z

objects/src/transaction/proven_tx.rs

-            .filter(|&k| !known_output_ids.contains(k))
-            .cloned()
-            .collect();
-        if !unknown_ids.is_empty() {


This is an example of improved type safety, the check is no longer necessary, because the data is no longer split into two places.

igamigo · 2024-04-06T20:50:25Z

miden-lib/src/notes/mod.rs

@@ -31,17 +34,19 @@ pub fn create_p2id_note<R: FeltRng>(
    assets: Vec<Asset>,
    note_type: NoteType,
    mut rng: R,
-) -> Result<Note, NoteError> {
+) -> Result<NoteConsumable, NoteError> {


I think I disagree slightly with this name change. I feel like Note/InputNote/NoteMetadata are good matches for what they represent already, and maybe Consumable collides a little bit semantically with Input? Don't feel strongly and this was likely discussed somewhere else, so I wouldn't mind either way but just my two cents.

If you look at Note/InputNote/NoteMetadata alone, yes. But we also have NoteEnvelope and NoteOutput. In comparison to these, it is not clear at all how do they differ from plain Note.

I'm happy to change this, but I have no better suffix, any suggestions?

igamigo · 2024-04-06T21:03:19Z

objects/src/transaction/outputs.rs

 /// For the purposes of this struct, anything that can be reduced to a note envelope can be an
 /// output note. However, [ToEnvelope] trait is currently implemented only for [OutputNote] and
 /// [NoteEnvelope], and so these are the only two allowed output note types.


Looks like this doc comment might be outdated.

igamigo · 2024-04-06T21:18:41Z

miden-tx/src/host/mod.rs

+            if data.len() != 12 {
+                return Err(TransactionKernelError::MalformedRecipientData(data.to_vec()));
+            }
+            let inputs_hash = Digest::new([data[0], data[1], data[2], data[3]]);
+            let script_root = Digest::new([data[4], data[5], data[6], data[7]]);
+            let serial_num = [data[8], data[9], data[10], data[11]];
+            let input_els = self.adv_provider.get_mapped_values(&inputs_hash);
+            let script_data = self.adv_provider.get_mapped_values(&script_root).unwrap_or(&[]);
+
+            let inputs = NoteInputs::new(input_els.map(|e| e.to_vec()).unwrap_or_default())
+                .map_err(TransactionKernelError::MalformedNoteInputs)?;
+            let script = NoteScript::try_from(script_data)
+                .map_err(|_| TransactionKernelError::MalformedNoteScript(script_data.to_vec()))?;
+            let recipient = NoteRecipient::new(serial_num, script, inputs);
+            NoteOutput::Public(NoteConsumable::new(vault, metadata, recipient))


Should these if branches validate the note type (or maybe the if should be based on note_type)? Feels like if there is some corruption up to this point, you could get a NoteOutput::Public() out of this function that contains a NoteType::OffChain on its metadata.

added check for public note

igamigo · 2024-04-06T21:21:16Z

objects/src/notes/output.rs

+pub enum NoteOutput {
+    Public(NoteConsumable),
+    Private(NoteEnvelope),
+}


Similarly (or alternatively) to the other note type validation comment, it might be a good idea to validate on creation that NoteOutput::Private(envelope) cannot contain an envelope with NoteType::Onchain and the reverse for NoteOutput::Public (unless there's some use-case I'm missing)

Added validation

bobbinth

Looks great! Thank you! This is not a full review, but I reviewed probably 90% of the code (the only thing left are some part os the changes to the transaction host).

I left some comments inline - but they are mostly related not naming/code organization.

bobbinth · 2024-04-08T10:04:18Z

objects/src/notes/metadata.rs

-/// The metadata consists of:
-/// - sender is the ID of the account which created the note.
-/// - note_type defines how the note is to be stored (e.g., on-chain or off-chain).
-/// - tag is a value which can be used by the recipient(s) to identify notes intended for them.
-/// - aux is arbitrary user-defined value.


There is one downside to this: the generated documentation will no longer include this info because the fields are private. So, one would be forced to look at the source code to understand what the metadata consists of. That was one of the main reasons why we originally put the description of fields into struct comments.

We could still keep the comments for each specific filed, but I think we also need similar detail in the struct comment.

objects/src/notes/recipient.rs

objects/src/notes/script.rs

bobbinth · 2024-04-08T10:20:28Z

objects/src/notes/vault.rs

+#[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))]
+pub struct NoteVault {


I would probably roll-back this change (would it be a simple find/replace?). One of the reasons is that we have a generic AssetVault (which is used by accounts, but also in other contexts), and having NoteVault while not having AccountVault is a bit confusing, IMO.

bobbinth · 2024-04-08T10:37:21Z

objects/src/notes/output.rs

+/// The types of note outputs supported by the transaction kernel.
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub enum NoteOutput {
+    Public(NoteConsumable),
+    Private(NoteEnvelope),
+}


I like the idea of using an enum instead of ToEnvelope a lot! One comment: why not just transform the existing OutputNote into the enum? Generally, I have two comments here:

I think NoteOutput name is a bit confusing (it may be interpreted as something that is an output of a note) - so, I would prefer OutputNote.

I would move this enum to the transactions module as it is related to transaction execution.

So, basically, I would rename this enum into OutputNote and move it into src/transaction/outputs.rs.

Another thing to consider in a future PR: I'm not sure the variant names here is what we want to have long term. Specifically, we may know full details of a private note, and it would make sense to be able to keep track of that. So, maybe the variant names should be something like:

pub enum OutputNote { Full(Note), Minimal(NoteEnvelope), // not a great name but couldn't come up with anything better }

Let's create an issue to come back to this later.

Let's create an issue for refactoring the OutputNote along the lines of what I described above.

objects/src/notes/output.rs

bobbinth · 2024-04-08T10:49:18Z

objects/src/notes/consumable.rs

+/// A note with all the data required for it to be consumed by executing it against the transaction
+/// kernel.
+///
+/// Notes are created with a script, inputs, assets, and a serial number. Fungible and non-fungible
+/// asset transfers are done by moving assets to the note's vault. The note's script determines the
+/// conditions required for the note consumpution, i.e. the target account of a P2ID or conditions
+/// of a SWAP, and the effects of the note. The serial number has a double duty of preventing double
+/// spend, and providing unlikability to the consumer of a note. The note's inputs allow for
+/// customization of its script.
+///
+/// To create a note, the kernel does not require all the information above, a user can create a
+/// note only with the commitment to the script, inputs, the serial number, and the kernel only
+/// verifies the source account has the assets necessary for the note creation. See [NoteRecipient]
+/// for more details.
+#[derive(Clone, Debug, PartialEq, Eq)]
+#[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))]
+pub struct NoteConsumable {
+    vault: NoteVault,
+    metadata: NoteMetadata,
+    recipient: NoteRecipient,
+
+    id: NoteId,
+    nullifier: Nullifier,
+}


I like the structural changes here - but I'm not so sure about the naming. First, I think ConsumableNote would probably be a better name than NoteConsumable, but more importantly, I'm not sure we need to make this renaming at this point in time.

There are a couple of reasons for this:

I think we need to have a note object which is independent of the transaction context.

We need to break out note metadata out of here, and I'd like to revisit note object naming more comprehensively as a part of that change (mostly because we have too many variations of "note" objects floating around across miden base and miden node).

So, my preference here would be to keep the structural changes but rename this back into Note and move the code into src/notes/mod.rs.

IMO we should stop putting stuff in the mod.rs, specially when we have sub modules. There is no need to put everything in the mod.rs, and having sub mods makes it easier to navigate the source code.

renamed and moved to mod.rs

IMO we should stop putting stuff in the mod.rs, specially when we have sub modules. There is no need to put everything in the mod.rs, and having sub mods makes it easier to navigate the source code.

I'm not opposed to this. Sometimes, when the inner and outer module names are the same, clippy may give an "inception" warning - but I don't think that's the case here (because notes and note would be different).

objects/src/notes/consumable.rs

miden-tx/src/tests.rs

bobbinth · 2024-04-08T11:09:54Z

miden-tx/src/host/mod.rs

 pub struct TransactionHost<A> {
    adv_provider: A,
-    account_delta: AccountDeltaTracker,
+    state_tracker: StateTracker,
    acct_procedure_index_map: AccountProcedureIndexMap,
+    notes: Vec<NoteOutput>,
 }


The struct doc comments need to be updated.

hackaugusto · 2024-04-08T16:25:48Z

miden-tx/src/tests.rs


    assert_eq!(executed_transaction.final_account().hash(), tx_outputs.account.hash());
    assert_eq!(executed_transaction.output_notes(), &tx_outputs.output_notes);
 }

 #[test]
+#[ignore]


We decided to skip the tests for the moment. This will be fixed in a follow up.

applied requested changes

bobbinth

Looks good! Thank you! I left more comments inline - but pretty much all of them are nits. After these are addressed, we are good to merge.

objects/src/notes/mod.rs

objects/src/notes/nullifier.rs

objects/src/transaction/outputs.rs

miden-lib/src/transaction/inputs.rs

miden-tx/tests/integration/scripts/swap.rs

miden-lib/asm/miden/kernels/tx/tx.masm

bobbinth · 2024-04-08T19:27:51Z

miden-tx/src/host/mod.rs

+            let script = NoteScript::try_from(script_data)
+                .map_err(|_| TransactionKernelError::MalformedNoteScript(script_data.to_vec()))?;


This is fine for now, but in the future, we'll also need to validate that the script data is consistent with script hash. This will become simpler once we move to MAST-based script serialization (i.e., we will not need to compile the script). Let's add a TODO about it here and also create an issue to come back to this later.

miden-tx/src/host/state_tracker.rs

bobbinth · 2024-04-08T19:30:38Z

objects/src/notes/output.rs

+/// The types of note outputs supported by the transaction kernel.
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub enum NoteOutput {
+    Public(NoteConsumable),
+    Private(NoteEnvelope),
+}


Let's create an issue for refactoring the OutputNote along the lines of what I described above.

Dominik1999 · 2024-04-09T08:16:30Z

Nice, merge!

hackaugusto changed the title ~~Extract notes via evnets issue 539~~ Extract notes via events issue #539 Apr 5, 2024

hackaugusto force-pushed the hacka-issue-539 branch 3 times, most recently from ebea437 to e1b7693 Compare April 5, 2024 21:25

hackaugusto marked this pull request as ready for review April 5, 2024 21:33

hackaugusto requested review from bobbinth, phklive and polydez April 6, 2024 15:55

hackaugusto commented Apr 6, 2024

View reviewed changes

hackaugusto force-pushed the hacka-issue-539 branch from e1b7693 to 4b4ec49 Compare April 6, 2024 16:15

igamigo reviewed Apr 6, 2024

View reviewed changes

igamigo mentioned this pull request Apr 6, 2024

Add support for on-chain notes to block-producer 0xPolygonMiden/miden-node#303

Closed

bobbinth previously requested changes Apr 8, 2024

View reviewed changes

hackaugusto mentioned this pull request Apr 8, 2024

Optimize note script encoding #576

Open

hackaugusto force-pushed the hacka-issue-539 branch 2 times, most recently from 085b12f to 1162f16 Compare April 8, 2024 16:24

hackaugusto commented Apr 8, 2024

View reviewed changes

hackaugusto requested review from igamigo and bobbinth April 8, 2024 16:29

bobbinth approved these changes Apr 8, 2024

View reviewed changes

notes: extracting notes via events (issue #539)

252f225

hackaugusto force-pushed the hacka-issue-539 branch from 1162f16 to 252f225 Compare April 9, 2024 12:08

hackaugusto merged commit f80276e into next Apr 9, 2024
9 checks passed

hackaugusto deleted the hacka-issue-539 branch April 9, 2024 13:41

hackaugusto mentioned this pull request Apr 9, 2024

Use events to build output note details for a transaction #539

Closed

igamigo mentioned this pull request Apr 9, 2024

fix: Make code compatible with miden-base's next 0xPolygonMiden/miden-node#307

Merged

This was referenced Apr 9, 2024

bugfix: fix note script serialization and re-enable tests #584

Merged

Ensure all data of a note is available for public notes. #585

Open

hackaugusto mentioned this pull request Apr 9, 2024

Rename OutputNote variants #586

Closed

bobbinth mentioned this pull request Apr 11, 2024

On-chain accounts and notes #403

Closed

		#[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))]
		pub struct NoteVault {

		let script = NoteScript::try_from(script_data)
		.map_err(\|_\| TransactionKernelError::MalformedNoteScript(script_data.to_vec()))?;

Extract notes via events issue #539 #572

Extract notes via events issue #539 #572

Conversation

hackaugusto commented Apr 4, 2024 • edited Loading

hackaugusto Apr 6, 2024 • edited Loading

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

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

igamigo Apr 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igamigo Apr 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobbinth left a 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

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

bobbinth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dominik1999 commented Apr 9, 2024

hackaugusto commented Apr 4, 2024 •

edited

Loading

hackaugusto Apr 6, 2024 •

edited

Loading

igamigo Apr 6, 2024 •

edited

Loading

igamigo Apr 6, 2024 •

edited

Loading