chore(testing): modified start-reth to enforce persistence

[abi87]

By default, reth does not persist every finalized block. Instead it stores blocks up to a certain threshold (by default 2 heights below tip, which in our case means a finalized block may be not persisted).
This may cause an issue while syncing Beacond, since we have SSF and we assume finalized blocks are duly persisted.

This PR setup configs in order to duly persist all the blocks we need

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 32.77%. Comparing base (a23a74a) to head (9cc3658).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2536   +/-   ##
=======================================
  Coverage   32.77%   32.77%           
=======================================
  Files         351      351           
  Lines       15857    15857           
  Branches       20       20           
=======================================
  Hits         5197     5197           
  Misses      10276    10276           
  Partials      384      384           

[shotes]

I think we should not make this the default verbosity - vvvvv should be specified when debugging. Logs will get crowded and very large very quickly.

[rezbera]

Yep definitely not for real nodes

[abi87]

I don't think people use make start-reth to run their production. If they do, they should not. I like increasing outputs in our tests and I think we can sustain it as is.

[rezbera]

    /// Maximum number of blocks to be kept only in memory without triggering
    /// persistence.
    persistence_threshold: u64,
    /// How close to the canonical head we persist blocks. Represents the ideal
    /// number of most recent blocks to keep in memory for quick access and reorgs.
    ///
    /// Note: this should be less than or equal to `persistence_threshold`.
    memory_block_buffer_target: u64,

Noting the above code in reth (slightly more verbose than the CLI info)

[abi87]

@rezbera do you want to me to include this somehow in the Makefile?
I think noting it here as you did is enough (and appreciated!)

[rezbera]

Here is enough - as long as we're aware @abi87

[fridrik01]

Awesome, there had to be some configuration that we could tune to change this behaviour! Great great

[rezbera]

Did a quick sanity check in the reth code for usage of these vars and this makes sense to me.

Noting that persistence_threshold must be >= memory_block_buffer_target.

This likely has performance implications on Reth so perf testing this before release would be appropriate

[abi87]

Did a quick sanity check in the reth code for usage of these vars and this makes sense to me.

Noting that persistence_threshold must be >= memory_block_buffer_target.

This likely has performance implications on Reth so perf testing this before release would be appropriate

@rezbera I agree with the performance hit, but I don't think we need to require people using this in production yet?
At the very least, I would suggest to use the configs while syncing and remove them once node is ready to close to tip.
If anything happens and blocks are lost, scripts/rollback.sh will help.
Thoughts?

[rezbera]

Did a quick sanity check in the reth code for usage of these vars and this makes sense to me.
Noting that persistence_threshold must be >= memory_block_buffer_target.
This likely has performance implications on Reth so perf testing this before release would be appropriate

@rezbera I agree with the performance hit, but I don't think we need to require people using this in production yet? At the very least, I would suggest to use the configs while syncing and remove them once node is ready to close to tip. If anything happens and blocks are lost, scripts/rollback.sh will help. Thoughts?

So the problem this solves is Reth nodes crashing and then losing blocks. Such a problem could also occur once at the tip of the chain, so we would want Reth users to run it even then right?

Agree that it's not an urgent rollout, but my understanding is that longer term, users should always run with these flags

[abi87]

So the problem this solves is Reth nodes crashing and then losing blocks. Such a problem could also occur once at the tip of the chain, so we would want Reth users to run it even then right?

That would be ideal, and maybe even recommended given our relative small validator set. However as long as we can count on a sufficient majority having stored the finalized blocks, all should be fine. Nodes can rollback via script and download the missing payloads/reverted blocks from peers.

[calbera]

tACK. running on my local mainnet fullnode that had EL behind CL by ~1k blocks. EL catches up from the new startup procedure that we trigger in the CL.

However, I have started seeing NewPayload calls time out way more often (even with an increased timeout of 1s or 2s)

2025-02-24T15:57:59-08:00 ERRR Received undefined error during new payload call service=execution-engine payload_block_hash=0x47b3bc54ef46cdc00d07e21e0a4bd5e481c4a72f32144529a82971d51e280d49 parent_hash=0x47b3bc54ef46cdc00d07e21e0a4bd5e481c4a72f32144529a82971d51e280d49 error=Post "http://localhost:8551": engine API call timed out
2025-02-24T15:57:59-08:00 ERRR Failed to process verified beacon block service=blockchain error=Post "http://localhost:8551": engine API call timed out
2025-02-24T15:57:59-08:00 ERRR Error in proxyAppConn.FinalizeBlock module=state err=Post "http://localhost:8551": engine API call timed out
panic: Failed to process committed block (1068277:6634401D9928F5B23094EC258A6E64D1C9260D4932D46EFE74673CE71FE50D07): Post "http://localhost:8551": engine API call timed out

[abi87]

tACK. running on my local mainnet fullnode that had EL behind CL by ~1k blocks. EL catches up from the new startup procedure that we trigger in the CL.

Thanks @calbera, we'll have to monitor this