prune state history on LC contract #1670

alysiahuggins · 2024-07-02T21:46:19Z

added functionality to prune the state history using a FIFO approach and added two additional tests

This PR:

ensures that there is only ever 10 days worth of state history stored by deleting the first entries first and storing the first index to use when traversing the state history

This PR does not:

truncate the array length to be no greater than 10 days worth of state updates

Key places to review:

FIFO approach to pruning and the fact that the array continues to grow though one only ever interacts with the 10 days worth of state updates due to storing the stateHistory first index

How to test this PR:

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

Things tested

updating max state history allowed
dealing with state updates greater than the max state history allowed

… added two additional tests

ImJeremyHe

What confuses me is that why not just override the old values with the new values? Does that consume more gas?

ImJeremyHe · 2024-07-10T02:08:08Z

contracts/src/LightClient.sol

+        stateUpdateBlockNumbers.push(blockNumber);
+        hotShotCommitments.push(HotShotCommitment(state.blockHeight, state.blockCommRoot));


Since we will just push the new elements into the array, why bother deleting the first one?

so that the storage is emptied and interacting with the array will consume less gas.

The goal is to maintain a history that isn't larger than maxStateHistoryAllowed. So i delete the first element and increment stateHistoryFirstIndex, so that the history is kept within this bound. The amount if storage required is a predictable and we also get a gas refund with the delete.

…pdates-and-hotshotcommitments-arrays-once-it-has-over-10-days-of-worth-of-data

contracts/src/LightClient.sol

sveitser · 2024-07-18T11:28:13Z

contracts/src/LightClient.sol

