STR-718 Rework L1 access from OL STF #469

delbonis · 2024-11-13T21:48:05Z

Description

The goal of this PR is to change the OL bookkeeping so we're only actually checking in with the L1 at the end of the epoch when we're preparing the epoch checkpoint.

This is incomplete, and may require some storage refactors if we go deep enough. Not sure there yet.

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature/Enhancement (non-breaking change which adds functionality or enhances an existing one)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation update
Refactor
New or updated tests

Checklist

I have performed a self-review of my code.
I have commented my code where necessary.
I have updated the documentation if needed.
My changes do not introduce new warnings.
I have added tests that prove my changes are effective or that my feature works.
New and existing tests pass with my changes.

codecov · 2024-11-14T17:37:37Z

Codecov Report

Attention: Patch coverage is 53.93939% with 228 lines in your changes missing coverage. Please review.

Project coverage is 56.17%. Comparing base (f81dc17) to head (6613eb0).

Files with missing lines	Patch %	Lines
crates/chaintsn/src/epoch.rs	54.77%	123 Missing ⚠️
crates/state/src/state_op.rs	46.08%	62 Missing ⚠️
crates/state/src/chain_state.rs	64.15%	19 Missing ⚠️
crates/consensus-logic/src/duty/block_assembly.rs	0.00%	10 Missing ⚠️
crates/proof-impl/cl-stf/src/lib.rs	0.00%	5 Missing ⚠️
crates/consensus-logic/src/fork_choice_manager.rs	0.00%	3 Missing ⚠️
crates/state/src/l1/maturation_queue.rs	0.00%	3 Missing ⚠️
bin/strata-client/src/main.rs	0.00%	1 Missing ⚠️
crates/chaintsn/src/transition.rs	85.71%	1 Missing ⚠️
...rates/consensus-logic/src/csm/client_transition.rs	0.00%	1 Missing ⚠️

@@            Coverage Diff             @@
##             main     #469      +/-   ##
==========================================
- Coverage   56.65%   56.17%   -0.48%     
==========================================
  Files         275      276       +1     
  Lines       29192    29226      +34     
==========================================
- Hits        16538    16419     -119     
- Misses      12654    12807     +153

Files with missing lines	Coverage Δ
crates/rocksdb-store/src/chain_state/db.rs	`98.73% <100.00%> (+0.03%)`	⬆️
crates/state/src/bridge_ops.rs	`9.09% <100.00%> (-7.58%)`	⬇️
crates/state/src/bridge_state.rs	`58.82% <100.00%> (-0.16%)`	⬇️
crates/state/src/l1/header.rs	`73.17% <100.00%> (-5.78%)`	⬇️
crates/test-utils/src/evm_ee.rs	`97.24% <100.00%> (+0.05%)`	⬆️
bin/strata-client/src/main.rs	`0.00% <0.00%> (ø)`
crates/chaintsn/src/transition.rs	`89.79% <85.71%> (+6.92%)`	⬆️
...rates/consensus-logic/src/csm/client_transition.rs	`62.02% <0.00%> (ø)`
crates/consensus-logic/src/fork_choice_manager.rs	`0.00% <0.00%> (ø)`
crates/state/src/l1/maturation_queue.rs	`0.00% <0.00%> (-40.00%)`	⬇️
... and 5 more

... and 9 files with indirect coverage changes

crates/chaintsn/src/epoch.rs

bewakes · 2024-11-15T03:19:10Z

crates/chaintsn/src/epoch.rs

+///
+/// Includes:
+/// * Processes L1 withdrawals that are safe to dispatch to specific deposits.
+/// * Reassigns deposits that have passed their deadling to new operators.


Suggested change

/// * Reassigns deposits that have passed their deadling to new operators.

/// * Reassigns deposits that have passed their deadline to new operators.

deadline? :D

bewakes · 2024-11-15T03:22:41Z

crates/chaintsn/src/epoch.rs

+    let new_exec_height = cur_block_height as u32 + params.dispatch_assignment_dur;
+
+    // Sequence in which we assign the operators to the deposits.  This is kinda
+    // shitty because it might not account for available funds but it works for


