feat(wallet): add rpc call to allow decrypting utxo values with view keys enabled - #1587
Conversation
WalkthroughReplaces brute-force balance logic with a new ViewableBalanceApi, adds a new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client as JS/Rust Client
participant JRPC as JSON-RPC Server
participant Handler as stealth_utxos::handlers
participant SDK as WalletSdk
participant VB as ViewableBalanceApi
participant LUT as ValueLookupTable
Client->>JRPC: stealth_utxos.decrypt_value(request)
JRPC->>Handler: handle_decrypt_value(token, request)
Handler->>Handler: validate ids (<=10), build `UtxoAddress` list
Handler->>Handler: fetch substates for UTXO ids
Handler->>SDK: derive secret view key
Handler->>Handler: load LUT (file or AlwaysMiss)
Handler->>SDK: sdk.viewable_balance_api()
SDK->>VB: get ViewableBalanceApi
Handler->>VB: try_brute_force_commitment_balances(secret_view_key, outputs, range, LUT)
VB->>LUT: batched lookup calls
LUT-->>VB: lookup results
VB-->>Handler: Vec<Option<u64>> balances
Handler-->>JRPC: StealthUtxosDecryptValueResponse { balances }
JRPC-->>Client: RPC response
Note over Handler,VB: duration logged around brute-force step
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)
✨ Finishing touches
🧪 Generate unit tests
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: 5
🧹 Nitpick comments (5)
crates/template_lib/src/models/utxo.rs (1)
127-129: Public fields on UtxoAddressContentsThis relaxes encapsulation and matches the TS bindings usage. If future invariants are needed, consider accessor methods or marking the struct non_exhaustive.
crates/wallet/sdk/src/sdk.rs (1)
226-229: New accessor for ViewableBalanceApi — consider a brief doc commentAdd a short doc to surface its purpose in rustdoc.
Apply this diff to document the accessor:
- pub fn viewable_balance_api(&self) -> ViewableBalanceApi { + /// Returns the ViewableBalance API used for decrypting/viewing UTXO balances with view keys. + pub fn viewable_balance_api(&self) -> ViewableBalanceApi { ViewableBalanceApi }applications/tari_walletd/src/handlers/confidential.rs (1)
288-314: Validate min/max range before brute-force to avoid empty/invalid scansIf minimum_expected_value > maximum_expected_value, the inclusive range becomes empty; better to fail fast with InvalidParams.
You can add a guard before constructing the range (illustrative snippet):
let min = req.minimum_expected_value.unwrap_or(0); let max = req.maximum_expected_value.unwrap_or(10_000_000_000); if min > max { return Err(invalid_params("minimum_expected_value", Some("must be <= maximum_expected_value"))); } let value_range = min..=max;applications/tari_walletd/src/handlers/stealth_utxos.rs (2)
65-71: Limit check is good; consider also validating non-empty.Optional: reject empty ids early to avoid unnecessary network calls.
- if req.ids.len() > 10 { + if req.ids.is_empty() { + return Err(invalid_params("ids", Some("Must request at least one UTXO"))); + } + if req.ids.len() > 10 { return Err(invalid_params( "ids", Some("Cannot request more than 10 UTXOs at a time"), )); }
103-127: Move blocking file IO and loading into blocking context; prefer spawn_blocking.File open and lookup loading are blocking; wrap them (and the brute-force) in spawn_blocking to avoid blocking the async scheduler.
- let balances = match context.config().value_lookup_table_file.as_ref() { - Some(file) => { - let mut file = fs::File::open(file) - .map_err(|e| anyhow!("Unable to load value lookup file '{}': {e}", file.display()))?; - let mut lookup = IoReaderValueLookup::load(&mut file)?; - - block_in_place(|| { - sdk.viewable_balance_api().try_brute_force_commitment_balances( - &view_key.key, - outputs.values().copied(), // Copying the reference, not the PrivateOutput - value_range, - &mut lookup, - ) - })? - }, - None => block_in_place(|| { - sdk.viewable_balance_api().try_brute_force_commitment_balances( - &view_key.key, - outputs.values().copied(), - value_range, - &mut AlwaysMissLookupTable, - ) - })?, - }; + let balances = match context.config().value_lookup_table_file.as_ref() { + Some(path) => { + let view_key = view_key.key.clone(); + let outputs_iter = outputs.values().copied().collect::<Vec<_>>(); + let path = path.clone(); + tokio::task::spawn_blocking(move || { + let mut file = fs::File::open(&path) + .map_err(|e| anyhow!("Unable to load value lookup file '{}': {e}", path.display()))?; + let mut lookup = IoReaderValueLookup::load(&mut file)?; + sdk.viewable_balance_api().try_brute_force_commitment_balances( + &view_key, outputs_iter.iter().copied(), value_range, &mut lookup, + ) + }) + .await?? + }, + None => { + let view_key = view_key.key.clone(); + let outputs_iter = outputs.values().copied().collect::<Vec<_>>(); + tokio::task::spawn_blocking(move || { + sdk.viewable_balance_api().try_brute_force_commitment_balances( + &view_key, outputs_iter.iter().copied(), value_range, &mut AlwaysMissLookupTable, + ) + }) + .await?? + }, + };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
applications/tari_walletd/src/handlers/confidential.rs(2 hunks)applications/tari_walletd/src/handlers/stealth_utxos.rs(2 hunks)applications/tari_walletd/src/jrpc_server.rs(1 hunks)bindings/src/types/wallet-daemon-client/StealthUtxosDecryptValueRequest.ts(1 hunks)bindings/src/types/wallet-daemon-client/StealthUtxosDecryptValueResponse.ts(1 hunks)bindings/src/wallet-daemon-client.ts(2 hunks)clients/javascript/wallet_daemon_client/package.json(1 hunks)clients/javascript/wallet_daemon_client/src/index.ts(4 hunks)clients/wallet_daemon_client/src/lib.rs(2 hunks)clients/wallet_daemon_client/src/types.rs(2 hunks)crates/template_lib/src/models/utxo.rs(2 hunks)crates/wallet/sdk/src/apis/confidential_crypto.rs(0 hunks)crates/wallet/sdk/src/apis/mod.rs(1 hunks)crates/wallet/sdk/src/apis/stealth_crypto.rs(1 hunks)crates/wallet/sdk/src/apis/viewable_balance.rs(1 hunks)crates/wallet/sdk/src/sdk.rs(2 hunks)
💤 Files with no reviewable changes (1)
- crates/wallet/sdk/src/apis/confidential_crypto.rs
🧰 Additional context used
🧬 Code graph analysis (10)
bindings/src/types/wallet-daemon-client/StealthUtxosDecryptValueResponse.ts (1)
bindings/src/types/UtxoId.ts (1)
UtxoId(3-3)
bindings/src/types/wallet-daemon-client/StealthUtxosDecryptValueRequest.ts (2)
bindings/src/types/ResourceAddress.ts (1)
ResourceAddress(6-6)bindings/src/types/UtxoId.ts (1)
UtxoId(3-3)
crates/wallet/sdk/src/apis/viewable_balance.rs (2)
bindings/src/types/PrivateOutput.ts (1)
PrivateOutput(6-11)crates/engine_types/src/crypto/elgamal.rs (1)
batched_brute_force(188-232)
clients/wallet_daemon_client/src/types.rs (4)
bindings/src/types/ResourceAddress.ts (1)
ResourceAddress(6-6)bindings/src/types/UtxoId.ts (1)
UtxoId(3-3)bindings/src/types/wallet-daemon-client/StealthUtxosDecryptValueRequest.ts (1)
StealthUtxosDecryptValueRequest(5-11)bindings/src/types/wallet-daemon-client/StealthUtxosDecryptValueResponse.ts (1)
StealthUtxosDecryptValueResponse(4-4)
clients/wallet_daemon_client/src/lib.rs (2)
bindings/src/types/wallet-daemon-client/StealthUtxosDecryptValueRequest.ts (1)
StealthUtxosDecryptValueRequest(5-11)bindings/src/types/wallet-daemon-client/StealthUtxosDecryptValueResponse.ts (1)
StealthUtxosDecryptValueResponse(4-4)
crates/template_lib/src/models/utxo.rs (3)
bindings/src/types/UtxoAddressContents.ts (1)
UtxoAddressContents(8-8)bindings/src/types/ResourceAddress.ts (1)
ResourceAddress(6-6)bindings/src/types/UtxoId.ts (1)
UtxoId(3-3)
applications/tari_walletd/src/handlers/stealth_utxos.rs (4)
bindings/src/types/wallet-daemon-client/StealthUtxosDecryptValueRequest.ts (1)
StealthUtxosDecryptValueRequest(5-11)bindings/src/types/wallet-daemon-client/StealthUtxosDecryptValueResponse.ts (1)
StealthUtxosDecryptValueResponse(4-4)crates/template_lib/src/models/utxo.rs (2)
id(35-37)new(27-29)applications/tari_walletd/src/handlers/context.rs (1)
new(35-53)
applications/tari_walletd/src/jrpc_server.rs (2)
applications/tari_swarm_daemon/src/webserver/server.rs (1)
call_handler(154-178)applications/tari_walletd/src/handlers/stealth_utxos.rs (2)
handle_list(29-55)handle_decrypt_value(57-134)
clients/javascript/wallet_daemon_client/src/index.ts (2)
bindings/src/types/wallet-daemon-client/StealthUtxosDecryptValueRequest.ts (1)
StealthUtxosDecryptValueRequest(5-11)bindings/src/types/wallet-daemon-client/StealthUtxosDecryptValueResponse.ts (1)
StealthUtxosDecryptValueResponse(4-4)
crates/wallet/sdk/src/apis/stealth_crypto.rs (1)
crates/engine_types/src/crypto/helpers.rs (1)
get_commitment_factory(50-52)
⏰ 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 nightly
- GitHub Check: clippy
- GitHub Check: test
- GitHub Check: check stable
- GitHub Check: machete
🔇 Additional comments (16)
clients/javascript/wallet_daemon_client/package.json (1)
3-3: Version bump looks goodIncrementing to 1.9.2 matches the new RPC surface you added for stealth UTXO decryption and keeps the published client aligned with the new API.
crates/wallet/sdk/src/apis/mod.rs (1)
18-18: Expose ViewableBalance API module — LGTMModule exposure is clear and consistent with SDK accessors added elsewhere.
crates/wallet/sdk/src/apis/stealth_crypto.rs (1)
9-9: Confirm ConvertFromByteType import pathEnsure
ConvertFromByteTypeis re-exported fromtari_engine_typesfor the Ristretto types; if not, import it from the crate that actually defines the trait impls to avoid trait not in scope errors.crates/template_lib/src/models/utxo.rs (1)
39-41: into_contents addition — LGTMConsuming extractor is useful and avoids clones. Call sites (e.g., walletd handlers) benefit directly.
applications/tari_walletd/src/jrpc_server.rs (1)
185-189: Add stealth_utxos.decrypt_value route — LGTMRouting cleanly wires to the new handler; default not-found fallback preserved.
Please ensure this method is included in any RPC discovery/metadata exposed to clients so it shows up in feature introspection.
crates/wallet/sdk/src/sdk.rs (1)
39-39: New API import — LGTMImport aligns with added accessor.
bindings/src/wallet-daemon-client.ts (1)
30-30: LGTM: new public type exports wiredRe-exports for StealthUtxosDecryptValueRequest/Response are correctly added and consistent with existing patterns.
Also applies to: 112-112
clients/javascript/wallet_daemon_client/src/index.ts (1)
353-355: LGTM: new RPC method added and correctly routedMethod name matches server route ("stealth_utxos.decrypt_value") and follows existing list pattern.
Ensure callers pass BigInt-compatible params and that the transport serializes BigInt safely (see related comments on bindings types).
applications/tari_walletd/src/handlers/confidential.rs (1)
297-305: LGTM: switch to viewable_balance_api for brute-force balance lookupThe API swap is appropriate and keeps the heavy work inside block_in_place. No functional regressions apparent.
Please confirm try_brute_force_commitment_balances from viewable_balance_api preserves previous semantics (output ordering/length matching the commitments iterator) to keep the keys().zip(balances) mapping correct.
bindings/src/types/wallet-daemon-client/StealthUtxosDecryptValueResponse.ts (1)
4-4: Use Partial<Record<UtxoId, bigint | null>> for balances and add BigInt-safe JSON handlingbindings/src/types/wallet-daemon-client/StealthUtxosDecryptValueResponse.ts:
-export type StealthUtxosDecryptValueResponse = { balances: { [key in UtxoId]?: bigint | null } }; +export type StealthUtxosDecryptValueResponse = { balances: Partial<Record<UtxoId, bigint | null>> };No BigInt-safe JSON transport found—verify JSON.stringify/parse uses a replacer/reviver for BigInt values.
clients/wallet_daemon_client/src/lib.rs (2)
117-119: Re-exports look correct.New request/response types are properly wired into the client’s type surface.
484-489: Confirm RPC route registration and bindings
- No occurrences of
stealth_utxos.decrypt_valuefound inapplications/tari_walletd/src/jrpc_server.rs,handlers/stealth_utxos.rs, or in the TS bindings.- Add the JSON-RPC method registration in
jrpc_server.rs, implement the handler inhandlers/stealth_utxos.rs, and expose it in the bindings.clients/wallet_daemon_client/src/types.rs (1)
55-63: Import additions are appropriate.UtxoId and VaultId exposure matches the new request/response structs below.
crates/wallet/sdk/src/apis/viewable_balance.rs (1)
51-57: Error type design LGTM.applications/tari_walletd/src/handlers/stealth_utxos.rs (2)
81-85: Permissions: Admin may be overly broad. Verify intended scope.If decrypting with a view key is permissible for users with AccountList or a narrower permission, consider reducing to a more specific permission.
What role should be allowed to decrypt with view keys? If Admin is correct, ignore.
131-133: Potential misalignment between keys and balances if any output has no viewable_balance.Today, API filters those out, shortening results. After applying the ViewableBalanceApi fix (returning a result per input), this zip remains correct. If not fixing API, pre-filter outputs and include explicit None entries for missing balances.
I recommend the API fix in crates/wallet/sdk/src/apis/viewable_balance.rs to guarantee positional alignment.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
applications/tari_walletd/src/handlers/confidential.rs (1)
307-313: Guard against pathological brute force without a lookup table.With AlwaysMissLookupTable and wide ranges (defaults up to 10B), this path can be prohibitively slow. Add a range-size guard.
- None => block_in_place(|| { - sdk.viewable_balance_api().try_brute_force_commitment_balances( - &view_key.key, - commitments.values().filter_map(|o| o.viewable_balance.as_ref()), - value_range, - &mut AlwaysMissLookupTable, - ) - })?, + None => { + // Prevent runaway work without a lookup table. + const MAX_RANGE_WITHOUT_LOOKUP: u64 = 1_000_000; + let span = (*value_range.end() - *value_range.start()).saturating_add(1); + if span > MAX_RANGE_WITHOUT_LOOKUP { + return Err(invalid_params( + "maximum_expected_value", + Some("range too large without a value lookup table; provide config.value_lookup_table_file or reduce the range"), + )); + } + block_in_place(|| { + sdk.viewable_balance_api().try_brute_force_commitment_balances( + &view_key.key, + commitments.values().filter_map(|o| o.viewable_balance.as_ref()), + value_range, + &mut AlwaysMissLookupTable, + ) + })? + },
🧹 Nitpick comments (2)
crates/wallet/sdk/src/apis/viewable_balance.rs (2)
28-36: Add index to error for precise diagnostics.When a proof fails to decompress, the error lacks location context. Enumerate and include the index in the error details.
- let outputs_viewable_balance_decompressed = proofs - .map(ElgamalVerifiableBalance::convert_from_byte_type) - .collect::<Result<Vec<_>, _>>() - .map_err(|_| WalletCryptoError::InvalidArgument { - name: "proofs", - details: "Malformed viewable balance in output when decompressing ElgamalVerifiableBalance for brute \ - forcing" - .to_string(), - })?; + let outputs_viewable_balance_decompressed = proofs + .enumerate() + .map(|(i, vb)| { + ElgamalVerifiableBalance::convert_from_byte_type(vb).map_err(|_| { + WalletCryptoError::InvalidArgument { + name: "proofs", + details: format!( + "Malformed viewable balance at index {} when decompressing ElgamalVerifiableBalance", + i + ), + } + }) + }) + .collect::<Result<Vec<_>, _>>()?;
16-27: Generalize iterator bounds for better ergonomics.Accept IntoIterator for proofs and value_range to support slices, Vec, and ranges uniformly.
- pub fn try_brute_force_commitment_balances<'a, TLookup, TProofsIter>( + pub fn try_brute_force_commitment_balances<'a, TLookup, TProofsIter, IRange>( &self, secret_view_key: &RistrettoSecretKey, - proofs: TProofsIter, - value_range: RangeInclusive<u64>, + proofs: TProofsIter, + value_range: IRange, lookup: &mut TLookup, ) -> Result<Vec<Option<u64>>, ViewableBalanceApiError> where TLookup: ValueLookupTable, - TProofsIter: Iterator<Item = &'a ElgamalVerifiableBalanceBytes>, + TProofsIter: IntoIterator<Item = &'a ElgamalVerifiableBalanceBytes>, + IRange: IntoIterator<Item = u64>,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
applications/tari_walletd/src/handlers/confidential.rs(2 hunks)applications/tari_walletd/src/handlers/stealth_utxos.rs(2 hunks)crates/wallet/sdk/src/apis/viewable_balance.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
crates/wallet/sdk/src/apis/viewable_balance.rs (2)
bindings/src/types/ElgamalVerifiableBalanceBytes.ts (1)
ElgamalVerifiableBalanceBytes(4-7)crates/engine_types/src/crypto/elgamal.rs (1)
batched_brute_force(188-232)
applications/tari_walletd/src/handlers/confidential.rs (2)
crates/engine_types/src/resource.rs (1)
view_key(126-128)crates/engine_types/src/resource_container.rs (1)
commitments(520-533)
applications/tari_walletd/src/handlers/stealth_utxos.rs (5)
bindings/src/types/wallet-daemon-client/StealthUtxosDecryptValueRequest.ts (1)
StealthUtxosDecryptValueRequest(5-11)bindings/src/types/wallet-daemon-client/StealthUtxosDecryptValueResponse.ts (1)
StealthUtxosDecryptValueResponse(4-4)crates/template_lib/src/models/utxo.rs (2)
id(35-37)new(27-29)applications/tari_walletd/src/handlers/context.rs (1)
new(35-53)crates/wallet/sdk/src/apis/viewable_balance.rs (1)
proofs(28-30)
⏰ 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: check nightly
- GitHub Check: check stable
- GitHub Check: machete
- GitHub Check: clippy
- GitHub Check: fmt
🔇 Additional comments (3)
applications/tari_walletd/src/handlers/stealth_utxos.rs (1)
86-87: Still missing an upper bound on the brute-force window.With
AlwaysMissLookupTablethis will iterate up to 10 000 000 000 commitments per UTXO, so a single request can DoS the daemon again. Please clamp the min/max span and reject ranges that exceed it.- let value_range = req.minimum_expected_value.unwrap_or(0)..=req.maximum_expected_value.unwrap_or(10_000_000_000); + const MAX_BRUTE_FORCE_SPAN: u64 = 1_000_000; + let min = req.minimum_expected_value.unwrap_or(0); + let max = req + .maximum_expected_value + .unwrap_or(min.saturating_add(MAX_BRUTE_FORCE_SPAN.saturating_sub(1))); + if max < min { + return Err(invalid_params( + "maximum_expected_value", + Some("maximum_expected_value must be >= minimum_expected_value"), + )); + } + let span = max.saturating_sub(min) + 1; + if span > MAX_BRUTE_FORCE_SPAN { + return Err(invalid_params( + "maximum_expected_value", + Some("value range too wide; supply narrower bounds"), + )); + } + let value_range = min..=max;crates/wallet/sdk/src/apis/viewable_balance.rs (1)
38-46: LGTM: correct use of batched_brute_force and error mapping.The call preserves input ordering and propagates lookup errors cleanly.
applications/tari_walletd/src/handlers/confidential.rs (1)
298-304: Confirm BTreeMap preserves iteration order:commitmentsis astd::collections::BTreeMap, so itsvalues()anditer()methods yield entries in the same (sorted‐by‐key) order.
| balances: commitments | ||
| .iter() | ||
| .filter_map(|(id, o)| o.viewable_balance.as_ref().map(|_| *id)) | ||
| .zip(balances) | ||
| .collect(), | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
Confirm API semantics: dropping non-viewable outputs from response.
This returns balances only for outputs that have a viewable_balance. If clients expect a full list with Nones for non-viewable items, this could be a breaking behavioral change.
🏁 Script executed:
#!/bin/bash
# Inspect response type to confirm expected semantics
rg -n -C3 'struct\s+ConfidentialViewVaultBalanceResponse|type\s+ConfidentialViewVaultBalanceResponse' --type=rustLength of output: 659
🏁 Script executed:
#!/bin/bash
# Locate handler function and context around the balances construction
rg -n 'ConfidentialViewVaultBalanceResponse' -C8 applications/tari_walletd/src/handlers/confidential.rs
rg -n 'view_vault_balance' -C5 applications/tari_walletd/src/handlers/confidential.rsLength of output: 2132
Include non-viewable outputs as null in response.
Handler currently drops outputs without a viewable_balance, but ConfidentialViewVaultBalanceResponse.balances: HashMap<PedersenCommitmentBytes, Option<u64>> expects keys with None for non-viewable items. In applications/tari_walletd/src/handlers/confidential.rs:319–324, replace the filter_map(...).zip(balances) logic with an iterator over all commitments, mapping each to Some(balance) or None accordingly.
🤖 Prompt for AI Agents
In applications/tari_walletd/src/handlers/confidential.rs around lines 319-324,
the current code filters out non-viewable outputs and zips only viewable
commitments with balances; instead iterate over all commitments and build the
balances map so each commitment key is present with Some(balance) for viewable
outputs and None for non-viewable ones. Implement this by creating a mutable
iterator over the balances vector and then mapping over commitments: for each
(id, output) if output.viewable_balance.is_some() take the next balance from the
balances iterator and assign Some(balance), otherwise assign None; collect that
into the HashMap<PedersenCommitmentBytes, Option<u64>>.
2 participants
Description
feat(wallet): add rpc call to allow decrypting utxo values with view keys enabled
Motivation and Context
'stealth_utxos.decrypt_value' method
How Has This Been Tested?
Not tested - code moved from previously working confidential vault decryption
What process can a PR reviewer use to test or verify this change?
Breaking Changes
Summary by CodeRabbit
New Features
Refactor
Chores
Security