ingest: full-history ingestion for cold + hot stores (#765)

[chowbao]

Implements #765 — the full-history data-ingestion path (cold + hot stores) for each LedgerCloseMeta. Code at cmd/stellar-rpc/internal/fullhistory/ingest/.

Rebased onto fh-ingest-base and absorbed the view layer from #778, which is now closed. The zero-copy XDR view extractors that #778 introduced have been promoted into the go-stellar-sdk (stellar/go-stellar-sdk#5949); this PR consumes them and keeps only the thin RPC-specific glue. Both retarget to feature/full-history once #756 lands.

SDK dependency (stellar/go-stellar-sdk#5949)

The view extractors now live in the SDK as the zero-copy twins of the parsed path. Per the #5949 review, the SDK exports only the complete extractors (the navigation scaffolding is unexported):

• ingest.ExtractTxHashes — per-ledger transaction hashes in apply order.
• ingest.ExtractLedgerEvents — per-transaction contract events + hash from one TxProcessing walk.
• ingest.LedgerTransactionViewByHash / LedgerTransactionViewRange (+ ingest.LedgerTransactionView) — view parallel of LedgerTransaction / LedgerTransactionReader.
• network.TransactionViewHasher — view twin of network.HashTransactionInEnvelope (returns just the hash).

go.mod pins the #5949 branch pseudo-version (f92b870f); bump to the merged version once #5949 lands.

RPC-side view adapters (internal/fullhistory/views/)

The local views package collapses to thin adapters that add only RPC-specific shapes/policy:

• ExtractEvents composes ingest.ExtractLedgerEvents (hash + events from one walk) with the events.Payload shape and the Stage→(TxIdx, OpIdx) cursor sentinels (events.StageSentinels).
• ExtractTxHashes wraps ingest.ExtractTxHashes into txhash.Entry.
• ExtractTxDetailsByHash / ExtractTransactions delegate to the SDK read path; views.Transaction aliases ingest.LedgerTransactionView.
• dispatch.go + envelopes.go deleted (moved to the SDK).

EventIdx removed from the events payload

The per-event index is positional and reconstructed at read time, so the eventIdx slot is dropped from the 0x01 payload layout. unmarshalHeader now requires the declared ContractEvent length to consume every remaining byte — a pre-removal record fails loudly (ErrPayloadLengthMismatch) rather than silently misparsing. LCMToPayloads keeps only the Stage→(TxIdx, OpIdx) sentinels (shared with the SQL path via events.StageSentinels); the ledger-wide before/after counters are gone.

Ingestion path (#765)

• Six ingesters — ledger/events/txhash × hot/cold — deriving per-LCM shapes via the SDK view extractors.
• Interfaces: HotIngester{ Ingest(ctx, xdr.LedgerCloseMetaView) }, ColdIngester{ Ingest, Finalize, Close }.
• Per-tier services: HotService (per-LCM parallel fan-out, waits-all) and ColdService (sequential ingest + Finalize).
• Metrics: MetricSink (NopSink default) + PrometheusSink under the daemon namespace.
• Sources: extensible ChunkSource — pack / GCS / S3 / DataStore.

Tests

Per-ingester readback (real temp-dir stores) incl. V0-as-empty events; HotService fan-out + failure/sibling-cancel; ColdService success + failure-path-no-artifact; the SDK differential tests prove the view extractors wire-identical to the parsed path across LCM V0/V1/V2 and meta V0–V4. -race clean.

Test plan

• go test ./cmd/stellar-rpc/internal/fullhistory/... ./cmd/stellar-rpc/internal/events/ (incl. -race), go vet, gofmt — green against the #5949 SDK branch.

[chowbao]

Updated to align with #780 (the authoritative txhash cold store, #728): this PR no longer bundles a txhash cold store — it now leaves pkg/stores/txhash identical to the base and uses the base hot-store API (NewHotStore/Get). The cold txhash store (build + read) comes from #780.

The integration seam is the per-chunk .bin file the cold txhash ingester writes — 8-byte LE count + 20-byte entries (16-byte key + LE uint32 seq), sorted by big-endian key prefix, named <chunkID:08d>.bin — which is byte-compatible with #780's BuildColdIndex input (verified against #780's documented format and keySize == streamhash.MinKeySize == 16). No code dependency between the two PRs; they meet only at that file format.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 686f3b8703

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9c579585b6

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[tamirms]

Requirement: events must be written to the event store in ascending getEvents cursor order — the order the SQLite path serves (ORDER BY id ASC in db/event.go). The event store serves in write order (event IDs are assigned by arrival position and the term bitmaps iterate in ID order), so write order is the cursor contract.

The current emission (LCMToPayloads / views.ExtractEvents, which copy InsertEvents' insertion traversal — per tx: tx-level events first, then op events) violates it in two ways:

1. Within a transaction: AfterTx events are emitted before the tx's op events, but their cursor (txIdx, OperationMask, …) sorts after every op event.
2. Across transactions: tx2's BeforeAllTxs event is emitted after tx1's op events, but its cursor (0, 0, …) sorts before them; AfterAllTxs is symmetric at the ledger tail.

For two txs each with a before-fee event, one op event, and a refund:

emission:   B1(0,0)  R1(M,0)  op1(1,0)  B2(0,0)  R2(M,0)  op2(2,0)
cursor asc: B1(0,0)  B2(0,0)  op1(1,0)  op2(2,0)  R1(M,0)  R2(M,0)

From protocol 23 onward every transaction emits fee events at the Before/After stages, so essentially every modern ledger gets stored out of cursor order — and frozen that way into immutable cold chunks. The visible effects: a term query returns a different sequence than the same query against the SQL path, and cursor pagination over the stream skips or duplicates stage events at page boundaries inside a ledger.

The fix is in the emitters: per ledger, emit all BeforeAllTxs events (the traversal already yields these in apply order, so a stable partition preserves it), then per tx in apply order its op events followed by its AfterTx events, then all AfterAllTxs events — in both LCMToPayloads and views.ExtractEvents, so the existing differential tests keep them pinned.

Once emission is cursor-ordered, payload.go's reconstruction CAVEAT should be updated too: every (txIdx, opIdx) group becomes contiguous in stream order, so eventIdx — the fourth cursor component, no longer stored — is recoverable from an event's position within its group, and the ledger-wide-counter caveat no longer applies.

On tests, two layers:

• An emission-order invariant: each ledger's payload slice is non-decreasing in (TxIdx, OpIdx). Cheap, and it catches both inversion classes directly.
• A differential against the real oracle: db/event_test.go already builds stage-event LCM fixtures and reads them back through GetEvents' cursor-range path — inserting the same LCM via InsertEvents and asserting the GetEvents sequence equals the payload emission sequence pins the requirement against the actual ORDER BY implementation rather than a re-derivation of it. For both: a fixture without stage events is vacuous, since the orders coincide when only op events exist.

[tamirms]

Proposal: dissolve the views package. After the SDK promotion it's down to thin wrappers, and its only importers are ingest/events.go and ingest/txhash.go. Piece by piece:

• ExtractEvents → internal/events. This is the one function with real content (payload assembly, the Stage→(TxIdx, OpIdx) sentinel mapping, the ErrV0Unsupported policy), and its natural home is next to LCMToPayloads: they're twins producing the same []events.Payload under the same ordering contract, and the cursor-ordering fix from the comment above touches both — one package means one shared partition helper and one differential suite proving they still agree. internal/events already imports the SDK's ingest package, so there's no cycle; the move actually removes an import edge. Naming suggestion: LCMViewToPayloads, paralleling LCMToPayloads.
• ExtractTxHashes → inline at the call sites. It's an SDK call plus a loop mapping xdr.Hash into txhash.Entry{Hash, LedgerSeq}; that mapping belongs in the ingest package the same way eventPayloads already does.
• ExtractTxDetailsByHash / ExtractTransactions / the Transaction alias → delete. They have no callers — the comment saying the alias "keeps the RPC read-path call sites stable" describes call sites that don't exist yet. The future serving PR can use ingest.LedgerTransactionViewByHash / Range directly and decide then whether it needs any local shape.

Tests follow the code:

• Move to internal/events: the payload-level differential (ExtractEvents vs LCMToPayloads), the V0-sentinel case, and the aliasing assertion. The SDK proves its extractors match the parsed path, but nothing on the SDK side pins the payload assembly — sentinels, applyIdx, ledger seq/close-time, emission order — and that differential is also the harness the ordering fix needs.
• Delete the rest: txdetails_test.go, pairing_test.go, coverage_test.go, diffextra_test.go exercise SDK behavior (by-hash pairing, reversed TxSet, TX_V0 arms, meta versions) through wrappers this change removes — that coverage now exists natively in the SDK's differential and real-ledger equivalence suites. txhash_test.go tests the LedgerSeq mapping loop, which the ingest package's per-ingester readback tests already catch end-to-end.

A side benefit: the views package name stops colliding with the SDK's views terminology, which means something different there.

[tamirms]

Going one step further: LCMToPayloads should be deleted too. It has no production callers — its remaining job is test oracle for the views differential, and it's a weak oracle: it was written to match the view emitter, so the pair can share a bug (the ordering issue above is exactly that — both copy InsertEvents' traversal and the differential blessed it). Replace the differential with one against the real oracle: InsertEvents + GetEvents over SQLite on stage-event fixtures, which pins sentinels and order against the actual v1 implementation. With the parsed twin gone, the ordering fix lands once, and StageSentinels' NOTE can be resolved by having db/event.go's InsertEvents import it (no cycle), making it the single sentinel definition for both backends. This does commit all ingestion — live included — to the view path; that's consistent with the raw-bytes-first direction (RawLedgers yields exactly what views consume), but stating it explicitly so it's a decision rather than a side effect. The doc references (hot_store.go's "typically produced by events.LCMToPayloads", the ErrV0Unsupported fall-back note) go with it.

[tamirms]

Simplification, superseding my earlier "no committed artifacts on failure" thread: the defensive machinery around cold artifacts protects against a consumer that doesn't exist, and should come out.

The design's model is that the orchestrator's completion record is the only source of truth for a chunk: completion is recorded after all artifacts land, a crash or failure before that point means the chunk is re-run from scratch, and nothing consumes cold artifacts by scanning directories — the index build takes explicit paths composed from the record. Under that model, disk under coldDir is a scratch workspace: stale or partial files are inert, and a retry's overwrite is the cleanup.

What follows:

• Delete the first-ledger probe and peekedStream. They exist to stop a failing retry from truncating previously finalized artifacts — but if the record says complete, nothing re-runs the chunk, and if it says incomplete, the artifacts were never trustworthy. runOneChunkCold collapses to build → drain → finalize, and the push→pull→replay adaptation goes away.
• Delete the unpublish rollback in ColdService.Finalize (this supersedes my earlier thread asking for it — it was reasoned from the same nonexistent directory-scanning consumer). Stopping at the first Finalize error is still right; removing already-published siblings is not.
• The txhash ingester should overwrite its destination directly, like the ledger and events writers, instead of the .tmp + rename dance — the atomicity buys nothing the completion record doesn't already provide, and a crash mid-attempt strands .tmp files nothing cleans up. Keep the fsync (the completion record must only be written once data is durable) and ReadColdBin's header-vs-size check (torn writes still fail loudly). The remove-stale-bin constructor step also becomes unnecessary once the writer overwrites.
• The two open Codex findings on driver.go (re-check ctx before building writers; validate the probed ledger) are refinements of the same unneeded protection and can be closed.

One addition: write the model down in this package's doc.go — cold artifacts are not authoritative without the orchestrator's completion record; nothing may consume them via directory scan; a chunk attempt owns its chunk's paths exclusively and overwrites freely. Every layer of defensive machinery here (my asks included) came from that contract being unstated.

[tamirms]

Every ingester re-derives the ledger sequence from the view (ledgerSeqOf in ledgers/txhash/events, both tiers), each with its own error path — but drain has already read and validated the sequence for every ledger before Ingest is called (the duplicate/out-of-order/overrun checks). Could we widen the interfaces to Ingest(ctx context.Context, seq uint32, lcm xdr.LedgerCloseMetaView) and pass the validated sequence through? The ingesters' stores already take the sequence as a separate parameter (IngestLedgerEvents(seq, …), ledger.Entry{Seq, …}), so this deletes ledgerSeqOf and the per-ingester malformed-header branches, and makes the contract explicit: seq is the driver-validated sequence of lcm.

[tamirms]

Follow-ups on txhashCold, mostly falling out of the artifact-model and views-dissolution comments above:

• WriteColdBin should truncate in place — with the tmp+rename gone, os.Create(path) (which is O_TRUNC) is the whole publish step. The constructor's remove-stale-.bin step deletes with it: remove-then-create buys nothing over truncate-on-open, and a partial file from a crash mid-write is already rejected loudly by ReadColdBin's header-vs-size check.
• Append ColdEntry directly from the SDK hashes when inlining the views wrapper. Today every hash is materialized three times per ledger: the SDK's []xdr.Hash, the wrapper's []txhash.Entry (full 32-byte hashes plus a seq the cold path ignores), then the truncated ColdEntry accumulator — two of the three are per-ledger garbage, roughly 200 MB of transient allocations over a ~3M-tx chunk.
• sort.Slice → slices.SortFunc in Finalize: the reflection-free version is a drop-in and meaningfully faster on a 3M-element sort.

The overall shape — accumulate everything, sort once at Finalize — is right; these are just the leaks inside it.

[tamirms]

On the metrics: the required signals from #765 are all here and correct (per-ingester hot/cold, both aggregates with the exactly-once emit, per-tier buckets). But the sink interface as shaped can't support the follow-up that is its stated reason for existing. #765's metrics section defines the sink as letting "the same ingester code report to Prometheus (production) or CSV (the bench-command migration, follow-up task) interchangeably," and the follow-up's end state is the rpc-hack bench commands rewritten on these production ingesters with no duplication of ingestion logic. The rpc-hack collectors that migration must reproduce are per-stage — eventSample on that branch is {Items, Extract, TermIndex, HotWrite, ColdAppend} plus a per-chunk Finish (pack Finish + WriteColdIndex) — while MetricSink carries only whole-Ingest durations. Since the stages happen inside Ingest, no CSV sink implementation can ever recover them through this interface; the granularity is an interface property, not a sink property. That makes it cheap to fix now and expensive after merge (interface change plus re-instrumenting every ingester).

Concretely: add one method, e.g. IngestStage(dataType, tier, stage string, d time.Duration, items int), emitted at the points where the rpc-hack collectors already sit (around the extract call, the store write/append, term indexing, finalize). PrometheusSink can map it to a stage-labeled histogram (three data types × ~four stages × two tiers — modest cardinality) or no-op it if per-stage isn't wanted in production; NopSink grows one empty method; the future CSV sink gets exactly what the bench reports need.

[chowbao]

All six addressed in d3c833a:

Cursor ordering (#issuecomment-4694645399): ExtractEvents now emits in ascending getEvents cursor order via stage-filtered passes — all BeforeAllTxs in apply order, then per tx its op events followed by its AfterTx events, then all AfterAllTxs (an unknown stage still errors in every pass). payload.go's reconstruction CAVEAT is replaced: every (txIdx, opIdx) group is contiguous, so the per-event index is positional within its group. TestExtractEvents_EmissionOrderCursorAscending pins the exact 10-key cursor sequence on a 2-tx stage-event fixture (including the (0,0) group spanning both tx hashes), and the differential runs on the review's exact before-fee/op/refund inversion shape.

Delete LCMToPayloads (#issuecomment-4694784966): gone — events/ingest.go became stage.go holding only StageSentinels, which db/event.go's InsertEvents now imports as the single sentinel definition (the lockstep NOTE is resolved). The views differential runs against the real oracle: InsertEvents + GetEvents over SQLite on stage-event fixtures, comparing tx hash, (TxIdx, OpIdx), the inner ContractEvent bytes, and the positionally-reconstructed per-group index against SQLite's stored cursor Event index. Doc references updated. Acknowledging the explicit decision: all ingestion, live included, now commits to the view path.

Artifact-model simplification (#issuecomment-4695072420): all of it came out — first-ledger probe + peekedStream (runOneChunkCold is ctx-check → OpenStream → build → drain → Finalize), the unpublish rollback (Finalize still stops at the first error; earlier artifacts stay as inert scratch), the txhash tmp+rename (WriteColdBin creates the final path directly; the fsync-before-close and ReadColdBin's header-vs-size check stay), the stale-bin constructor removal, the orphan-pack removal, and the bucket-dir pre-validation. The model is written into the package doc.go as the three-point contract, with the failure semantics re-derived from it. Both open Codex findings replied-closed as refinements of the removed protection.

Ingest(ctx, seq, lcm) (#issuecomment-4695108393): both interfaces widened with the driver-validated-seq contract documented; drain passes the sequence it just validated, the services pass it through, and ledgerSeqOf plus every per-ingester malformed-header branch are deleted.

txhashCold follow-ups (#issuecomment-4695233108): WriteColdBin truncates in place (TestColdBin_OverwritesPriorAttempt pins the overwrite semantics); the views ExtractTxHashes wrapper is deleted and the cold path appends truncated ColdEntry keys straight from the SDK's []xdr.Hash — one materialization instead of three (the hot tier keeps its single []txhash.Entry build since the store API takes it); Finalize sorts with slices.SortFunc.

Per-stage sink granularity (#issuecomment-4695488161): added IngestStage(dataType, tier, stage string, d time.Duration, items int) with stages extract / term_index / write / finalize, emitted at the rpc-hack collector seams (the events cold term-index and pack-append durations are accumulated across the interleaved per-payload loop and emitted once per ledger). PrometheusSink maps it to per-tier stage histograms with children pre-resolved at construction; per-stage items ride the interface for the future CSV sink but aren't exported to Prometheus (items_total already carries volume). NopSink grew the empty method.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d3c833a2c4

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[tamirms]

Re-reviewed ced7200e. The six asks from the earlier wave are correctly addressed — cursor-ordered emission is verified against a real SQLite InsertEvents→GetEvents oracle (with the CAP-67 inversion fixture), the artifact-model machinery is gone and the contract is documented, and the seq-through / txhash / metrics changes all landed. A few things remain.

1. Eliminate the views package entirely. This is the one comment from the last round that wasn't picked up (the ExtractTxHashes wrapper went, but as a side effect of the txhashCold comment, not this one). After the SDK promotion, the only live function left in the package is ExtractEvents, with a single importer (ingest/events.go); everything in txdetails.go is dead. Concretely:

• Move ExtractEvents (+ ErrV0Unsupported, appendStageEventPayloads, lcmVersion) to internal/events, next to StageSentinels — they're the event-payload emitter and belong there now that LCMToPayloads is gone (rename to LCMViewToPayloads if you want it to parallel the deleted one; update the views: error strings to events:). The one call site in ingest/events.go changes from views.ExtractEvents to events.ExtractEvents.
• Move events_test.go with it — it's self-contained (defines its own buildLCM / buildContractEvent / txMetaWith* helpers).
• Delete txdetails.go and its tests: ExtractTxDetailsByHash / ExtractTransactions / the Transaction alias have no production callers, and txdetails_test.go / pairing_test.go / coverage_test.go / diffextra_test.go exist only to exercise them — re-testing by-hash pairing and TX_V0 handling the SDK already covers natively. The future serving PR can call ingest.LedgerTransactionViewByHash / Range directly.

That empties the package, so it goes away — which also stops views colliding with the SDK's own "views" terminology.

2. golangci-lint is red — mechanical, blocking.

• views/events.go:94 — unused //nolint:gosec directive (same one from the previous round; survived the rewrite). It moves/dies with the elimination above.
• TestColdService_Success funlen 52 > 50; TestExtractEvents_MatchesSQLite funlen 112 > 100.
• Long lines: ingest_test.go:1896, metrics.go:314.

3. Stage-metric coverage gaps. The stage histograms partition the per-ingester ColdIngest total for ledger (write + finalize == accum + extra, exactly) but not for the other two:

• txhashCold.Ingest emits extract right after ExtractTxHashes, so the truncate-and-append loop — the ingester's only real per-ledger CPU — is in the per-chunk total but in no stage.
• eventsCold.ingestSeq leaves the trailing offsets.Append commit in observe but in no stage.

So a CSV built from the stages reconciles for ledger but leaves an unexplained remainder for txhash/events — and for txhash the remainder is exactly the work the rpc-hack collector measured as its own field. Since the stages exist to reproduce that breakdown, they should partition the observe window the way ledgerCold's already do (extend extract past the txhash append; fold offsets.Append into write or a small commit stage). Related: empty/V0 ledgers emit an extract sample but no term_index/write, so the stage histograms carry different sample counts — a consumer can't recover a per-ledger average by dividing a stage total by the ledger count.

4. Minor — ExtractEvents. The three-pass emission re-decodes each TransactionEvent's Stage three times (once per pass, filtering); and payloads isn't preallocated though ExtractLedgerEvents gives the per-tx event counts. Optional.

Not actionable: the build-matrix red is an infra flake (canceled stellar-core download; sibling matrix entries passed and it builds clean locally), and dependency-sanity-checker is just the SDK pin being a pseudo-version — #5949 merged and go.mod is already bumped to the post-merge commit, so it clears once the SDK cuts a release tag.

[chowbao]

Thanks — all four landed in 992363a.

1. views package eliminated. ExtractEvents moved to internal/events as events.LCMViewToPayloads (next to StageSentinels), renamed to parallel the deleted LCMToPayloads; ErrV0Unsupported, appendStageEventPayloads, and lcmVersion came with it, and the views: error strings are now events:. events_test.go moved alongside as extract_test.go (still self-contained). txdetails.go and its tests (txdetails_test.go, pairing_test.go, coverage_test.go, diffextra_test.go) are deleted — ExtractTxDetailsByHash / ExtractTransactions / the Transaction alias had no production callers, and the serving PR can call ingest.LedgerTransactionViewByHash / Range directly. The package directory is gone, and all doc/comment references (payload.go, stage.go, hot_store.go, ingest/doc.go) now point at events.LCMViewToPayloads.

2. golangci-lint green. The unused //nolint:gosec died with the move. TestColdService_Success (the 8 per-stage require.Equals collapsed into one exact-map assertion) and TestExtractEvents_MatchesSQLite (two inline fixture closures hoisted to stagedTxAllStages / cap67FeeTx helpers) are back under funlen. Long lines wrapped: metrics.go help string and the finalizeErrCold.Finalize stub. Verified clean with golangci-lint run on internal/events/... and internal/fullhistory/ingest/... (0 issues).

3. Stage-metric coverage closed.

• txhashCold.Ingest now emits extract after the truncate-and-append loop, so the per-ledger stage equals the Ingest wall-clock and extract + finalize partitions the per-chunk ColdIngest total — the loop the rpc-hack collector measured is no longer an unexplained remainder.
• eventsCold.ingestSeq folds the trailing offsets.Append commit into the write stage, and now emits term_index/write (zero-duration when empty) for every ledger including empty/V0. So extract + term_index + write reconciles to observe, and all three cold stage histograms carry one sample per ledger — a consumer can divide a stage total by the ledger count.

4. LCMViewToPayloads allocation. payloads is now preallocated from the exact emitted count (sum of per-tx top-level + per-op events, via a small countPayloads helper that also keeps cyclomatic complexity in budget). I left the three-pass Stage decode as-is for now — eliminating the re-decode means either buffering per-tx stage buckets or a second pass structure, which trades the allocation win for more complexity; happy to take it in a follow-up if you'd prefer.

On the not-actionable notes: agreed — build-matrix red is the canceled stellar-core download (builds clean locally), and dependency-sanity is just the SDK pseudo-version pending the post-#5949 release tag.

[tamirms]

🎉

[urvisavla]

EventIdx removed from the events payload
The per-event index is positional and reconstructed at read time, so the eventIdx slot is dropped from the 0x01 payload layout.

@chowbao What's the reconstruction strategy for the case where there are multiple events emitted per operation?

Today each event in an operation gets a distinct id, so the trailing component of the eventid (as returned by getEvents) represents its position within the operation. For an N-event operation that's <TOID>-0, <TOID>-1, ..., <TOID>-(N-1). Current sqlite implementation stores the <TOID>-x as id.

Without stored EventIdx in the payload, a query that returns a single event from the middle of a multi event operation has (TxIdx, OpIdx) in the payload but no positional context. There's no way to derive its position within the operation from the matched event alone.

If the plan is to assign events a ledger-wide incremental id in getEvents response, that would differ from the id (and cursor) currently returned by getEvents. That seems like a breaking change unless there's some mechanism to preserve or reconstruct the existing event Ids.

[chowbao]

EventIdx removed from the events payload
The per-event index is positional and reconstructed at read time, so the eventIdx slot is dropped from the 0x01 payload layout.

@chowbao What's the reconstruction strategy for the case where there are multiple events emitted per operation?

Today each event in an operation gets a distinct id, so the trailing component of the eventid (as returned by getEvents) represents its position within the operation. For an N-event operation that's <TOID>-0, <TOID>-1, ..., <TOID>-(N-1). Current sqlite implementation stores the <TOID>-x as id.

Without stored EventIdx in the payload, a query that returns a single event from the middle of a multi event operation has (TxIdx, OpIdx) in the payload but no positional context. There's no way to derive its position within the operation from the matched event alone.

If the plan is to assign events a ledger-wide incremental id in getEvents response, that would differ from the id (and cursor) currently returned by getEvents. That seems like a breaking change unless there's some mechanism to preserve or reconstruct the existing event Ids.

There isn't a set plan right now for ledger-wide incremental id although the SEP proposal for standardized event ids exists.

I think we can defer defining the event index for now because it's pretty easy to add back into the payload but we don't know exactly what format the index should take right now and we don't know if preserving or reconstructing the rpc v1 event ids is needed or not

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 486f518abd

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

Reject overlapping pack sources before truncating ledgers

When RunCold is used to derive stores from an existing ledger-pack root such as NewPackSource(filepath.Join(coldDir, "ledgers")) and cfg.Ledgers is left enabled, this opens the ledger cold writer at the same packPath that packStream.RawLedgers will later read. ledger.NewColdWriter truncates that file immediately, so the subsequent source read fails after the original ledger pack has already been destroyed. Either reject overlapping pack-source/output paths or open and hold the source reader before constructing the ledger writer.

Useful? React with 👍 / 👎.

[urvisavla]

EventIdx removed from the events payload
The per-event index is positional and reconstructed at read time, so the eventIdx slot is dropped from the 0x01 payload layout.

@chowbao What's the reconstruction strategy for the case where there are multiple events emitted per operation?
Today each event in an operation gets a distinct id, so the trailing component of the eventid (as returned by getEvents) represents its position within the operation. For an N-event operation that's <TOID>-0, <TOID>-1, ..., <TOID>-(N-1). Current sqlite implementation stores the <TOID>-x as id.
Without stored EventIdx in the payload, a query that returns a single event from the middle of a multi event operation has (TxIdx, OpIdx) in the payload but no positional context. There's no way to derive its position within the operation from the matched event alone.
If the plan is to assign events a ledger-wide incremental id in getEvents response, that would differ from the id (and cursor) currently returned by getEvents. That seems like a breaking change unless there's some mechanism to preserve or reconstruct the existing event Ids.

There isn't a set plan right now for ledger-wide incremental id although the SEP proposal for standardized event ids exists.

I think we can defer defining the event index for now because it's pretty easy to add back into the payload but we don't know exactly what format the index should take right now and we don't know if preserving or reconstructing the rpc v1 event ids is needed or not

Agree with deferring the format decision but I'd suggest keeping EventIdx in the payload for now even if we end up not using it in the future. Adding it back later would require a payload version bump and reingestion. We can always ignore the field if a different cursor format is ultimately chosen.

Removing EventIdx from the payload isn't fully backward compatible. It either requires reconstructing EventIdx when generating event IDs (behaviorally same but algorithmically different).

Until we make a deliberate decision to change the cursor/ID format, I think we should preserve backward compatibility and keep the current ID format (-) since it's part of the public API today.

[chowbao]

EventIdx removed from the events payload
The per-event index is positional and reconstructed at read time, so the eventIdx slot is dropped from the 0x01 payload layout.

@chowbao What's the reconstruction strategy for the case where there are multiple events emitted per operation?
Today each event in an operation gets a distinct id, so the trailing component of the eventid (as returned by getEvents) represents its position within the operation. For an N-event operation that's <TOID>-0, <TOID>-1, ..., <TOID>-(N-1). Current sqlite implementation stores the <TOID>-x as id.
Without stored EventIdx in the payload, a query that returns a single event from the middle of a multi event operation has (TxIdx, OpIdx) in the payload but no positional context. There's no way to derive its position within the operation from the matched event alone.
If the plan is to assign events a ledger-wide incremental id in getEvents response, that would differ from the id (and cursor) currently returned by getEvents. That seems like a breaking change unless there's some mechanism to preserve or reconstruct the existing event Ids.

There isn't a set plan right now for ledger-wide incremental id although the SEP proposal for standardized event ids exists.
I think we can defer defining the event index for now because it's pretty easy to add back into the payload but we don't know exactly what format the index should take right now and we don't know if preserving or reconstructing the rpc v1 event ids is needed or not

Agree with deferring the format decision but I'd suggest keeping EventIdx in the payload for now even if we end up not using it in the future. Adding it back later would require a payload version bump and reingestion. We can always ignore the field if a different cursor format is ultimately chosen.

Removing EventIdx from the payload isn't fully backward compatible. It either requires reconstructing EventIdx when generating event IDs (behaviorally same but algorithmically different).

Until we make a deliberate decision to change the cursor/ID format, I think we should preserve backward compatibility and keep the current ID format (-) since it's part of the public API today.

Yeah that's fair. I'll add it back in

[chowbao]

@urvisavla added back event index here