TQ: Integrate protocol with `NodeTask` #9296

andrewjstone · 2025-10-28T17:45:50Z

This builds on #9258

NodeTask now uses the trust_quorum_protocol::Node and trust_quorum_protocol::NodeCtx to send and receive trust quorum messages. An API to drive this was added to the NodeTaskHandle.

The majority of code in this PR is tests using the API.

A follow up will deal with saving persistent state to a Ledger.

Builds on #9296 This commit persists state to a ledger, following the pattern used in the bootstore. It's done this way because the `PersistentState` itself is contained in the sans-io layer, but we must save it in the async task layer. The sans-io layer shouldn't know how the state is persisted, just that it is, and so we recreate the ledger for every time we write it. A follow up will PR will deal with the early networking information saved by the bootstore, and will be very similar.

trust-quorum/src/task.rs

sunshowers

Didn't look too closely at the tests, but the code itself looks great! just a few minor comments that I'll trust you to resolve :)

sunshowers · 2025-10-30T21:33:50Z

trust-quorum/src/task.rs

+    /// Return `Ok(true)` if the configuration has committed, `Ok(false)` if
+    /// it hasn't committed yet, or an error otherwise.
+    ///
+    /// Nexus will retry this operation and so we should only try once here.
+    /// This is in contrast to operations like `load_rack_secret` that are
+    /// called directly from sled agent.
+    pub async fn prepare_and_commit(


Could you return a two-valued enum here rather than bool?

What is the "otherwise" in "return an error otherwise" here? Just send and receive errors or something else?

~~Also since this doesn't loop I'd consider calling this try_prepare_and_commit.~~ not relevant if we change load_rack_secret to not retry.

Totally. As I put that bool there, I knew you'd ask for this. Next time I'll do it before you ask ;)

sunshowers · 2025-10-30T21:34:12Z

trust-quorum/src/task.rs

+    /// Nexus will retry this operation and so we should only try once here.
+    /// This is in contrast to operations like `load_rack_secret` that are
+    /// called directly from sled agent.
+    pub async fn commit(


Same comments as above.

sunshowers · 2025-10-30T21:34:45Z

trust-quorum/src/task.rs

        Ok(res)
    }

+    pub async fn status(&self) -> Result<NodeStatus, NodeApiError> {


Add a doc comment here?

trust-quorum/src/task.rs

sunshowers · 2025-10-30T21:48:41Z

trust-quorum/src/task.rs

+            for envelope in self.ctx.drain_envelopes() {
+                self.conn_mgr.send(envelope).await;
            }


do we want to do this concurrently, or is serially okay? I guess this shouldn't be cancelled since there's an instruction to make run a top-level task.

sunshowers · 2025-10-30T21:49:47Z

trust-quorum/src/task.rs

        }
    }

+    // TODO: Process `ctx`: save persistent state


What's ctx here?

sunshowers · 2025-10-30T21:50:42Z

trust-quorum/src/task.rs

+    /// Return the status of this node if it is a coordinator
+    CoordinatorStatus { responder: oneshot::Sender<Option<CoordinatorStatus>> },
+
+    /// Load a rack secret for the given epoch
+    LoadRackSecret {
+        epoch: Epoch,
+        responder: oneshot::Sender<
+            Result<Option<ReconstructedRackSecret>, LoadRackSecretError>,
+        >,
+    },


would consider calling all of the oneshot channels tx or similar

sunshowers · 2025-10-31T02:31:17Z

trust-quorum/src/task.rs

+            &poll_interval,
+            &poll_max,


hmm, honestly this should take a Duration, not a reference to it. Worth fixing at some point.

`NodeTask` now uses the `trust_quorum_protocol::Node` and `trust_quorum_protocol::NodeCtx` to send and receive trust quorum messages. An API to drive this was added to the `NodeTaskHandle`. The majority of code in this PR is tests using the API. A follow up will deal with saving persistent state to a Ledger.

Builds on #9296 This commit persists state to a ledger, following the pattern used in the bootstore. It's done this way because the `PersistentState` itself is contained in the sans-io layer, but we must save it in the async task layer. The sans-io layer shouldn't know how the state is persisted, just that it is, and so we recreate the ledger for every time we write it. A follow up will PR will deal with the early networking information saved by the bootstore, and will be very similar.

pietroalbini · 2025-10-30T10:12:26Z

trust-quorum/src/connection_manager.rs

+    pub async fn send(&self, envelope: Envelope) {
+        let Envelope { to, msg, .. } = envelope;
+        info!(self.log, "Sending {msg:?}"; "peer_id" => %to);
+        if let Some(handle) = self.established.get1(&to) {


I was quite confused when I first saw this, since it silently discards the message if a connection with the recipient is not established.

Originally I was going to suggest to rename the function to try_send or similar, but poking at the rest of the code I learned errors are discarded everywhere (EstablishedConn::run only logs the error message and kills the connection, without reporting the failure down the stack).

This makes sense, as in general RFD 238 is designed to be resilient to nodes disappearing at any point in time. I'm not sure if I would do anything in response to this comment. Just leaving this as a note for future me.

pietroalbini · 2025-10-31T15:36:16Z

trust-quorum/src/connection_manager.rs

-    async fn disconnect_client(&mut self, addr: SocketAddrV6) {
+    ///
+    /// Return the `BaseboardId` of the peer if an established connection is
+    // torn down.


Suggested change

// torn down.

/// torn down.

pietroalbini · 2025-10-31T16:29:55Z

trust-quorum/src/task.rs

+        // Tell all but the last node how to reach each other
+        for h in &setup.node_handles {
+            h.load_peer_addresses(setup.listen_addrs.iter().cloned().collect())
+                .await
+                .unwrap();
+        }


While in this case the code comment doesn't reflect the test, this is a more general review comment on these tests.

There is a lot of boilerplate copy/pasted between tests in each test, which makes it hard to see at a glance what a test is actually testing and what is the difference between tests.

We have a TestSetup struct we can add methods to. As an example, replacing connecting nodes with:

setup.connect_nodes(..).await; setup.connect_nodes(1..).await;

...and similar for the rest of the large boilerplate blocks would make it way easier to review tests and make sure we cover every case.

I fully agree. Will do.

andrewjstone force-pushed the tq-sprockets-2 branch 3 times, most recently from 4e7f80b to a505cda Compare October 28, 2025 18:24

andrewjstone mentioned this pull request Oct 29, 2025

TQ: Support persisting state to ledger #9310

Open

andrewjstone mentioned this pull request Oct 29, 2025

Trust Quorum Tracking #8262

Open

19 tasks

andrewjstone requested a review from pietroalbini October 30, 2025 14:59

sunshowers reviewed Oct 30, 2025

View reviewed changes

trust-quorum/src/task.rs Show resolved Hide resolved

sunshowers approved these changes Oct 31, 2025

View reviewed changes

andrewjstone force-pushed the tq-sprockets branch from 809559c to 99e5192 Compare October 31, 2025 15:38

andrewjstone force-pushed the tq-sprockets-2 branch from a505cda to 1ac30a3 Compare October 31, 2025 16:13

pietroalbini reviewed Oct 31, 2025

View reviewed changes

Do not internally retry for load_rack_secret

aaad0c4

TQ: Integrate protocol with NodeTask #9296

Are you sure you want to change the base?

TQ: Integrate protocol with NodeTask #9296

Conversation

andrewjstone commented Oct 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

sunshowers left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

TQ: Integrate protocol with `NodeTask` #9296

TQ: Integrate protocol with `NodeTask` #9296

andrewjstone commented Oct 28, 2025 •

edited

Loading