Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Argument Buffer Corruption #118

Closed
miloszm opened this issue Dec 9, 2022 · 2 comments
Closed

Fix Argument Buffer Corruption #118

miloszm opened this issue Dec 9, 2022 · 2 comments
Labels
fix:bug Something isn't working mark:next Strategic issues related to next versions of Testnet and mid/long term plans team:Core Low Level Core Development Team (Rust)

Comments

@miloszm
Copy link
Contributor

miloszm commented Dec 9, 2022

Describe the bug
When called by transfer contract and stage contract, contract methods receive argument buffer with initial 32 bytes set to zero.

To Reproduce
Run transfer contract test or stake contract test in Rusk

Expected behaviour
Arguments should be received by contracts in a non corrupted form, esp. not set to zero.

Logs/Screenshot
Here is what was sent to the contract:
[00, 00, 00, 00, 00, 00, 00, 00, 1A, B6, 02, A4, B6, 0F, B3, 58, 3B, F6, CB, F6, 66, C4, 57, D2, 42, 51, 84, 3A, A1, 90, B2, B4, 52, 3B, 68, 8D, 3B, B2, 4F, 61, D0, D9, 70, ... more same bytes]

and here is what the contract has seen upon entry:
[00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 52, 3B, 68, 8D, 3B, B2, 4F, 61, D0, D9, 70, ... more same bytes]

Additional context
This problem could not be easily reproduced in Piecrust tests.
transfer and stake contracts in Rusk seem to be the only ones where this problem occurs,
reproducing this problem in Piecrust test is be part of this issue

@miloszm miloszm added fix:bug Something isn't working mark:next Strategic issues related to next versions of Testnet and mid/long term plans team:Core Low Level Core Development Team (Rust) labels Dec 9, 2022
@miloszm miloszm self-assigned this Dec 9, 2022
@miloszm
Copy link
Contributor Author

miloszm commented Dec 9, 2022

Additional info:

Original problem was the PolynomialDegreeTooLarge error from a call to prove into Plonk - during an execution
of transfer contract.
The cause was identified as a corrupted x coordinate of value_commitment (of type JubJubExtended) of the input note.
value_commitment, being a JubJubExtended, consists of members: x, y, z, t1, t2, each of type BlsScalar.
BlsScalar is technically an array of four u64 numbers.
In the x coordinate these numbers looked as follows before the note was sent to the contract:

n0=6391469573278971418
n1=15156799017444963899
n2=13020628494827409730
n3=7012019113684384594

After the note returned from the contract, x coordinate looked as follows:

n0=0
n1=0
n2=0
n3=7012019113684384594

It turns out that already wasm code inside contract received the Note in this form, so the Note was corrupted
after it was serialized, and before it was deserialized. This exluded Rkyv as a culprit.
The push_note call that was causing the problem accepts two parameters: u64 as block height and a Note,
since block height is 8 bytes (being zero its corruption was not noticed), together with 24 zeros for the corrupted x in Note give together 32 corrupted (zeroed) bytes.

Here is what was sent to the contract:
[00, 00, 00, 00, 00, 00, 00, 00, 1A, B6, 02, A4, B6, 0F, B3, 58, 3B, F6, CB, F6, 66, C4, 57, D2, 42, 51, 84, 3A, A1, 90, B2, B4, 52, 3B, 68, 8D, 3B, B2, 4F, 61, D0, D9, 70, ... more same bytes]

and here is what the contract has seen upon entry:
[00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 52, 3B, 68, 8D, 3B, B2, 4F, 61, D0, D9, 70, ... more same bytes]
It is obvious that first 32 bytes of arg buffer are being zeroed between the time call is being made and the time contract callee looks at its arg buffer.
This brings to mind a workaround: declare contract function instead of:

push_note(&mut self, block_height: u64, note: Note)

declare it as follows:

push_note(&mut self, block_height: u64, gap: [u8;32], note: Note)

Adding an extra parameter gap pushes the note further away from the corruptible area of the arg buffer.
With this workaround the test works fine.

@ureeves
Copy link
Member

ureeves commented May 9, 2023

Closing as N/A anymore

@ureeves ureeves closed this as completed May 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix:bug Something isn't working mark:next Strategic issues related to next versions of Testnet and mid/long term plans team:Core Low Level Core Development Team (Rust)
Projects
None yet
Development

No branches or pull requests

2 participants