-
Notifications
You must be signed in to change notification settings - Fork 124
NoteInputs's trial unhashing produces incorrect results #2065
Description
In the client (0xMiden/miden-client#1454), a couple of tests were failing with some checks related to the NoteScreener. Specifically, P2IDE inputs were not matching. When looking at why, we saw the note inputs length for the P2IDE note was 3. This had happened before so we looked at the recent changes regarding note inputs and found the following:
- The values stored in the advice map are
inputs.to_elements(), which pads the note inputs to the next multiple ofWORD_SIZE * 2. For a P2IDE note, that's 8. - In this test, the code was first creating a P2IDE note with reclaim height but no timelock, the inputs of which would be
[suffix, prefix, reclaim_height, 0], padding produces[suffix, prefix, reclaim_height, 0, 0, 0, 0, 0]. - When the note is reconstructed,
read_note_inputs_from_adv_maplooks at that padded vector and picks the last non-zero entry to guess the original length. Because the 4th element is zero, the “last non-zero” is the third element, so it tries[suffix, prefix, reclaim_height]. NoteInputs::newpads that 3‑element list to eight elements before hashing, resulting in[suffix, prefix, reclaim_height, 0, 0, 0, 0, 0]again. The commitment therefore matches and the shorter vector is accepted.
Unless I'm missing something, this approach of unhashing would not work unless we were able to persist the non-padded inputs commitment all the way through to an output note that another user can use as an input note.
Related to the above, I think if we had had a test like the one we outlined in #1363 this might have been caught here; although it depends on the last note input(s) being purposefully 0. We caught it because we left some static checks on the NoteScreener (that we were plannign to remove with the changes done on the NoteConsumptionChecker).
Curious about why the previous approach of passing the length as the first element was not enough. Maybe we can roll back to that version until a more permanent solution lands.