-
Notifications
You must be signed in to change notification settings - Fork 15
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
Resolve todos in the codebase #90
base: master
Are you sure you want to change the base?
Conversation
d87e934
to
c26f8bb
Compare
c26f8bb
to
5d087c3
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #90 +/- ##
==========================================
- Coverage 60.84% 60.33% -0.51%
==========================================
Files 26 26
Lines 3833 3787 -46
==========================================
- Hits 2332 2285 -47
+ Misses 1385 1378 -7
- Partials 116 124 +8 ☔ View full report in Codecov by Sentry. |
@@ -628,7 +628,7 @@ contract RISCV_Test is CommonTest { | |||
|
|||
function test_lrw_succeeds() public { | |||
bytes32 value = hex"1e0acbdd44d41d85"; | |||
uint64 addr = 0x233f3d38d3ce666b; | |||
uint64 addr = 0x233f3d38d3ce6668; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These random addresses in tests were not properly aligned for the call to succeed. I modified them to be aligned in 4 bytes.
@@ -200,21 +199,6 @@ func (m *Memory) SetUnaligned(addr uint64, dat []byte) { | |||
if d == len(dat) { | |||
return // if all the data fitted in the page, we're done | |||
} | |||
|
|||
// continue to remaining part |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're removing the ability to do unaligned writes across page boundaries, can we also add a check at the top of SetAligned
that the write is 8-byte aligned?
I see that in the VM, we check for 4-byte alignment, but it is possible to read over multiple pages if we're writing full double words and only enforcing 4-byte alignment.
@@ -894,7 +875,9 @@ func (inst *InstrumentedState) riscvStep() (outErr error) { | |||
revertWithCode(riscv.ErrBadAMOSize, fmt.Errorf("bad AMO size: %d", size)) | |||
} | |||
addr := getRegister(rs1) | |||
// TODO check if addr is aligned | |||
if addr&3 != 0 { // quick addr alignment check |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See other comment on removing the ability to write across multiple pages in SetAligned
.
rvgo/slow/vm.go
Outdated
@@ -132,11 +128,11 @@ func Step(calldata []byte, po PreimageOracle) (stateHash common.Hash, outErr err | |||
} | |||
|
|||
proofContentOffset := shortToU64(uint16(stateContentOffset) + paddedStateSize + 32) | |||
// TODO: validate abi offset values? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please :D
Cannon does this as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to look at where cannon does this, but can't find it since I"m not familiar with the codebase :( Could you please give me quick pointer on where to look?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah for sure! Apologies for the late response. Here are the calldata layout checks cannon does: https://github.com/ethereum-optimism/optimism/blob/85248727fe59498ed62788eca8d9c5849cfa5074/packages/contracts-bedrock/src/cannon/MIPS.sol#L216-L231
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! It's fixed in eb34592
eb34592
to
22f0c71
Compare
This PR cleans up some TODOs in the codebase.