fix: fix fee claiming, indexer syncs VN fee pool substates - #1580
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds fee_claim_public_key to validator identity (server, Rust client, TS bindings, UI); implements batched SubstateId retrieval and chunked processing for validator fee pools (SDK, walletd); threads and persists SubstateData in indexer; removes substate timestamp fields and refactors indexer store/writer APIs; small CLI, key-manager, and UI tweaks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Walletd
participant SDK as SubstatesApi
participant Net as NetworkInterface
participant Indexer as IndexerStore
Walletd->>Walletd: derive SubstateIds from fee_claim_public_key bytes
Walletd->>SDK: get_substates_from_network(ids_chunk)
SDK->>Net: request batch(ids_chunk)
Net-->>SDK: return HashMap<SubstateId, Substate>
SDK-->>Walletd: substates map
Walletd->>Walletd: validate ids/types, extract amount/address, map to shard
Walletd->>Indexer: upsert_substate(&SubstateData) per found substate
Walletd-->>Walletd: aggregate fee entries, log per-chunk summary
rect rgba(230,255,230,0.12)
note right of SDK: New batched substate fetch replaces per-address sequential queries
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (26)
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: 0
🧹 Nitpick comments (3)
applications/tari_swarm_daemon/src/process_definitions/wallet_daemon_create_key.rs (1)
42-53: Prefer OsStr for file paths to avoid UTF‑8 dependency.Passing the output path as OsStr avoids failing on non‑UTF8 paths and matches Command’s native API.
Apply this diff:
- command.args([ - "create-account", - "--name", - "Validator Fees", - "--key", - "0", - "--set-active", - "--output", - output_path - .to_str() - .context("Non-UTF8 output path in WalletDaemonCreateAccount")?, - ]); + command + .args([ + "create-account", + "--name", + "Validator Fees", + "--key", + "0", + "--set-active", + ]) + // Pass path as OsStr to support non‑UTF8 paths + .arg("--output") + .arg(&output_path);applications/tari_validator_node/web_ui/src/routes/VN/Components/Info.tsx (1)
65-68: Label clarity and readability for long keys
- Prefer “Fee claim public key” to match backend naming.
- Apply the same monospace/wrapping style as Shard key for long values.
- Also, verify that “Listen addresses” render newlines correctly; if not, render as
or comma-separated.Suggested edit:
- <TableRow><TableCell>Public key</TableCell> <DataTableCell>{identity.public_key}</DataTableCell> - </TableRow> - <TableRow><TableCell>Claim key</TableCell> <DataTableCell>{identity.fee_claim_public_key}</DataTableCell> - </TableRow> + <TableRow> + <TableCell>Public key</TableCell> + <DataTableCell className="key">{identity.public_key}</DataTableCell> + </TableRow> + <TableRow> + <TableCell>Fee claim public key</TableCell> + <DataTableCell className="key">{identity.fee_claim_public_key}</DataTableCell> + </TableRow>applications/tari_walletd/src/handlers/validator.rs (1)
74-75: Fix log denominator to reflect actual chunk sizeThe log uses CHUNK_SIZE as the denominator, which is misleading for the last (smaller) chunk.
Apply this diff:
- info!(target: LOG_TARGET, "Found {}/{} fee pool substates", substates.len(), CHUNK_SIZE); + info!(target: LOG_TARGET, "Found {}/{} fee pool substates", substates.len(), id_chunk.len());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
applications/tari_swarm_daemon/src/process_definitions/wallet_daemon_create_key.rs(2 hunks)applications/tari_validator_node/src/json_rpc/handlers.rs(1 hunks)applications/tari_validator_node/web_ui/src/routes/VN/Components/Info.tsx(2 hunks)applications/tari_walletd/src/handlers/validator.rs(2 hunks)bindings/src/types/validator-node-client/VNGetIdentityResponse.ts(1 hunks)clients/validator_node_client/src/types.rs(1 hunks)crates/wallet/sdk/src/apis/substate.rs(2 hunks)crates/wallet/storage_sqlite/src/writer.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
clients/validator_node_client/src/types.rs (1)
bindings/src/types/RistrettoPublicKeyBytes.ts (1)
RistrettoPublicKeyBytes(6-6)
crates/wallet/sdk/src/apis/substate.rs (3)
bindings/src/types/Substate.ts (1)
Substate(4-4)bindings/src/types/SubstateId.ts (1)
SubstateId(6-6)bindings/src/types/SubstateValue.ts (1)
SubstateValue(12-21)
bindings/src/types/validator-node-client/VNGetIdentityResponse.ts (1)
bindings/src/types/RistrettoPublicKeyBytes.ts (1)
RistrettoPublicKeyBytes(6-6)
applications/tari_walletd/src/handlers/validator.rs (2)
crates/common_types/src/fee_pool.rs (1)
derive_fee_pool_address(9-20)crates/common_types/src/substate_address.rs (1)
from_substate_id(40-42)
⏰ 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: file licenses
- GitHub Check: check nightly
- GitHub Check: check stable
- GitHub Check: test
- GitHub Check: clippy
- GitHub Check: machete
🔇 Additional comments (9)
applications/tari_swarm_daemon/src/process_definitions/wallet_daemon_create_key.rs (1)
32-32: LGTM: network arg construction is correct.Placing the network value as a separate arg is correct and keeps parsing unambiguous.
clients/validator_node_client/src/types.rs (1)
68-69: Identity response extended with fee_claim_public_key — looks goodField type and placement align with existing serialization and TS export patterns.
Please confirm all consumers (server handlers, bindings, and UI) are regenerated/updated together to avoid version skew during deployment.
bindings/src/types/validator-node-client/VNGetIdentityResponse.ts (1)
11-12: TS binding updated to include fee_claim_public_key — OKMatches the Rust type and name; required field is appropriate for strongly-typed consumers.
applications/tari_validator_node/src/json_rpc/handlers.rs (1)
167-176: Expose configured fee_claim_public_key in get_identity — OK; verify config is always presentThe conversion via to_byte_type is consistent with other uses. Ensure fee_claim_public_key is required in config and cannot be unset at runtime.
If there’s any scenario where the key could be missing, consider returning a sensible default or a null/optional field instead of assuming presence.
applications/tari_validator_node/web_ui/src/routes/VN/Components/Info.tsx (2)
38-41: Prop signature formatting change — OKNo behavioral change; matches the updated identity shape.
70-71: Shard key row — OKConsistent styling with key class applied.
crates/wallet/storage_sqlite/src/writer.rs (1)
224-226: LGTM: pre-insert avoids NotFound on activationEnsuring the row exists before switching the active index is a solid defensive fix and removes the race with missing rows.
Please confirm that key_manager_states has a UNIQUE constraint on (branch_seed, index) so insert-or-ignore remains idempotent and performant.
crates/wallet/sdk/src/apis/substate.rs (1)
66-74: LGTM: batched substate fetch APISimple wrapper with correct error mapping. This unblocks clients to batch queries and reduce request volume.
applications/tari_walletd/src/handlers/validator.rs (1)
59-64: LGTM: correct derivation of fee pool SubstateIdsDeriving fee pool addresses per shard and mapping to SubstateId is correct.
db3f759 to
67f9489
Compare
sdbondi
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
applications/tari_walletd/web_ui/src/utils/helpers.tsx (2)
311-313: Timezone detection bug can append extra 'Z' (e.g., ISO strings ending with 'Z')Current regex fails to match a trailing 'Z', producing invalid "...ZZ" timestamps.
Apply this diff:
- if (!/[Z+\-]\d{2}:?\d{2}$/.test(formatted)) { - formatted += "Z"; - } + // Append 'Z' only if there is no timezone designator + if (!/(?:Z|[+\-]\d{2}:?\d{2})$/.test(formatted)) { + formatted += "Z"; + }
258-266: Avoid precision loss when formatting BigInt amountsCasting BigInt to Number can overflow for large balances; format the BigInt directly.
Apply this diff:
- return `${Number(integerPart).toLocaleString("en-US")}.${fractionalPart} ${currencySymbol}`; + return `${integerPart.toLocaleString("en-US")}.${fractionalPart} ${currencySymbol}`;crates/wallet/sdk/src/apis/accounts.rs (1)
114-121: Do not implicitly activate keys when creating an account — insertion currently sets is_active for the first keykey_manager_insert_or_ignore is idempotent (uses ON CONFLICT DO NOTHING) but computes is_active = count == 0 and inserts that value, so calling it from crates/wallet/sdk/src/apis/accounts.rs:114 will mark the first Account key active. Change the insert to avoid setting is_active here (or avoid calling this helper from walletd CLI flows); activation must be explicit via key_manager_set_active_index.
applications/tari_indexer/src/network_state_sync/worker.rs (1)
361-373: Bug: template changes from earlier chunks are dropped.templates_buf is reinitialized per chunk and you continue on has_more, so only the last chunk’s template changes are enqueued. Move templates_buf outside the loop or enqueue per chunk.
Apply:
- let mut is_first_iter = true; + let mut is_first_iter = true; + let mut templates_buf = Vec::new(); @@ - // Allocations are unavoidable for templates (since the call to the template manager requires owned data due - // to async service call). This is completely fine as published templates are expected to be - // relatively rare compared to other substate updates. - let mut templates_buf = Vec::new(); + // Collect template changes across chunks for this shard streamOptionally, if memory is a concern, enqueue templates per chunk before the continue:
if msg.has_more { + if !templates_buf.is_empty() { + if let Err(err) = self.template_manager.enqueue_template_changes(std::mem::take(&mut templates_buf)).await { + error!(target: LOG_TARGET, "⚠️ Failed to enqueue template changes: {}", err); + } + } debug!(target: LOG_TARGET, "🌍️ more updates for shard {shard} (epoch: {msg_epoch}, state version: {state_version})"); continue; }Also applies to: 429-434
applications/tari_walletd/src/handlers/validator.rs (2)
122-126: Fix: iterator is not Clone; collect addresses and reuse
fee_pool_addresses.clone()will not compile (the mapped iterator with a closure is notClone), and the iterator is consumed twice. Collect to aVecand iterate over it. Also updatewith_inputsaccordingly.Apply this diff:
- let fee_pool_addresses = req - .shards - .into_iter() - .map(|shard| derive_fee_pool_address(&claim_public_key.to_byte_type(), NUM_PRESHARDS, shard)); + let claim_public_key_bytes = claim_public_key.to_byte_type(); + let fee_pool_addresses: Vec<_> = req + .shards + .into_iter() + .map(|shard| derive_fee_pool_address(&claim_public_key_bytes, NUM_PRESHARDS, shard)) + .collect(); @@ - let mut bucket_names = vec![]; - fee_pool_addresses - .clone() + let mut bucket_names = Vec::with_capacity(fee_pool_addresses.len()); + fee_pool_addresses + .iter() + .cloned() .enumerate() .fold(builder, |builder, (i, address)| { bucket_names.push(format!("b{}", i)); builder .claim_validator_fees(address) .put_last_instruction_output_on_workspace(bucket_names.last().unwrap()) }) @@ - .with_inputs(fee_pool_addresses.map(SubstateRequirement::unversioned)) + .with_inputs(fee_pool_addresses.iter().cloned().map(SubstateRequirement::unversioned))Also applies to: 134-144, 155-155
157-162: Bug: wrong signer pairing; use claim_public_key with claim secretYou’re adding a signature with
account_public_keypaired to the claim secret. This will produce an invalid signature. Use the claim public key.Apply this diff:
- if let Some(secret) = claim_secret { - // If the claim key is different from the account secret, we need to sign with both - builder - .with_authorized_seal_signer() - .add_signer(&account_public_key.to_byte_type(), &secret.key) + if let Some(secret) = claim_secret { + // If the claim key is different from the account secret, we need to sign with both + builder + .with_authorized_seal_signer() + .add_signer(&claim_public_key.to_byte_type(), &secret.key)
🧹 Nitpick comments (23)
crates/engine_types/src/substate.rs (1)
779-785: Streamline via as_component(); confirm Copy semantics (or clone).
Refactor to reuse the existing accessor; also please confirm TemplateAddress: Copy. If not, switch to clone or return a ref.Apply this diff:
- pub fn related_template_address(&self) -> Option<TemplateAddress> { - match self { - SubstateValue::Component(component) => Some(component.template_address), - _ => None, - } - } + pub fn related_template_address(&self) -> Option<TemplateAddress> { + self.as_component().map(|c| c.template_address) + }If TemplateAddress is not Copy, use
.map(|c| c.template_address.clone()). Also, confirm it’s intentional to return None for Template and other variants.crates/state_store_rocksdb/src/options.rs (1)
15-16: Doc default value is inconsistent with implementation (says 1, code uses 2).Update the comment to reflect the new default and clarify retention semantics.
- /// The default is 1, which means we keep the previous epoch's data until this epoch has passed. It is not - /// recommended to set this to 0. + /// The default is 2, which means we keep the previous two epochs of data (in addition to the current epoch). + /// It is not recommended to set this to 0.applications/tari_swarm_daemon/src/process_definitions/wallet_daemon_create_key.rs (1)
42-53: Avoid UTF‑8 conversion for output path; pass OsStr directlyThis removes a needless failure mode on non‑UTF8 paths and is more idiomatic for process args.
- command.args([ - "create-account", - "--name", - "Validator Fees", - "--key", - "0", - "--set-active", - "--output", - output_path - .to_str() - .context("Non-UTF8 output path in WalletDaemonCreateAccount")?, - ]); + command + .arg("create-account") + .arg("--name") + .arg("Validator Fees") + .arg("--key") + .arg("0") + .arg("--set-active") + .arg("--output") + .arg(output_path.as_os_str());applications/tari_validator_node/src/p2p/rpc/block_sync_task.rs (3)
332-336: Fix error message: clarify it's about transaction receipts, not substates.The error string is misleading in the receipts branch.
Apply this diff:
- self.send(Err(RpcStatus::general("number of substates exceeds u32"))) + self.send(Err(RpcStatus::general("number of transaction receipts exceeds u32")))
355-357: Fix error message: clarify it's about transactions, not substates.Same nit in the transactions branch.
Apply this diff:
- self.send(Err(RpcStatus::general("number of substates exceeds u32"))) + self.send(Err(RpcStatus::general("number of transactions exceeds u32")))
117-123: Avoid re-parsingstream_substatesinside the inner loop.Parse once per batch before
with_read_txto reduce repeated conversions and keep error handling consistent.Apply this diff:
) -> Result<BlockId, StorageError> { - self.store.with_read_tx(|tx| { + let substates_selection = + proto::rpc::StreamSubstateSelection::try_from(req.stream_substates).map_err(|e| { + StorageError::General { + details: format!("{} is not a valid StreamSubstateSelection: {}", req.stream_substates, e), + } + })?; + self.store.with_read_tx(|tx| { @@ - let substates_selection = - proto::rpc::StreamSubstateSelection::try_from(req.stream_substates).map_err(|e| { - StorageError::General { - details: format!("{} is not a valid StreamSubstateSelection: {}", req.stream_substates, e), - } - })?;Optional follow-up: consider passing
substates_selectionintosend_block_datato avoid a secondtry_fromthere.Also applies to: 174-180
applications/tari_indexer/src/storage_sqlite/reader.rs (5)
90-95: Consider ordering by updated_at to match the timestamp being returned.To reflect most‑recently updated substates (and avoid id ordering drift), order by
updated_atthenidas a tiebreaker.- .order_by(substates::id.desc()) + .order_by(substates::updated_at.desc()) + .then_order_by(substates::id.desc())Ensure an index exists on
substates(updated_at DESC, id DESC)or compatible composite index to keep this query performant.
101-101: Avoid lossy cast for version; use TryFrom to catch negatives.Casting
i32 -> u32can wrap negatives. Prefer fallible conversion to surface data inconsistencies.- let version = s.version as u32; + let version: u32 = s + .version + .try_into() + .map_err(|_| anyhow::anyhow!("list_substates: negative version for {}", s.address))?;
154-156: Fix error context string (plural).Minor copy/paste nit: use
get_substatesto match the method name.- reason: format!("get_substate: {}", e), + reason: format!("get_substates: {}", e),
323-330: Correct error messages to reference list_recent_transactions.Both error paths currently mention
get_last_scanned_block_id.- .map_err(|e| StorageError::QueryError { - reason: format!("get_last_scanned_block_id: {}", e), - })?; + .map_err(|e| StorageError::QueryError { + reason: format!("list_recent_transactions: {}", e), + })?; @@ - r.map_err(|e| StorageError::QueryError { - reason: format!("get_last_scanned_block_id: {}", e), - }) + r.map_err(|e| StorageError::QueryError { + reason: format!("list_recent_transactions: {}", e), + })
350-350: Align OPERATION constant with function name.For clearer diagnostics, make the operation label match
key_value_get_raw.- const OPERATION: &str = "key_value_get_json"; + const OPERATION: &str = "key_value_get_raw";applications/tari_walletd/web_ui/src/utils/helpers.tsx (1)
64-64: Use semantic class and simplifyPrefer a dedicated class and simpler conversion.
Apply this diff:
- if (typeof json === "boolean") return <span className="number">{json ? "true" : "false"}</span>; + if (typeof json === "boolean") return <span className="boolean">{String(json)}</span>;crates/wallet/sdk/src/apis/accounts.rs (1)
114-121: Validate key-index ↔ account address consistency (preflight).To prevent inconsistent state, consider verifying that owner_key_index derives to account_address before opening the write tx (avoid KM deadlocks). If mismatched, return an error. This keeps DB invariants tight for externally supplied inputs.
crates/storage/src/consensus_models/block.rs (2)
819-820: Consider making returned updates deterministically ordered.For reproducibility (block sync, indexing), sort updates by substate_id before returning.
775-776: Minor: capacity preallocation is likely undersized.Vec::with_capacity(committed.len()) underestimates when transactions output multiple substates. Either drop the capacity hint or consider a rougher estimate if available.
applications/tari_indexer/src/storage_sqlite/writer.rs (2)
173-197: Simplify value/component extraction and store valid JSON when value is missing.
- Avoid repeated
.value().component()calls.- Prefer storing
"null"(valid JSON) over empty string when the substate has no value to prevent downstream JSON parsing errors.Apply:
- let template_address = substate - .value - .value() - .and_then(|s| s.component()) - .map(|c| c.template_address.to_string()); - let module_name = substate - .value - .value() - .and_then(|s| s.component()) - .map(|c| c.module_name.clone()); - let new_substate = NewSubstate { - address: substate.substate_id.to_string(), - version: substate.version as i32, - data: substate - .value - .value() - .map(serialize_json) - .transpose()? - .unwrap_or_default(), - template_address, - module_name, - }; + let value = substate.value.value(); + let component = value.and_then(|s| s.component()); + let template_address = component.map(|c| c.template_address.to_string()); + let module_name = component.map(|c| c.module_name.clone()); + let data = value + .map(serialize_json) + .transpose()? + .unwrap_or_else(|| "null".to_string()); + let new_substate = NewSubstate { + address: substate.substate_id.to_string(), + version: substate.version as i32, + data, + template_address, + module_name, + };
199-235: Use a single UPSERT instead of SELECT + UPDATE/INSERT.Reduces one query per substate and simplifies logging.
Replace the block with:
- let address = &new_substate.address; - let current_substate = substates::table - .filter(substates::address.eq(address)) - .first::<SubstateRecord>(self.connection()) - .optional() - .map_err(|e| StorageError::QueryError { - reason: format!("find_by_address: {}", e), - })?; - - match current_substate { - Some(_) => { - diesel::update(substates::table) - .set(&new_substate) - .filter(substates::address.eq(address)) - .execute(self.connection()) - .map_err(|e| StorageError::QueryError { - reason: format!("Update leaf node: {}", e), - })?; - debug!( - target: LOG_TARGET, - "Updated substate {} version to {}", address, new_substate.version - ); - }, - None => { - diesel::insert_into(substates::table) - .values(&new_substate) - .execute(self.connection()) - .map_err(|e| StorageError::QueryError { - reason: format!("Update substate error: {}", e), - })?; - info!( - target: LOG_TARGET, - "Added new substate {} with version {}", address, new_substate.version - ); - }, - }; + diesel::insert_into(substates::table) + .values(&new_substate) + .on_conflict(substates::address) + .do_update() + .set(&new_substate) + .execute(self.connection()) + .map_err(|e| StorageError::QueryError { + reason: format!("upsert_substate: {}", e), + })?; + debug!( + target: LOG_TARGET, + "Upserted substate {} to version {}", + new_substate.address, new_substate.version + );applications/tari_indexer/src/network_state_sync/block_scanner.rs (2)
130-151: Use with_write_tx for automatic commit/rollback and consider DOWN handling.
- Minor: Prefer
with_write_txto ensure rollback on errors.- Minor: DOWN updates are ignored; this may leave stale rows in
substates. Consider marking as deleted or pruning.Refactor:
-fn store_substates_in_db(&self, updates: &[SubstateUpdateProof]) -> Result<(), anyhow::Error> { - let mut tx = self.substate_store.create_write_tx()?; - // store/update up substates if any - for update in updates { - match update { - SubstateUpdateProof::Create(create) => { - if create.substate.value.value().is_none() { - warn!(target: LOG_TARGET, "⚠️ Received UP substate {} without value. This indicates that the substate has been pruned. Some event data is not available.", create.substate.as_versioned_substate_id_ref(),); - } - debug!(target: LOG_TARGET, "Saving substate: {:?}", create.substate); - tx.upsert_substate(&create.substate)?; - }, - SubstateUpdateProof::Destroy(_) => {}, - } - } - tx.commit()?; - Ok(()) -} +fn store_substates_in_db(&self, updates: &[SubstateUpdateProof]) -> Result<(), anyhow::Error> { + self.substate_store.with_write_tx(|tx| { + for update in updates { + if let SubstateUpdateProof::Create(create) = update { + if create.substate.value.value().is_none() { + warn!(target: LOG_TARGET, "⚠️ Received UP substate {} without value. This indicates that the substate has been pruned. Some event data is not available.", create.substate.as_versioned_substate_id_ref()); + } + debug!(target: LOG_TARGET, "Saving substate: {:?}", create.substate); + tx.upsert_substate(&create.substate)?; + } + // TODO(optional): handle Destroy by pruning/marking deleted + } + Ok(()) + })?; + Ok(()) +}
145-146: Reduce log volume for large substates (optional).Consider logging only ID/version to avoid large payloads in debug logs.
Example:
- debug!(..., "Saving substate {} v{}", create.substate.substate_id, create.substate.version);
applications/tari_indexer/src/network_state_sync/worker.rs (3)
414-418: Upserting fee pools: OK, but consider batching.Looping upserts inside one transaction is fine. If write-amplification becomes noticeable, add a batch_upsert_substates API to keep a single prepared statement.
504-510: Minor: use update.version() for consistency.The version should equal create.substate.version, but using update.version() keeps consistency with other paths and avoids field access.
- validator_fee_pools_buf.push(SubstateData { - substate_id: create.substate.substate_id().clone(), - version: create.substate.version, - value: create.substate.value().clone(), - }); + validator_fee_pools_buf.push(SubstateData { + substate_id: create.substate.substate_id().clone(), + version: update.version(), + value: create.substate.value().clone(), + });
444-455: Event stats may be overstated.increase_events uses transactions_buf.len(), but persist_transaction_receipts filters events. Consider counting post-filter for accurate metrics.
applications/tari_walletd/src/handlers/validator.rs (1)
59-90: Minor: fix log formatting and count; batch flow looks goodUse the actual chunk size and avoid assuming Display for the key; prefer debug formatting.
Apply this diff:
- info!(target: LOG_TARGET, "🔍️ Found {}/{} fee pool substates for claim key {}", substates.len(), CHUNK_SIZE, claim_public_key); + info!( + target: LOG_TARGET, + "🔍️ Found {}/{} fee pool substates for claim key {:?}", + substates.len(), + id_chunk.len(), + claim_public_key + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
applications/tari_indexer/src/network_state_sync/block_scanner.rs(3 hunks)applications/tari_indexer/src/network_state_sync/worker.rs(10 hunks)applications/tari_indexer/src/storage_sqlite/migrations/2023-02-16-145719_initial/up.sql(0 hunks)applications/tari_indexer/src/storage_sqlite/models/substate.rs(0 hunks)applications/tari_indexer/src/storage_sqlite/reader.rs(1 hunks)applications/tari_indexer/src/storage_sqlite/schema.rs(0 hunks)applications/tari_indexer/src/storage_sqlite/store_factory.rs(3 hunks)applications/tari_indexer/src/storage_sqlite/writer.rs(2 hunks)applications/tari_swarm_daemon/src/process_definitions/wallet_daemon_create_key.rs(2 hunks)applications/tari_validator_node/src/json_rpc/handlers.rs(1 hunks)applications/tari_validator_node/src/p2p/rpc/block_sync_task.rs(1 hunks)applications/tari_validator_node/web_ui/src/routes/VN/Components/Info.tsx(2 hunks)applications/tari_walletd/src/handlers/validator.rs(2 hunks)applications/tari_walletd/web_ui/src/utils/helpers.tsx(1 hunks)bindings/src/types/validator-node-client/VNGetIdentityResponse.ts(1 hunks)clients/validator_node_client/src/types.rs(1 hunks)crates/engine_types/src/substate.rs(2 hunks)crates/state_store_rocksdb/src/options.rs(1 hunks)crates/storage/src/consensus_models/block.rs(1 hunks)crates/storage/src/consensus_models/command.rs(0 hunks)crates/wallet/sdk/src/apis/accounts.rs(2 hunks)crates/wallet/sdk/src/apis/substate.rs(2 hunks)crates/wallet/storage_sqlite/src/writer.rs(1 hunks)
💤 Files with no reviewable changes (4)
- applications/tari_indexer/src/storage_sqlite/schema.rs
- applications/tari_indexer/src/storage_sqlite/models/substate.rs
- applications/tari_indexer/src/storage_sqlite/migrations/2023-02-16-145719_initial/up.sql
- crates/storage/src/consensus_models/command.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- applications/tari_validator_node/web_ui/src/routes/VN/Components/Info.tsx
- crates/wallet/storage_sqlite/src/writer.rs
- bindings/src/types/validator-node-client/VNGetIdentityResponse.ts
- crates/wallet/sdk/src/apis/substate.rs
🧰 Additional context used
🧬 Code graph analysis (8)
clients/validator_node_client/src/types.rs (1)
bindings/src/types/RistrettoPublicKeyBytes.ts (1)
RistrettoPublicKeyBytes(6-6)
crates/engine_types/src/substate.rs (1)
bindings/src/types/SubstateValue.ts (1)
SubstateValue(12-21)
crates/wallet/sdk/src/apis/accounts.rs (1)
bindings/src/types/wallet-daemon-client/KeyBranch.ts (1)
KeyBranch(3-10)
applications/tari_indexer/src/storage_sqlite/store_factory.rs (1)
applications/tari_indexer/src/storage_sqlite/writer.rs (1)
upsert_substate(173-237)
applications/tari_indexer/src/network_state_sync/worker.rs (3)
crates/engine_types/src/substate.rs (2)
new(71-76)new(858-864)bindings/src/types/SubstateValue.ts (1)
SubstateValue(12-21)bindings/src/types/ValidatorFeePool.ts (1)
ValidatorFeePool(3-3)
applications/tari_walletd/src/handlers/validator.rs (3)
crates/common_types/src/fee_pool.rs (1)
derive_fee_pool_address(9-20)crates/engine_types/src/substate.rs (16)
from(307-309)from(313-315)from(319-321)from(325-327)from(331-333)from(337-339)from(343-345)from(349-351)from(355-357)from(796-798)from(802-804)from(808-810)from(814-816)from(820-822)from(826-828)from(832-834)crates/common_types/src/substate_address.rs (1)
from_substate_id(40-42)
applications/tari_indexer/src/network_state_sync/block_scanner.rs (4)
crates/storage/src/consensus_models/block.rs (4)
id(310-312)epoch(347-349)height(343-345)create(116-150)applications/tari_indexer/src/storage_sqlite/writer.rs (1)
updates(92-108)crates/storage/src/consensus_models/substate_change.rs (1)
substate(58-63)crates/storage/src/consensus_models/substate.rs (1)
as_versioned_substate_id_ref(370-372)
applications/tari_indexer/src/storage_sqlite/writer.rs (3)
applications/tari_indexer/src/storage_sqlite/store_factory.rs (1)
upsert_substate(212-212)applications/tari_indexer/src/storage_sqlite/reader.rs (3)
substates(97-112)substates(150-153)substates(183-187)applications/tari_indexer/src/storage_sqlite/serialization.rs (1)
serialize_json(28-34)
⏰ 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: test
- GitHub Check: file licenses
- GitHub Check: check nightly
- GitHub Check: check stable
- GitHub Check: machete
- GitHub Check: clippy
🔇 Additional comments (21)
crates/engine_types/src/substate.rs (1)
45-46: LGTM: Import for TemplateAddress is correct and used.
The new import is used by related_template_address(); no issues.crates/state_store_rocksdb/src/options.rs (1)
24-24: Confirm impact of increasing epoch retention from 1 → 2
- Default now set to Epoch(2) in crates/state_store_rocksdb/src/options.rs (
epoch_history_length: Epoch(2)).- Pruning uses this value in crates/state_store_rocksdb/src/writer.rs (epoch_cleanup). Increasing to 2 delays pruning, increasing disk usage and altering pruning-related test expectations.
- Action: verify disk/pruning behavior under the new default, update release notes/config docs to note the changed default, and adjust any pruning/retention-sensitive tests or alerts.
applications/tari_swarm_daemon/src/process_definitions/wallet_daemon_create_key.rs (1)
32-32: Global flags before subcommand: LGTMPlacing
--network(and the keyring override earlier) beforecreate-accountmatches typical CLI parsing and aligns with the PR objective.applications/tari_validator_node/src/p2p/rpc/block_sync_task.rs (1)
186-193: LGTM: Receipt retrieval is correctly gated and defaulted.The
transaction_receiptsfetch is aligned withTransactionReceiptsOnlyand safely defaults to empty. Looks correct.applications/tari_indexer/src/storage_sqlite/reader.rs (1)
97-111: Swapping to updated_at for timestamp looks correct; please confirm nullability and client expectations.Assuming schema migration removed
timestampand guaranteesupdated_atis always set, this aligns with the new model. Please verify:
substates.updated_atis NOT NULL and backfilled in migrations.ListSubstateItem.timestampexpects the same type/semantics (updated time vs created time) as consumers may display this field.clients/validator_node_client/src/types.rs (1)
68-68: LGTM - Consistent addition of fee_claim_public_key field.The addition of the
fee_claim_public_keyfield to theGetIdentityResponsestruct is consistent with the broader PR objectives of adding fee claim functionality. The field typeRistrettoPublicKeyBytesmatches the existingpublic_keyfield pattern and aligns with the default implementation for Tari ECC is the Ristretto255 curve.applications/tari_validator_node/src/json_rpc/handlers.rs (3)
167-167: LGTM - Proper extraction of fee claim public key from configuration.The code correctly extracts the
fee_claim_public_keyfrom configuration and converts it to the expected byte type format using theto_byte_type()method.
175-175: LGTM - Proper assignment of fee_claim_public_key to response.The
fee_claim_public_keyis correctly assigned to theGetIdentityResponsestruct, maintaining consistency with the API contract changes.
810-810: LGTM - Consistent use of fee_claim_public_key in registration flow.The code correctly uses the same
fee_claim_public_keyvalue from configuration in both the identity response flow and the layer one transaction registration flow. The assignment on Line 844 toclaim_public_keyin theValidatorRegistrationParamsensures the registration payload carries the correct fee claim public key.Also applies to: 844-844
applications/tari_walletd/web_ui/src/utils/helpers.tsx (1)
64-64: Boolean rendering LGTM — fixes false=>null bugThis correctly handles booleans that previously fell through and rendered "null" for false.
crates/wallet/sdk/src/apis/accounts.rs (1)
25-26: Importing KeyBranch is correct.Needed for the new key-manager insert in add_account.
crates/storage/src/consensus_models/block.rs (1)
765-768: Confirmed: get_substate_updates is only used for block-sync and downstreams do not require the omitted QCs / implicit transitions.
- Only call site: applications/tari_validator_node/src/p2p/rpc/block_sync_task.rs (child.get_substate_updates(...)).
- p2p RPC conversions/proto have created_justify/destroyed_justify commented out (crates/p2p/src/conversions/rpc.rs, crates/p2p/proto/rpc.proto) so QC fields are not serialized.
- Indexer/state-sync consume SubstateUpdateProof::Create (and ignore Destroy) and do not rely on QC or implicit state transitions (applications/tari_indexer/src/network_state_sync/, crates/rpc_state_sync/src/).
applications/tari_indexer/src/storage_sqlite/store_factory.rs (2)
20-22: Imports and model exposure look correct.Bringing
SubstateDatainto scope and narrowing public models is aligned with the new write path.Also applies to: 36-39
212-213: API change to upsert_substate(&SubstateData) — verifiedCall sites updated to pass &SubstateData: applications/tari_indexer/src/network_state_sync/worker.rs:417 and applications/tari_indexer/src/network_state_sync/block_scanner.rs:147. NewSubstate is only used in applications/tari_indexer/src/storage_sqlite/writer.rs and applications/tari_indexer/src/storage_sqlite/models/substate.rs.
applications/tari_indexer/src/storage_sqlite/writer.rs (1)
12-14: Importing SubstateData for the write path is correct.applications/tari_indexer/src/network_state_sync/block_scanner.rs (2)
10-10: Imports updated to consensus_models and narrowed models — OK.Also applies to: 16-21
109-117: Helpful logging addition.Good visibility into per‑block substate writes.
applications/tari_indexer/src/network_state_sync/worker.rs (3)
285-285: Plumbing for validator fee pools looks consistent.Buffers are threaded through call sites and flushed at commit. Reads well and keeps the change localized.
Also applies to: 301-305, 309-321, 335-337, 397-398, 467-468
409-415: Resolved — batching semantics are correct: chunks share a single state_version, so committing the accumulated buffer once is safe.Server-side splits a single StateVersionTransitions into multiple SyncStateResponse chunks with the same state_version/has_more flag (applications/tari_validator_node/src/p2p/rpc/state_sync_task.rs:119-125); client-side rpc_state_sync enforces/validates the expected_state_version while buffering (crates/rpc_state_sync/src/state_sync.rs:200-256); the writer API writes the provided state_version per-row (applications/tari_indexer/src/storage_sqlite/writer.rs:94-101). The current approach of accumulating update_buf across chunks and calling batch_insert_substate_transitions(shard, state_version, ...) is correct.
355-357: Verify VN/SDK compatibility for VALIDATOR_FEE_POOL filter.SubstateValueFilterFlags::VALIDATOR_FEE_POOL is defined in crates/storage/src/consensus_models/state_transition.rs and used in applications/tari_indexer/src/network_state_sync/worker.rs, but no SyncStateResponse/VN emission was found in the repo — confirm peers/SDKs include this substate or gate behind config to avoid empty-stream regressions.
applications/tari_walletd/src/handlers/validator.rs (1)
12-12: LGTM: imports and byte-type conversion are correctImports align with usage; converting the claim key to byte type before address derivation is correct.
Also applies to: 52-52
67f9489 to
ca262b9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
applications/tari_walletd/src/handlers/validator.rs (2)
122-126: Iterator cloning will likely not compile; collect once and iterate.
Cloning a mapped iterator requires F: Clone which closures typically don't satisfy. This likely fails to compile. Collect to Vec and reuse.Apply this diff:
- let fee_pool_addresses = req - .shards - .into_iter() - .map(|shard| derive_fee_pool_address(&claim_public_key.to_byte_type(), NUM_PRESHARDS, shard)); + let fee_pool_addresses: Vec<_> = req + .shards + .into_iter() + .map(|shard| derive_fee_pool_address(&claim_public_key.to_byte_type(), NUM_PRESHARDS, shard)) + .collect();- fee_pool_addresses - .clone() - .enumerate() - .fold(builder, |builder, (i, address)| { + fee_pool_addresses + .iter() + .enumerate() + .fold(builder, |builder, (i, address)| { bucket_names.push(format!("b{}", i)); builder - .claim_validator_fees(address) + .claim_validator_fees(address.clone()) .put_last_instruction_output_on_workspace(bucket_names.last().unwrap()) })- .with_inputs(fee_pool_addresses.map(SubstateRequirement::unversioned)) + .with_inputs(fee_pool_addresses.iter().cloned().map(SubstateRequirement::unversioned))Also applies to: 135-143, 155-155
156-163: Fix signer mismatch: wrong public key used with the claim secret.
When claim key differs, add the claim keypair, not the account pubkey with the claim secret. This will otherwise fail signature verification.Apply this diff:
- .add_signer(&account_public_key.to_byte_type(), &secret.key) + .add_signer(&claim_public_key.to_byte_type(), &secret.key)
🧹 Nitpick comments (3)
applications/tari_walletd/src/handlers/validator.rs (3)
52-52: Nit: name suggests a PK, but the value is bytes.
Consider renaming to claim_public_key_bytes for clarity.
68-75: Use actual chunk size in log.
CHUNK_SIZE is constant (20) and may not match the final chunk length. Log id_chunk.len() instead.Apply this diff:
- info!(target: LOG_TARGET, "🔍️ Found {}/{} fee pool substates for claim key {}", substates.len(), CHUNK_SIZE, claim_public_key); + info!(target: LOG_TARGET, "🔍️ Found {}/{} fee pool substates for claim key {}", substates.len(), id_chunk.len(), claim_public_key);
76-91: Minor log message inconsistency.
Message says “type found at address” but prints a SubstateId. Prefer “ID”.Apply this diff:
- warn!(target: LOG_TARGET, "Incorrect substate type found at address {}", substate_id); + warn!(target: LOG_TARGET, "Incorrect substate type found for ID {}", substate_id);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
applications/tari_indexer/src/network_state_sync/block_scanner.rs(3 hunks)applications/tari_indexer/src/network_state_sync/worker.rs(10 hunks)applications/tari_indexer/src/storage_sqlite/migrations/2023-02-16-145719_initial/up.sql(0 hunks)applications/tari_indexer/src/storage_sqlite/models/substate.rs(0 hunks)applications/tari_indexer/src/storage_sqlite/reader.rs(1 hunks)applications/tari_indexer/src/storage_sqlite/schema.rs(0 hunks)applications/tari_indexer/src/storage_sqlite/store_factory.rs(3 hunks)applications/tari_indexer/src/storage_sqlite/writer.rs(2 hunks)applications/tari_swarm_daemon/src/process_definitions/wallet_daemon_create_key.rs(2 hunks)applications/tari_validator_node/src/json_rpc/handlers.rs(1 hunks)applications/tari_validator_node/src/p2p/rpc/block_sync_task.rs(1 hunks)applications/tari_validator_node/web_ui/src/routes/VN/Components/Info.tsx(2 hunks)applications/tari_walletd/src/handlers/validator.rs(2 hunks)applications/tari_walletd/web_ui/src/utils/helpers.tsx(1 hunks)bindings/src/types/validator-node-client/VNGetIdentityResponse.ts(1 hunks)clients/validator_node_client/src/types.rs(1 hunks)crates/engine_types/src/substate.rs(2 hunks)crates/state_store_rocksdb/src/options.rs(1 hunks)crates/storage/src/consensus_models/block.rs(1 hunks)crates/storage/src/consensus_models/command.rs(0 hunks)crates/wallet/sdk/src/apis/accounts.rs(2 hunks)crates/wallet/sdk/src/apis/substate.rs(2 hunks)crates/wallet/storage_sqlite/src/writer.rs(1 hunks)
💤 Files with no reviewable changes (4)
- crates/storage/src/consensus_models/command.rs
- applications/tari_indexer/src/storage_sqlite/migrations/2023-02-16-145719_initial/up.sql
- applications/tari_indexer/src/storage_sqlite/schema.rs
- applications/tari_indexer/src/storage_sqlite/models/substate.rs
🚧 Files skipped from review as they are similar to previous changes (14)
- crates/wallet/storage_sqlite/src/writer.rs
- clients/validator_node_client/src/types.rs
- applications/tari_walletd/web_ui/src/utils/helpers.tsx
- bindings/src/types/validator-node-client/VNGetIdentityResponse.ts
- crates/storage/src/consensus_models/block.rs
- applications/tari_validator_node/src/p2p/rpc/block_sync_task.rs
- applications/tari_validator_node/src/json_rpc/handlers.rs
- crates/wallet/sdk/src/apis/substate.rs
- crates/wallet/sdk/src/apis/accounts.rs
- applications/tari_swarm_daemon/src/process_definitions/wallet_daemon_create_key.rs
- applications/tari_indexer/src/storage_sqlite/reader.rs
- applications/tari_validator_node/web_ui/src/routes/VN/Components/Info.tsx
- crates/state_store_rocksdb/src/options.rs
- crates/engine_types/src/substate.rs
🧰 Additional context used
🧬 Code graph analysis (5)
applications/tari_indexer/src/storage_sqlite/writer.rs (3)
applications/tari_indexer/src/storage_sqlite/store_factory.rs (1)
upsert_substate(212-212)applications/tari_indexer/src/storage_sqlite/reader.rs (3)
substates(97-112)substates(150-153)substates(183-187)applications/tari_indexer/src/storage_sqlite/serialization.rs (1)
serialize_json(28-34)
applications/tari_indexer/src/storage_sqlite/store_factory.rs (1)
applications/tari_indexer/src/storage_sqlite/writer.rs (1)
upsert_substate(173-237)
applications/tari_walletd/src/handlers/validator.rs (4)
crates/common_types/src/fee_pool.rs (1)
derive_fee_pool_address(9-20)crates/engine_types/src/validator_fee.rs (1)
claim_public_key(156-158)crates/engine_types/src/substate.rs (16)
from(307-309)from(313-315)from(319-321)from(325-327)from(331-333)from(337-339)from(343-345)from(349-351)from(355-357)from(793-795)from(799-801)from(805-807)from(811-813)from(817-819)from(823-825)from(829-831)crates/common_types/src/substate_address.rs (1)
from_substate_id(40-42)
applications/tari_indexer/src/network_state_sync/worker.rs (3)
crates/engine_types/src/substate.rs (2)
new(71-76)new(855-861)bindings/src/types/SubstateValue.ts (1)
SubstateValue(12-21)bindings/src/types/ValidatorFeePool.ts (1)
ValidatorFeePool(3-3)
applications/tari_indexer/src/network_state_sync/block_scanner.rs (4)
crates/storage/src/consensus_models/block.rs (4)
id(310-312)epoch(347-349)height(343-345)create(116-150)applications/tari_indexer/src/storage_sqlite/writer.rs (1)
updates(92-108)crates/storage/src/consensus_models/substate_change.rs (1)
substate(58-63)crates/storage/src/consensus_models/substate.rs (1)
as_versioned_substate_id_ref(370-372)
⏰ 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 stable
- GitHub Check: clippy
- GitHub Check: check nightly
- GitHub Check: test
- GitHub Check: machete
- GitHub Check: fmt
🔇 Additional comments (21)
applications/tari_indexer/src/storage_sqlite/store_factory.rs (3)
20-20: LGTM! Imports aligned with API change.The addition of
SubstateDatato the consensus models import is consistent with the newupsert_substatesignature that takes&SubstateDatainstead ofNewSubstate.
36-36: LGTM! Module exports updated correctly.The removal of
NewSubstatefrom the public exports aligns with the migration away from directNewSubstateusage in favor ofSubstateData-based operations.
212-212: LGTM! Method signature updated appropriately.The
upsert_substatemethod signature change from accepting aNewSubstatevalue to accepting&SubstateDatareference is consistent with the storage layer refactoring and follows Rust best practices by avoiding unnecessary ownership transfer.applications/tari_indexer/src/storage_sqlite/writer.rs (2)
12-12: LGTM! Import updated for new data model.The addition of
SubstateDatato the consensus models import supports the refactoredupsert_substateimplementation.
173-197: Excellent refactor to use SubstateData consistently.The implementation correctly transforms
SubstateDatainto aNewSubstatefor database operations while extracting template address and module name from the component data when available. The data serialization logic properly handles optional values usingunwrap_or_default().applications/tari_indexer/src/network_state_sync/block_scanner.rs (4)
10-10: LGTM! Import updated for consistent data model.The import change to use
SubstateUpdateProofinstead of removed types aligns with the storage layer refactoring.
16-16: LGTM! Module imports cleaned up.The removal of
NewSubstatefrom the models import is consistent with the migration toSubstateData-based operations.
109-117: Good observability improvement.The detailed logging of substate update count and block information (ID, epoch, height) will help with debugging and monitoring the indexer's operation.
130-130: LGTM! Simplified storage path using SubstateData.The method signature change and direct usage of
create.substatefor upserting aligns with the storage layer refactoring. The removal of manual field extraction and timestamp handling simplifies the code while maintaining correctness.Also applies to: 136-147
applications/tari_indexer/src/network_state_sync/worker.rs (9)
25-25: LGTM! Import updated for validator fee pool support.The addition of
SubstateDatato the imports enables proper handling of validator fee pool substates in the sync process.
285-285: Good addition for validator fee pool collection.The new
validator_fee_pools_bufbuffer will collectValidatorFeePoolsubstates during sync for proper persistence.
301-301: LGTM! Consistent buffer threading.The
validator_fee_pools_bufis properly passed to both global shard and per-shard sync operations.Also applies to: 316-316
334-334: LGTM! Method signature updated consistently.The addition of the
validator_fee_pools_bufparameter maintains consistency with the sync flow.
355-355: LGTM! Value filter includes validator fee pools.The inclusion of
SubstateValueFilterFlags::VALIDATOR_FEE_POOLin the sync request ensures validator fee pool substates are fetched during state synchronization.
397-397: LGTM! Buffer passed to extension function.The
validator_fee_pools_bufis properly threaded through to the substate update processing function.
414-418: Appropriate handling of validator fee pools with clear documentation.The comment clearly explains that this approach allows wallet queries for validator fee pool values, addressing the gap where block sync doesn't include these substates. The upsert operation for each validator fee pool substate is straightforward and correct.
467-467: LGTM! Function signature updated consistently.The addition of
validator_fee_pools_bufparameter maintains consistency throughout the sync pipeline.
504-510: Excellent handling of ValidatorFeePool substates.The implementation correctly constructs
SubstateDatainstances forValidatorFeePoolsubstates, capturing the substate ID, version, and value. This ensures validator fee pools are properly collected for persistence.applications/tari_walletd/src/handlers/validator.rs (3)
12-12: LGTM: import updates are correct.
59-67: LGTM: batched ID derivation is correct.
74-74: Verify formatting of claim_public_key in logs.
If RistrettoPublicKeyBytes doesn’t implement Display, switch to {:?} or hex.
ca262b9 to
8c2668b
Compare
2 participants
Description
fix(swarm): fix create account for fee claiming
fix(wallet): request VN fee pool substates in batches
Motivation and Context
The fee claiming setup in swarm was broken
Querying each shard for fee pool substates was slow, using the new
get_substatesRPC for batching requests improves performance and greatly reduces requests.It was also discovered that the indexer does not sync validator fee pools. This was corrected.
How Has This Been Tested?
Manually
What process can a PR reviewer use to test or verify this change?
Breaking Changes
Summary by CodeRabbit
New Features
UI Improvements
Performance
Bug Fixes