Fix light client bug #2509
Fix light client bug #2509
Conversation
For some reason when branching off of main running `just gen-bindings` also regenerates the LightClientArbitrum_bytecode file and I can't get the commit to work without including it.
|
The light client test is still failing locally. I need to figure out how to add one additional entry to its state to give us a valid target as the "max height" to read from. When we only have one state we can't read it with this change, and we can't read from an index less than zero. I also want to add a case where we expect a revert when trying to read from the most recent state |
There was a problem hiding this comment.
Change looks good to me, pending the additional testing you suggested. It would also be good to have @alysiahuggins @alxiong @0xkato look at this
contracts/src/LightClient.sol
Outdated
contracts/test/LightClient.t.sol
Outdated
| // Get the highest HotShot blockheight recorded | ||
| uint256 numCommitments = lc.getStateHistoryCount(); | ||
| (,, hotShotBlockHeight, hotShotBlockComm) = lc.stateHistoryCommitments(numCommitments - 1); | ||
| (,, hotShotBlockHeight, hotShotBlockComm) = lc.stateHistoryCommitments(numCommitments - 2); |
There was a problem hiding this comment.
Hi there, There's an integer underflow here because at this point there's only one state history commitment that would have been set so numCommitments = 1. What was the reason for subtracting 2?
There was a problem hiding this comment.
I hadn't fully fixed this test yet. It should be fixed in this commit. We don't actually need to subtract by 2, but now the function semantic is different in that it will return 1 index in the list past what it previously did. Because of this I had to adjust the height passed into the next function.
zacshowa
requested review from
philippecamacho,
ImJeremyHe,
sveitser,
tbro and
imabdulbasit
as code owners
This PR:
Fixes some bugs in the light client that would return invalid snapshots when the snapshot of the height requested was in the last index of
stateHistoryCommitmentsKey places to review:
contracts/src/LightClient.solThings tested:
Consumers of the light client are no longer affected by this bug