DRAFT PR: 1470 escape hatch add sliding windows to light client contract

[alysiahuggins]

Closes #1470

This PR:

• lagOverEscapeHatchThreshold(l1BlockHeight, delayThreshold)
• getHotShotCommitment(hotShotBlockHeight)
• getL1BlockUpdatesCount()
• getHotShotBlockCommitmentsCount()

This PR does not:

• Purge the newly added arrays once there is 10 days of data, another issue has been created
• The edgecase: the user submits a blocknumber > block.number and the last light client update block - block.number > threshold

Key places to review:

• lagOverEscapeHatchThreshold function
• getHotShotCommitment function

How to test this PR:

forge test --match-contract LightClient_StateUpdatesTest -vvv --ffi
forge test --match-contract LightClient_HotShotCommUpdatesTest -vvv --ffi

Things tested

• lagOverEscapeHatchThreshold happy paths and edge cases
• getHotShotCommitment functionality
• stateUpdateBlockNumbers is updated when the newFinalizedState function is successfully called
• hotShotCommitments is updated when the newFinalizedState function is successfully called

[ImJeremyHe]

For the fraud proof in nitro integration, I also need an endpoint to access the hotshot commitment via l1 height, like:

function commitments(uint256) external view returns (uint256);

L1 arbitrator needs to access the light client contract to check if the commitment provided by stakers is correct or not.

[alysiahuggins]

I actually handled it that way before but the linter complained that the function was too complex. I’ve since changed the function a bit so I can try that again

…

On Thu, May 30, 2024 at 12:02 Mathis ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In contracts/src/LightClient.sol <#1493 (comment)> : > + // Handling Edge Cases + // Edgecase 1: The block is in the future or in the past before HotShot was live + if (l1BlockNumber > block.number || (updatesCount > 0 && l1BlockNumber < l1BlockUpdates[0])) + { + revert InvalidL1BlockForCheckingL1Updates(); + } + + // Edgecase 2: There have only been two HotShot updates so far + // we only start checking if HotShot is live after it has received atleast two updates + if (updatesCount < 2) { + return true; + } + + for (uint256 i = 0; i < updatesCount; i++) { + uint256 blockStart = l1BlockUpdates[i]; + uint256 blockEnd = (i < updatesCount - 1) ? l1BlockUpdates[i + 1] : block.number; Maybe this code would be easier to read if the iteration only went until i < updatesCount - 1 and the last case was handled after the loop? — Reply to this email directly, view it on GitHub <#1493 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABLWBGMCVHEPORBWP4VUBHTZE32M5AVCNFSM6AAAAABIDYASCCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAOBXHAZTMNJVG4> . You are receiving this because you were mentioned.Message ID: ***@***.*** com>

[sveitser]

I propose to invert the meaning of this function because I think it makes it clearer what it is for and easier to explain the logic. Currently this function is queried to turn on the escape hatch if the function returns false. With the change the function is called to turn on the escape hatch if the function returns true.

    /// @notice check if more than threshold blocks passed since the last state update before
    /// l1BlockNumber
    /// @param blockNumber The L1 block number
    /// @param threshold The number of blocks updates to this contract is allowed to lag behind
    function lagOverEscapeHatchThreshold(uint256 blockNumber, uint256 threshold) public view returns (bool) {
        uint256 latestUpdateBefore;
        bool updateFound;
        // Iterate over updates in revers order to find the latest update before the given block
        // number.
        uint256 index = stateUpdateBlockNumbers.length - 1;
        while (!updateFound) {
            if (stateUpdateBlockNumbers[index] <= blockNumber) {
                updateFound = true;
                latestUpdateBefore = stateUpdateBlockNumbers[index];
            }
            if (index == 0) {
                break;
            }
            index--;
        }

        // If no snapshot is found, we don't have enough history stored to tell whether HotShot was
        // down.
        if (!updateFound) {
            revert InsufficientSnapshotHistory();
        }

        return blockNumber - latestUpdateBefore > threshold;
    }

thoughts?

cc @alysiahuggins @ImJeremyHe @nomaxg

[alysiahuggins]

i like this too actually - It matches with the mindset of the person who's calling the function and wants to determine if they need to do some action in event of a lag. @ImJeremyHe @nomaxg thoughts?

[nomaxg]

make sense to me

[nomaxg]

LGTM just a couple of nits and questions

[ImJeremyHe]

If the block is in the future, doen't that mean hotshot has already down?

[alysiahuggins]

hmm, if the current ethereum block is 10 and the user requested for block 12, i'd say that we can't say whether our light client contract is missing an update / hotshot is down because we don't have enough data points to be able to assert that.

[sveitser]

IMO it's not meaningful to ask if the light client contract lagged behind at an Ethereum block number in the future so I think it's correct to throw an error here.

[alysiahuggins]

An issue was created to handling purging of the newly added arrays, hotShotCommitments and stateUpdateBlockNumbers
#1541

[nomaxg]

LGTM