fix: remove padding from output note inputs#1360
Conversation
|
Something that would probably be good to have (even if we decide this PR is not the best solution) is a test that executes a transaction that uses a note that was output from a previous transaction. This was the only difference between client and base tests, and why it wasn't caught here in the first place. |
Fumuran
left a comment
There was a problem hiding this comment.
Looks good, thank you very much for spotting this bug! I left just a few comments inline.
| let inputs = match inputs_data { | ||
| None => NoteInputs::default(), | ||
| Some(inputs) => { | ||
| let num_inputs = data[12].as_int() as usize; |
There was a problem hiding this comment.
Since we moved the number of inputs to the 0'th position in the vector, should we get it using data[0]? Also probably other data indexes should be updated because of that.
There was a problem hiding this comment.
Yes! Just realized the CI was failing, was trying to create a unit test for the failing case but wasn't able to. I'll create an issue for this so we can merge this.
Fumuran
left a comment
There was a problem hiding this comment.
LGTM!
I think that solution with adding the length to the vector of recipient values is good. Potentially we could change that if we decide to move the entire note inputs to the memory, but it is still an open question.
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I left one comment for the future inline.
| /// Returns the recipient formatted to be used with the advice map. | ||
| /// | ||
| /// The format is `inputs_length || INPUTS_COMMITMENT || SCRIPT_ROOT || SERIAL_NUMBER` | ||
| /// | ||
| /// Where: | ||
| /// - inputs_length is the length of the note inputs | ||
| /// - INPUTS_COMMITMENT is the commitment of the note inputs | ||
| /// - SCRIPT_ROOT is the commitment of the note script (i.e., the script's MAST root) | ||
| /// - SERIAL_NUMBER is the recipient's serial number | ||
| pub fn format_for_advice(&self) -> Vec<Felt> { | ||
| let mut result = Vec::with_capacity(13); | ||
| result.push(self.inputs.num_values().into()); |
There was a problem hiding this comment.
I think this is fine for now, but ideally we'd move towards a setup where a key in the advice map corresponds to the hash of the underlying data. It's not clear to me what's the best way to do this - so, let's just create an issue for this.
After
cargo updateing in the client we started getting errors related to the number of note inputs. It seems like there was an error after #1327 with output notes that were then used as input notes. Specifically, the input note derived from the output note contained padding in its note inputs.Our solution was to add the number of note inputs into the recipient's advice map entry, we're not sure if this is the correct approach though.