fix(sep41): stop re-fetching token metadata we already have - #679
Open
aditya1702 wants to merge 9 commits into
Open
fix(sep41): stop re-fetching token metadata we already have#679aditya1702 wants to merge 9 commits into
aditya1702 wants to merge 9 commits into
Conversation
aditya1702
force-pushed
the
replay-loadtest-backend-pr6
branch
from
August 7, 2026 18:25
8bf66b4 to
d5ec0ae
Compare
This was referenced Aug 13, 2026
Open
aditya1702
force-pushed
the
replay-loadtest-backend-pr6
branch
5 times, most recently
from
August 25, 2026 22:11
646a06b to
9b51e14
Compare
aditya1702
changed the title
Contributor
Author
|
Scope change: this PR no longer adds the That backend merged, renumbered, paced and timestamped apply-load meta inside the ingest binary. The loadtest rig now writes datastore-layout objects instead, so wallet-backend reads them through the stock What's left here is the SEP-41 metadata caching, which is a production fix and was only bundled in because the rig surfaced it. Previous tip is preserved at |
aditya1702
added a commit
that referenced
this pull request
Aug 25, 2026
Catches #684 up with its base, which gained `protocol-migrate --rebuild` (#692) and hot-archive checkpoint reads (#694). The base's four SEP-41 metadata commits are already on this branch under different SHAs from the #679 review round, so they arrive as no-ops. Conflicts, all in the ingest configuration surface, resolved as the union of two independent deletions rather than by picking a side. Both branches were removing configurable ledger-cursor names from opposite ends: this branch dropped the inert `latest-ledger-cursor-name` flag along with the dead `AppTracker` and `GetLedgersLimit` fields, while #692 dropped the `oldest-ledger-cursor-name` flag and hard-coded `data.OldestLedgerCursorName`. Neither cursor name is configurable now. - cmd/ingest.go: both flag blocks removed; `deprecatedLatestLedgerCursorName` no longer had a declaration to bind to. - internal/ingest/ingest.go: `IngestServiceConfig` has neither `OldestLedgerCursorName` nor `AppTracker`, so both assignments are gone. The ingest-package config keeps its own fields, which other callers use. - internal/services/ingest_live.go: kept the pipelined persist path. The base's side of this hunk was the sequential `ingestProcessedDataWithRetry` body this branch replaced; its only real change, the switch to `data.OldestLedgerCursorName`, is ported into the relocated cursor-sync block. - ingest_test.go, ingest_live_test.go: dead fields dropped from the `IngestServiceConfig` literals. `processHotArchive` reaches `hotArchiveIterFactory` through the service, and TestCheckpointService_PopulateFromCheckpoint_UnsupportedLiquidityPoolBodySkipsShares is new on this branch, so #694 never wired the factory into its fixture and the pass nil-panicked. It now gets the empty iterator the neighbouring checkpoint tests use. go build, go vet, make check and make unit-test all pass.
| // first group from contract_tokens before any fetch runs, so a restart does not | ||
| // refetch what is already stored. | ||
| // | ||
| // A restart clears all of it, which is also how metadata that changed on chain |
Contributor
There was a problem hiding this comment.
is this still true? in fbd6218 it seeds haveIt from contract_tokens where name IS NOT NULL
Contributor
Author
There was a problem hiding this comment.
Not true any more — fixed in 982d2c1.
Seeding haveIt from contract_tokens is what broke it: a restart clears the map, then Prefetch refills it from the same rows, so RPC stays suppressed for every contract that already has a name. The restart re-suppresses exactly the fetches it used to refresh.
The comment now says stored metadata is never refetched, and names the one thing that makes a contract eligible again — clearing contract_tokens.name. --rebuild is not a refresh either: no rebuild path wipes that table (internal/data/sep41/wipe.go:10).
aditya1702
force-pushed
the
replay-loadtest-backend-pr6
branch
from
September 3, 2026 23:44
c547a7f to
a0af594
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Optimizes SEP-41 metadata fetching while also introducing substantial protocol migration, checkpoint, indexing, and Protocol 28 changes.
Changes:
- Caches successful metadata fetches and backs off failures.
- Adds protocol rebuilds, locking, frontier gating, and contract-data extraction.
- Adds Protocol 28, hot-archive, muxed-address, and deterministic folding support.
Reviewed changes
Copilot reviewed 60 out of 61 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Updates integration image versions. |
| internal/services/sep41/validator.go | Skips metadata already stored. |
| internal/services/sep41/validator_test.go | Tests persisted metadata caching. |
| internal/services/sep41/processor.go | Orders events and adds wipe support. |
| internal/services/sep41/processor_test.go | Tests ordering and muxed balances. |
| internal/services/sep41/metadata.go | Adds metadata cache and retry backoff. |
| internal/services/sep41/metadata_test.go | Tests caching and backoff. |
| internal/services/sep41/events.go | Normalizes muxed addresses. |
| internal/services/sep41/events_test.go | Tests muxed event addresses. |
| internal/services/protocol_processor.go | Expands processor interface and input. |
| internal/services/protocol_migrate.go | Adds frontier gating and contract-data extraction. |
| internal/services/protocol_migrate_rebuild_test.go | Tests destructive rebuild workflows. |
| internal/services/protocol_migrate_lock.go | Adds per-protocol advisory locks. |
| internal/services/protocol_migrate_lock_test.go | Tests migration locking. |
| internal/services/protocol_migrate_history.go | Locks history migrations. |
| internal/services/protocol_migrate_history_test.go | Tests history migration locking. |
| internal/services/protocol_migrate_history_rebuild.go | Implements history rebuilding. |
| internal/services/protocol_migrate_current_state.go | Locks current-state migrations. |
| internal/services/protocol_migrate_current_state_test.go | Tests current-state locking. |
| internal/services/protocol_migrate_current_state_rebuild.go | Implements current-state rebuilding. |
| internal/services/mocks.go | Updates processor mocks. |
| internal/services/ingest.go | Returns materialized ledger transactions. |
| internal/services/ingest_live.go | Integrates contract-data processing. |
| internal/services/ingest_live_test.go | Updates ingest configuration tests. |
| internal/services/ingest_backfill.go | Standardizes oldest cursor usage. |
| internal/services/checkpoint.go | Processes hot-archive entries. |
| internal/metrics/ingestion.go | Adds external-reference metrics. |
| internal/metrics/ingestion_test.go | Tests the new metric. |
| internal/integrationtests/infrastructure/testconstants.go | Sets protocol version 28. |
| internal/integrationtests/infrastructure/containers.go | Updates Stellar container versions. |
| internal/integrationtests/data_migration_test.go | Updates protocol-contract query usage. |
| internal/ingest/timescaledb_test.go | Standardizes cursor names in tests. |
| internal/ingest/ingest.go | Removes configurable oldest cursor. |
| internal/ingest/datastore_backend.go | Updates frontier documentation. |
| internal/indexer/types/types.go | Normalizes muxed address storage. |
| internal/indexer/types/types_test.go | Tests address normalization. |
| internal/indexer/protocol28_test.go | Tests Protocol 28 XDR support. |
| internal/indexer/processors/protocol_contracts.go | Handles external-reference executables. |
| internal/indexer/processors/protocol_contracts_test.go | Tests external-reference handling. |
| internal/indexer/indexer.go | Adds contract-data extraction. |
| internal/indexer/indexer_test.go | Tests extraction and ordering. |
| internal/indexer/indexer_buffer.go | Preserves highest-order removals. |
| internal/indexer/indexer_buffer_test.go | Tests revised folding behavior. |
| internal/data/statechanges.go | Adds namespace-range deletion. |
| internal/data/statechanges_test.go | Tests scoped history deletion. |
| internal/data/sep41/wipe.go | Adds SEP-41 state truncation. |
| internal/data/sep41/wipe_test.go | Tests SEP-41 wiping. |
| internal/data/sep41/balances_test.go | Tests muxed balance normalization. |
| internal/data/protocol_contracts.go | Supports transactional queries. |
| internal/data/mocks.go | Updates data-model mocks. |
| internal/data/ingest_store_test.go | Updates canonical cursor names. |
| internal/data/contract_tokens.go | Queries tokens with metadata. |
| go.sum | Updates Stellar dependency checksums. |
| go.mod | Upgrades Stellar SDK dependencies. |
| docs/data-migrations/running-a-data-migration.md | Documents rebuild operations. |
| cmd/utils/custom_set_value_test.go | Updates SDK error expectations. |
| cmd/protocol_migrate.go | Adds rebuild CLI flags. |
| cmd/ingest.go | Removes the cursor-name flag. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
aditya1702
changed the base branch from
main
to
reland/migrate-rebuild-and-hot-archive
September 4, 2026 17:43
aditya1702
force-pushed
the
replay-loadtest-backend-pr6
branch
from
September 4, 2026 19:30
a0af594 to
40d2a74
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
internal/services/sep41/metadata.go:98
- A restart cannot pick up changed on-chain metadata as documented here and in the PR trade-off.
Prefetchimmediately reloads every row with a non-NULLnameviaGetWithMetadataand marks ithaveIt, so RPC remains skipped after restart too. Either add an explicit refresh/staleness mechanism if restart refresh is required, or document that persisted metadata is never refreshed automatically.
// A restart clears all of it, which is also how metadata that changed on chain
// gets picked up.
aditya1702
force-pushed
the
replay-loadtest-backend-pr6
branch
from
September 8, 2026 19:03
40d2a74 to
82e699c
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
internal/data/contract_tokens.go:36
- The PR description mentions that the metadata UPDATE stops firing for unchanged rows to avoid contract_tokens.updated_at churn, but BatchUpdateMetadata still issues a plain UPDATE for every supplied row (and the migration defines a BEFORE UPDATE trigger that always refreshes updated_at). If avoiding churn is a goal, BatchUpdateMetadata likely needs a value-difference guard (e.g., using IS DISTINCT FROM on the updated columns) so unchanged rows are skipped.
internal/services/sep41/metadata.go:232 - FetchMetadata records a 5-minute failure backoff for any per-contract error, including when the caller context is canceled/deadline-exceeded. That can incorrectly suppress retries for healthy contracts after a transient timeout/shutdown. Consider skipping recordFailure (and the warning log) when ctx.Err() is non-nil so only real RPC/validation failures are backed off.
aristidesstaffieri
approved these changes
Sep 9, 2026
A persistently failing name() simulation costs a full retry-with-backoff (600ms) every ledger the contract is active in, because claimed contracts re-enter Prefetch on each classification pass. A 5-minute negative cache keeps eventual enrichment without the per-ledger tax.
Prefetch runs for every claimed contract observed in a ledger's changes and has no database access by design, so it re-simulated name/symbol/decimals for tokens whose metadata was already persisted — on every ledger that touched their instance. An in-process success cache skips them; a restart refetches each contract once, which doubles as the refresh path for tokens whose on-chain metadata changed. Persist retries are unaffected: the fetched values live in the classification plan, which is reused across retry attempts.
The metadata fetcher's fetched/failure caches are in-memory only, so a token whose contract_tokens row already carries metadata was still re-fetched over RPC — once per process lifetime after every restart on a real network, and forever on a 5-minute backoff against a deployment whose RPC can never resolve metadata (the loadtest rig's dead endpoint with externally seeded rows), where each retry burned the ~850ms simulate backoff ladder inside prepare_classification. After Apply persists a batch's rows, contracts whose row has metadata are marked fetched, making the database the durable cache. Tokens whose metadata is genuinely missing stay unmarked and keep the existing backoff retry.
The fetched/failedUntil caches sit on the production ingestion path but had no coverage: nothing verified that a resolved contract stops hitting the RPC, that a failed fetch is suppressed for the backoff window, or that expiry re-fetches and success clears the failure entry. Add those tests; shortening metadataFailureBackoff here is what makes its var-for-tests comment true.
The contract_tokens lookup ran in Apply, inside the persist transaction, where it could only stop the next wasted fetch. Prefetch now does it before any fetch runs, so a token whose metadata is already stored never reaches RPC at all. Prefetch reads the connection pool rather than a transaction, alongside the classification lookup ingest already does there, so no query is added inside the persist transaction. Only contracts the fetcher does not already account for are queried, so the lookup stops being issued once every claimed contract is known. Also collapses the fetcher's two cache maps into one entry per contract, which turns filterCached into a single lookup and makes "we already have this" a property of the type rather than the order of two checks.
Nothing exercised the contract_tokens read: the only Apply test built a validator with no fetcher, so it returned before touching the database. Two tests, both asserting on the RPC call count so a regression reports a mismatch rather than deadlocking on an unexpected mock call raised from inside a pool worker: - a batch where the database knows one of two contracts fetches only the other - a second pass over the same batch issues no query at all, since every contract is accounted for by then
The fetcher's godoc claimed a restart is how metadata that changed on chain gets picked up. Seeding haveIt from contract_tokens made that false: Prefetch reseeds the same rows on the next run, so a restart re-suppresses the fetch for exactly the contracts it used to refresh. Say what actually happens instead, and name the one thing that makes a contract eligible again — clearing contract_tokens.name. No rebuild path wipes that table; classification owns it.
unknownAddrs skipped only contracts whose metadata we hold, so a contract inside its five-minute failure back-off was reported as unknown and Prefetch asked contract_tokens about it again every ledger. filterCached then dropped it anyway, so the query bought nothing, and it falsified the docstring's claim that the query stops once every claimed contract is accounted for. Both functions ask the same question — is there work to do for this contract? — so give them one predicate to ask it with instead of two that already drifted apart once. filterCached keeps its own delete arm: it mutates state, unknownAddrs does not. A contract becomes DB-checkable again the moment its back-off lapses, since expired entries still fall through.
The query behind the SEP-41 fetch skip was only exercised through a mocked model, so nothing caught a change to its name IS NOT NULL filter or its scan. Test it against a real database next to its siblings, seeding a resolved row alongside the default NULL-name row Apply writes before enrichment — the pair that has to come apart. Reads through the pool and through a transaction, since accepting either is why the signature takes a db.Querier and Prefetch calls it on the pool.
aditya1702
force-pushed
the
replay-loadtest-backend-pr6
branch
from
September 9, 2026 21:07
9b41962 to
7af2f61
Compare
Comment on lines
+105
to
+108
| // A restart clears this state but not its effect: Prefetch reseeds the first | ||
| // group from the same rows, so stored metadata is never refetched, on restart | ||
| // or otherwise. Clearing contract_tokens.name is what makes a contract eligible | ||
| // again — classification owns that table and no rebuild path wipes it. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ingest asked RPC for the same SEP-41 token metadata over and over. Now each contract is fetched once.
The problem
Every ledger that touches a known SEP-41 contract runs it back through the metadata fetch —
collectClaimedContractsadmits anything already classified as SEP-41, not just new matches. Nothing remembered the previous answer, so per ledger, per contract, we paid:name(),symbol(),decimals())UPDATE contract_tokensrewriting metadata that had not changedWhat changed
Prefetchnow askscontract_tokenswhich of this batch's contracts already have a name, and skips them. Whatever is left goes to RPC, and the answer is remembered:Two things about where that read lives:
GetClassifiedByHasheslookup ingest already does there. No query is added inside the transaction.Doing this in
Prefetchrather than after the fact is what makes it prevent the first doomed fetch, not just the next one.The metadata
UPDATEalso stops firing for unchanged rows, socontract_tokens.updated_atno longer churns once per ledger.Trade-off
Metadata that changes on chain is not picked up automatically, and a restart does not change that —
Prefetchreseeds the cache from the same stored rows. Clearingcontract_tokens.nameis what makes a contract eligible for a refetch. Token name, symbol, and decimals effectively never change, so this is the right trade.Notes
GetWithMetadatato thecontract_tokensmodel, reading through anydb.Querierso it works on the pool or in a transaction.RPC_URLat a dead endpoint on purpose and seeds token metadata with a sidecar that writes straight to Postgres. Readingcontract_tokensfirst is what makes those seeded rows visible to ingest — without it every synthetic token pays a doomed fetch every five minutes forever.