feat(solana-indexer) PR 8: wire the decoder to the persistence seam by squadgazzz · Pull Request #4676 · cowprotocol/services

squadgazzz · 2026-07-27T16:11:29Z

Description

The decoder produced settlement events and dropped them. This wires them into the Persistence trait: per-slot event batches flow through persist_events together with the watermark advance, transactions that fail to decode are dead-lettered whole, and reverted transactions stop emitting events. The Persistence bodies stay documented no-ops until the Postgres adapter PR.

Changes

Decoder::run buffers events and dead letters per slot and flushes a slot once the stream moves FLUSH_HOLDBACK_SLOTS past it, through later transactions or slot-status messages. The hold-back gives late-delivered transactions a window to join their slot's still-open buffer.
A complete flushed slot advances the watermark to itself, resume is slot-granular (from_slot = watermark + 1), and a slot with no events advances it through write_watermark. Redelivery after a restart is absorbed by idempotent writes (spec §7, §10).
A transaction whose decode fails is dead-lettered whole through write_dead_letter: its events are not independent, so none of them persist and recovery replays the transaction by signature (spec §12).
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.
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.

…vior

…test

claude · 2026-07-28T12:13:43Z

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.

…on id to the wire

…out meta

… from the logs

The parsing-generic branch is merged and its ref deleted, so the branch pin would break the next cargo update. Main's interface types the push amount as u64, dropping the byte-array decode on our side.

…arser' into solana-indexer/PR8-persist

Drops the pending-adapter and later-PR breadcrumbs, the spec anchor, and the leftover cast comment resolved by the i64 pass-through.

…runcating The order/push zip silently truncated to the shorter side and the pull sum saturated. Both are layout violations, so the pair is skipped with a warning. Also merges the no-event instruction arms, drops the intermediate push collect, and inlines the test instruction helper.

MartinquaXD · 2026-08-05T08:13:44Z

@@ -1,10 +1,9 @@
-#![expect(dead_code)]
+#![allow(dead_code, reason = "dead in the lib build, exercised by tests")]


Why not gate the respective identifiers with #[cfg(test)] then?

These are dead only until the wiring PR makes the binary call them. I was choosing between the allow line and #[cfg(test)] on each item (not needed in the future PRs). So the latter is just an intermediate step we can avoid.

why the change from expect though? when they are no longer dead_code it would trigger a warning reminding you to remove it — is it doing that for tests?

MartinquaXD · 2026-08-05T08:16:14Z

            } = update;
-            self.decode(&inner, slot, signature);
+            let (events, decode_failed) = self.decode(&inner, slot, signature);
+            tracing::debug!(slot = %slot, event_count = events.len(), "decoded events");


shouldn't we also log the decode failures?

They are logged. Every failure site warns with the error, instruction index, and tx signature, see

services/crates/solana-indexer/src/indexer/decoder.rs

Lines 252 to 259 in fa6488d

