feat: update base node proto to search bytes#7201
feat: update base node proto to search bytes#7201SWvheerden merged 1 commit intotari-project:developmentfrom
Conversation
WalkthroughA new repeated Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant gRPC_Server
participant NodeService
Client->>gRPC_Server: SearchPaymentReferencesRequest (hex and/or bytes)
gRPC_Server->>gRPC_Server: Validate and convert all payment references
loop For each valid payment reference
gRPC_Server->>NodeService: get_output_for_payment_reference(payref)
NodeService-->>gRPC_Server: Output info or error
gRPC_Server->>gRPC_Server: Build response with payref as hex
end
gRPC_Server-->>Client: Stream responses for each found output
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
applications/minotari_node/src/grpc/base_node_grpc_server.rs (2)
2965-2994: 🛠️ Refactor suggestionUse
HashSet<FixedHash>to pre-deduplicate payment references
payrefsis built as aVecby first walkingpayment_reference_hexand thenpayment_reference_bytes.
If a caller supplies the same pay-ref in both formats (or duplicates inside one format) the node will:
- Waste time looking the same output up multiple times.
- Emit duplicate
PaymentReferenceResponses.A
HashSetremoves both issues with negligible cost and conveys intent.-use std::collections::HashSet; // add at top of file - -let mut payrefs = Vec::new(); +use std::collections::HashSet; + +let mut payrefs: HashSet<FixedHash> = HashSet::new(); ... - payrefs.push(payref_bytes); + payrefs.insert(payref_bytes); ... - payrefs.push(payref_fixed_hash); + payrefs.insert(payref_fixed_hash); ... -for payref in payrefs { +for payref in payrefs.into_iter() {This keeps the public API unchanged, eliminates duplicates, and slightly improves throughput.
3025-3034: 💡 Verification agent🧩 Analysis chain
include_spentflag in proto is ignored
SearchPaymentReferencesRequestexposes aninclude_spentboolean, yet the server always performs a spent-status check and returns it.
Respecting the flag can avoid an expensive DB lookup when the client does not care.if request.include_spent { // existing spent check } else { is_spent = false; spent_height = 0; spent_block_hash = vec![]; }Please verify the intended semantics; ignoring the field now risks confusing API consumers.
🏁 Script executed:
#!/bin/bash # Show the handler signature and its surrounding context rg -n "SearchPaymentReferencesRequest" -C5 applications/minotari_node/src/grpc/base_node_grpc_server.rs # Check for any use of the include_spent flag rg -n "include_spent" -C5 applications/minotari_node/src/grpc/base_node_grpc_server.rs # Inspect how is_spent, spent_height, and spent_block_hash are set rg -n "is_spent" -C5 applications/minotari_node/src/grpc/base_node_grpc_server.rs rg -n "spent_height" -C5 applications/minotari_node/src/grpc/base_node_grpc_server.rsLength of output: 3849
Respect
include_spentflag to avoid unnecessary spent‐status lookupsThe handler for
SearchPaymentReferencesRequestalways callsnode_service.check_output_spent_status(output_hash).awaitregardless of
request.include_spent, which defeats the purpose of the flag and adds an expensive database query when the client doesn’t need spent status.Suggested change in
applications/minotari_node/src/grpc/base_node_grpc_server.rs(around lines 3016–3023):- let (is_spent, spent_height, spent_block_hash) = match node_service - .check_output_spent_status(output_hash) - .await - { - Ok(Some(input_info)) => (true, input_info.spent_height, input_info.header_hash.to_vec()), - Ok(None) | Err(_) => (false, 0, vec![]), - }; + let (is_spent, spent_height, spent_block_hash) = if request.include_spent { + match node_service.check_output_spent_status(output_hash).await { + Ok(Some(input_info)) => (true, input_info.spent_height, input_info.header_hash.to_vec()), + Ok(None) | Err(_) => (false, 0, vec![]), + } + } else { + // Skip spent lookup when client does not request it + (false, 0, vec![]) + };Please confirm that this aligns with the intended API semantics.
🧹 Nitpick comments (1)
applications/minotari_node/src/grpc/base_node_grpc_server.rs (1)
2995-3010: Avoid cloning large byte vectors unnecessarily
FixedHash::try_from(payref_bytes.clone())incurs an extra allocation for each element.
try_from(&payref_bytes[..])(or iterating by value and callinginto()) achieves the same goal without the clone.-let payref_fixed_hash = match FixedHash::try_from(payref_bytes.clone()) { +let payref_fixed_hash = match FixedHash::try_from(&payref_bytes[..]) {Minor, but worth it if thousands of references are streamed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
applications/minotari_app_grpc/proto/base_node.proto(1 hunks)applications/minotari_node/src/grpc/base_node_grpc_server.rs(4 hunks)common/config/presets/c_base_node_b_mining_allow_methods.toml(1 hunks)common/config/presets/c_base_node_b_non_mining_allow_methods.toml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: cargo check with stable
- GitHub Check: ci
- GitHub Check: Cucumber tests / FFI
- GitHub Check: Cucumber tests / Base Layer
🔇 Additional comments (2)
common/config/presets/c_base_node_b_non_mining_allow_methods.toml (1)
54-54: New gRPC method entry added (commented).Consistent with other commented methods in the non-mining preset. Ensure to document this new option in the configuration guide if non-mining nodes plan to enable it in the future.
common/config/presets/c_base_node_b_mining_allow_methods.toml (1)
54-54: Enabled new gRPC methodsearch_payment_references.This addition aligns with the updated server implementation and mining preset usage.
* development: (607 commits) Wallet GRPC port comment fix from 18142 to 18143 (tari-project#7221) feat: integrated address support for Ledger (tari-project#7198) chore: new release v4.1.1-pre.0 (tari-project#7211) fix: migration can now correctly resume after stopping (tari-project#7210) fix: only revalidated rejected transactions on startup (tari-project#7209) fix: add filtering flag back (tari-project#7208) feat: improve wallet balance checks from external clients (tari-project#7207) feat!: update grpc supply query (tari-project#7137) docs: Updated API GRPC and Exchange Guide (tari-project#7205) chore: new release v4.4.0-pre.0 (tari-project#7202) feat: update base node proto to search bytes (tari-project#7201) feat: full PayRef implementation (tari-project#7154) test: add ffi cucumber wallet balance test (tari-project#7189) chore: fix tests (tari-project#7196) fix(network-discovery): add back idle event handling (tari-project#7194) Update SECURITY.md (tari-project#7193) fix: transaction manager service unmined lookup (tari-project#7192) fix: wallet ffi database name mismatch for mobile wallet (tari-project#7191) fix: payment_id deserialize (tari-project#7187) fix: remove code for deleting stale peers (tari-project#7184) ...
1 participant
Description
updates base node proto to allow search via bytes and hex
update config settings to allow search method
Summary by CodeRabbit