Skip to content

chore: fix fork_choice test cases #119

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.

Sign up for GitHub

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

Draft
SoarinSkySagar wants to merge 7 commits into
base: devnet-5
Choose a base branch
from
Draft

chore: fix fork_choice test cases #119

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
66b03e2
ci fix
SoarinSkySagar Jul 26, 2026
167fbaf
fmt
SoarinSkySagar Jul 26, 2026
5b180ae
insert genesis to block_labels
SoarinSkySagar Jul 26, 2026
ce58d9b
increase max attestations data from 8 to 16
SoarinSkySagar Jul 26, 2026
b845954
dupe checks for block attestation data
SoarinSkySagar Jul 26, 2026
a7245ca
update outdated tests
SoarinSkySagar Jul 27, 2026
8250ec2
block_root
SoarinSkySagar Jul 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lean_client/containers/src/state.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ use crate::{

/// Maximum number of distinct AttestationData entries per block (spec: chain/config.py:36).
/// Used by the import-side validation so we accept any spec-valid block from other clients.
pub(crate) const MAX_ATTESTATIONS_DATA: usize = 8;
pub(crate) const MAX_ATTESTATIONS_DATA: usize = 16;

@bomanaps bomanaps Jul 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
pub(crate) const MAX_ATTESTATIONS_DATA: usize = 16;
pub(crate) const MAX_ATTESTATIONS_DATA: usize = 8;

@bomanaps bomanaps Jul 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please do not change this

@ArtiomTr ArtiomTr Jul 27, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Agreed, what's the point of changing this? Looks like spec restricts attestation count to 8?

https://github.com/leanEthereum/leanSpec/blob/0f5b8e58e58c0e155ae8b95eabbbde0b9ad8840a/src/lean_spec/spec/forks/lstar/config.py#L58-L59

@SoarinSkySagar SoarinSkySagar Jul 27, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

3 test cases have blocks for testing with 16 distinct attestation data, and the tests are expecting them to be declared valid by the client. These tests were failing because MAX_ATTESTATIONS_DATA was 8 which is too low. Changing this to 16 makes those 3 cases pass (and no new failing cases)

@SoarinSkySagar SoarinSkySagar Jul 27, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Looks like spec restricts attestation count to 8?

@ArtiomTr looks like there is a mismatch between the tests and the spec then. Which should we follow?

@ArtiomTr ArtiomTr Jul 27, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

which test cases? maybe those test cases are just outdated? do they still exist in latest leanSpec revision?

@SoarinSkySagar SoarinSkySagar Jul 27, 2026 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

  1. test_block_with_maximum_attestations.json
  2. test_attestation_target_justifiable_constraint.json
  3. test_produce_block_enforces_max_attestations_data_limit.json

@SoarinSkySagar SoarinSkySagar Jul 27, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

maybe those test cases are just outdated?

yes. I will update the tests instead of the const


/// Producer-side cap on distinct AttestationData entries when *we* build a block.
/// Lower than the spec ceiling so the per-block Type-2 MultiMessageAggregate prove
Expand Down
11 changes: 11 additions & 0 deletions lean_client/fork_choice/src/handlers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -690,6 +690,17 @@ pub fn on_block(
return Ok(BlockOutcome::AlreadyKnown);
}

let attestations = &signed_block.block.body.attestations;
let distinct = attestations
.into_iter()
.map(|attestation| attestation.data.hash_tree_root())
.collect::<HashSet<_>>()
.len();
ensure!(
distinct == attestations.len_usize(),
"block contains duplicate AttestationData"
);

let parent_root = signed_block.block.parent_root;

if !store.states.contains_key(&parent_root) && !parent_root.is_zero() {
Expand Down
6 changes: 6 additions & 0 deletions lean_client/fork_choice/tests/fork_choice_test_vectors.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -473,13 +473,16 @@ fn forkchoice(spec_file: &str) {
let mut cache = BlockCache::new();
let mut block_labels: HashMap<String, H256> = HashMap::new();

block_labels.insert("genesis".to_string(), store.head);

for (step_idx, step) in case.steps.into_iter().enumerate() {
match step {
ForkChoiceStep::Block {
valid,
checks,
block: test_block,
} => {
let block_root_label = test_block.block_root_label.clone();
let result = std::panic::catch_unwind(AssertUnwindSafe(|| {
let block: Block = test_block.into();
let signed_block = SignedBlock {
Expand Down Expand Up @@ -519,6 +522,9 @@ fn forkchoice(spec_file: &str) {
}

if valid && result.is_ok() {
if let Some(label) = block_root_label {
block_labels.insert(label, *result.as_ref().unwrap());
}
verify_checks(&store, &checks, &block_labels, step_idx).expect(&format!(
"Step: {step_idx}: Should be valid but checks failed"
));
Expand Down
6 changes: 3 additions & 3 deletions lean_client/fork_choice/tests/unit_tests/validator.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -641,9 +641,8 @@ fn produce_and_apply(
let num_validators = store.states[&store.head].validators.len_u64();
let proposer = slot.0 % num_validators;
let _ = keys;
let (_block_root, block, _sigs) =
produce_block_with_signatures(&mut store, slot, proposer, 1, true)
.expect("block production failed");
let (_block_root, block, _sigs) = produce_block_with_signatures(store, slot, proposer, 1, true)
.expect("block production failed");
let signed = SignedBlock {
block,
proof: MultiMessageAggregate::default(),
Expand Down Expand Up @@ -752,6 +751,7 @@ fn test_produce_block_closes_justification_gap() {
&known_block_roots,
&aggregated_payloads,
1,
false,
)
.expect("build_block for sibling block_6 failed");
let signed_block_6 = SignedBlock {
Expand Down
Loading