feat: payref tracking#7734
Conversation
|
If you're new to commit signing, there are different ways to set it up: Sign commits with
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements historical PayRef tracking to handle blockchain reorgs and adds a rejected_reason field to transaction metadata. Key changes include a new payref_history database table, updated gRPC endpoints for transaction lookups, and integration tests for reorg scenarios. Review feedback focuses on enhancing error propagation in the gRPC server and refactoring the completed_tx_to_transaction_info helper function to encapsulate commitment extraction and reduce code duplication.
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs
Outdated
Show resolved
Hide resolved
SWvheerden
force-pushed
the
sw_payref_tracking
branch
2 times, most recently
from
0ee538e to
be87e39
Compare
|
/gemini-review |
brianp
left a comment
brianp
left a comment
There was a problem hiding this comment.
Looks good. Clean refactoring with completed_tx_to_transaction_info, migration and indexes are solid, tests cover the key paths.
A few things worth considering:
-
retry_dbusesthread::sleepwhich blocks the tokio runtime since these DB methods are called from async contexts withoutspawn_blocking. With 10 retries and up to 5s backoff, worst case stalls all async tasks on that thread for a while. Might want to cap it tighter or move callers to a blocking thread. -
Payref archival in
process_reorgisn't wrapped in a transaction. If it fails mid-loop, some payrefs get archived and others don't. A crash + retry could also duplicate history entries. Worth wrapping inconn.transaction(). -
ascasts betweeni64/u64fortx_idsilently wrap on negative values.TryFromwould be safer for the new code paths.
None of these are blockers but 1 and 2 could bite in production under load or during actual reorgs.
SWvheerden
force-pushed
the
sw_payref_tracking
branch
from
d138d73 to
baa599d
Compare
…ransactionInfo Track superseded PayRefs in a new payref_history table so that PayRefs from before a blockchain reorg can still be looked up. When a transaction is unmined during a reorg, its current PayRefs are archived before being deleted. The GetTransactionPayRefs and GetPaymentByReference gRPC endpoints now return historical PayRefs separately from current ones. Also adds a rejected_reason string field to the TransactionInfo gRPC message, populated from the existing TxCancellationReason enum so callers can see why a transaction was rejected. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> test: add cucumber integration tests for payref history and rejected_reason Adds a new PayRefHistory.feature with two scenarios: 1. PayRef history is preserved after a reorg - verifies that when a blockchain reorg unmines and re-mines transactions, the old PayRefs are archived and returned via GetTransactionPayRefs historical field. 2. Rejected transaction shows cancellation reason - verifies that mined transactions have an empty rejected_reason field in TransactionInfo. Adds three new step definitions in wallet_steps.rs: - "wallet X has PayRefs for all mined transactions" - "wallet X has historical PayRefs from before the reorg" - "all mined transactions for wallet X have empty rejected_reason" Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> fix(test): trigger wallet validation after reorg in PayRef history test The wallet needs to explicitly validate its transactions after a reorg to detect that previously mined transactions are no longer in the winning chain. Without validation, set_as_unmined is never called and PayRefs are not archived to history. Replace the simple 5-second wait with: - all wallets validate their transactions - wait for wallet to have scanned to the new chain height (7) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> fix(test): rely on automatic reorg detection for PayRef history test Remove explicit wallet validation — the wallet automatically detects reorgs via its UTXO scanner when connected to a base node that reorgs. Increase retry window to TWO_MINUTES_WITH_HALF_SECOND_SLEEP to give the UTXO scanner sufficient time to detect the missing scanned blocks and trigger process_reorg. Add early-exit on first success and periodic progress logging. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> fix: move payref archival from set_as_unmined to process_reorg only set_as_unmined is called during normal transaction validation (not just reorgs) via the transaction_validation_protocol. The extra DB operations (find_all_by_tx_id + insert into payref_history) were holding the SQLite connection longer, causing "database is locked" errors under high load (e.g. 500 concurrent transactions in the performance test). Move the archival logic to process_reorg, which is only called during actual blockchain reorgs detected by the UTXO scanner. This keeps the normal validation path fast while still preserving PayRef history when a reorg actually occurs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> remove tags fix: add retry logic for SQLite "database is locked" errors SQLite allows only one writer at a time. Under high concurrency (e.g. 500 transactions + 1000 mined blocks), the output manager's UTXO selection and encumbering competes with the transaction service's update_mined_height for the write lock. When a deferred transaction tries to upgrade from read to write, SQLite can fail immediately without honoring busy_timeout (the "deferred write pitfall"). Add a retry_db utility in common_sqlite that retries operations up to 10 times with exponential backoff (100ms to 5s) when a "database is locked" error is detected. Apply it to the three hot paths: - OutputManager::fetch_unspent_outputs_for_spending (UTXO selection) - OutputManager::short_term_encumber_outputs (output reservation) - TransactionService::update_mined_height (mining confirmation) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> refactor: use typed FixedHash and PaymentReference in get_payref_history_by_tx_id Replace Vec<(Vec<u8>, Vec<u8>)> return type with Vec<(FixedHash, PaymentReference)> for type safety across the trait, database, service, handle, and gRPC layers. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> clippy refactor: make retry_db async to avoid blocking the tokio runtime Change retry_db to use tokio::time::sleep instead of std::thread::sleep so it yields to the tokio runtime during backoff instead of blocking the executor thread. Callers in sync trait methods use Handle::current().block_on() to bridge the sync/async boundary. Add tokio "time" feature to tari_common_sqlite dependencies. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> refactor: move payref archival into set_as_unmined for atomicity Move the PayRef history archival logic from process_reorg into CompletedTransactionSql::set_as_unmined so that archive + delete happens atomically on the same connection. This covers all callers: both process_reorg (reorg path) and transaction_validation_protocol (normal validation path). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> refactor: wrap set_as_unmined in a diesel transaction Wrap the entire set_as_unmined body in conn.transaction() so that the status update, payref archival, and payref deletion all happen atomically. If any step fails, the entire operation rolls back. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> fmt fix: revert retry_db to sync std::thread::sleep Handle::current().block_on() panics in unit tests that don't run inside a tokio runtime. Revert retry_db back to sync with std::thread::sleep. The sleep durations are short (100ms-5s) and the callers are already blocking sync trait methods doing synchronous diesel calls, so blocking the thread briefly during retry backoff is acceptable. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> fmt
SWvheerden
force-pushed
the
sw_payref_tracking
branch
from
d8ba497 to
736f91e
Compare
2 participants
Description
Add historical payref tracking