feat: enable public output note creation during NTX execution#1995
feat: enable public output note creation during NTX execution#1995
Conversation
partylikeits1983
changed the title
partylikeits1983
changed the title
partylikeits1983
added
standards
PhilippGackstatter
left a comment
PhilippGackstatter
left a comment
There was a problem hiding this comment.
Looks good!
I left a few comments, primarily about the format of the RECIPIENT data in the advice inputs, as well as making the OutputNoteBuilder more type safe.
crates/miden-lib/asm/miden/note.masm
Outdated
| # inserting NoteDetails into AdviceMap | ||
| mem_storew.0 dropw mem_storew.4 dropw mem_storew.8 dropw | ||
| # => [RECIPIENT] |
There was a problem hiding this comment.
We cannot overwrite memory like that, there could be other things there. Instead, increase the number of locals in build_recipient and use loc_storew.
I think we should also stick to the existing order of [INPUTS_COMMITMENT, SCRIPT_ROOT, SERIAL_NUMBER] (here we have the reverse). This means, once we have pushed the recipient to the advice stack with adv.push_mapval, we can pop the words off the advice stack in the right order and use hmerge to compute the RECIPIENT (though would be good to double-check).
I believe we can generally remove the inputs_length from the RECIPIENT advice entry (e.g. change NoteRecipient::format_for_advice accordingly), because if we need the length in MASM, we can use adv.push_mapvaln to get the length. If this turns out to be too big of a refactor, we can also just stick to the existing format completely (including inputs_length) and then open an issue and handle this in a follow-up PR.
Finally, if we touch NoteRecipient::format_for_advice anyway, let's rename it to to_elements, to match the naming of SequentialCommit (though we don't need to implement that trait).
There was a problem hiding this comment.
I think I addressed all of your comments here, except for removing inputs_length from the RECIPIENT advice entry. I think keeping inputs_length in the RECIPIENT entry keeps things on the Rust side in the host slightly simpler. What do you think?
There was a problem hiding this comment.
I think keeping
inputs_lengthin theRECIPIENTentry keeps things on the Rust side in the host slightly simpler. What do you think?
Yeah that's totally fine. I'm not sure if we really need it or have it in other places for advice map entries, so being consistent would be nice, but this is something we can address separately if at all.
Co-authored-by: Philipp Gackstatter <PhilippGackstatter@users.noreply.github.com>
Co-authored-by: Philipp Gackstatter <PhilippGackstatter@users.noreply.github.com>
|
Refactored |
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I left some comments inline - the main one is about how to read NoteInputs from the advice provider.
…eading double words from AdviceMap
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I left a few more small comments inline. Also, would be good for @PhilippGackstatter or @mmagician to do another review.
PhilippGackstatter
left a comment
PhilippGackstatter
left a comment
There was a problem hiding this comment.
Looks good to me!
Left some questions and comments for simplifciation. The main thing I don't understand is the need for the trial unhashing of note inputs, but nothing that needs to hold up the PR probably.
| /// Extracts and validates note inputs from the advice provider using trial unhashing. | ||
| /// | ||
| /// This function tries to determine the correct number of inputs by: | ||
| /// 1. Finding the last non-zero element as a starting point | ||
| /// 2. Building NoteInputs and checking if the hash matches inputs_commitment | ||
| /// 3. If not, incrementing num_inputs and trying again (up to 6 more times) | ||
| /// 4. If num_inputs grows to the size of inputs_data and there's still no match, returning an error | ||
| fn extract_note_inputs( |
There was a problem hiding this comment.
I don't understand why we need to trial unhash. Can you add a comment here explaining why we do this?
NoteInputs::new is already lenient in that it pads the elements if they are not already a multiple of 8, so I wonder why we need to be even more lenient here.
There was a problem hiding this comment.
This is a good point. I think trial unhashing doesn't give us any benefit here because the input will always be padded to the next multiple of 8 during hashing.
The main issue here is then that we won't be able to determine the "true" number of inputs. For example, the number of inputs may be 5, but if the last input is 0, we'll only pick the first 4. I think this is a broader issue (which we tried to solve a couple of times now) - so, let's create a separate issue to discuss how to fix it (the trivial solution is to just use the RPO padding rule, but that may make things a bit less efficient).
There was a problem hiding this comment.
Opened this issue to address this: #2036
Co-authored-by: Philipp Gackstatter <PhilippGackstatter@users.noreply.github.com>
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
All looks good! Thank you!
4 participants
Brief Summary: This PR adds the required functionality to enable the creation of public output notes during network transactions.
TODO list:
build_recipientprocedure to pushSCRIPT_ROOTintoAdviceMapon_note_after_created()inTransactionBaseHost- Now checks if note script exists inAdviceMap.add_note_script()method toTransactionContextBuilder- mockchain method for testing out adding note script toTransactionExecutorBaseon_note_script_requested()handler in TransactionExecutorHost: Fetches missing note scripts fromDataStoreand adds them toAdviceMapRelated to #1748
Resolves #1972
After this PR is merged, the
MINTnote script in #1925 will need to be updated so that itsNoteInputscontain the note metadata of the public output note to create during the network transaction. Will open a separate issue to address this once this is merged or close to being merged.