Suggested change

// shitty because it might not account for available funds but it works for

// XXX because it might not account for available funds but it works for

Suggested change

// shitty because it might not account for available funds but it works for

// suboptimal because it might not account for available funds but it works for

crates/chaintsn/src/epoch.rs

…e, deferred processing withdrawals until end of epoch

…t at the end of an epoch

…l fn

bewakes · 2024-12-03T07:01:57Z

crates/state/src/chain_state.rs

-    pub(crate) l1_state: l1::L1ViewState,
+    /// Epoch-level state that we only change as of the last block of an epoch.
+    // TODO this might be reworked to be managed separately
+    pub(crate) epoch_state: EpochState,

    /// Pending withdrawals that have been initiated but haven't been sent out.
    pub(crate) pending_withdraws: StateQueue<bridge_ops::WithdrawalIntent>,

    /// Execution environment state.  This is just for the single EE we support
    /// right now.
    pub(crate) exec_env_state: exec_env::ExecEnvState,


Curious, can the ExecEnvState be somehow derived from Chainstate, or maybe designed to be derived that way? Because I think it would be more transparent and explicit to prepare it before we call EE(s) and also less storage footprint. Something like derive_env_state(Chainstate, Params, ...) -> ExecEnvState

Perhaps, could you expand on what the value of this would be? I'm trying to be as data-oriented as possible here since I anticipate some refactors in the future here when we support async EE consensus.

bewakes · 2024-12-03T07:03:45Z

functional-tests/utils.py

+        cur_ckpt_index = seqrpc.strata_getLatestCheckpointIndex()
+        return cur_ckpt_idx > first_ckpt_idx
+
+    wait_until_with_value(_f, lambda v: v, "Epoch never finalized", timeout, step)


The caller doesn't seem to use the returned value, so could be simplified with wait_until.

wait_until(_f, "Error never vinalizesd", timeout, step)

Oh good point, I didn't realize what I was doing.

bewakes · 2024-12-03T07:03:57Z

crates/chaintsn/src/clock.rs

+    let epoch_init_slot = get_epoch_initial_slot(epoch, params);
+    epoch_init_slot + (params.target_l2_batch_size - 1)


This is simpler?

Suggested change

let epoch_init_slot = get_epoch_initial_slot(epoch, params);

epoch_init_slot + (params.target_l2_batch_size - 1)

get_epoch_initial_slot(epoch + 1, params) - 1

bewakes · 2024-12-03T07:04:02Z

crates/chaintsn/src/epoch.rs

