Skip to content

fix(zakura): keep serving during legacy fallback#488

Merged
p0mvn merged 6 commits into
ironwood-mainfrom
fix/zakura-fallback-bridge
Jul 6, 2026
Merged

fix(zakura): keep serving during legacy fallback#488
p0mvn merged 6 commits into
ironwood-mainfrom
fix/zakura-fallback-bridge

Conversation

@p0mvn

@p0mvn p0mvn commented Jul 6, 2026

Copy link
Copy Markdown

Motivation

Dual-stack Zakura fallback should not turn a fallback node into a dead Zakura peer. When the stall watchdog resumes legacy ChainSync, the node can still serve headers/bodies and advertise local state to Zakura-only peers.

Solution

  • Keep Zakura header- and block-sync reactors alive when legacy fallback engages.
  • Add a ZakuraApplyGate so fallback stops new Zakura body applies and drains in-flight applies before legacy ChainSync drives commits.
  • Update fallback tests to lock in the bridge-mode contract and add a changelog entry.

Tests

  • cargo fmt --all
  • cargo test -p zebrad --lib components::sync
  • cargo test -p zebrad --lib block_sync_driver

Specifications & References

Follow-up Work

  • None for this PR.

AI Disclosure

  • No AI tools were used in this PR
  • AI tools were used: Cursor/GPT-5.5 for implementation, test updates, and PR preparation under author direction.

PR Checklist

  • The PR title follows conventional commits format: type(scope): description
  • The PR follows the contribution guidelines.
  • This change was discussed in an issue or with the team beforehand.
  • The solution is tested.
  • The documentation and changelogs are up to date.

Keep Zakura reactors alive when the dual-stack watchdog falls back to legacy ChainSync, while draining the Zakura block-apply pipeline so only one body committer drives state.
@v12-auditor

v12-auditor Bot commented Jul 6, 2026

Copy link
Copy Markdown

Note

Complete: Audit complete. V12 found one issue worth reviewing.

Open the full results here.

FindingSeverityDetails
F-96442 🟠 High
Fallback queues unbounded bodies

After legacy fallback yields the Zakura apply gate, the block-sync driver still accepts every BlockSyncAction::SubmitBlock from the live reactor and stores the decoded body in pending_applies. The queue is an unbounded VecDeque of PendingBlockApply entries that each retain an Arc<Block>, and the SubmitBlock arm pushes into it before checking the gate. Once apply_gate.is_yielded() is true, drain_pending_block_applies returns immediately, so those queued bodies are neither applied nor freed and no BlockApplyFinished acknowledgement is sent. The fallback path deliberately keeps Zakura reactors alive while legacy ChainSync runs, and the sequencer can continue submitting bodies when legacy frontier advances release applied slots. A remote Zakura peer that serves valid requested bodies during fallback can therefore make the driver consume the bounded action stream into an unbounded local block-body queue.

And one more auto-invalidated finding.

Analyzed four files, diff da19234...91f5a4f.

@p0mvn

p0mvn commented Jul 6, 2026

Copy link
Copy Markdown
Author

@cursor review

Comment thread zebrad/src/commands/start/zakura/mod.rs
Comment thread zebrad/src/commands/start/zakura/block_sync_driver.rs Outdated
p0mvn added 3 commits July 6, 2026 01:17
Register the apply-gate drain waiter before checking in-flight applies so the final release cannot be missed and delay legacy fallback by the timeout.
Drain queued Zakura block applies once the legacy fallback gate is yielded so the bridge does not retain block bodies or token accounting for work it will no longer commit.
@p0mvn

p0mvn commented Jul 6, 2026

Copy link
Copy Markdown
Author

@cursor review

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 1c11595. Configure here.

@evan-forbes evan-forbes left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

nice, very sensible

certainly not going to block on tech debt or readability as the repo is fubar, just to note that imo we could get an easier to read implementation here.

instead of the other methods and "apply gate", something resembling

  impl BlockSyncHandoff {
      async fn yield_to_legacy(&self) {
          self.stop_new_applies();
          self.wait_for_applies().await;
      }
  }

this way we're being explicit with what our goals are with yielding and apply gating, which consolidating things

our ai naming is very very extremely bad in zakura, and to a large extent I gave up fixing each time... but fwiw these names are confusing.

  • release_yielded_pending_applies
  • yielded_pending_apply_finished_events
  • yielded_block_apply_finished_event
  • abandon_yielded_block_apply

even just shortening them slightly to

  • release_pending_applies
  • yielded_pending_apply_finished_events
  • yielded_block_apply_finished_event
  • abandon_block_apply

helps my brain

if we choose to fix in the future I'd opt to the refactor mentioned earlier

as for testing

we don't currently have a fallback integration/e2e test, which given the issues we've had with it could prove useful. although it doesn't seem like we're triggering the zakura e2e when we're modifying things here 🤔

@p0mvn

p0mvn commented Jul 6, 2026

Copy link
Copy Markdown
Author

@evan-forbes feedback has been integrated: abstraction and tests

@p0mvn
p0mvn merged commit 7cacaf4 into ironwood-main Jul 6, 2026
47 of 48 checks passed
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