feat(solana-indexer): PR 4 — Component structs (skeleton declarations)#4514
feat(solana-indexer): PR 4 — Component structs (skeleton declarations)#4514tilacog wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces the skeleton structure for the Solana settlement indexer components, including the Ingester, Decoder, PartialEventWatchdog, and FinalizationWorker, along with necessary dependency updates and shared type definitions. Feedback on the changes suggests replacing the global static LATEST_CHAIN_SLOT with a shared Arc passed via constructors to avoid process-wide shared mutable state and potential race conditions during parallel testing.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| /// The sole writer is the ingester, on every slot-filter message. Anchors the | ||
| /// partial-event watchdog and the finalization worker. Cold start is zero; the | ||
| /// watchdog skips its comparison on the first tick. | ||
| pub static LATEST_CHAIN_SLOT: AtomicU64 = AtomicU64::new(0); |
There was a problem hiding this comment.
Using a global static AtomicU64 for LATEST_CHAIN_SLOT introduces shared mutable state across the entire process. This causes race conditions and flakiness when running unit/integration tests in parallel (Cargo's default behavior). It also prevents running multiple indexer instances in the same process.
Actionable Suggestion:
Remove the global static and instead pass an Arc<AtomicU64> (or a shared state struct) to the constructors of Ingester, Decoder, PartialEventWatchdog, and FinalizationWorker.
References
- Focus exclusively on identifying missing edge cases, potential race conditions, or logic that deviates from the PR's stated goals. (link)
|
This pull request has been marked as stale because it has been inactive a while. Please update this pull request or it will be automatically closed. |
| /// Typical number of slots for a transaction to finalize (~12.8 s). The | ||
| /// promotion pass skips rows fresher than this. |
There was a problem hiding this comment.
What does typical mean here? In which circumstances are 32 slots NOT correct?
|
|
||
| /// Typical number of slots for a transaction to finalize (~12.8 s). The | ||
| /// promotion pass skips rows fresher than this. | ||
| #[allow(dead_code)] |
There was a problem hiding this comment.
Instead of using #[allow(lint)] please use #[expect(lint)]. The reason is that clippy will automatically generate a warning when the lint is no longer violated which forces you to clean up stale lint exceptions.
|
|
||
| /// Transaction finalization worker. See the module docs for the two flows it | ||
| /// runs. | ||
| pub struct FinalizationWorker<St: Store, R: SolanaClient> { |
There was a problem hiding this comment.
IMO going down the route of generics instead of Arc<dyn Trait> is a slippery slope. Type parameters need to be propagated throughout the code base and the tiny overhead of chasing a pointer when using dyn trait hardly matters if the component issues network requests.
| /// The sole writer is the ingester, on every slot-filter message. Anchors the | ||
| /// partial-event watchdog and the finalization worker. Cold start is zero; the | ||
| /// watchdog skips its comparison on the first tick. | ||
| pub static LATEST_CHAIN_SLOT: AtomicU64 = AtomicU64::new(0); |
There was a problem hiding this comment.
What's the reason that this value does not live inside a specific component that gets accessed via a getter?
Also the API is currently fragile. The type is pub and allows anyone to access it. A safer API would be a new type wrapping the counter with an update function that is only accessible in this module for example.
| /// Capacity of the channel from the ingester to the decoder. | ||
| pub const INGEST_TO_DECODER_CAPACITY: usize = 1024; |
There was a problem hiding this comment.
This and a few other consts seems like it should be configurable by a config file.
| /// Capacity of the channel from the ingester to the decoder. | ||
| pub const INGEST_TO_DECODER_CAPACITY: usize = 1024; |
There was a problem hiding this comment.
Can you explain why the ingester is necessary? Why is it better to have it forward the events to a different channel? If the other channel it pushed into does not get cleared fast enough the backpressure will end up here anyway.
| /// Sends `StreamUpdate` to the decoder. Should be bounded to | ||
| /// `RECONNECT_BACKOFF_CAP` entries. | ||
| pub tx: Sender<StreamUpdate>, |
There was a problem hiding this comment.
Comment says the size of the channel should be bounded to a unit of time which does not seem to make sense.
| /// Shared in-memory map of partial events keyed by `PartialEventKey`. | ||
| /// | ||
| /// The decoder holds a clone of this `Arc` and both inserts and removes | ||
| /// halves as it processes them. | ||
| pub partials: Arc<DashMap<PartialEventKey, PartialEvent>>, |
There was a problem hiding this comment.
What was the reasoning for using channels for some of the communication but not for this partials map?
| /// The sole writer is the ingester, on every slot-filter message. Anchors the | ||
| /// partial-event watchdog and the finalization worker. Cold start is zero; the | ||
| /// watchdog skips its comparison on the first tick. | ||
| pub static LATEST_CHAIN_SLOT: AtomicU64 = AtomicU64::new(0); |
There was a problem hiding this comment.
It should be documented that this slot number is completely unrelated to the indexing progress of the system. Obviously depends on the rest of the indexer logic but it sounds like a counter that tracks fully processed slots might be more useful.
| /// | ||
| /// The watchdog holds a clone of the same `partials` map, so the two operate on | ||
| /// the same concurrent map without any message passing between them. | ||
| pub struct Decoder<St: Store> { |
There was a problem hiding this comment.
St looks a bit odd to me.
| pub struct Decoder<St: Store> { | |
| pub struct Decoder<S: Store> { |
| /// Key for the shared decoder↔watchdog partials map: the `(slot, signature)` | ||
| /// pair identifying which on-chain event a `PartialEvent` belongs to. | ||
| #[derive(Clone, Copy, Debug, Eq, Hash, PartialEq, PartialOrd, Ord)] | ||
| pub struct PartialEventKey(pub u64, pub Signature); |
There was a problem hiding this comment.
Already mentioned in previous PRs, but having a global Slot type should probably improve readability.
| pub struct PartialEventKey(pub u64, pub Signature); | |
| pub struct PartialEventKey(pub Slot, pub Signature); |
| /// Cap on the exponential backoff between reconnect attempts. | ||
| #[allow(dead_code)] | ||
| pub const RECONNECT_BACKOFF_CAP: std::time::Duration = std::time::Duration::from_secs(30); | ||
|
|
||
| /// Capacity of the channel from the ingester to the decoder. | ||
| pub const INGEST_TO_DECODER_CAPACITY: usize = 1024; |
There was a problem hiding this comment.
Those should probably be a part of some config with default config values, which aligns with the general codebase.
Description
Adds the
indexer/module with skeleton declarations for the four components that will do the actual work of the indexer. Each struct declares its fields, a constructor, and doc comments describing what it will do; therunbodies areunimplemented!and the behavior lands in later PRs.The four components and their roles:
confirmedcommitment level. This worker re-checks them against the chain and promotes them tofinalized, or marks them rolled back if the transaction disappeared. It uses a cheap batched RPC call for recent rows and falls back to one-call-per-row lookups for rows old enough that the batched method no longer reports them.The field declarations also pin down how the components talk to each other: the ingester feeds the decoder over a bounded channel, while the decoder and the watchdog share a concurrent map with no message passing between them. That second decision reshaped the partial-event types, so
types/channel.rsbecametypes/shared.rs.Changes
indexer/ingester.rs: theIngesterstruct. Generic over the connection so unit tests can drive it with a mock (the trait-bound approach chosen in PR 3 instead of a third in-crate trait). Also declares the shared latest-chain-slot counter and the constants for reconnect backoff and channel capacity.indexer/decoder.rs: theDecoderstruct, holding the receiving end of the channel, the shared partial-event map, and the two program ids it filters for.indexer/watchdog.rs: thePartialEventWatchdogstruct, holding the store and its view of the shared partial-event map.indexer/finalization.rs: theFinalizationWorkerstruct, with module docs explaining the two promotion flows and the constants that bound them (finalization window, batch size, retention horizon).types/channel.rstotypes/shared.rsand reworked the partial-event types: instead of payloads sent over a channel, a partial event is now a key (slot + signature) and a value (whichever half arrived first) in the map shared by the decoder and the watchdog.yellowstone-grpc-clientto the workspace (source of the connection trait the ingester is generic over), plusdashmapand a few tokio features to the crate.How to test
cargo check -p solana-indexercargo clippy --locked -p solana-indexer --all-features --all-targets -- -D warningsNo unit tests are included: the structs declare shape only, and every
runbody isunimplemented!until the behavior PRs.This is a follow-up PR to #4508