feat!: impl epoch birthday for utxo scanning - #1633
Conversation
WalkthroughAdds epoch tracking to UTXO storage and streaming, threads epoch/birthday through wallet/account APIs, renames substate proof types, centralizes global DB path handling, and refines error handling and request guards across indexer and wallet services. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Indexer as Indexer API
participant Store as UTXO Store
participant DB as SQLite DB
Client->>Indexer: stream_utxo_updates(from_epoch, ...)
Indexer->>Store: utxos_get_updates(from_epoch, ...)
Store->>DB: SELECT ... WHERE epoch >= from_epoch ...
DB-->>Store: Utxo rows (updates) + max_state_version + max_epoch
Store-->>Indexer: UtxoStateUpdateSet { updates, max_state_version, max_epoch }
Indexer-->>Client: StartOfShard, [updates...], EndOfShard (includes max_epoch)
sequenceDiagram
participant Init as App Init
participant WalletSDK as Wallet SDK
participant KeyMgr as Key Manager
participant Accounts as Accounts API
Init->>WalletSDK: initialize(store, indexer, config, EpochBirthday)
WalletSDK->>KeyMgr: key_manager_api(..., epoch_birthday)
WalletSDK->>Accounts: accounts_api(..., epoch_birthday)
Accounts->>Accounts: calculate_current_epoch() -> birthday_epoch
Accounts->>Storage: add_account(..., birthday_epoch, ...)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas to focus on:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
applications/tari_indexer/src/rest_api/streaming/utxo_stream.rs (1)
101-132: Don't dropmax_epochfrom the streaming payload
get_utxo_updatesnow returns{ updates, max_state_version, max_epoch }so that wallets can persist both the state-version and epoch high-water marks. In this path we only logmax_epochand then discard it, so REST-streaming clients never learn the new epoch and cannot advance their stored birthday/epoch filter—they’re forced back to wall-clock heuristics and will continue to re-scan the same range. Please propagatemax_epochthroughPendingUpdatesand encode it into the stream payload (e.g.,StartOfShard/EndOfShard), mirroring whatever you did on the gRPC side, so downstream consumers can persist the server-provided epoch.crates/state_store_rocksdb/src/reader.rs (2)
1044-1049: Bug: wrong block used for membership check (can return stale change)You’re checking
applicable_blocks.contains(block_id)whereblock_idis the function parameter (always present in the set). This ignores the change’s actual block and can pick a change from outside the pending chain.Apply this fix:
- if max_change.as_ref().is_none_or(|c| c.version < key.version) && applicable_blocks.contains(block_id) { + if max_change.as_ref().is_none_or(|c| c.version < key.version) + && applicable_blocks.contains(&key.block_id) + {
1080-1084: Bug: same membership check issue in versioned-substate querySame problem here; membership must be evaluated against the key’s block, not the function parameter.
- if versioned.version() == key.version && applicable_blocks.contains(block_id) { + if versioned.version() == key.version && applicable_blocks.contains(&key.block_id) {crates/wallet/storage_sqlite/src/writer.rs (1)
656-667: Store birthday_epoch with a safe conversion
Epochisu64; SQLite INTEGER is signed 64-bit. Casting withas i64silently wraps on overflow. Usei64::try_fromand return a clear error, or store as TEXT/unsigned.- diesel::insert_into(accounts::table) + let birthday_epoch_i64 = i64::try_from(birthday_epoch.as_u64()) + .map_err(|_| WalletStorageError::bad_query("accounts_insert", "birthday_epoch exceeds i64::MAX"))?; + + diesel::insert_into(accounts::table) .values(( ... - accounts::birthday_epoch.eq(birthday_epoch.as_u64() as i64), + accounts::birthday_epoch.eq(birthday_epoch_i64), ... ))If the schema permits, alternatively store
birthday_epochas a string to avoid signed-range constraints. Based on learnings.Also applies to: 677-691
🧹 Nitpick comments (8)
applications/tari_indexer/src/lib.rs (1)
82-82: LGTM!The refactoring to use
config.global_db_path()centralizes database path configuration, improving maintainability by eliminating hardcoded path construction logic.applications/tari_validator_node/src/config.rs (1)
135-137: LGTM!The new
get_global_db_path()method centralizes the global database path construction, improving consistency with similar changes in the indexer configuration.applications/tari_validator_node/src/lib.rs (1)
95-95: LGTM! Good refactor for maintainability.Encapsulating the global database path in a dedicated method improves consistency and makes it easier to modify path construction logic in the future.
crates/state_store_rocksdb/src/reader.rs (1)
218-221: Minor: duplicate logic between get_pending_chain_until and get_pending_chain_orderedBoth functions traverse the same parent chain. Consider extracting a shared iterator over parents and then collect into HashSet/Vec to DRY this up.
Also applies to: 240-254
crates/wallet/sdk_services/src/indexer_rest_api.rs (1)
181-195: Avoid magic 1000; derive has_more from the request limitUse a single constant for per-shard page size and compare with equality (the server should not exceed the requested limit).
+const PER_SHARD_LIMIT: u32 = 1000; ... - .stream_utxo_updates_protobuf(GetUtxoUpdatesRequest { - from_epoch, - shard_state_versions, - resource_address, - unspent_only, - per_shard_limit: 1000, - }) + .stream_utxo_updates_protobuf(GetUtxoUpdatesRequest { + from_epoch, + shard_state_versions, + resource_address, + unspent_only, + per_shard_limit: PER_SHARD_LIMIT, + }) ... - let sos = res.sos.map(|sos| StartOfShard { + let sos = res.sos.map(|sos| StartOfShard { shard: Shard::from(sos.shard), max_state_version: StateVersion::from(sos.max_state_version), - has_more: sos.num_updates >= 1000, + has_more: sos.num_updates == PER_SHARD_LIMIT, });Also applies to: 200-205
utilities/tariswap_test_bench/src/accounts.rs (1)
65-73: Birthday epoch defaulting to Epoch::zero() in testsFine for synthetic runs, but note this forces scanning from genesis. If tests aim to validate birthday filtering, consider passing a non-zero epoch derived from the harness’ configured epoch timing.
Also applies to: 146-154
crates/wallet/sdk/src/apis/accounts.rs (1)
47-53: Guard against epoch drift with an overlap/clamp strategyComputing birthday from local time risks missing UTXOs if epochs progress slower than assumed. Mitigate by:
- subtracting a small overlap (e.g., 1–2 epochs) from the computed birthday, and
- clamping the request’s from_epoch to the indexer’s current epoch when streaming.
Example minimal overlap in create_account:
- let birthday_epoch = self.epoch_birthday.calculate_current_epoch(); + const OVERLAP_EPOCHS: u64 = 2; + let computed = self.epoch_birthday.calculate_current_epoch(); + let birthday_epoch = Epoch(computed.as_u64().saturating_sub(OVERLAP_EPOCHS));Pair this with a clamp at call-sites building GetUtxoUpdatesRequest (or store a per-account overlap policy).
Please confirm the streaming path already clamps
from_epochto the indexer’s current epoch; otherwise, wallet may miss earlier updates after long pauses. Based on PR objectives.Also applies to: 59-74, 89-99, 116-125, 147-157
crates/wallet/sdk/src/models/epoch_birthday.rs (1)
29-36: Document the magic number1200.The hardcoded value
1200on Line 34 appears to represent epoch duration in seconds (20 minutes), but this isn't documented. Consider extracting this to a named constant or adding a comment explaining why this specific value is used infar_future().pub const fn far_future() -> Self { Self { epoch_time_secs: NonZeroU64::new(u64::MAX).unwrap(), + // 1200 seconds (20 minutes) is the standard epoch duration rel_zero_epoch_secs: 1200, } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (57)
applications/tari_indexer/src/config.rs(1 hunks)applications/tari_indexer/src/lib.rs(1 hunks)applications/tari_indexer/src/network_state_sync/worker.rs(1 hunks)applications/tari_indexer/src/rest_api/handlers/substates.rs(1 hunks)applications/tari_indexer/src/rest_api/handlers/transactions.rs(1 hunks)applications/tari_indexer/src/rest_api/streaming/utxo_stream.rs(3 hunks)applications/tari_indexer/src/storage_sqlite/migrations/2023-02-16-145719_initial/up.sql(1 hunks)applications/tari_indexer/src/storage_sqlite/models/utxo.rs(3 hunks)applications/tari_indexer/src/storage_sqlite/reader.rs(3 hunks)applications/tari_indexer/src/storage_sqlite/schema.rs(1 hunks)applications/tari_indexer/src/storage_sqlite/writer.rs(3 hunks)applications/tari_indexer/src/store.rs(3 hunks)applications/tari_indexer/src/substate_manager.rs(2 hunks)applications/tari_validator_node/src/config.rs(1 hunks)applications/tari_validator_node/src/lib.rs(1 hunks)applications/tari_validator_node/src/p2p/rpc/block_sync_task.rs(2 hunks)applications/tari_walletd/src/lib.rs(3 hunks)applications/tari_walletd/src/main.rs(1 hunks)bindings/src/types/Account.ts(2 hunks)bindings/src/types/UtxoStateUpdateSet.ts(1 hunks)bindings/src/types/tari-indexer-client/GetUtxoUpdatesRequest.ts(1 hunks)clients/tari_indexer_client/src/types.rs(1 hunks)clients/tari_indexer_client/tests/streaming.rs(1 hunks)crates/epoch_manager/src/service/epoch_manager_service.rs(1 hunks)crates/indexer_lib/src/error.rs(1 hunks)crates/indexer_lib/src/substate_scanner.rs(1 hunks)crates/p2p/proto/rpc.proto(2 hunks)crates/p2p/src/block_sync.rs(2 hunks)crates/p2p/src/conversions/rpc.rs(3 hunks)crates/rpc_state_sync/src/state_sync.rs(2 hunks)crates/state_store_rocksdb/src/reader.rs(3 hunks)crates/storage/src/consensus_models/block.rs(5 hunks)crates/storage/src/consensus_models/substate.rs(4 hunks)crates/storage_sqlite/src/sqlite_db_factory.rs(2 hunks)crates/wallet/sdk/src/apis/accounts.rs(8 hunks)crates/wallet/sdk/src/apis/key_manager.rs(6 hunks)crates/wallet/sdk/src/key_managers/backend.rs(1 hunks)crates/wallet/sdk/src/key_managers/local.rs(1 hunks)crates/wallet/sdk/src/local_key_store.rs(1 hunks)crates/wallet/sdk/src/models/account.rs(4 hunks)crates/wallet/sdk/src/models/epoch_birthday.rs(1 hunks)crates/wallet/sdk/src/models/mod.rs(2 hunks)crates/wallet/sdk/src/models/utxo_update.rs(2 hunks)crates/wallet/sdk/src/network.rs(2 hunks)crates/wallet/sdk/src/sdk.rs(8 hunks)crates/wallet/sdk/src/storage/writer.rs(2 hunks)crates/wallet/sdk/tests/support/harness.rs(4 hunks)crates/wallet/sdk_services/src/account_recovery/service.rs(6 hunks)crates/wallet/sdk_services/src/indexer_rest_api.rs(2 hunks)crates/wallet/sdk_services/src/utxo_scanner/scanner_round.rs(1 hunks)crates/wallet/storage_sqlite/migrations/2023-02-08-122514_initial/up.sql(1 hunks)crates/wallet/storage_sqlite/src/models/account.rs(3 hunks)crates/wallet/storage_sqlite/src/schema.rs(1 hunks)crates/wallet/storage_sqlite/src/writer.rs(3 hunks)crates/wallet/storage_sqlite/tests/accounts.rs(2 hunks)utilities/tariswap_test_bench/src/accounts.rs(3 hunks)utilities/tariswap_test_bench/src/runner.rs(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-19T10:49:02.115Z
Learnt from: sdbondi
Repo: tari-project/tari-ootle PR: 1543
File: crates/common_types/src/shard_state_versions.rs:59-74
Timestamp: 2025-08-19T10:49:02.115Z
Learning: In crates/common_types/src/shard_state_versions.rs, MAX_SHARDS represents the maximum shard number + 1 (257) used for bounds checking, not the maximum capacity. The current code correctly validates that shard group end numbers don't exceed the maximum possible shard number (256).
Applied to files:
applications/tari_indexer/src/rest_api/streaming/utxo_stream.rs
🧬 Code graph analysis (36)
applications/tari_indexer/src/rest_api/handlers/transactions.rs (1)
applications/tari_indexer/src/rest_api/error.rs (1)
service_unavailable(91-96)
bindings/src/types/Account.ts (1)
bindings/src/types/Epoch.ts (1)
Epoch(3-3)
clients/tari_indexer_client/src/types.rs (1)
bindings/src/types/Epoch.ts (1)
Epoch(3-3)
crates/wallet/storage_sqlite/src/schema.rs (1)
crates/wallet/sdk/src/models/account.rs (2)
birthday_epoch(43-45)birthday_epoch(81-84)
crates/wallet/sdk/src/network.rs (2)
bindings/src/types/Shard.ts (1)
Shard(3-3)bindings/src/types/Epoch.ts (1)
Epoch(3-3)
crates/wallet/sdk_services/src/account_recovery/service.rs (2)
bindings/src/types/Epoch.ts (1)
Epoch(3-3)crates/wallet/sdk/src/models/account.rs (2)
birthday_epoch(43-45)birthday_epoch(81-84)
applications/tari_indexer/src/rest_api/handlers/substates.rs (1)
applications/tari_indexer/src/rest_api/error.rs (2)
anyhow(44-48)service_unavailable(91-96)
crates/wallet/sdk/src/models/account.rs (1)
bindings/src/types/Epoch.ts (1)
Epoch(3-3)
crates/wallet/sdk/src/local_key_store.rs (1)
crates/wallet/sdk/src/key_managers/backend.rs (1)
cipher_seed_birthday(29-29)
crates/wallet/sdk/src/models/utxo_update.rs (2)
bindings/src/types/Shard.ts (1)
Shard(3-3)bindings/src/types/Epoch.ts (1)
Epoch(3-3)
utilities/tariswap_test_bench/src/runner.rs (2)
crates/wallet/sdk/src/sdk.rs (2)
initialize(76-103)store(145-147)crates/wallet/sdk/src/models/epoch_birthday.rs (1)
far_future(31-36)
applications/tari_indexer/src/rest_api/streaming/utxo_stream.rs (2)
bindings/src/types/UtxoStateUpdateSet.ts (1)
UtxoStateUpdateSet(6-10)bindings/src/types/WalletUtxoUpdate.ts (1)
WalletUtxoUpdate(6-6)
applications/tari_indexer/src/substate_manager.rs (2)
bindings/src/types/Epoch.ts (1)
Epoch(3-3)bindings/src/types/UtxoStateUpdateSet.ts (1)
UtxoStateUpdateSet(6-10)
applications/tari_indexer/src/storage_sqlite/writer.rs (2)
crates/storage/src/consensus_models/block.rs (1)
epoch(347-349)bindings/src/types/Epoch.ts (1)
Epoch(3-3)
applications/tari_indexer/src/storage_sqlite/schema.rs (1)
crates/storage/src/consensus_models/block.rs (1)
epoch(347-349)
applications/tari_indexer/src/storage_sqlite/reader.rs (2)
bindings/src/types/UtxoStateUpdateSet.ts (1)
UtxoStateUpdateSet(6-10)bindings/src/types/Epoch.ts (1)
Epoch(3-3)
crates/storage/src/consensus_models/substate.rs (1)
crates/p2p/src/conversions/rpc.rs (8)
from(41-45)from(60-65)from(81-88)from(107-113)from(119-124)from(164-173)from(189-194)value(153-157)
applications/tari_indexer/src/store.rs (3)
bindings/src/types/UtxoStateUpdateSet.ts (1)
UtxoStateUpdateSet(6-10)bindings/src/types/Epoch.ts (1)
Epoch(3-3)bindings/src/types/Shard.ts (1)
Shard(3-3)
crates/wallet/sdk/src/key_managers/backend.rs (1)
crates/wallet/sdk/src/local_key_store.rs (3)
derive_secret(47-54)get_imported_secret(56-70)cipher_seed_birthday(72-75)
crates/wallet/sdk/src/models/epoch_birthday.rs (2)
crates/wallet/sdk/src/apis/accounts.rs (1)
new(60-74)crates/wallet/sdk/src/apis/key_manager.rs (1)
new(55-69)
crates/wallet/sdk/src/storage/writer.rs (3)
bindings/src/types/Shard.ts (1)
Shard(3-3)bindings/src/types/Epoch.ts (1)
Epoch(3-3)crates/wallet/sdk/src/models/account.rs (2)
birthday_epoch(43-45)birthday_epoch(81-84)
bindings/src/types/tari-indexer-client/GetUtxoUpdatesRequest.ts (1)
bindings/src/types/Epoch.ts (1)
Epoch(3-3)
applications/tari_walletd/src/main.rs (1)
crates/wallet/sdk/src/models/account.rs (4)
birthday_epoch(43-45)birthday_epoch(81-84)name(47-49)name(98-100)
crates/wallet/sdk_services/src/indexer_rest_api.rs (2)
bindings/src/types/Epoch.ts (1)
Epoch(3-3)bindings/src/types/tari-indexer-client/GetUtxoUpdatesRequest.ts (1)
GetUtxoUpdatesRequest(7-13)
bindings/src/types/UtxoStateUpdateSet.ts (2)
bindings/src/types/WalletUtxoUpdate.ts (1)
WalletUtxoUpdate(6-6)bindings/src/types/Epoch.ts (1)
Epoch(3-3)
crates/wallet/storage_sqlite/src/writer.rs (2)
bindings/src/types/Epoch.ts (1)
Epoch(3-3)crates/wallet/sdk/src/models/account.rs (2)
birthday_epoch(43-45)birthday_epoch(81-84)
crates/wallet/sdk/src/apis/accounts.rs (3)
bindings/src/types/Epoch.ts (1)
Epoch(3-3)crates/wallet/sdk/src/sdk.rs (3)
network(157-159)store(145-147)key_manager_api(174-183)crates/wallet/sdk/src/models/account.rs (2)
birthday_epoch(43-45)birthday_epoch(81-84)
crates/wallet/sdk/src/apis/key_manager.rs (1)
crates/wallet/sdk/src/cipher_seed.rs (1)
cipher_seed(29-34)
crates/storage/src/consensus_models/block.rs (1)
applications/tari_indexer/src/storage_sqlite/writer.rs (1)
updates(93-109)
applications/tari_walletd/src/lib.rs (7)
crates/wallet/sdk/src/key_managers/backend.rs (1)
cipher_seed_birthday(29-29)crates/wallet/sdk/src/local_key_store.rs (1)
cipher_seed_birthday(72-75)crates/wallet/sdk/src/apis/accounts.rs (1)
new(60-74)crates/wallet/sdk/src/apis/key_manager.rs (1)
new(55-69)crates/wallet/sdk_services/src/account_recovery/service.rs (1)
new(43-55)crates/wallet/sdk/src/sdk.rs (3)
initialize(76-103)store(145-147)network(157-159)crates/wallet/sdk/src/models/epoch_birthday.rs (1)
far_future(31-36)
crates/wallet/storage_sqlite/src/models/account.rs (1)
bindings/src/types/Epoch.ts (1)
Epoch(3-3)
crates/wallet/sdk/tests/support/harness.rs (3)
bindings/src/types/Epoch.ts (1)
Epoch(3-3)crates/wallet/sdk/src/sdk.rs (2)
initialize(76-103)store(145-147)crates/wallet/sdk/src/apis/accounts.rs (1)
new(60-74)
utilities/tariswap_test_bench/src/accounts.rs (3)
bindings/src/types/Epoch.ts (1)
Epoch(3-3)bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)crates/wallet/sdk/src/models/key.rs (1)
derived(322-324)
crates/wallet/sdk/src/sdk.rs (1)
bindings/src/types/Epoch.ts (1)
Epoch(3-3)
clients/tari_indexer_client/tests/streaming.rs (3)
bindings/src/types/Epoch.ts (1)
Epoch(3-3)bindings/src/types/NumPreshards.ts (1)
NumPreshards(3-3)bindings/src/types/tari-indexer-client/GetUtxoUpdatesRequest.ts (1)
GetUtxoUpdatesRequest(7-13)
crates/p2p/src/conversions/rpc.rs (2)
crates/storage/src/consensus_models/substate.rs (6)
value(314-319)value(363-365)from(344-346)from(350-352)from(381-390)from(450-452)crates/common_types/src/substate_address.rs (1)
from_bytes(61-71)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: check stable
- GitHub Check: test
- GitHub Check: check nightly
- GitHub Check: machete
- GitHub Check: clippy
🔇 Additional comments (62)
crates/epoch_manager/src/service/epoch_manager_service.rs (1)
116-116: LGTM!The rocket emoji addition is consistent with the existing log message style throughout the file and improves log readability.
crates/rpc_state_sync/src/state_sync.rs (2)
659-663: LGTM! Function signature correctly updated with renamed type.The parameter type has been properly updated from
SubstateCreatedProoftoSubstateCreate. The function body's field accesses remain unchanged, indicating the new type maintains interface compatibility.
31-31: Type rename is correctly applied and complete.Verification confirms that the import and function parameter have been properly updated from
SubstateCreatedProoftoSubstateCreate. No remaining references to the old type name exist in the codebase, and the field accesses within the function body (create.substate.substate_id,create.substate.value,create.substate.to_value_hash()) remain compatible with the new type.crates/indexer_lib/src/error.rs (1)
29-30: LGTM! Clear error variant for empty committee scenario.The new
NoCommitteeMemberserror variant appropriately captures scenarios where no committee members are available, with helpful diagnostic details.crates/indexer_lib/src/substate_scanner.rs (1)
124-128: Good defensive check preventing operations on empty committee.This guard prevents downstream issues including:
- A meaningless
committee.shuffle()operation on line 130- Potential arithmetic issues in the Byzantine fault tolerance calculation
(committee.len() - 1) / 3on line 132 whencommittee.len()is 0The error message provides helpful context for debugging.
applications/tari_indexer/src/rest_api/handlers/transactions.rs (2)
71-74: LGTM! Appropriate error handling for empty committee scenario.The
NoCommitteeMemberscase is correctly mapped to a 503 Service Unavailable status, consistent with theAllValidatorsFailederror handling.
79-81: LGTM! Proper validation error for missing transaction inputs.Returns a 400 Bad Request status for transactions without inputs, which correctly categorizes this as a client error rather than a server error.
applications/tari_indexer/src/rest_api/handlers/substates.rs (1)
70-79: LGTM! Prevents queries before indexer is ready.The initial scanning check correctly returns a 503 Service Unavailable status with a helpful message, improving the user experience by preventing incomplete or inconsistent query results during the initial sync phase.
applications/tari_validator_node/src/p2p/rpc/block_sync_task.rs (2)
15-15: Type rename:SubstateCreatedProof→SubstateCreate.This change is part of a broader type renaming effort across the codebase. The rename improves clarity by using a more concise name.
32-32: Field type updated to match renamed type.Consistent with the import change on line 15.
applications/tari_indexer/src/config.rs (1)
84-86: LGTM! Centralized global database path configuration.The new method follows the same pattern as
state_db_path()and provides a consistent way to access the global database path. This aligns with the refactoring inSqliteDbFactoryto accept complete database paths.clients/tari_indexer_client/src/types.rs (1)
376-377: Epoch default value is appropriate for backward compatibility.Verification confirms that
Epochis defined incrates/common_types/src/epoch.rsas a newtype struct wrappingu64with aDefaultderive. This meansEpoch::default()returnsEpoch(0), which correctly represents the first epoch and enables backward-compatible deserialization without filtering historical UTXOs. The implementation is sound.crates/storage_sqlite/src/sqlite_db_factory.rs (1)
47-52: All call sites properly updated; no issues found.Verification confirms both
SqliteDbFactory::new()call sites have been correctly updated to pass complete database file paths:
applications/tari_validator_node/src/lib.rs:95: passesconfig.validator_node.get_global_db_path()→ returnsdata_dir.join("global_storage.sqlite")applications/tari_indexer/src/lib.rs:82: passesconfig.global_db_path()→ returnsto_data_dir().join("global_storage.sqlite")No other instantiation patterns exist. The breaking API change has been properly managed across the codebase.
clients/tari_indexer_client/tests/streaming.rs (2)
6-6: LGTM!The import addition correctly brings
Epochinto scope for the newfrom_epochfield.
13-24: LGTM!The test correctly initializes the new
from_epochfield. UsingEpoch::zero()ensures the test scans from the beginning, which is appropriate for a comprehensive dev test.applications/tari_indexer/src/storage_sqlite/migrations/2023-02-16-145719_initial/up.sql (1)
146-146: LGTM! Index optimized for epoch-based queries.The updated index
utxos_resource_state_version_shard_epoch_idxnow includes theepochcolumn, which will optimize queries filtering by epoch—a key feature of this PR.crates/wallet/storage_sqlite/src/schema.rs (1)
11-11: LGTM!The Diesel schema correctly reflects the
birthday_epochcolumn added in the migration. TheBigInttype mapping is appropriate for the SQLBIGINTcolumn type.applications/tari_indexer/src/storage_sqlite/schema.rs (1)
111-111: LGTM!The Diesel schema correctly reflects the
epochcolumn added in the migration. TheBigInttype mapping is appropriate for the SQLbigintcolumn type.crates/wallet/storage_sqlite/migrations/2023-02-08-122514_initial/up.sql (1)
90-90: Review comment contains inaccurate technical reasoning.The concern about
NOT NULLwithout a default is technically incorrect for a CREATE TABLE statement. AddingNOT NULLcolumns without defaults is standard and correct when creating new tables—the constraint only causes issues when altering existing tables with data.However, this appears to be a recent change introducing epoch birthday functionality (commit a474ad9). If this modifies an already-deployed schema, the breaking change concern is valid but stems from adding a required column, not from the
NOT NULLdefinition itself.Clarification needed: Verify whether this migration requires users to delete existing data directories, and if so, ensure that requirement is clearly documented in upgrade guidance or release notes. The current codebase shows no explicit documentation confirming this requirement.
bindings/src/types/Account.ts (1)
1-16: LGTM! Auto-generated bindings correctly reflect the backend changes.The addition of the
birthday_epochfield aligns with the epoch birthday feature introduced in this PR.crates/wallet/sdk/src/models/utxo_update.rs (1)
7-7: LGTM! Clean addition of epoch tracking to UTXO updates.The
max_epochfield properly extends theUtxoStateUpdateSetto include epoch information alongside state version tracking.Also applies to: 44-44
bindings/src/types/tari-indexer-client/GetUtxoUpdatesRequest.ts (1)
1-13: LGTM! Auto-generated bindings correctly add epoch filtering capability.The
from_epochfield enables the wallet to limit UTXO scanning to epochs after account creation, as described in the PR objectives.utilities/tariswap_test_bench/src/runner.rs (1)
10-10: LGTM! Appropriate use ofEpochBirthday::far_future()in test code.Using
far_future()in the test bench ensures no historical UTXO scanning occurs during performance testing, which is appropriate for this context.Also applies to: 122-122
crates/wallet/storage_sqlite/src/models/account.rs (1)
5-5: LGTM! Field addition aligns with birthday epoch feature.The
birthday_epochfield is properly added to the SQLite model.Also applies to: 23-23
bindings/src/types/UtxoStateUpdateSet.ts (1)
1-10: LGTM! Auto-generated bindings correctly include epoch tracking.The
max_epochfield properly extends the UTXO update set type to include epoch information.crates/wallet/sdk/src/models/mod.rs (1)
8-8: LGTM! Standard module organization.The
epoch_birthdaymodule is properly integrated into the models module hierarchy.Also applies to: 25-25
crates/wallet/sdk/src/key_managers/backend.rs (1)
22-29: Well-designed trait extension.The new
cipher_seed_birthdaymethod is properly documented and returnsOption<u16>to support implementations that may not have birthday support. The error propagation throughSelf::Erroris consistent with other trait methods.crates/wallet/sdk/src/key_managers/local.rs (1)
62-63: Improved error message clarity.The updated message "Key store error" more accurately describes the wrapped
TKeyStoreErrtype compared to the previous "Cipher error" message, reducing confusion with the separateCipherErrortype.crates/wallet/sdk/src/models/account.rs (1)
8-8: LGTM! Account struct properly extended.The
birthday_epochfield and getter are correctly added to theAccountstruct with proper type imports.Also applies to: 21-21, 43-45
crates/wallet/sdk/src/local_key_store.rs (1)
72-75: Clean implementation of cipher_seed_birthday.The implementation correctly retrieves the cipher seed and extracts its birthday value, with appropriate error propagation.
crates/wallet/sdk/src/storage/writer.rs (1)
10-10: Breaking API change - signature extended appropriately.The
accounts_insertmethod now requires abirthday_epochparameter, which aligns with theAccountmodel extension. This is a necessary breaking change for the epoch birthday feature.Also applies to: 93-104
crates/p2p/src/block_sync.rs (1)
8-8: Type rename applied correctly.The import and return type are updated to use the renamed
SubstateCreatetype. The method logic remains unchanged.Also applies to: 63-68
crates/wallet/storage_sqlite/tests/accounts.rs (1)
6-6: Test updated for new API signature.The test correctly passes
Epoch::zero()to match the updatedaccounts_insertsignature.Also applies to: 22-33
applications/tari_indexer/src/storage_sqlite/models/utxo.rs (1)
20-20: UTXO models extended with epoch field.The epoch field is correctly added to all UTXO record structs:
- Optional in
UtxoRecordUpdatefor partial updates- Required in
UtxoRecordInsertandUtxoRecordfor new/stored recordsThe epoch field is appropriately not used in the conversion methods since it's metadata for scanning/filtering rather than part of the UTXO content.
Also applies to: 40-40, 57-57
applications/tari_indexer/src/network_state_sync/worker.rs (1)
415-415: LGTM! Epoch parameter correctly threaded through UTXO updates.The
msg_epochis properly extracted from the message (lines 375-380) with validation, and is now correctly passed tobatch_insert_utxo_updatesto align with the updated API signature.crates/wallet/sdk/src/network.rs (1)
64-70: LGTM! Trait signature properly updated for epoch filtering.The addition of the
from_epochparameter tostream_stealth_utxo_updatesis correctly placed and aligns with the epoch-based UTXO scanning feature.applications/tari_walletd/src/main.rs (1)
95-104: Verify epoch 0 birthday limitation doesn't cause UTXO scanning issues.Based on the relevant code snippet from
account.rs, thecalculate_birthday_epoch()implementation returns a hardcodedEpoch(0)with a TODO comment. This aligns with the PR description's note about hardcoding a zero-epoch time. However, this could lead to:
- Scanning extra history if epochs have progressed beyond 0
- Missing UTXOs if the epoch timing assumptions are incorrect
Please verify that starting from epoch 0 is acceptable for the current network state, or if this needs immediate follow-up work before release.
crates/storage/src/consensus_models/substate.rs (2)
274-289: LGTM! Type renames improve clarity.The refactoring from
SubstateCreatedProof→SubstateCreateandSubstateDestroyedProof→SubstateDestroyis consistent throughout the codebase (as evidenced by the conversions incrates/p2p/src/conversions/rpc.rs).
394-462: LGTM! Enum and trait implementations properly updated.All references to the renamed types have been correctly updated, including the enum variants, the
as_createmethod, theFromimplementation, and theDisplaytrait.applications/tari_indexer/src/storage_sqlite/writer.rs (3)
117-121: LGTM! Epoch parameter correctly added to UTXO batch updates.The new
epochparameter extends the method signature appropriately and will be threaded through the insert/update logic below.
131-149: LGTM! Epoch persisted for unspent UTXOs.The epoch value is correctly converted and stored in the
UtxoRecordInsertstruct at line 140.
151-170: LGTM! Epoch persisted for spent UTXOs.The epoch value is correctly wrapped in
Some()and stored in theUtxoRecordUpdatestruct at line 155, maintaining consistency with the update pattern.applications/tari_walletd/src/lib.rs (2)
95-102: LGTM! Birthday epoch correctly integrated into account recovery.The cipher seed birthday is properly retrieved and passed to
AccountRecoveryService, enabling epoch-aware UTXO scanning during account recovery.
199-202: LGTM! Birthday epoch correctly passed to SDK initialization.The epoch birthday is properly obtained from the helper function and passed to
WalletSdk::initialize.applications/tari_indexer/src/storage_sqlite/reader.rs (3)
519-527: LGTM! Method signature properly updated for epoch filtering.The addition of
from_epochparameter and the return type change toUtxoStateUpdateSetalign with the epoch-aware UTXO scanning feature.
531-544: LGTM! Epoch filter correctly applied to query.The epoch filter at line 534 using
.ge()(greater than or equal) correctly implements "from epoch" semantics, allowing UTXOs from the specified epoch onwards.
551-570: LGTM! Max epoch tracking and return structure properly implemented.The
max_epochis tracked alongsidemax_state_versionand correctly included in the returnedUtxoStateUpdateSetstructure. The epoch extraction from the row at line 558 and the max calculation at line 561 follow the same pattern as the state version tracking.applications/tari_indexer/src/substate_manager.rs (1)
108-128: LGTM! Method signature and delegation properly updated.The
get_utxo_updatesmethod correctly threads the newfrom_epochparameter through to the underlying store transaction and returns the updatedUtxoStateUpdateSettype. The changes are consistent with the broader epoch filtering feature.crates/state_store_rocksdb/src/reader.rs (1)
1622-1645: Rename to SubstateCreate/SubstateDestroy wired correctlyConstruction of SubstateUpdateProof::{Create,Destroy} with the new SubstateCreate/SubstateDestroy looks consistent with value_filter and SubstateData fields. No functional concerns here.
crates/storage/src/consensus_models/block.rs (1)
56-56: Type rename propagation looks good; receipts now return SubstateCreate
- SubstateUpdateProof::{Create,Destroy} now carry SubstateCreate/SubstateDestroy consistently.
- get_transaction_receipts() returning Vec is coherent with downstream uses.
Please confirm p2p/rpc and SQL writer paths were updated to persist/read
is_upandvalue_hashfrom the new types (e.g., applications/tari_indexer/.../writer.rs). Based on learnings.Also applies to: 66-66, 800-815, 825-846
applications/tari_indexer/src/store.rs (1)
175-180: Epoch threading on write pathAdding
epoch: Epochto batch_insert_utxo_updates is aligned with the read API. Validate that writers persist epoch and that max_epoch is computed when building UtxoStateUpdateSet.crates/wallet/sdk/tests/support/harness.rs (1)
52-60: Verify EpochBirthday test parameters:u64::MAXforces all epochs to zeroThe parameters are semantically correct but unconventional. The
rel_zero_epoch_secs: u64::MAXis safe—epoch calculations usesaturating_subto prevent underflow. However, with any realistic timestamp < u64::MAX, all subtractions saturate to 0, making every epoch calculate asEpoch(0), which appears intentional given the explicitEpoch::zero()passed toadd_account.Confirm whether forcing all epochs to zero is the intended test behavior or if
rel_zero_epoch_secsshould use a realistic Minotari-epoch-relative timestamp (e.g., genesis time or current time offset).Also applies to: 71-74, 200-206
crates/wallet/sdk/src/models/epoch_birthday.rs (6)
1-10: LGTM: Imports and constants are appropriate.The imports and constant definition are clean and well-structured.
11-19: LGTM: Well-designed struct with appropriate safeguards.The use of
NonZeroU64forepoch_time_secsprevents division-by-zero errors in epoch calculations. The documentation appropriately warns about potential inaccuracies due to network conditions.
38-40: LGTM: Simple getter.
42-48: Verify that silently returning 0 is the desired behavior for all error cases.This method returns 0 when:
- System time calculations fail (
.ok()on Line 45)- Subtraction underflows (
.and_then(|t| t.as_secs().checked_sub(...))on Line 46)While returning 0 for underflow is intentional (as noted in Line 58's comment about saturating_sub), system time failures might indicate actual problems (e.g., incorrect system clock) that should not be silently ignored. Consider whether logging or different handling is appropriate for system time errors versus intentional underflow cases.
50-66: LGTM: Epoch calculation logic is sound.The use of
saturating_subin Line 59 appropriately handles cases whererel_zero_epoch_secsis in the future (returning epoch zero), and the comment clearly explains the rationale. The division on Line 64 is protected from divide-by-zero through theNonZeroU64type.
85-124: LGTM: Comprehensive test coverage.The tests appropriately cover the various epoch calculation methods with clear assertions and test data.
crates/p2p/src/conversions/rpc.rs (4)
11-19: LGTM: Import statements correctly updated.The imports now reference
SubstateCreateandSubstateDestroy, properly reflecting the type rename.
26-46: LGTM: SubstateCreate conversions are correct.The
TryFromandFromimplementations properly handle the type rename with appropriate error handling and field mapping.
48-66: LGTM: SubstateDestroy conversions are correct.The bidirectional conversions properly handle
substate_idbyte serialization/deserialization andversionfield mapping.
68-89: LGTM: SubstateUpdateProof correctly uses renamed types.The
TryFromandFromimplementations forSubstateUpdateProofproperly delegate to theSubstateCreateandSubstateDestroyconversions, maintaining consistency with the type rename.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/storage_sqlite/src/sqlite_db_factory.rs(2 hunks)crates/wallet/sdk/src/models/account.rs(4 hunks)crates/wallet/sdk/src/models/epoch_birthday.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/wallet/sdk/src/models/account.rs
🧰 Additional context used
🧬 Code graph analysis (2)
crates/wallet/sdk/src/models/epoch_birthday.rs (3)
bindings/src/types/Epoch.ts (1)
Epoch(3-3)crates/wallet/sdk/src/apis/key_manager.rs (1)
new(55-69)crates/wallet/sdk/src/local_key_store.rs (1)
new(27-37)
crates/storage_sqlite/src/sqlite_db_factory.rs (1)
crates/storage_sqlite/src/sqlite_transaction.rs (1)
connection(46-48)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: check nightly
- GitHub Check: check stable
- GitHub Check: test
- GitHub Check: machete
- GitHub Check: clippy
- GitHub Check: fmt
🔇 Additional comments (4)
crates/storage_sqlite/src/sqlite_db_factory.rs (4)
23-27: LGTM!The import changes appropriately support the refactored path handling and directory creation logic.
41-44: LGTM!The rename from
data_dirtodb_pathbetter conveys that this field stores the complete database file path rather than just a directory.
47-52: LGTM!The generic
AsRef<Path>parameter is idiomatic and provides flexibility for callers to pass various path types.
55-59: Parent directory handling improved.The
if let Some(parent)pattern correctly addresses the previous review concern about panicking whendb_pathhas no parent. However, verify that silently skipping directory creation when there's no parent (e.g., for a bare filename like"db.sqlite") aligns with the intended behavior - SQLite will attempt to create the file in the current working directory in such cases.
| details: format!("Failed to create parent directory for database file: {}", e), | ||
| })?; | ||
| } | ||
| let database_url = self.db_path.to_str().expect("database_url utf-8 error"); |
There was a problem hiding this comment.
Handle non-UTF-8 paths gracefully instead of panicking.
The .expect() call on to_str() will panic if db_path contains invalid UTF-8 sequences. While uncommon, non-UTF-8 paths are possible on some systems (particularly Unix).
Consider returning a proper error instead:
- let database_url = self.db_path.to_str().expect("database_url utf-8 error");
+ let database_url = self.db_path.to_str().ok_or_else(|| StorageError::General {
+ details: "Database path contains invalid UTF-8 characters".to_string(),
+ })?;🤖 Prompt for AI Agents
In crates/storage_sqlite/src/sqlite_db_factory.rs around line 60, replace the
panic-causing to_str().expect(...) call with a non-panicking conversion and
return an error on failure: detect if the path cannot be represented as UTF-8
and return a Result::Err with a clear error variant (or map into the function's
existing error type) instead of panicking; alternatively use to_string_lossy()
if accepting lossy conversion is acceptable, but prefer returning a descriptive
error (e.g., InvalidDatabasePath) so callers can handle non-UTF-8 paths
gracefully.
2 participants
Description
feat!: impl epoch birthday for utxo scanning
Motivation and Context
Tracks the birthday epoch for wallet accounts and adds an epoch filter to the indexer utxo stream request.
This allows the wallet to limit UTXO scanning to epochs after the account/cipher seed's creation.
caveats: the birthday epoch is calculated based on time, it does not fetch the current epoch from the indexer at account creation time. Therefore, the calculation will be inaccurate if epoch time is not as expected e.g. due to network conditions.
If epochs occur faster than the target, the wallet will do extra scanning, if significantly slower, the wallet could miss UTXOs.
Currently, the wallet hardcodes a zero-epoch time that will always yield an epoch 0 birthday.
How Has This Been Tested?
New unit test and manually
What process can a PR reviewer use to test or verify this change?
Breaking Changes
Summary by CodeRabbit
New Features
Bug Fixes
Refactor