+            for tx in e.deposit_update_txs() {
+                if let ProtocolOperation::Deposit(deposit_info) = tx.tx().protocol_operation() {


Asking for deposit_update_txs and then checking if the pattern match and .tx().protocol_operation() look a bit weird, the interface could be simpler I guess, just getting the deposit infos from payload.

bewakes · 2024-12-03T07:04:11Z

crates/chaintsn/src/epoch.rs

+
+            DepositState::Accepted => {
+                // If we have an intent to assign, we can dispatch it to this deposit.
+                if have_ready_intent {


Can move this to match line to avoid one nesting

bewakes · 2024-12-03T07:04:15Z

crates/chaintsn/src/epoch.rs

+
+            DepositState::Dispatched(dstate) => {
+                // Check if the deposit is past the threshold.
+                if cur_block_height >= dstate.exec_deadline() {


Same for this maybe?

bewakes

Some nit comments. I think the process_deposit_updates function can be made better with helper functions.

delbonis · 2024-12-03T17:55:28Z

@bewakes Appreciate the comments. Going to take a while to circle back to most of these since I'm working on the more tricky areas right now for stuff that breaks due to the consensus/bridging refactors.

…me L1 types

…d epoch

bewakes requested changes Nov 18, 2024

View reviewed changes

delbonis force-pushed the STR-291-stf-refacs branch 2 times, most recently from 05ee729 to 9df4b98 Compare December 2, 2024 19:20

delbonis added 18 commits December 2, 2024 19:06

chaintsn: started refactoring for epoch-level bookkeeping

266c8db

chaintsn, state: changed exec update handling to use withdrawals queu…

a7504a9

…e, deferred processing withdrawals until end of epoch

chaintsn: fixed test

5bb6ada

chaintsn, state: split out some state types to epoch-level state

4a602fe

state: reworked WriteBatch related types

26b806b

misc: reworked epoch state relationships some more

11dac4f

state: reworked StateCache more to remove state ops from system

aa33fe6

misc: fixed a bunch of compilation issues

e4a266f

state: fixed overwriting epoch state, fixed broken tests

fc82355

chaintsn: renamed process_l1_view_update

4d1de82

proof-impl/cl-stf: fix something

4d55f95

chaintsn: fix broken test

c97cc97

misc: fixed a bunch more unit tests

c2d4281

state: removing unnecessary StateOps

f0360b3

chaintsn, consensus-logic: rework L1Segment rules to only be presen…

a7966b5

…t at the end of an epoch

state: fix accessors on block types

bad2a7b

state: fix ZST issue

63d6362

fntest: shortened epoch length, added new wait_until_checkpoint uti…

6d8d186

…l fn

delbonis force-pushed the STR-291-stf-refacs branch from 1038b42 to 6d8d186 Compare December 3, 2024 00:06

bewakes reviewed Dec 3, 2024

View reviewed changes

delbonis added 3 commits December 9, 2024 15:58

misc: cleanups to improve testing

63d1d6a

misc: rework a LOT of types

56064a0

misc: heavy refactoring on status types and finalization code paths

415c710

john-light changed the title ~~STR-291 Rework L1 access from OL STF~~ STR-718 Rework L1 access from OL STF Dec 11, 2024

delbonis added 4 commits December 11, 2024 18:28

misc: integrated epoch transition into slot transition, refactored so…

8acc5b9

…me L1 types

status: fixed hot spinning when waiting for new client state

8def6cb

status: fixed it a little more

9ae11f5

chaintsn, consensus-logic: fixed slot not advancing past end of secon…

bf084a1

…d epoch

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

STR-718 Rework L1 access from OL STF #469

STR-718 Rework L1 access from OL STF #469

delbonis commented Nov 13, 2024

codecov bot commented Nov 14, 2024 •

edited

Loading

bewakes Nov 15, 2024

bewakes Nov 15, 2024

storopoli Nov 29, 2024

bewakes Dec 3, 2024

delbonis Dec 3, 2024

bewakes Dec 3, 2024 •

edited

Loading

delbonis Dec 3, 2024

bewakes Dec 3, 2024

bewakes Dec 3, 2024 •

edited

Loading

bewakes Dec 3, 2024

bewakes Dec 3, 2024

bewakes left a comment

delbonis commented Dec 3, 2024

	/// * Reassigns deposits that have passed their deadling to new operators.
	/// * Reassigns deposits that have passed their deadline to new operators.

	// shitty because it might not account for available funds but it works for
	// XXX because it might not account for available funds but it works for

	// shitty because it might not account for available funds but it works for
	// suboptimal because it might not account for available funds but it works for

		let epoch_init_slot = get_epoch_initial_slot(epoch, params);
		epoch_init_slot + (params.target_l2_batch_size - 1)

	let epoch_init_slot = get_epoch_initial_slot(epoch, params);
	epoch_init_slot + (params.target_l2_batch_size - 1)
	get_epoch_initial_slot(epoch + 1, params) - 1

		for tx in e.deposit_update_txs() {
		if let ProtocolOperation::Deposit(deposit_info) = tx.tx().protocol_operation() {

STR-718 Rework L1 access from OL STF #469

Are you sure you want to change the base?

STR-718 Rework L1 access from OL STF #469

Conversation

delbonis commented Nov 13, 2024

Description

Type of Change

Checklist

codecov bot commented Nov 14, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bewakes Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bewakes Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bewakes left a comment

Choose a reason for hiding this comment

delbonis commented Dec 3, 2024

codecov bot commented Nov 14, 2024 •

edited

Loading

bewakes Dec 3, 2024 •

edited

Loading

bewakes Dec 3, 2024 •

edited

Loading