let Ok((discriminator, _)) = recover_discriminator(&instruction.data) else {

decode_failed = true;

// Warn, not debug: this dead-letters the transaction, so it needs to

// be findable in the logs alongside the row.

tracing::warn!(

signature = %ctx.signature,

instruction_index = instruction.instruction_index,

err = %DecodeError::UnknownDiscriminator,

The dead-letter row stays minimal (signature + slot), recovery only needs to know what to re-fetch. Right now, a failed tx produces a second warn from the persistence stub saying the write was dropped, which will be removed in the DB adapter PR.

MartinquaXD · 2026-08-05T08:22:13Z

+            // Stream resume is slot-granular (`from_slot = watermark + 1`), and
+            // the slot may still have more transactions in flight, so marking it
+            // done here could skip its remaining transactions after a crash.
+            // Writing `slot - 1` on every transaction only ever marks fully
+            // delivered slots. A redelivery of this slot after a restart is
+            // absorbed by idempotent writes.


Seems like some of the complications (watermark - 1, spammy DB updates) could be resolved by buffering the incoming stream until we have all events for a given slot. Is there a good reason to issue multiple DB writes per slot?

Makes sense. Those are buffered now per slot and flushed once a transaction of a later slot arrives.

MartinquaXD · 2026-08-05T08:23:40Z

+    /// a flag reporting whether any settlement instruction failed to decode.
+    /// The settlement half runs through the pure [`decode_settlement`], the
+    /// SolFlow half is a stub.
+    #[tracing::instrument(skip_all, fields(slot = %slot, signature = %signature))]


I can understand instrumenting the slot but the signature seems unnecessary. This data will be added to every trace downstream. Is the signature really that important?

Not really, dropped it.

I didn't know that you find transactions on the block explorer via the signature (as opposed to the tx hash for EVM). In that case I'd be fine with instrumenting the signature. Sorry for the confusion.

MartinquaXD · 2026-08-05T08:25:19Z

+        // instead of skipping: replay re-fetches by signature, and
+        // `getTransaction` returns the meta.
+        let Some(meta) = tx.meta.as_ref() else {
+            tracing::warn!("transaction update without meta");


We should log information to identify the actual tx.

Yep, we should. Added.

MartinquaXD · 2026-08-05T08:39:13Z

we are iterating over the instructions twice and clone them when we don't have to. There could be a loop pushing an instruction either into a settlement contract vector or a solflow vector.

Replaced with a partition. One pass, no clones.

MartinquaXD · 2026-08-05T08:40:02Z

-            "decoded settlement events"
-        );
+        (
+            events.into_iter().map(DecodedEvent::Settlement).collect(),


why does decode_settlement not already return a DecodedEvent::Settlemen to avoid this conversion here?

decode_settlement handles only the settlement program's events. DecodedEvent is the enum over both SolFlow and settlement programs. If decode_settlement returned DecodedEvent itself, its signature would say it can also emit SolFlow events, which it can't.

MartinquaXD · 2026-08-05T08:43:10Z

    ctx: &TxContext,
    resolve_order: impl Fn(&Pubkey) -> Option<ResolvedOrder>,
-) -> Vec<SettlementEvent> {
+) -> (Vec<SettlementEvent>, bool) {


This interface is very error prone. The function should return a Result so that the caller has to acknowledge that an error happened to get at the partially decoded data (if handling partially decoded transactions even makes sense).

Updated. It now returns Result<Vec<SettlementEvent>, PartialDecode>.

MartinquaXD · 2026-08-05T08:44:09Z

+            // be findable in the logs alongside the row.
+            tracing::warn!(
                instruction_index = instruction.instruction_index,
+                err = %DecodeError::UnknownDiscriminator,


why was all this error handling not added to the previous PR? Would have made reviewing simpler.

True, it should've been done in the previous PR.

MartinquaXD · 2026-08-05T08:48:12Z

+        // No-op seam (no Postgres adapter). The adapter writes the
+        // events and advances the watermark in one SQL transaction: append rows
+        // as INSERT ON CONFLICT DO NOTHING, the watermark UPDATE guarded with
+        // WHERE slot < $new_watermark.
+        Ok(())


what's the reason to replace todo!() with a no-op here? This doesn't seem good.
At the very least we should emit a warning.

run_drains in this PR drives run() over a transaction that fails to decode, which reaches write_dead_letter. With todo!() that test panics. The pipeline test in #4677 then asserts all three writes through a call recorder.

The CreateOrder and BeginSettle/FinalizeSettle fixtures go through the client crate's builders as a dev-dependency, so the tests round-trip the real encoder into our parser instead of hand-rolling bytes. The invalid-instruction fixtures stay hand-rolled: they craft data no builder would produce, and they double as the wire-layout pin.

…arser' into solana-indexer/PR8-persist # Conflicts: # crates/solana-indexer/src/indexer/decoder/tests.rs

…xer/PR8-persist # Conflicts: # crates/solana-indexer/src/indexer/decoder/tests.rs

MartinquaXD · 2026-08-07T07:50:16Z

+            // A transaction of a later slot proves the pending slot is fully
+            // delivered, so it is safe to flush and mark done.


This seems very reasonable to me but we have no experience with the RPC behavior yet. Maybe we should add a log that informs us when we receive events out of order. 🤔

Makes sense. Added a warn log.

I agree we should buffer by slot, but I’m concerned about flushing only when a new Settlement program transaction occurs. This means the latest events for a slot S might not be seen until events from a later slot S+N arrive, where N is arbitrary.

It might be worth adding bare slot‑update messages for confirmed slots to the channel, so a new slot can trigger a flush. (Also, this aligns with our plan to use an unlogged Postgres table to track the chain tip.)

Good catch! At some point, I was thinking about this, but then switched to another task and forgot to finish it. Thanks! Updated the code.

Also, added a two-slot hold-back with per-slot buffers, since a transaction delivered a couple of slots late still joins its own unflushed buffer, which keeps it replayable after a crash.

MartinquaXD · 2026-08-07T07:56:55Z

                // Partial decode is expected: events that did decode persist and
                // the watermark advances. Recovery replays the whole transaction
                // by signature, and idempotent writes absorb the overlap.
                self.persistence.write_dead_letter(signature, slot).await?;


The comment says that this advances the watermark, right?
Don't know for sure but it sounds like that could lead to skipped events when we fail to decode an event and the service crashes before processing the remaining events in the slot.
But it seems like the existing buffering logic should be easy to extend to also buffering the dead_letter stuff.

The skip couldn't quite happen since the watermark only ever advances at the slot flush, never per transaction. But I updated the code as you suggested. Dead letters now buffer with the slot, and the flush writes them first, before the events and the watermark advance, so a crash can never leave the watermark past a slot.

MartinquaXD · 2026-08-07T08:01:49Z

+    /// a flag reporting whether any settlement instruction failed to decode.
+    /// The settlement half runs through the pure [`decode_settlement`], the
+    /// SolFlow half is a stub.
+    #[tracing::instrument(skip_all, fields(slot = %slot, signature = %signature))]


I didn't know that you find transactions on the block explorer via the signature (as opposed to the tx hash for EVM). In that case I'd be fine with instrumenting the signature. Sorry for the confusion.

MartinquaXD · 2026-08-07T08:06:18Z

+        tx: SubscribeUpdateTransactionInfo,
+        slot: Slot,
+        signature: Signature,
+    ) -> (Vec<DecodedEvent>, bool) {


This function has the same issue with the bool indicating an error that can easily be ignored.

Oh yeah, thanks! Updated.

linear-code · 2026-08-07T08:12:11Z

BE-202

…the test doc

…xer/PR8-persist # Conflicts: # crates/solana-indexer/src/indexer/decoder.rs

MartinquaXD

just a nit remaining.

MartinquaXD · 2026-08-10T08:34:31Z

+struct SlotBuffer {
+    slot: Slot,
+    events: Vec<DecodedEvent>,
+    dead_letters: Vec<(Signature, Slot)>,


Why do we have the Slot in the tuples when we already have it on the SlotBuffer?

I'm a bit confused about the meaning of "a late transaction" landing "in a newer slot buffer", can you clarify that please?

Mainly because two can differ when the stream delivers a transaction late. For example, if the buffer is open for slot 100, then a delayed transaction from slot 99 can arrive, so it folds into the slot-100 buffer instead of triggering a flush. If that transaction fails to decode and we stamp its dead-letter row with the buffer's slot, the row would claim it landed in slot 100 while it actually landed in 99. We can technically switch to BTreeMap<Slot, SlotBuffer>, but it looks like unnecessary machinery. Added a comment explaining that.

tilacog

I'd like to understand the design decisions better before approving this.

tilacog · 2026-08-10T20:44:41Z

+            // A transaction of a later slot proves the pending slot is fully
+            // delivered, so it is safe to flush and mark done.


I agree we should buffer by slot, but I’m concerned about flushing only when a new Settlement program transaction occurs. This means the latest events for a slot S might not be seen until events from a later slot S+N arrive, where N is arbitrary.

It might be worth adding bare slot‑update messages for confirmed slots to the channel, so a new slot can trigger a flush. (Also, this aligns with our plan to use an unlogged Postgres table to track the chain tip.)

tilacog · 2026-08-10T23:57:13Z

+struct SlotBuffer {
+    slot: Slot,
+    events: Vec<DecodedEvent>,
+    dead_letters: Vec<(Signature, Slot)>,


I'm a bit confused about the meaning of "a late transaction" landing "in a newer slot buffer", can you clarify that please?

tilacog · 2026-08-11T00:07:32Z

+/// The events that did decode from a transaction in which at least one
+/// instruction failed to decode. The caller persists them and dead-letters
+/// the transaction for replay.
+#[derive(Debug, PartialEq, Eq)]
+struct PartialDecode<T> {
+    events: Vec<T>,
+}
+


I have some reservations about persisting events from partially decoded slots or transactions, since that could compromise the integrity of our indexing state.

I think we're better off scoping success or failure at the transaction level, so we dead-letter a whole transaction rather than a subset of its events.

Yeah, makes sense. A decode failure now discards the whole transaction's events and only the dead letter is written.

…rash-safe

…rsist # Conflicts: # crates/solana-indexer/src/indexer/decoder.rs # crates/solana-indexer/src/indexer/decoder/tests.rs

tilacog · 2026-08-12T17:16:09Z

+/// A transaction delivered up to this many slots late still joins its own
+/// unflushed buffer instead of racing the watermark.
+const FLUSH_HOLDBACK_SLOTS: u64 = 2;
+


I think 2 slots is fine for indexing speed, I'm just not sure whether it would add noticeable lag anywhere else, mainly the autopilot.

I don't know how to gauge this, though.

Given the speed of the chain, I don't expect any issues with just 2 slots. Will be measuring that in prod anyway.

squadgazzz added 3 commits July 24, 2026 08:07

refactor(solana-indexer): decode settlements via shared interface parser

8ebc951

docs(solana-indexer): reword decode comments to describe current beha…

138a48d

…vior

feat(solana-indexer): wire the decoder to the persistence seam

dd95347

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 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 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

test(solana-indexer): fold the decode-wrap and revert cases into one …

77a8d05

…test

squadgazzz marked this pull request as ready for review July 28, 2026 12:13

squadgazzz requested a review from a team as a code owner July 28, 2026 12:13

squadgazzz requested a review from tilacog July 28, 2026 12:15

claude Bot reviewed Jul 28, 2026

View reviewed changes

Comment thread crates/solana-indexer/src/indexer/decoder.rs Outdated

squadgazzz mentioned this pull request Jul 28, 2026

feat(solana-indexer) PR 9: cover the ingester to decoder pipeline end to end #4677

Merged

squadgazzz added 9 commits July 28, 2026 14:02

refactor(solana-indexer): use Slot for slot arguments and match aucti…

a4d7a09

…on id to the wire

fix(solana-indexer): dead-letter transaction updates that arrive with…

16d35c5

…out meta

fix(solana-indexer): make every dead-lettered transaction diagnosable…

ec232d1

… from the logs

Pin settlement-interface to solana-programs main

1e6439f

The parsing-generic branch is merged and its ref deleted, so the branch pin would break the next cargo update. Main's interface types the push amount as u64, dropping the byte-array decode on our side.

Merge remote-tracking branch 'origin/solana-indexer/PR7.3-interface-p…

74bdc50

…arser' into solana-indexer/PR8-persist

Drop semicolons and inference emphasis from decoder comments

d671baf

Merge PR7.3 and sweep comments for standalone reading

6a4c45a

Drops the pending-adapter and later-PR breadcrumbs, the spec anchor, and the leftover cast comment resolved by the i64 pass-through.

Fold the meta-state tests into one and drop the duplicate decode test

0daa1af

MartinquaXD reviewed Aug 5, 2026

View reviewed changes

Reword the settlement fixture comments in plain terms

da359ae

squadgazzz mentioned this pull request Aug 5, 2026

feat(solana-indexer): PR 7.3 decode settlements via the shared interface parser #4666

Merged

squadgazzz and others added 5 commits August 5, 2026 09:49

Merge branch 'main' into solana-indexer/PR7.3-interface-parser

462ad94

Merge remote-tracking branch 'origin/solana-indexer/PR7.3-interface-p…

6a2fbd1

…arser' into solana-indexer/PR8-persist # Conflicts: # crates/solana-indexer/src/indexer/decoder/tests.rs

Buffer decoded events per slot and make partial decodes a typed result

525532a

Fix the unused test imports and the lints section order

dd8d7c5

Merge branch 'solana-indexer/PR7.3-interface-parser' into solana-inde…

fa6488d

…xer/PR8-persist # Conflicts: # crates/solana-indexer/src/indexer/decoder/tests.rs

squadgazzz requested a review from MartinquaXD August 5, 2026 13:12

MartinquaXD reviewed Aug 7, 2026

View reviewed changes

squadgazzz added 3 commits August 7, 2026 12:21

Use a labeled continue for corrupt pairs and fix the push wording in …

451120f

…the test doc

Merge branch 'solana-indexer/PR7.3-interface-parser' into solana-inde…

e0a07e0

…xer/PR8-persist # Conflicts: # crates/solana-indexer/src/indexer/decoder.rs

Buffer dead letters with the slot and type the decode failure

6bd207d

MartinquaXD approved these changes Aug 10, 2026

View reviewed changes

squadgazzz added 2 commits August 10, 2026 13:11

State why dead letters carry their own slot

f4eeeb8

Reword the dead-letter slot comment

9543051

tilacog reviewed Aug 11, 2026

View reviewed changes

Base automatically changed from solana-indexer/PR7.3-interface-parser to main August 11, 2026 09:09

squadgazzz added 4 commits August 11, 2026 09:33

Flush buffered slots on slot-status messages

5de8cdf

Hold slots back two ticks before flushing so late transactions stay c…

e57e5fa

…rash-safe

Dead-letter whole transactions instead of persisting partial decodes

8622517

Merge remote-tracking branch 'origin/main' into solana-indexer/PR8-pe…

4bc629b

…rsist # Conflicts: # crates/solana-indexer/src/indexer/decoder.rs # crates/solana-indexer/src/indexer/decoder/tests.rs

squadgazzz requested a review from tilacog August 11, 2026 10:37

tilacog approved these changes Aug 12, 2026

View reviewed changes

Expect dead code only in the lib build so the wiring reminds us

9b595ea

squadgazzz added this pull request to the merge queue Aug 13, 2026

Merged via the queue into main with commit e9761d6 Aug 13, 2026
22 checks passed

squadgazzz deleted the solana-indexer/PR8-persist branch August 13, 2026 09:48

github-actions Bot locked and limited conversation to collaborators Aug 13, 2026

		@@ -1,10 +1,9 @@
		#![expect(dead_code)]
		#![allow(dead_code, reason = "dead in the lib build, exercised by tests")]

	let Ok((discriminator, _)) = recover_discriminator(&instruction.data) else {
	decode_failed = true;
	// Warn, not debug: this dead-letters the transaction, so it needs to
	// be findable in the logs alongside the row.
	tracing::warn!(
	signature = %ctx.signature,
	instruction_index = instruction.instruction_index,
	err = %DecodeError::UnknownDiscriminator,

		// A transaction of a later slot proves the pending slot is fully
		// delivered, so it is safe to flush and mark done.

Uh oh!

Conversation

squadgazzz commented Jul 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes

How to test

Related issues

Uh oh!

claude Bot commented Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review: wire the decoder to the persistence seam

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!

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!

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!

linear-code Bot commented Aug 7, 2026

Uh oh!

squadgazzz commented Jul 27, 2026 •

edited

Loading

claude Bot commented Jul 28, 2026 •

edited

Loading