Add filters to indexer.GetVirtualTxs#1090
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughExtends GetVirtualTxs with CEL-based and time-range filtering, persists ARK extension packet-type metadata per offchain transaction for structured matching, replaces single-record repository reads with filtered batch queries, and implements asynchronous packet-type backfill with graceful lifecycle management across Badger, Postgres, and SQLite backends. ChangesOffchain Transaction Filtering with Packet Support
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![arkanaai[bot]](https://avatars.githubusercontent.com/u/170820085?s=60&v=4){kind=small}
There was a problem hiding this comment.
Arkana Code Review — indexer.GetVirtualTxs Filter Support
Thorough review of the full diff. The CEL filter allowlist design is solid — the AST walker is strict, correctly rejects OR/NOT/!=/size()/.contains(), validates hex payloads, and range-checks packet types to [0, 255]. Good work there. Several issues need addressing before merge.
🔴 CRITICAL — Unbounded queries / full table scan (DoS vector)
SelectOffchainTxs (the no-txids path) in both Postgres and SQLite has no LIMIT clause. When a caller sends GetVirtualTxs with only a time-range filter and no txids, the query returns ALL matching rows. Pagination (paginate()) runs in Go after the full result set is loaded into memory. On a mature server with millions of offchain txs, this is an OOM/DoS vector.
- Postgres:
query.sql,SelectOffchainTxsquery - SQLite:
query.sql,SelectOffchainTxsquery - Badger:
ark_repo.go,GetOffchainTxs— doesr.store.Find(&all, nil)loading the entire table into memory regardless of filter
Fix: Push LIMIT into the SQL queries (e.g., LIMIT maxPageSizeVirtualTxs + 1), or add a guard in GetOffchainTxs that rejects unbounded queries. The handler currently guards this via parseTxids requiring non-empty txids, but that guard is fragile — a future code path could bypass it.
🔴 HIGH — No max-length guard on CEL expression string
offchain_tx_filter.go: txfilter.TxFilterEnv.Compile(expression) runs full CEL parsing before the AST walker validates the shape. There is no length limit on the expression string. A malicious client could send a multi-MB CEL expression to force CPU-intensive parsing before type-check rejects it.
Fix: Add if len(expression) > 4096 { return error } before Compile().
🟡 MEDIUM — Backfill creates silent data-consistency window
startBackfill (both postgres/offchain_tx_repo.go and sqlite/offchain_tx_repo.go) runs a background goroutine to populate the new packets column. During backfill, queries with WithExtension: true or WithPacket will silently miss rows that haven't been backfilled yet (their packets column is still NULL, excluded by packets IS NOT NULL AND packets <> '').
There is no mechanism for the RPC handler to know whether backfill is complete, and no warning to clients. A client doing gap-fill after a subscription drop could receive incomplete results.
Suggestions:
- Expose a backfill-complete flag, and either warn or block filter queries until done.
- Or fall back to PSBT decoding for rows with
NULLpackets during the backfill window.
🟡 MEDIUM — FinalizeOffchainTx conflates DB errors with "not found"
internal/core/application/service.go, FinalizeOffchainTx:
offchainTxs, err := s.repoManager.OffchainTxs().GetOffchainTxs(...)
if err != nil || len(offchainTxs) == 0 {
return "", fmt.Errorf("TX_NOT_FOUND")
}A database timeout or connection error now returns TX_NOT_FOUND instead of a proper internal error. This is protocol-critical — FinalizeOffchainTx commits funds. Masking real DB errors behind "not found" could cause silent failures.
Fix: Check err and len(offchainTxs) == 0 separately with different error codes.
🟡 MEDIUM — SQL vs Badger inconsistency on failed-tx filtering
The SQL queries filter with COALESCE(fail_reason, '') = '', which excludes rows with a non-empty fail_reason. The badger implementation checks off.IsFailed(), which checks the domain Stage.Failed flag. A row could have Stage.Failed = true but an empty FailReason — SQL would include it, badger would exclude it. This backend inconsistency could cause hard-to-debug behavior differences.
- Postgres/SQLite:
query.sql,COALESCE(fail_reason, '') = '' - Badger:
ark_repo.go,off.IsFailed()
🟡 MEDIUM — Semantic breaking change not gated
GetVirtualTxs now returns only offchain txs. Previously it also returned checkpoint and round-commitment txs via Rounds().GetTxsWithTxids(). Existing consumers (rust-sdk, go-sdk, ts-sdk, fulmine, emulator) that query checkpoint txids will silently get empty results. The proto comment documents this, but there's no versioning or deprecation period.
Cross-repo consumers found:
| Consumer | Risk |
|---|---|
rust-sdk (ark-grpc/src/client.rs:472) |
Has vendored indexer.proto — needs proto sync |
| go-sdk → emulator, fulmine | Interface signature may need updating |
| ts-sdk → banco | REST gateway, additive fields — safe |
Suggestion: Consider a include_round_txs field (default true for backward compat) or a versioned RPC.
🟢 LOW — context.Background() for backfill (no graceful shutdown)
Both postgres/offchain_tx_repo.go and sqlite/offchain_tx_repo.go call startBackfill(context.Background()). The goroutine runs until the process exits with no graceful cancellation. On large databases, this could consume DB resources for an extended period during shutdown.
Fix: Accept a context from the constructor and cancel it on Close().
🟢 LOW — Migration uses DROP+CREATE VIEW
20260603162258_offchain_tx_packets.up.sql (both backends): DROP VIEW IF EXISTS offchain_tx_vw then CREATE VIEW creates a brief window where concurrent queries against the view fail. Postgres supports CREATE OR REPLACE VIEW which is atomic.
🟢 LOW — No test coverage for backfill
startBackfill / backfillPackets has no dedicated test. service_test.go runs against a clean DB where backfill has nothing to do. There's no test verifying that pre-existing rows with packets IS NULL are correctly populated.
🟢 LOW — Duplicated encode/decode functions
encodePacketsColumn and decodePacketsColumn are duplicated verbatim between postgres/offchain_tx_repo.go and sqlite/offchain_tx_repo.go. Extract to a shared package.
⚠️ PROTOCOL-CRITICAL FLAG
Changes to FinalizeOffchainTx and SubmitOffchainTx error handling touch the offchain tx lifecycle. These paths handle fund commitment. Even though the changes look correct, human review is required per protocol-critical code policy.
Summary
| Priority | Count | Items |
|---|---|---|
| 🔴 Critical/High | 2 | Unbounded queries, CEL expression length |
| 🟡 Medium | 3 | Backfill consistency window, FinalizeOffchainTx error masking, SQL/Badger inconsistency |
| 🟢 Low | 4 | context.Background, DROP+CREATE, no backfill tests, code duplication |
The filter design (CEL allowlist, AST walker, MatchPackets) is well-engineered. The main issues are infrastructure-level: unbounded queries, missing guards at system boundaries, and a data-consistency gap during backfill. Fix the critical/high items and the FinalizeOffchainTx error masking, and this is in good shape.
🤖 Reviewed by Arkana
bitcoin-coder-bob
force-pushed
the
bob/GetVirtualTxs-filters
branch
from
fecb579 to
16d2581
Compare
|
@arkanaai @coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get PR details and file list
gh pr view 1090 --repo arkade-os/arkd --json title,body,files,additions,deletions,changedFiles | jq '{title, additions, deletions, changedFiles, files: [.files[].path]}'Length of output: 2857 🏁 Script executed: #!/bin/bash
# Get the PR diff
gh pr diff 1090 --repo arkade-os/arkd 2>/dev/null | head -5000Length of output: 195665 🏁 Script executed: #!/bin/bash
# Get the PR diff - second part
gh pr diff 1090 --repo arkade-os/arkd 2>/dev/null | tail -n +5001 | head -5000Length of output: 883
Review SummaryThis is a well-structured PR that adds CEL-expression and time-range filtering to Correctness1. Badger backend: scan cap applies to output, not to the scan itself In // Cap unconstrained queries to bound memory, matching the SQL
// backends' SelectOffchainTxs LIMIT.
if len(wantTxids) == 0 && len(out) >= domain.OffchainTxsScanLimit {
break
}The cap is checked against 2. Both func (v *offchainTxRepository) startBackfill(ctx context.Context) {
go func() {
if err := BackfillPackets(ctx, v.db); err != nil { … }
}()
}But the call site passes repo.startBackfill(context.Background())If the repo is closed (e.g., in a test that creates and immediately tears down a repo), the backfill goroutine has no way to be cancelled and will run against the already-closed DB, producing an error. Consider tying this to a context derived from the repo's own lifecycle (e.g., stored in the struct and cancelled in 3. Ordering inconsistency between Badger and SQL backends The SQL queries order results Minor Issues4. Postgres backfill test hardcodes a local DSN
dsn := "postgres://root:secret@localhost:5432/event?sslmode=disable"This test will always be skipped or fail in CI unless a Postgres container happens to be present. The PR description acknowledges Postgres tests need CI, but if the CI runner doesn't expose that DSN, the test effectively never runs in automation. Consider using 5. n, err := strconv.Atoi(p)
if err != nil {
continue // silent drop
}A persisted value like 6.
7. // hasPacket then equality on same packet merges to payload
"hasPacket(tx.extension, 9) && tx.extension[9] == 'aa'"The test covers this, but the merge logic depends on processing if _, ok := f.WithPacket[pt]; !ok {
f.WithPacket[pt] = ""
}If the 8.
Nits
🐇 🧠 Learnings used |
|
@arkanaai @coderabbitai review |
|
🧠 Learnings used✅ Action performedReview finished.
|
Review: Add filters to indexer.GetVirtualTxsSolid PR overall — the layering is clean, the design rationale is well-documented, and the test coverage (unit tests for 🐛 Bug: down migration ordering will fail in PostgreSQLIn DROP VIEW IF EXISTS offchain_tx_vw; -- (1) view dropped
CREATE VIEW offchain_tx_vw AS SELECT offchain_tx.*, ...; -- (2) new view created — packets column still exists, so it's included
ALTER TABLE offchain_tx DROP COLUMN packets; -- (3) fails: view created in step (2) depends on the packets columnPostgreSQL resolves
The fix is to swap steps (2) and (3): DROP VIEW IF EXISTS offchain_tx_vw;
ALTER TABLE offchain_tx DROP COLUMN packets; -- column gone first
CREATE VIEW offchain_tx_vw AS SELECT offchain_tx.*, ...; -- now * doesn't include packetsSame issue in the SQLite down migration (SQLite won't error, but the view would silently reference a ghost column until rebuilt).
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/core/domain/offchain_tx.go (1)
73-97:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDefensively copy packet slices at event/state boundaries.
Line 73 and Line 243 assign packet slices by reference. Because slices alias backing arrays, later mutation of the original/event slice can silently mutate aggregate state. Snapshot the slice when raising and when applying the event.
Suggested fix
func (s *OffchainTx) Request( arkTxid, arkTx string, unsignedCheckpointTxs map[string]string, packets []int, ) (Event, error) { + packetCopy := append([]int(nil), packets...) if s.IsFailed() || s.Stage.Code != int(OffchainTxUndefinedStage) { return nil, fmt.Errorf("not in a valid stage to request offchain tx") } @@ event := OffchainTxRequested{ @@ - Packets: packets, + Packets: packetCopy, } @@ case OffchainTxRequested: @@ - s.Packets = e.Packets + s.Packets = append([]int(nil), e.Packets...)Also applies to: 243-243
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/core/domain/offchain_tx.go` around lines 73 - 97, The Packets slice is being assigned by reference into the OffchainTxRequested event and later into state (causing aliasing bugs); make defensive copies both when creating the event (replace direct assignment of Packets into OffchainTxRequested with a new slice copy, e.g. append([]int(nil), packets...)) and when applying the OffchainTxRequested event to state (copy the event.Packets into state instead of assigning the slice directly). Update the event creation site where OffchainTxRequested is constructed and the event application/reducer that handles OffchainTxRequested (the code that reads event.Packets and writes it into the aggregate/state) to use make+copy or append to produce an independent slice.internal/core/application/indexer.go (1)
430-456:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winToken whitelist is bypassed when
filter.WithTxidsis empty.At Line 448, auth scope is only enforced by iterating
filter.WithTxids; then Line 456 forwards the unmodified filter to storage. Since the gRPC path can pass filters without txids, a valid token can query outside its txid scope (private mode = full auth bypass; withheld mode can incorrectly avoid signature stripping).Suggested fix
func (i *indexerService) GetVirtualTxs( ctx context.Context, authToken string, filter domain.OffchainTxFilter, page *Page, ) (*VirtualTxsResp, error) { var valid bool switch i.txExposure { @@ case exposureWithheld: @@ - valid = true - for _, txid := range filter.WithTxids { + valid = true + if len(filter.WithTxids) == 0 { + filter.WithTxids = make([]string, 0, len(txidWhitelist)) + for txid := range txidWhitelist { + filter.WithTxids = append(filter.WithTxids, txid) + } + } + for _, txid := range filter.WithTxids { if _, ok := txidWhitelist[txid]; !ok { valid = false break } } @@ case exposurePrivate: @@ + if len(filter.WithTxids) == 0 { + filter.WithTxids = make([]string, 0, len(txidWhitelist)) + for txid := range txidWhitelist { + filter.WithTxids = append(filter.WithTxids, txid) + } + } for _, txid := range filter.WithTxids { if _, ok := txidWhitelist[txid]; !ok { return nil, fmt.Errorf("auth token is not for txid %s", txid) } } valid = true }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/core/application/indexer.go` around lines 430 - 456, The auth token whitelist is effectively bypassed when filter.WithTxids is empty; in validateAuthToken/tokenCache.getTxids handling you must enforce the token's txid scope before calling getVirtualTxs. After computing hash via i.validateAuthToken and retrieving txidWhitelist via i.tokenCache.getTxids, if filter.WithTxids is empty, populate filter.WithTxids with the whitelisted txids (or return an error if none) so the storage query is limited to the token scope; if filter.WithTxids is non-empty, validate each entry against txidWhitelist as currently done. Ensure these changes occur before calling i.getVirtualTxs and keep references to validateAuthToken, tokenCache.getTxids, filter.WithTxids and getVirtualTxs to locate the code.
🧹 Nitpick comments (1)
internal/core/domain/offchain_tx_test.go (1)
54-55: ⚡ Quick winAdd one happy-path assertion for non-empty packet propagation.
All updated
Requestcalls currently passnil, so this suite doesn’t validate the newly addedPacketsflow. Add one valid request case with non-empty packets and assert both aggregate state and emittedOffchainTxRequested.Packets.Suggested test update
t.Run("valid", func(t *testing.T) { offchainTx := domain.NewOffchainTx() + packets := []int{0, 255} @@ - event, err := offchainTx.Request(txid, arkTx, unsignedCheckpointTxs, nil) + event, err := offchainTx.Request(txid, arkTx, unsignedCheckpointTxs, packets) require.NoError(t, err) require.NotNil(t, event) @@ require.Equal(t, unsignedCheckpointTxs, offchainTx.CheckpointTxs) + require.Equal(t, packets, offchainTx.Packets) require.NotEmpty(t, offchainTx.StartingTimestamp) + + reqEvt, ok := event.(domain.OffchainTxRequested) + require.True(t, ok) + require.Equal(t, packets, reqEvt.Packets)Also applies to: 124-126, 136-137, 266-267, 344-345
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/core/domain/offchain_tx_test.go` around lines 54 - 55, Add a happy-path assertion that exercises the new Packets flow by calling Request with a non-nil, non-empty packets slice (instead of the current nil) and asserting both the aggregate state and the emitted OffchainTxRequested event contain that same packets data; update the test that currently calls offchainTx.Request(txid, arkTx, unsignedCheckpointTxs, nil) to include a sample packets value, then assert the returned aggregate (from Request) reflects packet propagation and that any emitted OffchainTxRequested.Packets equals the sample packets slice; apply the same pattern to the other analogous Request test cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api-spec/protobuf/ark/v1/indexer.proto`:
- Around line 267-277: The RPC uses a broad SubscriptionFilter type but
parseVirtualTxsFilter only accepts a single expression and forbids scripts;
define and use a unary-specific filter message (e.g., UnarySubscriptionFilter or
VirtualTxsFilter) that exposes exactly one optional expressions string and no
scripts, replace the field declaration SubscriptionFilter filter = 5; with that
new message type, and update the parser/function parseVirtualTxsFilter to accept
and validate the new unary-specific message so the proto and generated OpenAPI
accurately reflect the allowed shape.
In `@internal/core/application/indexer_exposure_test.go`:
- Around line 199-205: The tests set expectations on mockedRoundRepo
(rounds.On(...)) but only call off.AssertExpectations(t) in TestGetVirtualTxs,
so round expectations are never verified; update each test case that calls
setupMocks (the setupMocks closure, mockedRoundRepo and mockedOffchainTxRepo
usage in TestGetVirtualTxs) to also call rounds.AssertExpectations(t) after
off.AssertExpectations(t) (or replace the single AssertExpectations(t) with
assertions for both mocks) so the mockedRoundRepo expectations are asserted for
all cases where rounds.On(...) is used.
In `@internal/core/domain/offchain_tx_repo.go`:
- Around line 32-55: OffchainTxFilter's WithAfterDate/WithBeforeDate use 0 as a
sentinel so a legitimate timestamp 0 cannot be expressed; change the struct to
track presence explicitly (either make WithAfterDate and WithBeforeDate pointers
*int64 or add boolean flags e.g. WithAfterDatePresent/WithBeforeDatePresent) and
update Validate to check presence rather than value (e.g. if present and <0
error; if both present enforce order). Also update any SQL repo code that
currently tests "> 0" for pushdown to instead test the presence flags or nil
pointers so a timestamp of 0 is honored.
In `@internal/infrastructure/db/badger/ark_repo.go`:
- Around line 261-276: GetOffchainTxs currently reads all offchain_tx rows into
memory via r.store.Find / r.store.TxFind and only afterwards applies
OffchainTxsScanLimit, causing unbounded scans; change the logic in
GetOffchainTxs to apply the scan cap at the query stage by using badgerhold
query predicates with a limit/stop condition (or iterate with an early exit) so
no more than OffchainTxsScanLimit entries are loaded; update both the tx-aware
branch (ctx.Value("tx") handling using tx.(*badger.Txn) with r.store.TxFind) and
the non-tx branch (r.store.Find) to use the bounded query/iterator approach and
stop once the limit is reached, returning the partial results and no full-table
read.
In
`@internal/infrastructure/db/postgres/migration/20260603162258_offchain_tx_packets.down.sql`:
- Around line 1-14: The rollback tries to recreate view offchain_tx_vw using
offchain_tx.* before removing the packets column, causing DROP COLUMN packets to
fail because the view depends on it; fix by reordering the down migration so the
ALTER TABLE offchain_tx DROP COLUMN packets runs before recreating/creating the
offchain_tx_vw (or alternatively recreate the view selecting explicit columns
that exclude packets), updating the migration to drop the column first then
create the view (refer to offchain_tx_vw and ALTER TABLE offchain_tx DROP COLUMN
packets to locate the statements).
In `@internal/infrastructure/db/postgres/offchain_tx_backfill_test.go`:
- Around line 26-28: Replace the hardcoded dsn with an env-configured one: read
the DSN via os.Getenv("POSTGRES_DSN") at the top of the test in
offchain_tx_backfill_test.go, and if it is empty call t.Skip("skipping Postgres
integration test; set POSTGRES_DSN to run") to gate the test; otherwise pass
that value into sql.Open instead of the literal string (keep the require.NoError
checks as-is). Use the variable name dsn, sql.Open and require.NoError
referenced in the test so the change is localized and obvious.
In `@internal/infrastructure/db/postgres/sqlc/query.sql`:
- Around line 290-299: The query SelectOffchainTxs currently applies LIMIT to
rows emitted by offchain_tx_vw (which can duplicate txids due to checkpoint
joins) so the cap is row-based not tx-based; change the query to first select
distinct base transactions (e.g., select txid and starting_timestamp from the
underlying offchain_tx base table or a subquery that selects distinct txids
using offchain_tx_vw but grouped by txid) applying the LIMIT there, and then
join that limited set back to offchain_tx_vw to expand rows per txid; update
references to offchain_tx_vw and the ORDER BY/LIMIT to ensure LIMIT
sqlc.arg('lim')::int is applied to the deduplicated txid set (SelectOffchainTxs
-> use a subquery/CTE that produces unique txids before joining to
offchain_tx_vw).
In `@internal/infrastructure/db/sqlite/offchain_tx_repo.go`:
- Around line 33-34: The backfill goroutine is started with context.Background()
via repo.startBackfill which leaves it running past repository shutdown; change
startBackfill to accept a context (or use a repo-scoped context created with
context.WithCancel stored on the repo struct) and start it with that cancellable
context, store the cancel func and a sync.WaitGroup (or other join mechanism) on
the repo, then modify Close to call the cancel func and wait for the
goroutine(s) to exit (using the WaitGroup or equivalent) so the backfill stops
and any DB access is finished before Close returns; update calls to
repo.startBackfill, the repo struct, and Close to use these new fields (context
cancel + wait) accordingly.
In `@internal/infrastructure/db/sqlite/sqlc/queries/query.sql.go`:
- Around line 767-775: The SQL template for SelectOffchainTxsByTxids mixes
slice-expanded anonymous placeholders for txids (txid IN (/*SLICE:txids*/?))
with explicitly numbered placeholders (?2..?6), causing bind parameter
misalignment when len(txids)>1; update the query template to use only anonymous
placeholders for the subsequent filter arguments (replace CAST(?2..?6 AS
INTEGER) with CAST(? AS INTEGER) placeholders in the conditions that reference
packets, starting_timestamp bounds, and then regenerate the sqlc code so the
generated SelectOffchainTxsByTxids function and its callers use consistent
anonymous ? bindings.
In `@internal/infrastructure/db/sqlite/sqlc/query.sql`:
- Around line 296-304: The query SelectOffchainTxs currently applies LIMIT `@lim`
to the expanded offchain_tx_vw result, which can under-return unique offchain
txids; instead first select the capped set of unique offchain txids (e.g., a
subquery selecting DISTINCT txid or using ROW_NUMBER() OVER (PARTITION BY txid)
to pick one row per txid) applying the same filters and ORDER BY/LIMIT `@lim`,
then join that subquery back to offchain_tx_vw to retrieve the full
view-expanded rows for only those capped txids; update the SelectOffchainTxs SQL
to perform that two-step (subquery cap by txid then join to offchain_tx_vw)
using the same parameters (`@with_extension`, `@with_after/`@after_ts,
`@with_before/`@before_ts, `@lim`).
In `@internal/interface/grpc/handlers/indexer.go`:
- Around line 391-403: The else branch currently always calls
parseTxids(request.GetTxids()) and errors when missing txids; change it to only
parse and set filter.WithTxids when the request actually contains txids (e.g.
len(request.GetTxids())>0 or request.GetTxids()!=nil), otherwise skip parsing
and leave filter.WithTxids empty/nil, then call e.indexerSvc.GetVirtualTxs(ctx,
request.GetToken(), filter, page) as-is; keep the same error handling for
parseTxids when present and use the existing symbols parseIndexerIntent,
parseTxids, GetVirtualTxsByIntent, and GetVirtualTxs to locate and update the
logic.
---
Outside diff comments:
In `@internal/core/application/indexer.go`:
- Around line 430-456: The auth token whitelist is effectively bypassed when
filter.WithTxids is empty; in validateAuthToken/tokenCache.getTxids handling you
must enforce the token's txid scope before calling getVirtualTxs. After
computing hash via i.validateAuthToken and retrieving txidWhitelist via
i.tokenCache.getTxids, if filter.WithTxids is empty, populate filter.WithTxids
with the whitelisted txids (or return an error if none) so the storage query is
limited to the token scope; if filter.WithTxids is non-empty, validate each
entry against txidWhitelist as currently done. Ensure these changes occur before
calling i.getVirtualTxs and keep references to validateAuthToken,
tokenCache.getTxids, filter.WithTxids and getVirtualTxs to locate the code.
In `@internal/core/domain/offchain_tx.go`:
- Around line 73-97: The Packets slice is being assigned by reference into the
OffchainTxRequested event and later into state (causing aliasing bugs); make
defensive copies both when creating the event (replace direct assignment of
Packets into OffchainTxRequested with a new slice copy, e.g. append([]int(nil),
packets...)) and when applying the OffchainTxRequested event to state (copy the
event.Packets into state instead of assigning the slice directly). Update the
event creation site where OffchainTxRequested is constructed and the event
application/reducer that handles OffchainTxRequested (the code that reads
event.Packets and writes it into the aggregate/state) to use make+copy or append
to produce an independent slice.
---
Nitpick comments:
In `@internal/core/domain/offchain_tx_test.go`:
- Around line 54-55: Add a happy-path assertion that exercises the new Packets
flow by calling Request with a non-nil, non-empty packets slice (instead of the
current nil) and asserting both the aggregate state and the emitted
OffchainTxRequested event contain that same packets data; update the test that
currently calls offchainTx.Request(txid, arkTx, unsignedCheckpointTxs, nil) to
include a sample packets value, then assert the returned aggregate (from
Request) reflects packet propagation and that any emitted
OffchainTxRequested.Packets equals the sample packets slice; apply the same
pattern to the other analogous Request test cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c91e2fd0-9c28-4a30-aced-a274fa6173a9
⛔ Files ignored due to path filters (2)
api-spec/protobuf/gen/ark/v1/indexer.pb.gois excluded by!**/*.pb.go,!**/gen/**api-spec/protobuf/gen/ark/v1/indexer.pb.rgw.gois excluded by!**/gen/**
📒 Files selected for processing (36)
api-spec/openapi/swagger/ark/v1/indexer.openapi.jsonapi-spec/protobuf/ark/v1/indexer.protointernal/core/application/indexer.gointernal/core/application/indexer_exposure_test.gointernal/core/application/offchain_tx_filter.gointernal/core/application/offchain_tx_filter_test.gointernal/core/application/service.gointernal/core/application/utils.gointernal/core/domain/offchain_tx.gointernal/core/domain/offchain_tx_event.gointernal/core/domain/offchain_tx_packets.gointernal/core/domain/offchain_tx_packets_test.gointernal/core/domain/offchain_tx_repo.gointernal/core/domain/offchain_tx_test.gointernal/infrastructure/db/badger/ark_repo.gointernal/infrastructure/db/postgres/migration/20260603162258_offchain_tx_packets.down.sqlinternal/infrastructure/db/postgres/migration/20260603162258_offchain_tx_packets.up.sqlinternal/infrastructure/db/postgres/migration/20260603195239_offchain_tx_packets_index.down.sqlinternal/infrastructure/db/postgres/migration/20260603195239_offchain_tx_packets_index.up.sqlinternal/infrastructure/db/postgres/offchain_tx_backfill_test.gointernal/infrastructure/db/postgres/offchain_tx_repo.gointernal/infrastructure/db/postgres/sqlc/queries/models.gointernal/infrastructure/db/postgres/sqlc/queries/query.sql.gointernal/infrastructure/db/postgres/sqlc/query.sqlinternal/infrastructure/db/service_test.gointernal/infrastructure/db/sqlite/migration/20260603162258_offchain_tx_packets.down.sqlinternal/infrastructure/db/sqlite/migration/20260603162258_offchain_tx_packets.up.sqlinternal/infrastructure/db/sqlite/migration/20260603195239_offchain_tx_packets_index.down.sqlinternal/infrastructure/db/sqlite/migration/20260603195239_offchain_tx_packets_index.up.sqlinternal/infrastructure/db/sqlite/offchain_tx_backfill_test.gointernal/infrastructure/db/sqlite/offchain_tx_repo.gointernal/infrastructure/db/sqlite/sqlc/queries/models.gointernal/infrastructure/db/sqlite/sqlc/queries/query.sql.gointernal/infrastructure/db/sqlite/sqlc/query.sqlinternal/interface/grpc/handlers/indexer.gointernal/interface/grpc/handlers/indexer_virtualtxs_filter_test.go
| // Optional filter on tx contents. The expressions field carries CEL | ||
| // expressions evaluated against the tx envelope (see SubscriptionFilter). | ||
| // Only the structured subset of CEL is supported on this unary RPC: a | ||
| // flat AND of `has(tx.extension)`, `hasPacket(tx.extension, N)`, and | ||
| // `tx.extension[N] == 'hex'` predicates. `tx.extension[N] == 'hex'` | ||
| // matches exactly (same semantics as the streaming SubscriptionFilter). | ||
| // Unsupported shapes (OR, NOT, !=, size(), .contains(), non-bool | ||
| // expressions, etc.) are rejected with InvalidArgument. The scripts | ||
| // field must be left empty; non-empty scripts are also rejected with | ||
| // InvalidArgument. | ||
| SubscriptionFilter filter = 5; |
There was a problem hiding this comment.
Use a unary-specific filter message here.
Reusing SubscriptionFilter makes this RPC's wire contract broader than the implementation: callers can legally send repeated expressions and scripts, but parseVirtualTxsFilter only accepts one expression and rejects any non-empty scripts. The generated OpenAPI inherits the same shape, so both gRPC and REST clients are told they can send requests that will always fail with InvalidArgument.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@api-spec/protobuf/ark/v1/indexer.proto` around lines 267 - 277, The RPC uses
a broad SubscriptionFilter type but parseVirtualTxsFilter only accepts a single
expression and forbids scripts; define and use a unary-specific filter message
(e.g., UnarySubscriptionFilter or VirtualTxsFilter) that exposes exactly one
optional expressions string and no scripts, replace the field declaration
SubscriptionFilter filter = 5; with that new message type, and update the
parser/function parseVirtualTxsFilter to accept and validate the new
unary-specific message so the proto and generated OpenAPI accurately reflect the
allowed shape.
| setupMocks: func(off *mockedOffchainTxRepo, rounds *mockedRoundRepo) { | ||
| off.On("GetOffchainTxs", mock.Anything, mock.Anything). | ||
| Return([]*domain.OffchainTx{}, nil) | ||
| // Pure-txid fallback into the rounds repo for txids | ||
| // the offchain repo didn't return. | ||
| rounds.On("GetTxsWithTxids", mock.Anything, testTxids). | ||
| Return([]string{}, nil) |
There was a problem hiding this comment.
mockedRoundRepo expectations are never asserted in TestGetVirtualTxs.
You set rounds.On(...) in several cases, but at Line 291 only off.AssertExpectations(t) is called. That can hide regressions in the rounds fallback path.
Suggested fix
require.NoError(t, err)
require.Len(t, resp.Txs, tc.wantTxs)
off.AssertExpectations(t)
+ rounds.AssertExpectations(t)
})
}
})Also applies to: 216-221, 230-235, 248-253, 291-292
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/core/application/indexer_exposure_test.go` around lines 199 - 205,
The tests set expectations on mockedRoundRepo (rounds.On(...)) but only call
off.AssertExpectations(t) in TestGetVirtualTxs, so round expectations are never
verified; update each test case that calls setupMocks (the setupMocks closure,
mockedRoundRepo and mockedOffchainTxRepo usage in TestGetVirtualTxs) to also
call rounds.AssertExpectations(t) after off.AssertExpectations(t) (or replace
the single AssertExpectations(t) with assertions for both mocks) so the
mockedRoundRepo expectations are asserted for all cases where rounds.On(...) is
used.
| type OffchainTxFilter struct { | ||
| WithTxids []string | ||
| WithExtension bool | ||
| WithPacket map[int]string | ||
| WithAfterDate int64 | ||
| WithBeforeDate int64 | ||
| } | ||
|
|
||
| // Validate enforces the structural invariants of the filter. The empty | ||
| // filter is allowed. WithAfterDate / WithBeforeDate may be set together | ||
| // (forming a "within" range) or individually. Bounds are inclusive on | ||
| // both sides, so before == after is permitted and selects rows whose | ||
| // starting_timestamp equals that value. | ||
| func (f OffchainTxFilter) Validate() error { | ||
| if f.WithAfterDate < 0 { | ||
| return fmt.Errorf("with_after_date must be non-negative") | ||
| } | ||
| if f.WithBeforeDate < 0 { | ||
| return fmt.Errorf("with_before_date must be non-negative") | ||
| } | ||
| if f.WithAfterDate > 0 && f.WithBeforeDate > 0 && f.WithBeforeDate < f.WithAfterDate { | ||
| return fmt.Errorf("with_before_date must be greater than or equal to with_after_date") | ||
| } | ||
| return nil |
There was a problem hiding this comment.
The time-range fields can't represent a valid 0 timestamp.
WithAfterDate and WithBeforeDate are optional, but this shape has no presence bit, so 0 is indistinguishable from "unset". The SQL repositories gate bound pushdown on > 0, which means after.timestamp = 0 and within(0, 0) silently drop the bound even though the public contract says the range is inclusive. This needs explicit presence flags or pointer timestamps instead of raw int64 sentinels.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/core/domain/offchain_tx_repo.go` around lines 32 - 55,
OffchainTxFilter's WithAfterDate/WithBeforeDate use 0 as a sentinel so a
legitimate timestamp 0 cannot be expressed; change the struct to track presence
explicitly (either make WithAfterDate and WithBeforeDate pointers *int64 or add
boolean flags e.g. WithAfterDatePresent/WithBeforeDatePresent) and update
Validate to check presence rather than value (e.g. if present and <0 error; if
both present enforce order). Also update any SQL repo code that currently tests
"> 0" for pushdown to instead test the presence flags or nil pointers so a
timestamp of 0 is honored.
| // NOTE: badgerhold has no per-field ordering or LIMIT push-down, so | ||
| // this call reads every offchain_tx row into memory before any | ||
| // predicate runs. Acceptable for the small-scale dev/test setups | ||
| // that use the badger backend; production deployments should use | ||
| // the SQL backends, which push the LIMIT to the DB engine. | ||
| var all []domain.OffchainTx | ||
| if ctx.Value("tx") != nil { | ||
| tx := ctx.Value("tx").(*badger.Txn) | ||
| if err := r.store.TxFind(tx, &all, nil); err != nil { | ||
| return nil, err | ||
| } | ||
| } else { | ||
| if err := r.store.Find(&all, nil); err != nil { | ||
| return nil, err | ||
| } | ||
| } |
There was a problem hiding this comment.
Scan cap is applied too late to protect Badger from full-table reads.
GetOffchainTxs loads all offchain_tx records before filtering and only then truncates to OffchainTxsScanLimit. This keeps no-txids requests unbounded in storage scan cost and memory, despite the cap.
Also applies to: 328-332
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/infrastructure/db/badger/ark_repo.go` around lines 261 - 276,
GetOffchainTxs currently reads all offchain_tx rows into memory via r.store.Find
/ r.store.TxFind and only afterwards applies OffchainTxsScanLimit, causing
unbounded scans; change the logic in GetOffchainTxs to apply the scan cap at the
query stage by using badgerhold query predicates with a limit/stop condition (or
iterate with an early exit) so no more than OffchainTxsScanLimit entries are
loaded; update both the tx-aware branch (ctx.Value("tx") handling using
tx.(*badger.Txn) with r.store.TxFind) and the non-tx branch (r.store.Find) to
use the bounded query/iterator approach and stop once the limit is reached,
returning the partial results and no full-table read.
| if request.GetIntent() != nil { | ||
| intent, parseErr := parseIndexerIntent(request.GetIntent()) | ||
| if parseErr != nil { | ||
| return nil, status.Error(codes.InvalidArgument, parseErr.Error()) | ||
| } | ||
| resp, err = e.indexerSvc.GetVirtualTxsByIntent(ctx, *intent, page) | ||
| resp, err = e.indexerSvc.GetVirtualTxsByIntent(ctx, *intent, filter, page) | ||
| } else { | ||
| txids, parseErr := parseTxids(request.GetTxids()) | ||
| if parseErr != nil { | ||
| return nil, status.Error(codes.InvalidArgument, parseErr.Error()) | ||
| } | ||
| resp, err = e.indexerSvc.GetVirtualTxs(ctx, request.GetToken(), txids, page) | ||
| filter.WithTxids = txids | ||
| resp, err = e.indexerSvc.GetVirtualTxs(ctx, request.GetToken(), filter, page) |
There was a problem hiding this comment.
Allow filtered/token-based lookups without txids.
This branch still calls parseTxids(request.GetTxids()) whenever intent is absent, so token + filter/time_range requests fail with missing txids even though GetVirtualTxsRequest now documents lookup "by id and/or by filter". The application layer already has an unpinned scan path, so the replay/catch-up flow stays blocked until the handler and HTTP binding both stop requiring a txid list.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/interface/grpc/handlers/indexer.go` around lines 391 - 403, The else
branch currently always calls parseTxids(request.GetTxids()) and errors when
missing txids; change it to only parse and set filter.WithTxids when the request
actually contains txids (e.g. len(request.GetTxids())>0 or
request.GetTxids()!=nil), otherwise skip parsing and leave filter.WithTxids
empty/nil, then call e.indexerSvc.GetVirtualTxs(ctx, request.GetToken(), filter,
page) as-is; keep the same error handling for parseTxids when present and use
the existing symbols parseIndexerIntent, parseTxids, GetVirtualTxsByIntent, and
GetVirtualTxs to locate and update the logic.
Resolve conflict in sqlite offchain_tx_repo.go: adopt master's split read/write SQLiteDB interface (withReadQuerier for GetOffchainTxs reads, v.db.Write() for writes and the packets backfill) on top of the PR's filter-based GetOffchainTxs and packets backfill.
Resolve conflicts from #1123 (return only accepted/finalized offchain txs). The PR replaced the singular SelectOffchainTx with the plural filter queries SelectOffchainTxs[ByTxids]; fold #1123's intent into them by changing the base filter to (stage_code = 2 OR stage_code = 3) for both sqlite and postgres, regenerate sqlc, and drop the now-unused singular query. service_test.go keeps the PR's filter-pushdown coverage and adopts #1123's request->accept->fail->finalize subtest, converted to GetOffchainTxs.
1 participant
Closes #1088.
Summary
Adds filter support to the unary
indexer.GetVirtualTxsRPC so clients can "fill the gap" after the streamingGetSubscriptionconnection drops, with the same expression shape and matching semantics they used on the stream.Wire surface
GetVirtualTxsRequest.filter(SubscriptionFilter): a single CEL expression, capped at 4 KiB. Supported predicates, combined with&&:has(tx.extension)hasPacket(tx.extension, N), whereNis an int packet type in the range[0, 255]tx.extension[N] == 'hex', an exact equality on the hex-encoded packet bytes, matching the streaming filter's semantics!=,size(),.contains(), non-bool, etc.) are rejected withInvalidArgument.filter.scriptsmust be empty.GetVirtualTxsRequest.time_range: aoneofoverTimeRangeAfter/TimeRangeBefore/TimeRangeWithinapplied tostarting_timestamp. Bounds are inclusive, sowithin(t, t)selects rows at exactlyt.Rounds().GetTxsWithTxidsso callers walking a mixed chain (e.g. client-lib's redeem path) keep working.tx.extension[N]andstarting_timestampare only defined for offchain txs.txidsset, the server applies a SQLLIMITofdomain.OffchainTxsScanLimit = 10000rows over distinct baseoffchain_tx.txids (via CTE, so checkpoint join expansion doesn't eat the cap). Callers using the gap-fill pattern should pair the filter with atime_rangebound to keep the scan set small. Cursor-based pagination over the filtered set is a planned follow-up.Domain + storage
domain.OffchainTx.Packets []intcarries the list of ARK extension packet types so repos do not have to decode raw PSBTs at query time. Populated byOffchainTx.Request(...)via theOffchainTxRequestedevent. The slice is snapshotted at both event-raise and state-apply time so a later mutation of the caller's backing array can't alias aggregate state.domain.OffchainTxRepository.GetOffchainTxbecomesGetOffchainTxs(ctx, filter)with adomain.OffchainTxFilter{WithTxids, WithExtension, WithPacket, WithAfterDate, WithBeforeDate}plus aValidate()method.MatchPacketslives onOffchainTxFilterindomainand is shared by all three backends. It decodes the persisted PSBT, looks up the requested packet type, and compares serialized bytes, with no base64-substring trickery. Range-checks the packet-type byte to[0, 255].packets TEXTcolumn (CSV of packet types) backed by an async, keyset-paginated backfill. The backfill runs in a goroutine started in the constructor with a cancellable context;Close()cancels it and waits for the goroutine to drain so we never tear down the DB out from under it. Rows whose PSBT cannot be decoded are marked with the empty string so they aren't revisited every restart. The helper is exposed aspgdb.BackfillPackets/sqlitedb.BackfillPacketsso tests can drive it synchronously.offchain_tx_with_packets_idx ON offchain_tx (txid) WHERE packets IS NOT NULL AND packets <> ''on both backends; SQLWHEREclauses use the matching predicate so the planner uses the index.packetscolumn before recreating the view soCREATE VIEW SELECT offchain_tx.*doesn't register a dependency on the column it's about to drop.SelectOffchainTxs*queries gainORDER BY starting_timestamp DESC, txid ASCso paginated responses are stable, andstage_code <> 0to skip undefined-stage rows (matching badger).txid IN (sqlc.slice('txids'))must remain the last WHERE clause so the sqlc-generated?1..?5numbered placeholders for the other args don't collide with the slice expansion whenlen(WithTxids) > 1. There's an in-SQL comment explaining this.(starting_timestamp DESC, txid ASC)to match the SQL backends, so cross-backend pagination viaafter=lastSeenis deterministic. The badger path still loads the whole offchain_tx table before predicate evaluation (badgerhold limitation); aNOTEin the repo points production users to the SQL backends.Application + handler
exposureWithheldandexposurePrivatepaths populatefilter.WithTxidsfrom the token's whitelist when empty, so a future caller bypassingparseTxidscan't silently exceed the token's scope.SubmitOffchainTxnow propagates malformed-extension errors asINVALID_ARK_PSBTinstead of silently persisting the tx as "no extension".FinalizeOffchainTxno longer conflates DB errors withTX_NOT_FOUND: a real DB error logs and returnsINTERNAL_ERROR; only an empty result returnsTX_NOT_FOUND.parseVirtualTxsFilterusesCompileplus acel.BoolTypecheck so non-bool expressions fail at the handler.asIntLiteraluses a type switch and range-checks to the packet-type byte range. Filter expressions over 4 KiB are rejected before parsing.decodePacketsColumnlogs a warning on corrupt CSV entries (e.g. a stored value of"0,abc,255") instead of silently dropping them, so storage corruption surfaces in logs.Test plan
make lint(locally: 0 issues).make test/ SQLite-side integration suite, including the newTestBackfillPackets(in-memory) which covers carrier, no-extension, and malformed-PSBT rows plus idempotency.TestService/repo_manager_with_postgres_stores,TestIntentTxidMigration,TestBackfillPackets), which runs in CI (the localpgcontainer in my dev box uses a different role).TestUnilateralExit,TestUnrolledVtxoRejoinBatch,TestOffchainTx,TestReactToFraud,TestSweep,TestAsset) regression-tests the pure-txid fallback toRounds().GetTxsWithTxids, since the client-lib redeem path walks mixed chains through this RPC.GetSubscriptionwith a filter, drop the connection, replay withGetVirtualTxs(filter, after=lastSeen), confirm the unary RPC returns the same rows the stream would have delivered.Summary by CodeRabbit
New Features
after,before,within) to narrow virtual transaction results by timestamp bounds.Bug Fixes