tx_indexer#89
Conversation
WalkthroughThis update introduces a transaction indexer service across multiple server and worker components, adds configuration and migration files for transaction indexing, and integrates transaction tracking into plugin message signing flows. New configuration options and a dedicated transaction indexer worker are included, along with a database migration for the transaction indexing schema. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API_Server
participant TxIndexerService
participant Database
Client->>API_Server: SignPluginMessages(request)
API_Server->>TxIndexerService: CreateTx (for each message)
TxIndexerService->>Database: Insert tx_indexer record
TxIndexerService-->>API_Server: Return TxIndexerID
API_Server-->>Client: Response with TxIndexerID(s)
sequenceDiagram
participant Main
participant ConfigLoader
participant TxIndexerStore
participant TxIndexerService
participant Server
Main->>ConfigLoader: Load configuration
ConfigLoader-->>Main: Config struct
Main->>TxIndexerStore: NewPostgresTxIndexStore(DSN)
TxIndexerStore-->>Main: Store instance
Main->>TxIndexerService: NewService(logger, store, chains)
TxIndexerService-->>Main: Service instance
Main->>Server: NewServer(..., txIndexerService)
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)level=warning msg="[runner] Can't run linter goanalysis_metalinter: buildir: failed to load package session: could not load export data: no export data for "github.com/vultisig/go-wrappers/go-dkls/sessions"" ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
config-tx-indexer.yaml (1)
1-3: Secrets in plain text — same concern as server configReplicate the fix suggested for
config-server.yaml; eliminate hard-coded credentials before this file lands in the main branch.🧰 Tools
🪛 Checkov (3.2.334)
[MEDIUM] 2-3: Basic Auth Credentials
(CKV_SECRET_4)
cmd/dca/server/main.go (1)
58-68: Repeat of store-closing issueApply the same
defer txIndexerStore.Close()pattern here.
🧹 Nitpick comments (10)
config-tx-indexer.yaml (1)
4-7: Place-holders: leave explicit TODO or removeEmpty
url:keys will unmarshal as empty strings and may cause runtime failures. Add clear comments/TODOs or supply sensible defaults.cmd/payroll/worker/main.go (2)
4-11: Alias collision: use distinct import name for tx-indexer storage
import … "github.com/vultisig/verifier/tx_indexer/pkg/storage"asstoragecan easily be confused with otherstoragepackages used elsewhere (plugin/storage, etc.). Alias it for clarity:-import ( - … - "github.com/vultisig/verifier/tx_indexer/pkg/storage" +import ( + … + txstorage "github.com/vultisig/verifier/tx_indexer/pkg/storage"
21-22: Context created but never cancelled
ctx := context.Background()is fine for short-lived functions, but this worker runs indefinitely. Preferctx, cancel := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM)and defercancel()to allow graceful shutdowns.cmd/dca/worker/main.go (1)
3-12: Prefer an explicit alias for the tx-indexer store importUsing a bare
storagealias next to the already importedvaultStoragevariable (and otherstoragepackages in the repo) makes it extremely hard to tell which package is referred to at call-sites.
Rename the import to something unambiguous, e.g.txindexerstorage, for immediate readability.- "github.com/vultisig/verifier/tx_indexer/pkg/storage" + txindexerstorage "github.com/vultisig/verifier/tx_indexer/pkg/storage"go.mod (1)
43-47: Vintage btcsuite packages pulled in – verify necessity & CVEs
github.com/btcsuite/btclog,go-socks, and a 2015 snapshot ofwebsocketare ~9 years old. Unless the newverifierversion truly depends on them, consider pruning or vendoring newer forks to avoid latent security issues and heavy dependency trees.Run
go mod why/go mod graph | grep btcsuiteand decide if they must stay.cmd/payroll/server/main.go (1)
3-18: Import alias inconsistency across commandsHere you correctly alias the store package as
tx_indexer_storage, whilecmd/dca/worker/main.gokeeps the defaultstorage. Harmonise the alias in all entrypoints to avoid mental context switching.cmd/tx_indexer/main.go (1)
42-45: Bubble up run error instead of panicking
worker.Run()is presumably long-running; panic on normal shutdown feels harsh and complicates supervisory tooling. Return the error code or log and exit cleanly.-if err != nil { - panic(fmt.Errorf("failed to start worker: %w", err)) -} +if err != nil && !errors.Is(err, context.Canceled) { + logger.Fatalf("worker terminated: %v", err) +}cmd/tx_indexer/config.go (2)
22-22: Consider adding more config search paths.The current implementation only searches for the config file in the current directory. Consider adding additional paths like the executable's directory or a dedicated config directory for better flexibility in deployment scenarios.
viper.SetConfigName(configName) viper.AddConfigPath(".") +viper.AddConfigPath("/etc/tx_indexer/") +viper.AddConfigPath("$HOME/.tx_indexer")
23-23: Consider setting environment variable prefix for better organization.While
AutomaticEnv()is called, consider usingSetEnvPrefix()to namespace your environment variables and avoid conflicts with other applications.viper.AddConfigPath(".") +viper.SetEnvPrefix("TX_INDEXER") viper.AutomaticEnv()api/plugin.go (1)
126-130: Important: Address transaction replay protection.The TODO comment correctly identifies the need for transaction replay protection. This is a critical security feature that should be implemented before production deployment. Consider implementing a transaction nonce or timestamp-based validation mechanism.
Would you like me to create a GitHub issue to track the implementation of transaction replay protection based on the strategy-specific periods mentioned in the comment?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
api/plugin.go(2 hunks)api/server.go(4 hunks)cmd/dca/server/main.go(4 hunks)cmd/dca/worker/config.go(1 hunks)cmd/dca/worker/main.go(2 hunks)cmd/payroll/server/main.go(4 hunks)cmd/payroll/worker/main.go(3 hunks)cmd/tx_indexer/config.go(1 hunks)cmd/tx_indexer/main.go(1 hunks)config-server.yaml(2 hunks)config-tx-indexer.yaml(1 hunks)go.mod(4 hunks)storage/postgres/migrations/plugin/20250603181247_tx_indexer.sql(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
cmd/tx_indexer/main.go (1)
cmd/tx_indexer/config.go (1)
GetConfigure(11-18)
🪛 Checkov (3.2.334)
config-server.yaml
[MEDIUM] 7-8: Basic Auth Credentials
(CKV_SECRET_4)
config-tx-indexer.yaml
[MEDIUM] 2-3: Basic Auth Credentials
(CKV_SECRET_4)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: migration-test
🔇 Additional comments (8)
config-server.yaml (1)
18-18: Nit: stray whitespace cleanup is fineJust noting that the trailing blanks on
bucketwere removed – no issues.cmd/payroll/worker/main.go (2)
63-67: Service instantiated but never startedIf
tx_indexer.NewServicerequires an explicitStart()(common pattern), remember to call it in a goroutine and stop it on shutdown.
58-67:Details
❓ Verification inconclusive
Postgres store not closed; potential connection leak
NewPostgresTxIndexStorelikely opens a connection pool. Defer itsClose()(orStop()) method if available.txIndexerStore, err := txstorage.NewPostgresTxIndexStore(ctx, cfg.Database.DSN) if err != nil { panic(fmt.Errorf("storage.NewPostgresTxIndexStore: %w", err)) } +defer txIndexerStore.Close() // ensure resources are released
#!/usr/bin/env bash set -e # Locate files defining the constructor files=$(rg -l "NewPostgresTxIndexStore" .) if [ -z "$files" ]; then echo "No files found containing NewPostgresTxIndexStore" exit 0 fi # For each matching file, inspect signature, type, and resource cleanup methods for file in $files; do echo "=== File: $file ===" rg -n "func NewPostgresTxIndexStore" "$file" rg -n "type .*Store" "$file" rg -n "Close(" "$file" rg -n "Stop(" "$file" done
🏁 Script executed:
#!/usr/bin/env bash set -e echo "=== Finding NewPostgresTxIndexStore constructor ===" rg -n "func NewPostgresTxIndexStore" -A2 -B2 . echo "=== Checking for Close methods on any type ===" rg -n "func \(.*\*.*\) Close" -A1 -B0 . echo "=== Checking for Stop methods on any type ===" rg -n "func \(.*\*.*\) Stop" -A1 -B0 .Length of output: 170
Verify resource cleanup for PostgresTxIndexStore
The
txIndexerStorereturned byNewPostgresTxIndexStorelikely opens a database connection pool that must be explicitly closed. Please confirm whether the store type provides aClose()(orStop()) method and, if so, add a deferred call immediately after initialization:Locations to review:
- cmd/payroll/worker/main.go: lines 58–67
Suggested diff:
txIndexerStore, err := txstorage.NewPostgresTxIndexStore(ctx, cfg.Database.DSN) if err != nil { panic(fmt.Errorf("storage.NewPostgresTxIndexStore: %w", err)) } +defer txIndexerStore.Close() // ensure DB connections are releasedIf the cleanup is handled elsewhere or there is no explicit teardown method, please document how resources are released.
api/server.go (2)
34-46: Field added, but access not shown here
txIndexerServiceis wired in, good. Double-check all handlers use the injected service, and nil checks guard against missing initialisation in older call-sites.
60-61: Constructor signature changed – ensure all callers updatedCompile will fail if any
NewServer(...)invocations weren’t amended with the new parameter; rungo vet ./...to verify.cmd/dca/worker/main.go (1)
54-58: Potential resource leak –TxIndexStoreis never closed
storage.NewPostgresTxIndexStorereturns an object that implementsio.Closer.
If the worker exits (e.g.srv.Runreturns an error) the underlying pgx pool will remain open.txIndexerStore, err := storage.NewPostgresTxIndexStore(ctx, cfg.Database.DSN) if err != nil { panic(fmt.Errorf("storage.NewPostgresTxIndexStore: %w", err)) } +defer txIndexerStore.Close()api/plugin.go (1)
102-120: Transaction tracking implementation looks good!The conditional check for
txIndexerServiceand proper error handling ensure graceful degradation when the service is unavailable. The transaction creation logic correctly captures all necessary metadata.storage/postgres/migrations/plugin/20250603181247_tx_indexer.sql (1)
1-33: Well-designed schema for transaction tracking!The migration properly defines status ENUMs, includes comprehensive fields for tracking transaction lifecycle, and has thoughtful comments explaining the
lostflag mechanism. The index on(status_onchain, lost)will efficiently support queries for active transactions.
2 participants
Scope of this PR, closes: #87:
tx_indexeron-chain worker instance forplugindb;tx_indexeras a package imported fromverifier, and proposed/verified/signed off-chain change states called withtx_indexer.Servicewrapper intoplugindb;tx_indexertable migration added forplugindb;To be implemented in next PR:
Summary by CodeRabbit
tx_indexertable to store transaction metadata and status, supporting improved transaction management and visibility.