+    function updateStateHistory(uint256 blockNumber, LightClientState memory state) internal {
+        if (maxStateHistoryAllowed == 0) revert InvalidMaxStateHistory();
+
+        if (stateHistoryCount == maxStateHistoryAllowed) {


The maxStateHistoryAllowed name is a bit misleading right now because if the setter setMaxStateHIstoryAllowed is used to reduce it then size of the history that is currently larger it won't be reduced because we only ever remove an element if we also add one. Unless I'm misunderstanding something.

I really doubt we ever want to set a smaller state history after deployment though so because all that it allows us to do is reclaim some gas. So maybe we can "fix it" by not allowing setting a lower state history?

We could also have functionality to prune multiple entries but the problem with that is that we need to then have potential issues where calls can run out of gas if the iteration is too long.

One way to resolve this is also to allow for a few (at least 2) pruning operations during an update. This makes the gas usage bounded and still allows the history to shrink.

your understanding is correct, it does not considering the reduction of the max state history allowed. Would you allow me to add the ability to prune +1 elements at the same time? and if so, I can make it so that the maxStateHistoryAllowed can be reduced. Otherwise, i can restrict modifications to only increasing the max state history allowed

I think any changes here are somewhat problematic because it potentially affects the result of challenges in the rollups. Increasing the history size definitely seems less problematic though.

ah true - also this light client may be deployed on L2s to support L3s - Would there be a need to shorten the maxStateHistoryAllowed in those instances?

Currently we hardcode the maxStateHistoryAllowed in the _initializeState method but considering that the light client would also deploy on L2s to support L3s. Should it be configurable at deployment?

L2s are also a lot cheaper so I wonder if it would be acceptable to default to not pruning the history on L2s.

Another alternative would be to use block.timestamp instead of block numbers. That way we could at least set a default that should keep working on L2s as long as their timestamps are correct. I'm currently not 100% clear on how trustworthy block.timestamp is, especially on L2s. I would hope that validators would only tolerate small deviations though.

Based on the answer below it sounds like the timestamp is pretty safe to use on L1 after the merge. Wdyt @alysiahuggins ?

https://ethereum.stackexchange.com/a/140818

this has been addressed in a previous commit

…ements-from-stateupdates-and-hotshotcommitments-arrays-once-it-has-over-10-days-of-worth-of-data

…pdates-and-hotshotcommitments-arrays-once-it-has-over-10-days-of-worth-of-data

contract-bindings/src/i_plonk_verifier.rs

contract-bindings/src/shared_types.rs

sveitser · 2024-08-13T12:15:59Z

contracts/src/LightClient.sol

+    /// @notice set Max Block States allowed
+    /// @param historySeconds The maximum duration worth of state history updates to store based on
+    /// the block timestamp
+    function setMaxStateHistoryDuration(uint32 historySeconds) public onlyOwner {


As I think we previously discussed I think we should not allow lowering the value here since we currently don't have code to actually reduce the size in that case and it might have unintended consequences for rollups.

ah yes, do we want to provide the ability to lower the value or do we think there's a little chance that will be necessary?

we spoke offline and agreed that it's out of scope for now

sorted here 703546f

sveitser · 2024-08-13T12:16:52Z

contracts/src/LightClient.sol

            }

            // We don't consider the lag time for the first two updates
            if (i < 2) {
                break;
            }
+
+            // We've reached the first recorded block
+            // i >= 2 because we normally clear out the array from a FIFO approach


Outdated comment?

updated comments 7720ab0

contracts/src/LightClient.sol

contracts/test/LightClient.t.sol

utils/src/deployer.rs

contracts/src/LightClient.sol

…tract when interacting with the rust bindings

…on period

…pdates-and-hotshotcommitments-arrays-once-it-has-over-10-days-of-worth-of-data

sveitser · 2024-08-22T17:03:17Z

contracts/script/README.md

@@ -232,8 +232,8 @@ Solution: `export FOUNDRY_PROFILE=default`
 ## LightClient Contract Deployment

 ```bash
-forge script contracts/script/LightClient.s.sol:DeployLightClientContractScript $numBlocksPerEpoch $numInitValidators \
--sig 'run(uint32, uint32)' \
+forge script contracts/script/LightClient.s.sol:DeployLightClientContractScript $numBlocksPerEpoch $numInitValidators $maxHistorySeconds \


We still use the old name maxHistorySeconds here.

sveitser · 2024-08-22T17:06:15Z

contracts/script/LightClient.s.sol

@@ -7,7 +7,7 @@ import { LightClient as LC } from "../src/LightClient.sol";
 import { ERC1967Proxy } from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";

 contract DeployLightClientContractScript is Script {
-    function run(uint32 numBlocksPerEpoch, uint32 numInitValidators)
+    function run(uint32 numBlocksPerEpoch, uint32 numInitValidators, uint32 maxHistorySeconds)


Suggested change

function run(uint32 numBlocksPerEpoch, uint32 numInitValidators, uint32 maxHistorySeconds)

function run(uint32 numBlocksPerEpoch, uint32 numInitValidators, uint32 stateHistoryRetentionPeriod)

Appears elsewhere too, need to do search/replace.

thanks, sorted 33663d7

sveitser

LGTM missing some renames.

…pdates-and-hotshotcommitments-arrays-once-it-has-over-10-days-of-worth-of-data

alxiong

~~Q: instead of storing the timestamp for every update, why can't we simply store a single value oldest_timestamp which gets updated whenever we evict an element from the FIFO queue?~~

alxiong · 2024-08-23T09:49:58Z

foundry.toml

+optimizer = true
+optimizer_runs = 200 # Increasing the number of runs saves gas but increases the size of the contract
+viaIR = true


is this necessary? it's making the compilation really slow

IMO, we usually can resolve the "stack too deep" through some refactoring. e.g. various ways to reduce local variables on the stack.

alxiong · 2024-08-23T09:59:33Z

contracts/src/LightClient.sol

+    struct StateHistoryCommitment {
+        uint64 l1BlockHeight;
+        uint64 l1BlockTimestamp;
+        HotShotCommitment hotShotCommitment;
+    }


is there a benefit to declare this as a new struct rather than just modify the old HotShotCommitment struct with two new fields?

more complicated nested structs lead to larger memory layout.

added functionality to prune the state history using a FIFO approach,…

d2ec465

… added two additional tests

alysiahuggins requested review from nomaxg, philippecamacho, ImJeremyHe, sveitser, jbearer and tbro as code owners July 2, 2024 21:46

alysiahuggins linked an issue Jul 2, 2024 that may be closed by this pull request

escape hatch : purge elements from stateUpdates and hotShotCommitments arrays once it has over 10 days of worth of data #1541

Closed

alysiahuggins added 2 commits July 8, 2024 14:48

optimizin LC and LC mock sso that the contract size is smaller

022110e

refactored LC and LC Mock so to optimize implementation

6970ec2

ImJeremyHe reviewed Jul 10, 2024

View reviewed changes

LCV2 now has all the variables from the original LC

5afd1e3

alysiahuggins changed the title ~~DRAFT PR | prune state history on LC contract~~ prune state history on LC contract Jul 10, 2024

Merge branch 'main' into 1541-escape-hatch-purge-elements-from-stateu…

a897618

…pdates-and-hotshotcommitments-arrays-once-it-has-over-10-days-of-worth-of-data

sveitser reviewed Jul 18, 2024

View reviewed changes

contracts/src/LightClient.sol Outdated Show resolved Hide resolved

sveitser reviewed Jul 18, 2024

View reviewed changes

contracts/src/LightClient.sol Outdated Show resolved Hide resolved

sveitser reviewed Jul 18, 2024

View reviewed changes

contracts/src/LightClient.sol Outdated Show resolved Hide resolved

sveitser reviewed Jul 18, 2024

View reviewed changes

alysiahuggins added 2 commits July 22, 2024 08:35

improved comments

f36a7c4

optimize storage by removing a non efficient variable

a2e165f

alysiahuggins requested a review from sveitser July 22, 2024 18:31

alysiahuggins added 5 commits July 23, 2024 12:18

Merge remote-tracking branch 'origin' into 1541-escape-hatch-purge-el…

f8556de

…ements-from-stateupdates-and-hotshotcommitments-arrays-once-it-has-over-10-days-of-worth-of-data

increased max state history allowed and added another test

9cf843e

add more test cases

6055b03

Merge branch 'main' into 1541-escape-hatch-purge-elements-from-stateu…

31f856d

…pdates-and-hotshotcommitments-arrays-once-it-has-over-10-days-of-worth-of-data

Merge branch 'main' into 1541-escape-hatch-purge-elements-from-stateu…

50e0617

…pdates-and-hotshotcommitments-arrays-once-it-has-over-10-days-of-worth-of-data

alysiahuggins requested a review from imabdulbasit as a code owner July 30, 2024 12:05

alysiahuggins added 3 commits July 30, 2024 08:06

Merge branch 'main' into 1541-escape-hatch-purge-elements-from-stateu…

edb380e

…pdates-and-hotshotcommitments-arrays-once-it-has-over-10-days-of-worth-of-data

Merge branch 'main' into 1541-escape-hatch-purge-elements-from-stateu…

eba9b9b

…pdates-and-hotshotcommitments-arrays-once-it-has-over-10-days-of-worth-of-data

Merge branch 'main' into 1541-escape-hatch-purge-elements-from-stateu…

5a0eea7

…pdates-and-hotshotcommitments-arrays-once-it-has-over-10-days-of-worth-of-data

Merge branch 'main' into 1541-escape-hatch-purge-elements-from-stateu…

f8456e7

…pdates-and-hotshotcommitments-arrays-once-it-has-over-10-days-of-worth-of-data

alysiahuggins requested a review from ImJeremyHe August 7, 2024 20:52