-
Notifications
You must be signed in to change notification settings - Fork 124
Description
note::build_recipient inserts the unpadded note inputs into the advice map instead of the padded entries, like TxArgs::add_output_note_recipient does:
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:
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_recipientshould insert padded note inputs into the advice map.- We can replace one of the cases in
test_build_recipientwith anote_inputs_8case, 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_notesfor consistency.