refactor(core): prefix chain-derived tables with core_ - #502
Open
rickyrombo wants to merge 1 commit into
Open
Conversation
Rename access_keys, launchpad_authority_rm, management_keys, sla_node_reports, sla_rollups, sound_recordings, storage_proof_peers, storage_proofs, track_releases and validator_history to carry the core_ prefix. core_ already means "derived from block execution, identical on every validator, must appear in the state-sync snapshot". These ten met that definition without the prefix, and because the snapshot list in createPgDump is hand-maintained while the schema is not, the ambiguity has caused the same bug twice: 00028_fix_missing_tables_from_state_sync exists solely to repair nodes that state-synced while five core_ tables were absent from the list, and core_auth_cids was missed the same way (#499). With the prefix exhaustive, a guard test can assert the invariant by prefix instead of by a list someone must remember to update. The rename is catalog-only (ALTER TABLE ... RENAME TO), which matters against production databases in the hundreds of gigabytes. Indexes, constraints and sequences intentionally keep their old names; nothing references them by name and a half-renamed set would hide whatever was missed. See the migration comment. Consumers updated beyond the sqlc queries and the snapshot list: cmd/rollback (hand-written DELETEs, both executed and printed), pkg/core/console, and six raw-SQL string literals in mediorum's cidstream authorization path, where a missed literal would not fail loudly but would silently ungate a gated track. Generated sqlc files were regenerated, not hand-edited. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Renames ten chain-derived tables to carry the
core_prefix. No behavior change; no data movement.access_keyscore_access_keyslaunchpad_authority_rmcore_launchpad_authority_rmmanagement_keyscore_management_keyssla_node_reportscore_sla_node_reportssla_rollupscore_sla_rollupssound_recordingscore_sound_recordingsstorage_proof_peerscore_storage_proof_peersstorage_proofscore_storage_proofstrack_releasescore_track_releasesvalidator_historycore_validator_historyWhy: making the prefix load-bearing
core_already means something specific in this schema — the table is derived from block execution, is therefore byte-identical on every validator, and must ship in the state-sync snapshot dumped bycreatePgDumpinpkg/core/server/state_sync.go. These ten tables have always met that definition. They predate the convention and never got the prefix.The gap is not cosmetic, because the snapshot table list is hand-maintained and the schema is not. Nothing connects "a migration added a chain-derived table" to "the table ships in snapshots" except whoever wrote the migration remembering. That has already failed twice:
00028_fix_missing_tables_from_state_sync.sqlexists for no other reason than to recreatecore_ern/core_mead/core_pie/core_rewards/core_uploadson nodes that state-synced while those five were absent from the list.core_auth_cids(added in00038) was missed the same way and is being added to the list separately in fix(core): include core_auth_cids in state sync snapshots #499.With the prefix exhaustive, that invariant becomes mechanically checkable: every
core_*table in the schema appears in the snapshot list, or is explicitly exempted with a stated reason. That is the payoff. The guard test in #499 now naturally covers all ten of these tables — after this lands it does not need a hand-maintained addition per table, it just needs the prefix rule. #499 is not merged or duplicated here; it rebases on top of this.Complete consumer list
The original framing of this change assumed the consumers were the sqlc queries and the snapshot list. They are not. The full set:
Schema
pkg/core/db/sql/migrations/00039_prefix_chain_derived_tables.sql(new)sqlc queries and generated code — regenerated with
make regen-sql, not hand-editedpkg/core/db/sql/reads.sql,pkg/core/db/sql/writes.sqlpkg/core/db/models.go,reads.sql.go,writes.sql.go(generated; model structs renameSlaRollup→CoreSlaRollup, etc. Query names and*Params/*Rowstruct names are unchanged, so call sites are unaffected except where a model type is named directly.)State-sync snapshot list
pkg/core/server/state_sync.go— thetablesslice increatePgDumpGo call sites naming a model type
pkg/core/console/uptime.go(db.SlaRollup→db.CoreSlaRollup)pkg/core/server/auditor.go(db.SlaNodeReport→db.CoreSlaNodeReport)Raw SQL outside sqlc — the risky ones, see below
pkg/mediorum/server/serve_blob.go(4 sites: 3 ×.Raw, 1 ×.Table)pkg/mediorum/server/serve_blob_grpc.go(2 ×.Raw)Operational tooling that the original brief missed
cmd/rollback/main.go— hand-writtenDELETE FROM validator_history …/DELETE FROM sla_node_reports …in both the executing path (cleanPG) and the copy-pasteable SQL it prints to stderr (printPGCleanup). A miss here would have made the rollback tool silently skip two tables during an incident.pkg/core/console/uptime.go— as above.Tests that create these tables themselves
pkg/core/server/registration_consensus_test.go(DDL + FK + cleanupDROP TABLEs)pkg/mediorum/server/serve_blob_test.go(DDL + fixture inserts + cleanup)Deliberately not changed
pkg/core/db/sql/migrations/00006,00013,00014,00018–00020,00023–00025,00030–00032,00034,00036— historical migrations, which run before00039and must keep referring to the pre-rename names.cmd/genesis-writer/testdata/dp_schema.sql— a dump of the legacy Audius discovery-provider database, a different database with its own unrelatedaccess_keys.access_keys/sla_rollupshits — different database (api_access_keys), unrelated.The raw-SQL risk, and how it was addressed
Mediorum reads
sound_recordingsandmanagement_keysthrough raw SQL string literals in the cidstream authorization path —.Raw("SELECT ... FROM management_keys ..."),.Table("sound_recordings"). These compile fine when wrong and fail at runtime, and none of them check the returned error:s.crud.DB.Raw(...).Scan(&trackID)discardsres.Error. A missed literal does not throw — it yields a zero value, which inrequireRegisteredSignaturemeans "this track has no access authorities", which means a gated track silently becomes streamable by any registered validator signature. That is the failure mode this change had to rule out.How the literals were found, and why the search is believed exhaustive:
(^|[^A-Za-z0-9_])name([^A-Za-z0-9_]|$), which excludescore_-prefixed matches). This is exhaustive by construction for literal occurrences: anyRaw/Exec/Query/Tablecall, templ-generated_templ.go,.sql, YAML, shell, or docs containing one of these names as a whole word had to match, regardless of which API wrapped it. After the change the only remaining hits repo-wide are the historical migrations and the genesis-writer discovery-provider testdata listed above.sla_,sound_,management,storage_proof,track_release,validator_hist,access_key, orlaunchpad_. All surviving hits are unrelated: transaction-type constants ("sla_rollup","storage_proof"), protobuf field tags, ETL materialized views (mv_sla_rollup*), Go struct JSON tags, and two singular row-level error strings inmanage_entity.go. No table name is assembled at runtime anywhere..Raw(sites and all 3.Table(sites in the repo were read individually. The ones not touched here queryops,cursors,uploads,pg_class— mediorum's own tables. There are noTableName()methods and noON CONFLICT ON CONSTRAINTanywhere.crudStatusTablesinserve_crud.go, the only place a table-name list is passed as data, contains mediorum tables only.cd pkg/core/db && sqlc generate); the result is byte-identical to what is committed here.What the tests actually prove, and what they do not.
TestRequireRegisteredSignatureWithLowercaseAccessAuthorityandTestRequireRegisteredSignatureWithUnrelatedAccessAuthorityinpkg/mediorum/server/serve_blob_test.godo execute the realrequireRegisteredSignaturehandler against a real Postgres with the memo cache cleared, so they genuinely run three of the six renamed literals —serve_blob.go:494,:496, and:517.This was confirmed with a negative control rather than assumed. Reverting the single literal at
serve_blob.go:494back tosound_recordingsand re-running turns both tests red:Note what that observed failure output is: the handler found no track for the cid, treated the gated track as ungated, and fell through to the registered-node signature path. That is the silent authorization bypass described above, reproduced. The literal was restored immediately after.
Three literals have no test coverage and were verified by inspection only:
serve_blob.go:56—.Table("core_sound_recordings")inInvalidateTrackAccessCacheForTrack, which is wrapped inrecover()and logs at Debug.serve_blob.go:704/:711— theserveTrackhandler.serve_blob_grpc.go:42/:49— thestreamTrackGRPChandler.There is also a limit on what those tests can ever prove: the fixture creates its own tables, so it only catches Go-literal/test disagreement, not Go-literal/migration disagreement. If the migration and the Go code drifted apart in the same direction, the unit tests would still pass. The repo-wide grep is what covers that, not the test suite.
Index and constraint naming: deliberately unchanged
ALTER TABLE … RENAME TOdoes not rename that table's indexes, constraints, or sequences. Applying the full migration chain to a fresh database and readingpg_indexesback confirms exactly what survives:…plus
idx_storage_proofs_block_heightoncore_storage_proofs,idx_track_releases_track_id,idx_management_keys_track_id, and theserialsequences (sla_rollups_id_seqand friends).This was left alone on purpose:
ON CONFLICT ON CONSTRAINT. Renaming buys zero safety, and PostgreSQL tracks these dependencies by OID rather than by name, so the stale names stay fully functional.*_pkey, andsla_node_reports_address_sla_rollup_id_key, generated from the unnamedunique (address, sla_rollup_id)in00006. That name above was recovered from a live catalog, not from the source; renaming them properly would mean doing that for every database.So this is stated explicitly here and in the migration's own comment: if you are looking at
pg_indexesand wondering whycore_sla_rollupshas an index calledidx_time, the rename was not left unfinished — index, constraint, and sequence names were out of scope. Old names remain fully functional; PostgreSQL tracks these dependencies by OID, not by name.Migration cost
ALTER TABLE … RENAME TOis catalog-only: it rewrites one row inpg_classand moves no data. That matters because these migrations run at node startup against production databases in the hundreds of gigabytes, where anything that rewrote a heap would mean a long outage. Ten renames in one transaction take anACCESS EXCLUSIVElock for the duration, which is milliseconds — but note it is still anACCESS EXCLUSIVElock, so it will queue behind any long-running transaction holding a conflicting lock on these tables.Cross-version state-sync hazard — read before deploying
This rename must not straddle a state-sync window. During a partial rollout, a new-code node's snapshot contains
core_sound_recordingswhile an old-code node's containssound_recordings. Both directions break, and they break differently.The mechanism is that
RestoreDatabasenever drops tables. It runspg_restore --section=pre-datawith errors ignored (_ = pgRestore("pre-data")), thenTRUNCATE … CASCADEover every table inpublic, then loads the data section. So a snapshot whose tables are named differently from the local schema does not fail —pre-datasimply creates the foreign-named tables alongside the local ones,TRUNCATEempties the local ones, and the data lands in the tables the running binary does not read.Old-code node state-syncing from a new-code peer. The old node's migrations created
sound_recordingset al. The snapshot creates and populatescore_sound_recordingset al.TRUNCATEempties everything first. The old binary then queries the unprefixed names and gets ten empty tables: no SLA rollups, no node reports, no validator history, no storage proofs, no management keys, no sound recordings.New-code node state-syncing from an old-code peer. Exact mirror. The snapshot's unprefixed tables are created and populated; the new binary reads the prefixed ones, which are empty.
Consequences in both directions:
Consensus divergence. These tables feed
FinalizeBlock—createRollupinauditor.goreadscore_sla_node_reportsto build the next SLA rollup, and the storage-proof and validator-history paths do the same. A validator computing over empty inputs produces a different app hash from its peers. This is precisely the class of failure00028was written to repair.Authorization bypass. Empty
core_management_keys/core_sound_recordingsmeansrequireRegisteredSignaturefinds no track for a cid, treats the track as ungated, and falls through to the registered-validator-signature path. Gated tracks become streamable by any registered signer on that node.The node will not restart.
core_db_migrationsis itself in the snapshot list, so the restore replaces the local migration bookkeeping with the peer's. The repo usessql-migratewith the defaultIgnoreUnknown=false.core_db_migrationsnow records00039, which the old binary's embedded migration set does not contain →unknown migration in database→error running migrations→ boot fails.core_db_migrationsno longer records00039, so it is replanned and re-runsalter table access_keys rename to core_access_keys— butcore_access_keysalready exists locally (created by the pre-restore migration run, then truncated) →relation already exists→ boot fails.So the node runs on silently-wrong state until it is restarted, and then refuses to start. Recovery is manual in both cases.
Deploy guidance. Do not let a node state-sync while the fleet is mixed-version. Either complete the rollout before any node state-syncs, or gate state sync off for the duration. Note that the hazard is not created by this PR — any change to the snapshot table set has it, including #499 — but this PR changes ten entries at once, which makes the mixed-version window unusually wide.
Verification
go build ./...— clean.go vet ./pkg/core/... ./pkg/mediorum/...— byte-identical to the pre-change baseline (captured by stashing the change); no new findings.gofmt -lon all touched files — clean. (The repo has 17 pre-existing unformatted files; none are touched here.)sqlc generatereproduces the committed generated files byte-for-byte — the generated.sql.gofiles were regenerated, not hand-edited.make test-mediorum— pass, plus the negative control above.make test-unit— pass.00039'sDownsection verified to restore all ten. Worth noting for reviewers that the DB-backed tests inpkg/core/server(registration_consensus_test.go,validator_state_test.go) skip in themake test-unitharness becauseTEST_DB_URLis unset there; they were run separately against a real Postgres for this change and pass.🤖 Generated with Claude Code