Skip to content

fix(lean): stop treating slot starts as proposals - #1541

Open
perfogic wants to merge 1 commit into
ReamLabs:masterfrom
perfogic:fix/lean-clock-proposal-signal
Open

fix(lean): stop treating slot starts as proposals#1541
perfogic wants to merge 1 commit into
ReamLabs:masterfrom
perfogic:fix/lean-clock-proposal-signal

Conversation

@perfogic

Copy link
Copy Markdown
Collaborator

What was wrong?

LeanChainService passed is_slot_start as the has_proposal argument to tick_interval:

tick_interval(is_slot_start, self.is_aggregator())

At the start of a slot, is_slot_start is true on every synced node, not just the proposer.
On a normal non-proposer node, clock_prebuilt_for remains None, so the prebuilt-block guard evaluates to false and execution enters the else branch.

if is_slot_start && self.clock_prebuilt_for == Some(wall_slot) {
   self.clock_prebuilt_for = None;
} else {
   self.store.write().await.tick_interval(is_slot_start, self.is_aggregator()).await?;
}

It then calls tick_interval(true, ...), where that true is interpreted as has_proposal. This causes the node to call accept_new_attestations() at interval 0 and move its new attestations into the known pool too early.

pub async fn tick_interval(...){
        let current_interval = {
            let time_provider = self.store.lock().await.time_provider();
            let time = time_provider.get()? + 1;
            time_provider.insert(time)?;
            time % INTERVALS_PER_SLOT
        };

        if current_interval == 0 {
            if has_proposal {
                self.accept_new_attestations().await?;
            }
        }
...
}

This will move the attestation from latest_new_aggregated_payloads_provider to latest_known_aggregated_payloads_provider.
As a result, update_safe_target() can miss votes that update_head() still sees, allowing the head to advance while safe-target and finality progress lag.

For example, suppose the safe-target threshold is 67 out of 100 validators:

  1. At the slot boundary, latest_new_aggregated_payloads contains 70 votes.
  2. The ordinary clock tick passes has_proposal = true.
  3. accept_new_attestations() drains those 70 votes into latest_known_aggregated_payloads.
  4. At interval 3, update_safe_target() reads only the new pool and can no longer see those votes.
  5. update_head() still reads the known pool, so the head can advance while safe-target and finality progress lag or stall.
    Besides, actual proposal handling already accepts new attestations through get_proposal_head(). Therefore, slot start and proposal production must be signalled independently.

How was it fixed?

  • Ordinary clock ticks now always pass has_proposal = false.
  • The existing prebuilt-block clock handling remains unchanged, preventing the store clock from advancing twice when a block was built ahead of its slot.
  • Proposal-specific attestation acceptance remains in get_proposal_head().
  • Added a regression test proving that:
    • an ordinary slot-start tick does not drain the new-attestation pool; and
    • the real proposal path moves those attestations into the known pool.

Verification performed:

cargo fmt --check -- crates/common/chain/lean/src/service.rs
cargo test -p ream-chain-lean test_non_proposer_clock_tick_does_not_accept_new_attestations_at_slot_start
git diff --check

To-Do

@perfogic perfogic self-assigned this Jul 28, 2026
@perfogic
perfogic marked this pull request as ready for review July 29, 2026 03:20
@perfogic

Copy link
Copy Markdown
Collaborator Author

@KolbyML

KolbyML commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

what is this PR fixing exactly?

@perfogic

perfogic commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator Author

Hi @KolbyML, I described it on the What's wrong.
It fixes when pending attestations are promoted from the new pool to the known pool.
Before this PR, every synced node did this at the start of every slot:

tick_interval(true, ...)

because is_slot_start was passed as has_proposal in the tick_interval function.

For easier review, it is this part

image

changing is_slot_start to always false - the only part that it can be true is in get_proposal_head(), i think

@KolbyML

KolbyML commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Hi @KolbyML, I described it on the What's wrong. It fixes when pending attestations are promoted from the new pool to the known pool. Before this PR, every synced node did this at the start of every slot:

tick_interval(true, ...)

because is_slot_start was passed as has_proposal in the tick_interval function.

For easier review, it is this part

image changing `is_slot_start` to always `false` - the only part that it can be `true` is in `get_proposal_head()`, i think

What is the bug though? I can read what the PR is doing

@perfogic

Copy link
Copy Markdown
Collaborator Author

@KolbyML Correct me if i'm wrong, I think the bug is the case that a non-proposer can skip valid votes when computing its safe target - this can happen when aggregation does not finish before interval 4.

The aggregation job starts at interval 2, but proof generation runs asynchronously and can take long enough to finish after interval 4.
When that happens, aggregate_apply() inserts the completed proof into latest_new_aggregated_payloads after the normal interval-4 acceptance has already passed, so it stays there until the next slot.

On a non-proposer node, the next slot-start tick was incorrectly treated as has_proposal = true, moving that proof into the known pool at interval 0 through accept_new_attestations - which move from new to known attestations.
After that, when interval 3 runs, update_safe_target() cannot see it because it reads only the new pool.

@perfogic

perfogic commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator Author

Just paste a case that it can be viewed on ci that i believe it comes from the bug i described above - https://github.com/ReamLabs/ream/actions/runs/30422733109/job/90482826834?pr=1542:
image

Thank you for taking time reviewing it

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.

2 participants