Unit test for SyscallReadPreimage
#2305
Conversation
…set (otherwise the word alignment was not being respected)
|
Merging #2311 into this branch to avoid underflow errors when accessing registers/memory. |
…call-read-preimage
…call-read-preimage
|
Making it a draft because despite constraints are satisfied after this PR, the demo halts 🤦🏻 Investigating. Running the block 13000378 with config With command Gives me whereas in the parent branch (and |
|
There was no issue with the interpreter code. Meaning, the interpreter did not have to compute the effective address. Instead, the instruction itself updates the address with the preimage offset at every call. Now the unit test behaves like the real interpreter. |
| // We read at most 4 bytes, ensuring that we respect word alignment. | ||
| // Here, if the address is not aligned, the first call will read < 4 |
There was a problem hiding this comment.
I don't think this comment is true. The code could be asking to write into the address 4 the first 4 bytes, and at the address 42 the next 3 bytes, after that the next 2 bytes at the address 82, etc (just using random values here for the example)
There was a problem hiding this comment.
Oh yeah, that comment is outdated. I wrote that back when I thought that was the case, but after talking to you I learnt that that was not necessarily the case. I will delete it in the next commit.
| use crate::mips::{interpreter::interpret_rtype, RTypeInstruction}; | ||
|
|
||
| #[test] | ||
| fn test_unit_syscall_read_preimage() { |
There was a problem hiding this comment.
This test is interesting to verify if the syscall works with writing into a contiguous piece of memory (addr/reg 5 is always increased by the (random) number of bytes that the user asks to read). However, what about this example? It doesn't need to be part of this PR, but it is a use case that might happen.
There was a problem hiding this comment.
The test seems to make sense and map the expected behavior. I would implement more "random read access" in the future (not blocking for the PR) as explained in the comments.
LGTM, modulo my comments.
I'm moving on the actual implementation in #2274, and I might have other properties I would like to check in this test, but still not blocking for this PR.
|
Merging. |
This uses @dannywillems's framework to emulate the preimage oracle in unit tests for the Keccak syscall.
This PR does not only include a unit test using the sample file, but it also proposes two main fixes:
get_preimage()function of the oracle trait was not reading the right bytes from the file, but in ASCII. Thus, I decode them as hexadecimal bytes to store them correctly.SyscallReadPreimagewas not updating the address with the offset correctly. Thus, the addresses where the preimage bytes were being stored in memory were mis-aligned. The unit test checks that now the bytes are stored correctly where desired.