Add agave-scheduler-bindings crate by apfitzge · Pull Request #7823 · anza-xyz/agave

apfitzge · 2025-09-02T15:04:08Z

Problem

scheduler-bindings relies on passing messages between external-pack, tpu, workers, etc.

Summary of Changes

Add message types and constants for scheduler-bindings
These are NOT complete, and some additional flags, tags, etc will be added as additional features are added. These represent a near-minimal set of message types for a functional external pack process to be run and communicate with agave to produce blocks

Fixes #

apfitzge · 2025-09-02T15:29:17Z

+    pub transactions: [SharableTransaction; MAX_TRANSACTIONS_PER_PACK_MESSAGE],
+}
+
+pub mod pack_message_flags {


Flags for supporting bundles:

DROP_ON_FAILURE

ALL_OR_NONE
will be added here once the functionality is supported.

Additional flags for:

checking balance

checking status (already processed or not)

outside of any slot (similar to RESOLVE) will also likely be added

Granularity is great but in general I think any scheduler is interested in 'is this still valid' so that includes basically everything in recv_and_buffer; If can enumerate everything separately; great; might never be used individually though

yeah, i'm planning to add the checking balance and status here as mentioned above. but it'll likely support having all 3 flags set to check/perform all of them.

apfitzge · 2025-09-02T15:40:47Z

+    pub num_transactions: u8,
+    /// Transactions in the message. Only the first `num_transactions`
+    /// entries are valid to read.
+    pub transactions: [SharableTransaction; MAX_TRANSACTIONS_PER_PACK_MESSAGE],


Thought about making this an allocation from the external pack, which would allow us an arbitrary number of txs per message.
But I don't think there's enough value in that vs the static sized array to avoid additional indirection.

We could start with variable length txs if unsure MAX_TRANSACTIONS_PER_PACK_MESSAGE will change often in the future.

codecov-commenter · 2025-09-02T17:22:40Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.0%. Comparing base (b8dba5a) to head (7cb9c3a).
⚠️ Report is 1748 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##           master    #7823     +/-   ##
=========================================
- Coverage    83.0%    83.0%   -0.1%     
=========================================
  Files         808      808             
  Lines      356265   356265             
=========================================
- Hits       296043   296006     -37     
- Misses      60222    60259     +37

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

dgski · 2025-09-04T15:47:39Z

+    /// begins.
+    pub progress: i16,
+    /// The number of compute units packed in the current block, if leader.
+    pub total_compute_units: u64,


Should this be total_compute_units_left? Signals capacity (easier forward compatibility for CU Limit increases)

That's a great idea!

made it remaining_cost_units since these are actually cost_units not compute_units 😬

dgski · 2025-09-04T15:51:37Z

+/// Message: [Worker -> Pack]
+/// Message from worker threads in response to a [`PackToWorkerMessage`].
+/// [`PackToWorkerMessage`] may have multiple response messages that
+/// will follow the order of transactions in the original message.


Might be easier to reason about things if the response message contains results for everything in one 'batch'

the actual message types here are somewhat large, if we had a static array of the tagged-unions, total batch size would be 2048 bytes. If doing small batch sizes of say 3 messages (bundles) then the messages would be 90% useless. Effectively that would make it necessary to increase the size of the queue to accomodate smaller batches.

While the responses for each tx are in different messages, agave will always commit them as a batch. i.e. if you sent a batch of 10 txs, you will not receive 8 of the responses but not the last 2; it's always 10 or none that are visible (the shared queue writer position) to the external scheduler.

if all possible, would be great to stick with one response per request. Speak of which, is Packer expected to have a timer or something when wait for response from Workers, or it will hold on to original message/txs until response is received?

if all possible, would be great to stick with one response per request.

See comment above - it's possible but wastes an incredible amount of space or causes additional indirection. It's a loop either way, this is a simpler approach that creating a pointer which then needs to be freed.

expected to have a timer

Not sure what value having a timer would have, wdym? Pack sends messages with transactions, worker will respond in order.

or it will hold on to original message/txs until response is received?

It doesn't need to hold onto anything from the original message, except ALT pubkey ptrs if it doesn't want to re-resolve for unlocking the keys in it's internal lock-tracking (if any).

what packer should do in case worker never responded (due to unexpected cause)?

If it never responds or responds in a different order it indicates a bug in the worker. So it'd probably be reasonable to shutdown the external scheduler to let agave fall back to internal scheduler, or to make that switch manually.

dgski · 2025-09-04T15:53:21Z

+    /// If [`pack_message_flags::RESOLVE`] flag is not set, this is the slot
+    /// the transactions can be processed in. If the flag is set to
+    /// [`ANY_SLOT`], the transactions can be processed in any slot in agave.
+    pub slot: u64,


Could be max_slot for flexibility

🤔 yeah that seems reasonable. then I don't even need to declare ANY_SLOT as a special flag...that's implied!

dgski · 2025-09-04T15:58:30Z

+        /// The transaction that was included.
+        pub transaction: SharableTransaction,
+        /// Compute units used by the transaction.
+        pub compute_units: u64,


I think need account_data_size_loaded too for proper CostModel updates

yeah this should be cost-units not compute-units

CUs vs CUs 😵

tao-stones · 2025-09-05T19:22:19Z

+
+/// Reference to a transaction that can shared safely across processes.
+#[repr(C)]
+pub struct SharableTransaction {


nit: I found calling it "...Transaction" is bit misleading, and we have so many "transaction" types already. What do you think call it "SharableTransactionRegion" or "SharableTransactionDescriptor"?

tao-stones · 2025-09-05T21:07:53Z

+/// Message: [Worker -> Pack]
+/// Message from worker threads in response to a [`PackToWorkerMessage`].
+/// [`PackToWorkerMessage`] may have multiple response messages that
+/// will follow the order of transactions in the original message.


if all possible, would be great to stick with one response per request. Speak of which, is Packer expected to have a timer or something when wait for response from Workers, or it will hold on to original message/txs until response is received?

tao-stones · 2025-09-05T21:50:49Z

+    /// the transaction will not be processed.
+    pub max_execution_slot: u64,
+    /// The number of transactions in this message.
+    /// MUST be in the range [1, [`MAX_TRANSACTIONS_PER_PACK_MESSAGE`]].


How do you think to implement basic validation alone with message type def?

"max_execution_slot MUST be in the range [1, [MAX_TRANSACTIONS_PER_PACK_MESSAGE]]" is a good example for impl MessageValidation for PackToWorkerMessage {...}

I'm really not sure what you mean. It'll be an if statement lol

Oh I see what you mean, you mean have validation inside this file.

I'm against that. I've intentionally kept any functions out of this file, it is intended only as type definitions.

edit: This file is intended to be a defintion of the binary interface. Any functionality should not be in here imo.

yea, I see your point. Ideally the message type doesn't have such subtle inner logic. Maybe it can be a reason to remove MAX_TRANSACTIONS_PER_PACK_MESSAGE, have Packer to dynamically reserve transactions?

tao-stones · 2025-09-05T22:02:38Z

+    pub num_transactions: u8,
+    /// Transactions in the message. Only the first `num_transactions`
+    /// entries are valid to read.
+    pub transactions: [SharableTransaction; MAX_TRANSACTIONS_PER_PACK_MESSAGE],


We could start with variable length txs if unsure MAX_TRANSACTIONS_PER_PACK_MESSAGE will change often in the future.

tao-stones

Thanks for explaining and updating, it looks so much better 🔥

Wondering your think on this, in case for some reason, Workers got backed up, Packer can't release Transactions, then can cascade to TPU can't reserve space for new Transaction. Add a TTL field to Pack->Work message can help in that particular case. wdyt?

apfitzge · 2025-09-09T14:56:03Z

Wondering your think on this, in case for some reason, Workers got backed up, Packer can't release Transactions, then can cascade to TPU can't reserve space for new Transaction. Add a TTL field to Pack->Work message can help in that particular case. wdyt?

Yeah there will be some logic inside of agave to fall back to an internal scheduler.
If we cannot reserve messages in queues, if we cannot allocate space in transactions, etc.
Set a signal to shutdown the threads, and then BS will respawn with the default scheduler.

tao-stones

thanks for documenting the workflows 👍🏼

tao-stones · 2025-09-12T01:03:51Z

+    /// this offset. The type of each inner message is indicated by `tag`.
+    /// There are `num_transaction_responses` inner messages.
+    /// See [`worker_message_types`] for details on the inner message types.
+    pub transaction_responses_offset: u32,


nit: to keep the existing pattern elsewhere, these two fields can be defined as a "region"?

struct TransactionResponseRegion { transaction_responses_offset: u32, num_transaction_responses: u8, }

tao-stones

The Packer ↔ Worker flow is much clearer now. Let me try to summarize it:

Packer → Worker: sends a TransactionBatchRegion, with process instructions such as vanilla execution (None) or resolve.
Expectation: Packer expects a single response message from Worker.
Worker → Packer: replies with the same TransactionBatchRegion (so Packer doesn’t need to retain a local copy) along with a ResponseRegion. The reply indicates whether the original Packer2Worker message was invalid, or, if valid, provides per-transaction processing results.

Hope that's accurate?

tao-stones · 2025-09-12T18:19:20Z

+    /// Tag indicating the type of message.
+    /// See [`worker_message_types`] for details.
+    /// If the tag is [`worker_message_types::INVALID_MESSAGE`],
+    /// no inner messages will be present and `num_transaction_responses`


note: would be useful to provide reasons of why original Packer2Worker message was invalid?

tao-stones · 2025-09-12T19:38:22Z

-    /// Tag indicating [`InvalidMessage`] inner message.
+    /// Tag indicating the sent message was invalid.
+    /// No inner messages will be present.
    pub const INVALID_MESSAGE: u8 = 0;


per early suggestion, this can be removed

tao-stones · 2025-09-12T19:51:07Z


    pub mod not_included_reasons {
+        /// The transaction was included in the block - i.e. no reason it was not included.
+        pub const NONE: u8 = 0;


nerdy nit: the double negative ("no reason it was not included" == "included, cost_units and fee_payer_balance can be looked at"). I don't know what's good way to straight it out without adding an additional field.

tao-stones

thanks for addressing all the comments. it looks great

apfitzge force-pushed the scheduler-bindings-messages branch 4 times, most recently from f8a3896 to ec77626 Compare September 2, 2025 15:24

apfitzge commented Sep 2, 2025

View reviewed changes

apfitzge added 3 commits September 2, 2025 10:34

agave-scheduler-bindings

a664b45

pack_message_flags mod comment

731b742

add not_included_reasons

8f8a871

apfitzge force-pushed the scheduler-bindings-messages branch from ec77626 to 8f8a871 Compare September 2, 2025 15:34

apfitzge changed the title ~~agave-scheduler-bindings~~ Add agave-scheduler-bindings crate Sep 2, 2025

num_transactions as u8

310923d

apfitzge commented Sep 2, 2025

View reviewed changes

apfitzge marked this pull request as ready for review September 2, 2025 16:38

apfitzge requested a review from tao-stones September 2, 2025 16:38

apfitzge added 2 commits September 4, 2025 09:04

Add src_addr to TpuToPackMessage

e3fb490

tpu_message_flags

738e2ba

dgski reviewed Sep 4, 2025

View reviewed changes

apfitzge added 2 commits September 4, 2025 11:00

remaining_cost_units

c7ec9c9

max_execution_slot

c10df52

tao-stones reviewed Sep 5, 2025

View reviewed changes

apfitzge added 4 commits September 8, 2025 08:54

Module comment on responsibility of setting up shmem

2a855ed

SharableTransactionRegion

c64ed8f

SharableTransactionBatchRegion + passback on response

8457f8b

flow comment

8125c7c

apfitzge force-pushed the scheduler-bindings-messages branch from 7696b96 to 8125c7c Compare September 8, 2025 16:51

tao-stones reviewed Sep 8, 2025

View reviewed changes

basic arch & message passing

1be3ecf

apfitzge force-pushed the scheduler-bindings-messages branch from 520ae84 to 1be3ecf Compare September 9, 2025 14:57

ProgressMessage - current slot + next_leader_slot

5996036

apfitzge requested a review from tao-stones September 11, 2025 22:02

tao-stones reviewed Sep 12, 2025

View reviewed changes

apfitzge added 3 commits September 12, 2025 15:01

ExecutionResponse

6eccb80

WorkerToPackMessage::processed

0fcc309

remove confusing comment

7cb9c3a

apfitzge force-pushed the scheduler-bindings-messages branch from def658d to 7cb9c3a Compare September 12, 2025 20:11

apfitzge requested a review from tao-stones September 22, 2025 16:22

tao-stones approved these changes Sep 22, 2025

View reviewed changes

apfitzge merged commit 1140c26 into anza-xyz:master Sep 23, 2025
54 checks passed

apfitzge deleted the scheduler-bindings-messages branch September 23, 2025 21:08

Conversation

apfitzge commented Sep 2, 2025

Problem

Summary of Changes

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!

codecov-commenter commented Sep 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

apfitzge Sep 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

tao-stones left a comment

Choose a reason for hiding this comment

Uh oh!

apfitzge commented Sep 9, 2025

Uh oh!

tao-stones left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov-commenter commented Sep 2, 2025 •

edited

Loading

apfitzge Sep 6, 2025 •

edited

Loading