domain,db: add VtxoKind discriminator for on-chain Arkade UTXOs (part of #1159) - #1161
domain,db: add VtxoKind discriminator for on-chain Arkade UTXOs (part of #1159)#1161bitcoin-coder-bob wants to merge 8 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. WalkthroughChangesThe VTXO domain now distinguishes offchain and onchain kinds. PostgreSQL and SQLite migrations, queries, repositories, and row converters persist and restore this discriminator. Onchain VTXOs are excluded from note classification and forfeiture checks, and never expire. Migration rollback coverage now includes both databases. ChangesVTXO kind support
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: 🔵 Low · up to The change adds a discriminator that correctly separates on-chain and off-chain lifecycle behavior, but rolling back the database migration after on-chain records exist can erase that distinction and restore incorrect handling. The PR is mergeable with explicit owner awareness and follow-up to make rollback value-preserving or prevent unsafe rollback. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Adds a vtxo_kind column (0 = offchain, 1 = onchain) so an on-chain Arkade UTXO (issue #1159) can be told apart from a batch leaf or offchain-tx output. Without it, such a vtxo misclassifies: empty commitment fields read as a note, and the sweeper would treat SpentBy as a checkpoint txid. - domain.Vtxo gains a Kind field; IsNote() keys off it so an on-chain vtxo with empty commitments is not a note. - sqlite + postgres migrations add the column (DEFAULT 0 backfills all history as offchain) and recreate vtxo_vw / intent_with_inputs_vw so it is visible. badger persists it via gob with no migration; old records decode to offchain. - UpsertVtxo and rowToVtxo carry Kind on both SQL backends. Chosen as an explicit enum over a bool so future on-chain sub-kinds need no further migration. Behaviour-preserving: nothing writes Onchain yet, so every existing row is offchain and all classifiers evaluate as before. The protective read-guards (sweeper/indexer filters) land with the on-chain write path, where they are testable against real on-chain rows. Part of #1159.
rowToVtxo on both SQL backends already carried Kind, but the round-replay converter (combinedRowToVtxo) and the marker converters built domain.Vtxo values without it, so a vtxo read through those paths came back as VtxoKindOffchain regardless of its stored kind. Any guard keyed on Kind downstream of round replay or marker preload would have been silently wrong. Note combinedRowToVtxo also drops Depth and MarkerIDs, which predates this work. Left alone here rather than widening the change, but it means round replay does not reconstruct the DAG fields either.
An on-chain Arkade UTXO has no batch expiry, so ExpiresAt is not meaningful for it and is left zero. Without this guard IsExpired would compare against the Unix epoch and report every on-chain vtxo as expired, which cascades through RequiresForfeit and the two spend-path checks in service.go and would make them permanently unspendable. Behaviour-preserving today: nothing writes VtxoKindOnchain yet, so every existing row is offchain and evaluates exactly as before.
be8c7fc to
fc48791
Compare
|
This PR has been open for 3+ days without review. @bitcoin-coder-bob is anyone looking at this? |
|
This PR has been open for 14 days without review. @bitcoin-coder-bob is anyone looking at this? |
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Arkana review — sha fc48791
Looks ready to merge.
The VtxoKind discriminator is a clean additive change. The two domain fixes it enables are necessary and correct:
IsNote(): An on-chain UTXO has no commitment txids, so without the kind check it would silently masquerade as a note. The earlyKind != VtxoKindOnchainguard is the right place.IsExpired(): A zeroExpiresAton an on-chain UTXO would read as the Unix epoch (1970), meaning every on-chain UTXO would appear permanently expired. The short-circuit is correct.
Migration safety: ALTER TABLE vtxo ADD COLUMN IF NOT EXISTS vtxo_kind INTEGER NOT NULL DEFAULT 0 — DEFAULT 0 means all existing rows map to VtxoKindOffchain with no backfill needed. The view recreation is straightforward.
The down.sql rolls back cleanly.
Tests cover both new cases. No concerns.
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Review — domain,db: add VtxoKind discriminator (part of #1159)
What changed:
Adds (uint8 enum) to with (0, default) and . Fixes two methods:
-
**** — an on-chain Arkade UTXO has no commitment txids (like a note), so without the kind check it would read as a note. The guard prevents that.
-
**** — an on-chain Arkade UTXO has (zero value), which would always return true (1970 is in the past). The guard returns for , preventing all such vtxos from reading as permanently expired and being treated as unspendable.
** as open enum (uint8):** Intentional design note says future on-chain sub-kinds can be added without a schema migration — this means callers that switch on the kind need to handle unknown values. Worth ensuring any switch in downstream code has a default case.
Tests: Both and have the onchain fixture case. Good.
Looks ready to merge.
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Review — VtxoKind discriminator
Verdict: looks ready to merge. Stacked on #1160.
Additive domain change to distinguish off-chain batch/offchain-tx VTXOs from on-chain Arkade UTXOs (#1159 feature).
Key correctness fixes this PR addresses:
IsNote(): on-chain Arkade UTXOs have no commitment txids, so without the kind check they'd read as notes. The guard is correct.IsExpired(): on-chain Arkade UTXOs carry a zeroExpiresAt(no batch expiry). Without the guard every such VTXO would be permanently "expired" and rejected by any check that callsIsExpired(). The guard is correct.
DB migration:
- Additive column:
ALTER TABLE vtxo ADD COLUMN IF NOT EXISTS vtxo_kind INTEGER NOT NULL DEFAULT 0. Existing rows getVtxoKindOffchain(0) automatically — no backfill needed. ✅ - Views dropped and recreated to expose the column. Down migration reverses this correctly. ✅
- sqlc-generated code updated consistently across all SELECT queries that project from
vtxo_vwandintent_with_inputs_vw. ✅
One observation: VtxoKind is typed as uint8 in Go but INTEGER (int32) in Postgres (via sqlc's sql.NullInt32/int32). The conversion domain.VtxoKind(row.VtxoKind) truncates int32 → uint8, which is fine as long as the column stays small (it will). A comment noting this would help future readers, but it's not blocking.
Tests are present and cover the two new cases (onchain kind not-a-note, onchain kind never-expired). ✅
|
This PR has been open for 5+ days without a review. @bitcoin-coder-bob is anyone looking at this? (VtxoKind discriminator (part of #1159); 5 days without review.) |
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
PROTOCOL-CRITICAL: human review required.
Summary
This PR adds a VtxoKind discriminator (VtxoKindOffchain=0, VtxoKindOnchain=1) to domain.Vtxo and wires it through SQLite, Postgres, and Badger persistence. The approach is sound for what it sets out to do, and the IsNote() / IsExpired() fixes are correct. However, there are several issues that need attention before merge.
🔴 Must-fix: RequiresForfeit() will misfire on VtxoKindOnchain vtxos
internal/core/domain/vtxo.go:81
func (v Vtxo) RequiresForfeit() bool {
return !v.Swept && !v.IsNote() && !v.Unrolled
}IsNote() now correctly returns false for VtxoKindOnchain. As a side-effect, an unspent, non-swept, non-unrolled on-chain vtxo will have RequiresForfeit() == true once #1159 begins writing them. This propagates to at least five call sites — some of which are not "sweeper/indexer" paths but round-processing and forfeit-initialization paths:
internal/core/application/service.go:1852— attempts taproot-script validation on the vtxo, which will either fail or pass on wrong script expectations.internal/infrastructure/live-store/inmemory/forfeits.go:37/redis/forfeits.go:51— adds the vtxo to the forfeit-signing list.internal/infrastructure/db/service.go:856— expects a matching forfeit txid inforfeitInputs; on-chain vtxos will silently map to an emptyspentBy.internal/core/domain/intent.go:117/round.go:174— gates round state transitions onRequiresForfeit().
The PR body says protective guards land with the write path (#1159). That is acceptable for the sweeper / SpentBy-as-checkpoint readers. But RequiresForfeit() is a domain-level invariant, not a sweeper concern; there is no call-site-level guard that prevents it from misfiring once on-chain vtxo records exist. A future PR writer or merge-conflict resolution can easily fail to add the guard before the write path is exercised.
Required action: Either extend RequiresForfeit() to return false for VtxoKindOnchain (the safest change here, symmetric with the IsNote() and IsExpired() fixes), or add an explicit TODO/compile-time note that every RequiresForfeit() call site must be guarded before #1159 lands. A test (TestVtxo_RequiresForfeit) covering the onchain-kind case would catch a regression.
🟡 Postgres down-migration has no automated roundtrip test
internal/infrastructure/db/postgres/migration/20260901000000_add_vtxo_kind.down.sql
TestAddVtxoKindDownMigration (in vtxo_kind_down_test.go) runs against an in-memory SQLite instance via newSweptVtxoMigrator. The Postgres down migration is untested at the migration-layer level; the integration run described in the PR ("fresh DB") exercises the up path only. If the Postgres down migration has a syntax or dependency-ordering error it will surface only in a live rollback.
This is lower risk than the RequiresForfeit issue but should be noted. Consider a CI job or separate test that applies the full Postgres migration chain and back.
🟡 combinedRowToVtxo still drops Depth and MarkerIDs
internal/infrastructure/db/postgres/round_repo.go:708 / internal/infrastructure/db/sqlite/round_repo.go:849
The PR correctly flags this pre-existing gap. After this change, Kind is now wired through round replay but Depth and MarkerIDs are still dropped. Any downstream code that branches on Kind for vtxos read through round replay will have a vtxo with correct Kind but incorrect Depth and no MarkerIDs. That mismatch could produce silent errors if DAG-aware logic is added before combinedRowToVtxo is fixed. A follow-up issue / TODO comment in the code would help.
🟡 No range-check on VtxoKind at deserialization
internal/infrastructure/db/postgres/vtxo_repo.go:648, internal/infrastructure/db/sqlite/vtxo_repo.go:737, and all combinedRowToVtxo sites.
Kind: domain.VtxoKind(row.VtxoKind),VtxoKind is declared as uint8; the stored column is INTEGER (32/64-bit). An out-of-range value stored by a future migration, a direct SQL edit, or a bug in #1159 will cast silently to an undefined VtxoKind value, bypassing all IsNote() / IsExpired() guards. Consider a small helper:
func vtxoKindFromInt(v int32) (VtxoKind, error) {
switch VtxoKind(v) {
case VtxoKindOffchain, VtxoKindOnchain:
return VtxoKind(v), nil
default:
return VtxoKindOffchain, fmt.Errorf("unknown vtxo_kind %d", v)
}
}Or at minimum a metric / log on unrecognised values so the operator knows before a customer reports funds gone.
🟡 SQLite down migration: DROP COLUMN without IF EXISTS
internal/infrastructure/db/sqlite/migration/20260901000000_add_vtxo_kind.down.sql:3
ALTER TABLE vtxo DROP COLUMN vtxo_kind;The Postgres down migration uses DROP COLUMN IF EXISTS. The SQLite variant does not. If the column is missing (interrupted partial rollback), the SQLite down migration will error out rather than succeed idempotently. On SQLite ≥ 3.35.0 IF EXISTS is supported for DROP COLUMN; prefer consistency with Postgres.
✅ Confirmed correct
DEFAULT 0backfill: existing rows correctly becomeVtxoKindOffchainwith no data migration.vtxo_vwisSELECT v.*and picks upvtxo_kindautomatically after recreation — no column enumeration risk.intent_with_inputs_vwenumerates columns and hasv.vtxo_kindadded explicitly in the up migration — correct.- Badger
vtxoDTOembedsdomain.Vtxovia gob; old records decode withKind=0(offchain) — correct. IsNote()andIsExpired()fixes are well-reasoned and tested.- View drop order (intent first, then vtxo_vw) is safe —
intent_with_inputs_vwselects from the basevtxotable, not fromvtxo_vw. combinedRowToVtxoKindwiring: Postgres usessql.NullInt32.Int32, SQLite usessql.NullInt64.Int64— consistent with the respective model types.- No cross-repo API surface change detected:
VtxoKindis internal toarkd; SDKs (ts, go, rust, dotnet) have novtxo_kindreferences.
Noted / out of scope (acknowledged in PR body)
SelectSweepableUnrolledVtxoswill surface on-chain vtxos if they ever haveunrolled=true; sweeper guard deferred to write path — acceptable for now.SpentBy-as-checkpoint reader routing for on-chain vtxos — same deferral, same reasoning.
An on-chain Arkade UTXO joins a batch as a boarding input, signed directly and never forfeited. IsNote already excludes the kind, which flipped RequiresForfeit to true for it, so guard it explicitly alongside IsNote and IsExpired.
Same up, down, re-apply sequence on both SQL backends, against a database the test owns on the shared test server.
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
PROTOCOL-CRITICAL: human review required.
Follow-up pass — 9c37440 vs b1e0e7b. Two substantive commits. Zero prior issues to close (previous pass at fc48791 raised none).
Prior issues
None raised in the last pass; nothing to close.
New commits in scope
Three commits land since b1e0e7b:
| sha | subject |
|---|---|
f863e48 |
Migrate to btcd v0.26 and the v2 sub-modules (#1173) — merged from master, reviewed separately |
4b3a433 |
domain: on-chain kind never requires a forfeit |
262e626 |
db: run the add_vtxo_kind down-migration test on postgres too |
The btcd v0.26 rename (f863e48) is a mechanical import-path rewrite that arrived via the master merge; it does not change any logic on this branch. I will not re-audit it here.
4b3a433 — RequiresForfeit() guard (internal/core/domain/vtxo.go:96)
Correct. Before this commit, RequiresForfeit() was:
return !v.Swept && !v.IsNote() && !v.UnrolledFor a VtxoKindOnchain VTXO that carries commitment txids (so IsNote() correctly returns false because of the kind check already there), the old expression evaluates true — signalling a forfeit path that onchain boarding inputs never take. The new guard:
return v.Kind != VtxoKindOnchain && !v.Swept && !v.IsNote() && !v.Unrolledshort-circuits immediately for any onchain kind, regardless of sweep/note/unroll state.
The added test case (vtxo_test.go:191-202) exercises exactly the dangerous scenario: an onchain VTXO with a non-empty CommitmentTxids slice (which would have defeated the IsNote() escape) and a future ExpiresAt, asserting requiresForfeit: false. Coverage is complete.
No concerns.
262e626 — Postgres down-migration test (internal/infrastructure/db/vtxo_kind_down_test.go)
Clean. The refactor extracts a schema interface and adds a postgresSchema implementation, so the same up/down/re-up assertion sequence runs on both backends.
Points checked:
newVtxoKindPostgresMigratorcallsnewMigrator().Drop()to wipe prior state, then constructs a fresh instance — correct, becauseDropinvalidates the migrator it ran on.information_schema.columnsquery is fully parameterised ($1,$2); no injection surface.- The SQLite
PRAGMA table_info(%s)path remains unparameterised, but table names are hardcoded literals in the test — no risk. - The dedicated database (
vtxo_kind_migration) is separate fromTestService's database, so the two cannot clobber each other. - Hard-coded test-server credentials in a test constant is the existing project pattern; no new exposure.
One observation worth a follow-up (not a blocker): t.Run("postgres", ...) will hard-fail on any machine without a reachable test postgres server. The SQLite sub-test will still pass in isolation. If there are developer environments without postgres, a testing.Short() skip or a build tag would protect them. This matches the existing TestService behaviour, so it is not a regression here.
Summary
All new logic is correct. The RequiresForfeit fix closes a real gap: without it, any onchain VTXO that carried commitment txids would be treated as requiring a forfeit signature it can never provide. The migration-test extension gives both backends the same rollback coverage. No blocking issues.
|
Changes were requested on this PR (open 40+ days). @bitcoin-coder-bob need any help addressing the feedback? (VtxoKind discriminator) |
Addressed on 9c37440: RequiresForfeit guards the on-chain kind with a test, and the down-migration test runs on postgres too. The follow-up review on 2026-09-02 confirmed both. Dismissing the stale request.
|
This PR has been open for 41+ days without a review. @bitcoin-coder-bob — VtxoKind discriminator: is this waiting on other PRs in the series to land first? |
|
This PR has been open for 41 days without review. @bitcoin-coder-bob is the VtxoKind discriminator still moving forward? |
2 participants
Second slice of #1159 (on-chain Arkade execution). Not stacked on #1160: it shares no files with #1160 or #1162, is based on master, and merges cleanly on its own. See "Merge ordering" below for why it should still go in first.
Adds the
VtxoKinddiscriminator. The design doc's original "no schema change" assumption was false. A purely on-chain-derived Arkade UTXO misclassifies against the existing vtxo fields (empty commitment fields read as a note viaIsNote();Unrolled=trueroutes into the sweeper which treatsSpentByas a checkpoint txid). A discriminator resolves this.Confirmed on the #1159 design call: the direction is a unified indexer with a type flag rather than a separate on-chain indexer, which is what this builds. An
on-chain unconfirmedstate is wanted eventually, which the enum absorbs without a second migration (see "Why an enum, not a bool" below).What this does
domain.Vtxogains aKindfield (VtxoKindOffchain=0,VtxoKindOnchain=1).IsNote()keys off it so an on-chain vtxo with empty commitments is not a note, andIsExpired()returns false for it since there is no batch expiry on-chain.RequiresForfeit()returns false for the on-chain kind. Excluding it fromIsNote()had flipped this to true, and an on-chain UTXO joins a batch as a boarding input that is signed directly and never forfeited. Guarded explicitly alongsideIsNote()andIsExpired().vtxo_kind INTEGER NOT NULL DEFAULT 0and recreatevtxo_vw/intent_with_inputs_vwso it is visible (vtxo_vwisSELECT v.*; the intent view enumerates columns, sov.vtxo_kindis added by hand).DEFAULT 0backfills all history as offchain, no data migration.UpsertVtxo(write) androwToVtxo(read) carryKindon both SQL backends.Why an enum, not a bool
Same migration cost, but future on-chain sub-kinds (assets, cosigned-vs-unilateral-exit) need no further migration. This was a judged decision (explicit enum vs bool vs derive-from-fields); derive-from-fields was rejected because it overloads
Preconfirmedfor every reader and opens a new fraud misfire.Scope / behaviour
Behaviour-preserving: nothing writes
Onchainyet, so every existing row is offchain and all classifiers evaluate as before. The protective read-guards (sweeper/indexer filters that keep on-chain rows away from theSpentBy-as-checkpoint readers) deliberately land with the on-chain write path, where they are testable against real on-chain rows.Second commit: carry Kind through the round and marker converters
rowToVtxocarriedKindfrom the start, but the round-replay converter (combinedRowToVtxo) and the marker converters builtdomain.Vtxovalues without it, so a vtxo read through those paths came back asVtxoKindOffchainno matter what was stored. Any guard keyed onKinddownstream of round replay or marker preload would have been silently wrong, which is the kind of gap that only shows up once something depends on it. Now wired on both SQL backends.Adjacent finding, left alone:
combinedRowToVtxoalso dropsDepthandMarkerIDs. That predates this PR (it came in with the DAG work), so round replay does not reconstruct the DAG fields either. Flagged rather than fixed here to keep this change scoped.Test plan
go build ./...,make lint(0 issues), gofmt cleanTestVtxo_IsNoteextended: an onchain-kind vtxo with empty commitments is not a noteTestVtxo_RequiresForfeitgains the on-chain-kind case, verified to fail without the guardTestAddVtxoKindDownMigration(new) runs on both sqlite and postgres: up adds the column and surfaces it invtxo_vw, down drops it and recreates the views, re-apply works. The postgres case runs against a database the test owns on the shared test server, so it never touchesTestServicestateTestService/.../postgres_stores) run on a fresh DB, migration chain applies and the vtxo round-trip carriesKindMerge ordering
Merge this one first of the open #1159 PRs (#1160, #1161, #1162, and #1174, which also waits on this: its onchain-spend tracking is scoped to
unrolled = truetoday, and the follow-up that widens it toKind = Onchainbuilds on the discriminator added here). Not for a code dependency, there is none and the three merge cleanly in any order, but because of the migration. It is stamped20260901000000, and the repo uses golang-migrate, which tracks a single current version and silently skips any migration numbered below it. If a later-stamped migration lands on master and reaches a deployed database before this merges, thevtxo_kindcolumn never gets added there. Master's latest migration is20260807120000today. If that changes before this merges, bump this migration's timestamp past it.Review notes
DROP COLUMN IF EXISTS(syntax error, checked on 3.37.2), so the sqlite down migration drops the column unconditionally while the postgres one keepsIF EXISTS. That asymmetry is deliberate.combinedRowToVtxostill dropsDepthandMarkerIDs. Pre-existing, tracked under "Known gaps in reused code" in On-chain Arkade execution: arkd as an on-chain co-signing oracle #1159.Status
Out of draft. The discriminator representation (unified indexer with a type flag, enum rather than bool) was confirmed on the #1159 design call.
Summary by CodeRabbit
New Features
Database
Tests