Fix of the rounding up note inputs number#1399
Conversation
| // | ||
| // Notice that note input values are not loaded to the memory, only their length. On order to obtain | ||
| // the input values the advice map should be used: they are stored there as | ||
| // `INPUTS_COMMITMENT -> INPUTS || PADDING`. | ||
| // | ||
| // As opposed to the asset values, input values are never used in kernel memory, so their presence | ||
| // there is unnecessary. |
There was a problem hiding this comment.
As a side task I added a brief comment explaining why note inputs are not loaded to the memory, as it was discussed in the corresponding issue: #1355
PhilippGackstatter
left a comment
There was a problem hiding this comment.
Looks good to me!
Left some minor comments, but they are optional.
crates/miden-lib/asm/miden/note.masm
Outdated
| # => [num_words, INPUTS_COMMITMENT, num_inputs, dest_ptr] | ||
| # AS => [[INPUT_VALUES]] | ||
|
|
||
| # round up the number of words the next multiple of 2 |
There was a problem hiding this comment.
| # round up the number of words the next multiple of 2 | |
| # round up the number of words to the next multiple of 2 |
Nit
| // - INPUTS_COMMITMENT is the key to look up note inputs in the advice map. | ||
| // - ASSETS_HASH is the key to look up note assets in the advice map. | ||
| // | ||
| // Notice that note input values are not loaded to the memory, only their length. On order to obtain |
There was a problem hiding this comment.
| // Notice that note input values are not loaded to the memory, only their length. On order to obtain | |
| // Notice that note input values are not loaded to the memory, only their length. In order to obtain |
| } | ||
|
|
||
| /// Value that represents under which condition a note can be consumed. | ||
| /// Note's recipient. |
There was a problem hiding this comment.
| /// Note's recipient. | |
| /// Returns the output note's recipient. | |
| /// | |
| /// Note that the recipient is only present on [`OutputNote::Full`]. |
Suggestion
| # execute the `get_inputs` procedure to trigger note inputs number assertion | ||
| push.0 exec.note::get_inputs |
There was a problem hiding this comment.
I would add an assertion here that num_inputs is 8.
| end | ||
| "; | ||
|
|
||
| tx_context.execute_code(tx_code).unwrap(); |
There was a problem hiding this comment.
Nit: For all new or updated tests I would change the return type of the test function to anyhow::Result<()> and then use .context("failed to do xyz")? instead of unwrap. If a test fails in the future, this leads to more easily readable errors and it's a way to dogfood our own errors to see if they are any good when debugging.
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I reviewed mostly non-test code, and left one small comment inline.
| /// Returns the output note's recipient. | ||
| /// | ||
| /// See [crate::note::NoteRecipient] for more details. | ||
| pub fn recipient(&self) -> Option<&NoteRecipient> { |
There was a problem hiding this comment.
I would mention that this only works for Full output notes.
* test: add checks for inputs number of output notes * refactor: rework rounding up inputs number, add test * chore: add comment abount absence of note inputs in memory * refactor: update docs and comments, return anyhow::Result from test * chore: update function doc comments
This PR:
miden::note::get_inputsassembly procedure where the real number of note inputs was rounded up to the next multiple of 8 incorrectly.executed_transaction_output_notestest to check the number of note inputs in the output notes.Closes: #1363