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

refactor(conductor): make firm, soft readers subtasks #1926

Merged
merged 3 commits into from
Feb 3, 2025

Conversation

SuperFluffy
Copy link
Member

@SuperFluffy SuperFluffy commented Jan 24, 2025

Summary

Makes the firm and soft readers subtasks of the main exection loop.

Background

Making the celestia (firm) and astria (soft)
readers subtasks of the executor loop is a more
faithful representation of their dependencies:

  1. the execution loop can run with either or both present.
  2. but the readers cannot run without the
    execution loop present (the tasks do not run at the
    same level of privilege).
  3. to fully initialize they the readers depend on
    data provided via the execution loop (rollup genesis
    info and commitment state).

This patch then spins up the readers during initialization
of the execution event loop, which allows removing a lot
of complexity from the conductor codebase:

  1. the readers need not explicitly wait for the state to be
    initialized but receive an already initialized state.
  2. there is no need for a bespoke channel to dynamically
    set permits (this was needed for dynamic backpressure
    handling by setting the channel capacity after reading the
    celestia variance from the rollup): a standard mpsc channel
    can be instantiated and then passed to the (soft/sequencer)
    reader.

executor::Initialized::run delegates to
executor::Initialized::run_event_loop to separate
the shutdown token from the other arms of the
select macro - this way, an else => {} arm can be
introduced that shuts down executor as a fallback

Changes

  • make the firm (celestia) and soft (sequencer) readers subtasks of the executor task
  • remove the JoinMap from the conductor::Inner business logic as this is
    now tracked by the executor task
  • initialize the reader tasks only after the initial rollup node state is received
  • in the executor task, separate the test for shutdown token cancellation
    from its main event loop so that the tokio::select! macro's else => {} arm
    can be used as a fallback.
  • remove the bespoke executor::channel for dynamically setting channel
    capacity at runtime (no longer necessary; the channel is initialized later
    after we know its capacity)
  • simplify the State watch channel notifying the readers of the latest rollup
    state by removing the StateIsInit and StateNotInit markers (the readers
    now get a fully initialized watch channel).

Testing

These changes should be transparent and not noticeable. The blackbox tests
still pass which implies that the (tested) conductor behavior works.

Changelogs

NOTE: Update before merge

Ensure all relevant changelog files are updated as necessary. See
keepachangelog for change
categories. Replace this text with e.g. "Changelogs updated." or "No updates
required." to acknowledge changelogs have been considered.

Related Issues

NOTE: Link the forma issue?

Link any issues that are related, prefer full GitHub links.

closes

@github-actions github-actions bot added the conductor pertaining to the astria-conductor crate label Jan 24, 2025
@SuperFluffy SuperFluffy marked this pull request as ready for review January 24, 2025 14:53
@SuperFluffy SuperFluffy requested a review from a team as a code owner January 24, 2025 14:53
@SuperFluffy SuperFluffy requested a review from noot January 24, 2025 14:53
Copy link
Contributor

@ethanoroshiba ethanoroshiba left a comment

Choose a reason for hiding this comment

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

Few nits, but otherwise LGTM. I think this refactor will be very nice to have!

crates/astria-conductor/src/executor/mod.rs Outdated Show resolved Hide resolved
crates/astria-conductor/src/celestia/mod.rs Show resolved Hide resolved
crates/astria-conductor/src/executor/mod.rs Show resolved Hide resolved
crates/astria-conductor/src/executor/mod.rs Outdated Show resolved Hide resolved
crates/astria-conductor/src/executor/mod.rs Outdated Show resolved Hide resolved
crates/astria-conductor/src/sequencer/mod.rs Outdated Show resolved Hide resolved
crates/astria-conductor/src/sequencer/mod.rs Show resolved Hide resolved
Copy link
Contributor

@Fraser999 Fraser999 left a comment

Choose a reason for hiding this comment

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

One minor comment only.

crates/astria-conductor/src/conductor/inner.rs Outdated Show resolved Hide resolved
Base automatically changed from superfluffy/fix-conductor-restart to main February 3, 2025 12:59
making the celestia (firm) and astria (soft)
readers subtasks of the executor task is a more
faithful representation of their dependencies:

executor can run with either or both present.
But the reader tasks cannot run without the
executor task present. To fully initialize they
also depend on data from the executor, and they
could be implemented by streams instead of
free standing tasks.

spin up readers only after commitment, genesis states are received

this allows removing a lot of complexity:

1. the readers need not explicitly wait for the state
to be initialized but receive an already initialized
watch channel.
2. there is no need for a bespoke channel to dynamically
set permits - a normal mpsc channel can be used with
its capacity initialized after receiving the genesis info.

executor::Initialized::run delegates to
executor::Initialized::run_event_loop to separate
the shutdown token from the other arms of the
select macro - this way, an else => {} arm can be
introduced that shuts down executor as a fallback
@SuperFluffy SuperFluffy force-pushed the superfluffy/factor-readers-into-executor branch from 7314f8a to 6a69de9 Compare February 3, 2025 14:48
@SuperFluffy SuperFluffy enabled auto-merge February 3, 2025 16:53
Copy link
Member

@joroshiba joroshiba left a comment

Choose a reason for hiding this comment

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

stamp

@SuperFluffy SuperFluffy added this pull request to the merge queue Feb 3, 2025
Merged via the queue into main with commit 77d0217 Feb 3, 2025
46 checks passed
@SuperFluffy SuperFluffy deleted the superfluffy/factor-readers-into-executor branch February 3, 2025 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conductor pertaining to the astria-conductor crate override-freeze
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants