Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix sequencer crash tests #653

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Fix sequencer crash tests #653

wants to merge 9 commits into from

Conversation

sapinb
Copy link
Contributor

@sapinb sapinb commented Feb 5, 2025

Description

  • fix for crash_advance_consensus_state
    • apply any extra blocks from db before startup in fcm
  • fix for sync_event_new_tip crash
  • move crash_* tests to own group

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
  • Dependency Update

Notes to Reviewers

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.

Related Issues

@sapinb sapinb requested review from a team as code owners February 5, 2025 01:01
Copy link
Contributor

github-actions bot commented Feb 5, 2025

Commit: c869cd2

SP1 Performance Test Results

program cycles success
BTC_BLOCKSPACE 30,370,596
EL_BLOCK 97,975
CL_BLOCK 89,783
L1_BATCH 30,419,814
L2_BATCH 3,566
CHECKPOINT 25,699

@sapinb sapinb force-pushed the fix-sequencer-crash-tests branch from e60a03e to 5e43408 Compare February 5, 2025 08:06
Copy link

codecov bot commented Feb 5, 2025

Codecov Report

Attention: Patch coverage is 23.40426% with 108 lines in your changes missing coverage. Please review.

Project coverage is 54.62%. Comparing base (004be7c) to head (a169b32).

Files with missing lines Patch % Lines
crates/consensus-logic/src/fork_choice_manager.rs 0.00% 44 Missing ⚠️
bin/strata-client/src/el_sync.rs 50.00% 33 Missing ⚠️
bin/strata-sequencer-client/src/duty_executor.rs 0.00% 23 Missing ⚠️
crates/evmexec/src/engine.rs 0.00% 4 Missing ⚠️
bin/strata-client/src/main.rs 0.00% 2 Missing ⚠️
bin/strata-sequencer-client/src/duty_fetcher.rs 0.00% 1 Missing ⚠️
crates/sequencer/src/duty/types.rs 0.00% 1 Missing ⚠️
@@            Coverage Diff             @@
##             main     #653      +/-   ##
==========================================
- Coverage   54.68%   54.62%   -0.06%     
==========================================
  Files         317      318       +1     
  Lines       33929    34050     +121     
==========================================
+ Hits        18553    18601      +48     
- Misses      15376    15449      +73     
Files with missing lines Coverage Δ
crates/consensus-logic/src/csm/worker.rs 0.00% <ø> (ø)
crates/consensus-logic/src/unfinalized_tracker.rs 91.72% <ø> (ø)
bin/strata-sequencer-client/src/duty_fetcher.rs 0.00% <0.00%> (ø)
crates/sequencer/src/duty/types.rs 0.00% <0.00%> (ø)
bin/strata-client/src/main.rs 0.00% <0.00%> (ø)
crates/evmexec/src/engine.rs 65.74% <0.00%> (-0.43%) ⬇️
bin/strata-sequencer-client/src/duty_executor.rs 0.00% <0.00%> (ø)
bin/strata-client/src/el_sync.rs 50.00% <50.00%> (ø)
crates/consensus-logic/src/fork_choice_manager.rs 0.00% <0.00%> (ø)

... and 5 files with indirect coverage changes

Copy link
Contributor

@bewakes bewakes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nits and perhaps engine call could be reduced to a single call instead of log(n) calls. Also perhaps the prints could be replaced with info/debugs. Looks good otherwise.

crates/consensus-logic/src/fork_choice_manager.rs Outdated Show resolved Hide resolved
bin/strata-client/src/el_sync.rs Outdated Show resolved Hide resolved
bin/strata-client/src/el_sync.rs Show resolved Hide resolved
bin/strata-client/src/el_sync.rs Outdated Show resolved Hide resolved
bin/strata-client/src/el_sync.rs Outdated Show resolved Hide resolved
bin/strata-client/src/el_sync.rs Outdated Show resolved Hide resolved
bin/strata-client/src/main.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@delbonis delbonis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something got really weird with the github review and some comments got submitted into multiple separate reviews without me intending to, quality software.

But the actual comment I wanted to make was that there's a few places here where the term idx is being used in a way that could be vague. It would be better to use a more explicit term like slot if possible.

bin/strata-sequencer-client/src/duty_fetcher.rs Outdated Show resolved Hide resolved
@sapinb sapinb force-pushed the fix-sequencer-crash-tests branch from 5e43408 to 3b4bdf3 Compare February 6, 2025 09:33
@sapinb sapinb requested a review from a team as a code owner February 6, 2025 09:33
@sapinb sapinb force-pushed the fix-sequencer-crash-tests branch from 2962acc to a169b32 Compare February 7, 2025 09:54
@sapinb sapinb requested review from delbonis and bewakes February 7, 2025 09:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants