Skip to content

note::build_recipient should insert padded inputs into advice map #2129

@PhilippGackstatter

Description

@PhilippGackstatter

note::build_recipient inserts the unpadded note inputs into the advice map instead of the padded entries, like TxArgs::add_output_note_recipient does:

https://github.com/0xMiden/miden-base/blob/0cd80afdbd9b9d26e27341fad895063ac53eacdc/crates/miden-objects/src/transaction/tx_args.rs#L177-L181

The call to NoteInputs::to_elements returns the padded values.

In other words, the behavior of note::build_recipient and TxArgs::add_output_note_recipient should be the same, but currently isn't.

If we pass num_inputs = 5, inputs_ptr = 0 to build_recipient, note::compute_inputs_commitment correctly computes the commitment, but we then insert the memory range inputs_ptr..(inputs_ptr+num_inputs) into the advice map entry, which is not padded.

To reproduce, this, we can add these assertions at the end of test_build_recipient, and the test fails:

for note_inputs in [note_inputs_4, note_inputs_5, note_inputs_13] {
    let inputs_advice_map_key = note_inputs.commitment();
    assert_eq!(
        exec_output.advice.get_mapped_values(&inputs_advice_map_key).unwrap(),
        note_inputs.to_elements(),
        "advice entry with note inputs should contain the padded values"
    );

    let num_inputs_advice_map_key =
        Hasher::hash_elements(note_inputs.commitment().as_elements());
    assert_eq!(
        exec_output.advice.get_mapped_values(&num_inputs_advice_map_key).unwrap(),
        &[Felt::from(note_inputs.num_values())],
        "advice entry with num note inputs should contain the original number of values"
    );
}

I think this is the reason why using extend_expected_output_notes clashes as mentioned here (#2123 (comment)). cc @partylikeits1983


I also think it would be good to have a case in this test that doesn't need any padding, e.g. having note inputs with length 8, so we explicitly test both unpadded and padded note inputs.


Separately, we do not seem to add the num inputs values when adding an input note, which I think we should add for consistency:

https://github.com/0xMiden/miden-base/blob/0cd80afdbd9b9d26e27341fad895063ac53eacdc/crates/miden-lib/src/transaction/inputs.rs#L337

Here we only add the inputs_commitment: [inputs] entry, but not the hash(inputs_commitment): [num_inputs] entry. I think this should've conceptually been done in #2066, but wasn't strictly necessary for the fix so I forgot to do this.


To summarize:

  • note::build_recipient should insert padded note inputs into the advice map.
  • We can replace one of the cases in test_build_recipient with a note_inputs_8 case, where we have 8 note inputs, i.e. that don't need padding.
  • We should add the number of input values for input notes in TransactionAdviceInputs::add_input_notes for consistency.

Metadata

Metadata

Assignees

Labels

standardsRelated to standard note scripts or account components

Type

Projects

No projects

Milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions