Skip to content

feat(solana-indexer) PR 8: wire the decoder to the persistence seam - #4676

Open
squadgazzz wants to merge 5 commits into
solana-indexer/PR7.3-interface-parserfrom
solana-indexer/PR8-persist
Open

feat(solana-indexer) PR 8: wire the decoder to the persistence seam#4676
squadgazzz wants to merge 5 commits into
solana-indexer/PR7.3-interface-parserfrom
solana-indexer/PR8-persist

Conversation

@squadgazzz

@squadgazzz squadgazzz commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Description

The decoder produced settlement events and dropped them. This wires them into the Persistence seam: events and the slot watermark flow through persist_events, decode failures land in a dead-letter seam, and reverted transactions stop emitting events. The Persistence bodies stay documented no-ops until the Postgres adapter PR.

Changes

  • Decoder::run hands each transaction's events to persist_events and advances the watermark, or calls write_watermark when a transaction yields no events.
  • The watermark value is slot - 1: stream resume is slot-granular (from_slot = watermark + 1), so marking a slot done while it may still have transactions in flight would skip its remainder after a crash. Redelivery after restart is absorbed by idempotent writes (spec §7, §10).
  • Transactions with meta.err set decode to nothing: a failed Solana transaction rolls back every account write, so the decoder must not emit state-changing events (spec §4). A revert-attribution event is a later PR.
  • Per-transaction decode failures set a flag and record one write_dead_letter(signature, slot, "decoder_error") entry, while the events that did decode still persist and the watermark still advances (spec §12).
  • write_dead_letter added to Persistence, and TransactionError re-exported from the wire types.

How to test

New and updated unit tests.

Related issues

Stacked on #4666. The SQL bodies land with the Postgres adapter PR later.

@squadgazzz squadgazzz changed the title solana-indexer: wire the decoder to the persistence seam feat(solana-indexer) PR 8: wire the decoder to the persistence seam Jul 27, 2026
@squadgazzz squadgazzz changed the title feat(solana-indexer) PR 8: wire the decoder to the persistence seam feat(solana-indexer): PR 8 wire the decoder to the persistence seam Jul 27, 2026
@squadgazzz squadgazzz changed the title feat(solana-indexer): PR 8 wire the decoder to the persistence seam feat(solana-indexer) PR 8: wire the decoder to the persistence seam Jul 27, 2026
@squadgazzz
squadgazzz marked this pull request as ready for review July 28, 2026 12:13
@squadgazzz
squadgazzz requested a review from a team as a code owner July 28, 2026 12:13
@claude

claude Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Claude finished @squadgazzz's task in 4m 21s —— View job


PR Review: wire the decoder to the persistence seam

  • Read changed files and diff against base
  • Analyze decoder/persistence wiring for correctness
  • Review tests
  • Post inline comments and summary

This is a clean, well-reasoned PR. The watermark = slot - 1 design is the subtle part and it holds up: because a slot is never marked done by its own transactions, any write_dead_letter for that slot is always durably written before a later slot advances the watermark past it — so the dead-letter vs. event-persist non-atomicity is genuinely absorbed by idempotent replay, as the description claims. The decode_failed propagation through decode_settlement / decode_settlements_finalized covers every parse/account/pairing failure, and recognized-but-eventless instructions (Initialize, ReclaimOrder) return Ok(vec![]) so they correctly don't trip the flag — only unknown discriminators do.

I left one inline comment: the revert guard treats a missing meta as "not reverted" (falls through to decoding), while post_token_balances a few lines down handles a missing meta defensively — an inconsistency worth pinning down.

Everything else — the reverted-tx skip, the empty-vs-non-empty watermark branch, the TransactionError re-export, and the documented no-op seam bodies — looks correct for this stage of the stack. Test coverage of decode is solid; the run-loop side effects being unobservable through the no-op seam is already acknowledged in the code.

Note: cargo check/nextest are blocked by the sandbox in this environment, so this review is based on static analysis only.

@squadgazzz
squadgazzz requested a review from tilacog July 28, 2026 12:15
// state-changing event may be emitted for it. Failed transactions are
// delivered on purpose (the revert is an attribution signal), and a
// dedicated revert-attribution event is a later PR.
if tx.meta.as_ref().is_some_and(|meta| meta.err.is_some()) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The revert guard treats a missing meta as "not reverted" and falls through to decoding, whereas a few lines down (post_token_balances, L138–142) a missing meta is handled defensively via unwrap_or_default(). That's an inconsistency in how the two paths reason about meta being absent.

If meta can ever be None on a transaction update, we'd emit state-changing events for a transaction whose success we can't actually confirm — the opposite of the safety property this guard is establishing (spec §4). In practice Yellowstone always populates meta for tx updates, so this is likely defensive-only, but if that invariant holds it would be cleaner to expect/error on the absent case; if it doesn't hold, a missing meta arguably should skip (or dead-letter) rather than decode. Worth a one-line comment stating which it is.

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.

1 participant