Skip to content

fix!: payref migration and indexes, add grpc query via output hash#7266

Merged
SWvheerden merged 15 commits intotari-project:developmentfrom
hansieodendaal:ho_outputs_by_hash
Jul 1, 2025
Merged

fix!: payref migration and indexes, add grpc query via output hash#7266
SWvheerden merged 15 commits intotari-project:developmentfrom
hansieodendaal:ho_outputs_by_hash

Conversation

@hansieodendaal
Copy link
Copy Markdown
Contributor

@hansieodendaal hansieodendaal commented Jun 26, 2025

Description

  • Added gRPC method to retrieve output information via output hash
  • Updated the gRPC payref search to return spent and unspent output information
  • Added migration to rebuild payref indexes due to missing payrefs (added periodic call to LMDBStore::resize_if_required)
  • Fixed error in lmdb migration counter where it recorded a higher version done than what it should

Fixes #7263

Motivation and Context

  • Payref information was deleted when the outputs were spent.
  • With migrations, payref information was only created for the current output set, not all outputs.
  • Payref lookups for spent outputs were not possible.
  • Output and payref information was not possible via output hash.

How Has This Been Tested?

  • New lmdb migration tested at the system level for fresh and existing base nodes.
  • New gRPC method system-level testing [tested with block explorer upgrade].
  • General system-level testing.

Fresh base node with a restart afterwards

2025-06-27 09:09:24.013840800 [c::cs::lmdb_db::lmdb_db] INFO  [MIGRATIONS] Blockchain database is at v0 (required version: 5)
2025-06-27 09:09:24.013873800 [c::cs::lmdb_db::lmdb_db] INFO  [MIGRATIONS] v1: No accumulated difficulty found for block height 14999
2025-06-27 09:09:24.013877800 [c::cs::lmdb_db::lmdb_db] INFO  [MIGRATIONS] v1: No migration to perform for version network
2025-06-27 09:09:24.013883100 [c::cs::lmdb_db::lmdb_db] INFO  [MIGRATIONS] v2: Starting PayRef migration
2025-06-27 09:09:24.013899700 [c::cs::lmdb_db::lmdb_db] INFO  [MIGRATIONS] v2: Cleared PayRef index
2025-06-27 09:09:24.038671500 [c::cs::lmdb_db::lmdb_db] INFO  [MIGRATIONS] v2: PayRef index rebuild completed
2025-06-27 09:09:24.038693200 [c::cs::lmdb_db::lmdb_db] INFO  [MIGRATIONS] Migrated database from version 2 to version 3
2025-06-27 09:09:24.039646900 [c::cs::lmdb_db::lmdb_db] INFO  [MIGRATIONS] Migrated database from version 3 to version 4
2025-06-27 09:09:24.040506700 [c::cs::lmdb_db::lmdb_db] INFO  [MIGRATIONS] Migrated database from version 4 to version 5
2025-06-27 09:10:24.526178200 [c::cs::lmdb_db::lmdb_db] INFO  [MIGRATIONS] Blockchain database is at v5 (required version: 5)

What process can a PR reviewer use to test or verify this change?

  • Code review.
  • System-level testing.

Breaking Changes

  • None
  • Requires data directory on base node to be deleted
  • Requires hard fork
  • Other - Please specify

BREAKING CHANGE:

  • A new base node gRPC method SearchPaymentReferencesViaOutputHash was added that returns a stream of PaymentReferenceResponse.
  • The gRPC PaymentReferenceResponse was also updated to return the spent timestamp and output hash additionally.

Summary by CodeRabbit

Summary by CodeRabbit

  • New Features

    • Added a new gRPC method to search payment references by output hash, providing detailed mined and spent information for outputs.
    • Expanded response data to include spent timestamps and output hashes for enhanced transaction tracing.
  • Refactor

    • Unified and extended backend logic for fetching mined information by payment reference or output hash.
    • Improved migration and index handling for payment references to ensure data consistency, including genesis block coverage.
  • Documentation

    • Clarified field comments and updated configuration files to reflect new gRPC capabilities.
  • Style

    • Updated test and code comments for clarity and consistency.

- Added payref reverse lookup lmdb table
- Added migration to rebuild payref indexes and to add reverse lookup lmdb table
- Fixed error in lmdb migration counter
- Added gRPC method to retrieve output information via output hash
- Updated the gRPC payref search to return spent and unspent output information
@hansieodendaal hansieodendaal requested a review from a team as a code owner June 26, 2025 12:31
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Jun 26, 2025

Walkthrough

This change adds a new gRPC streaming method SearchPaymentReferencesViaOutputHash to query payment references by output hash. It refactors internal request and response enums, local comms interfaces, and blockchain storage backends to support mined info retrieval by both payment reference and output hash. The PayRef index migration and reorg logic are improved for correctness and completeness. Configuration and tests are updated accordingly.

Changes

File(s) Change Summary
applications/minotari_app_grpc/proto/base_node.proto Added SearchPaymentReferencesViaOutputHash RPC, extended PaymentReferenceResponse with spent_timestamp and output_hash, clarified comments.
applications/minotari_node/src/grpc/base_node_grpc_server.rs Added streaming gRPC method for searching payment references via output hash; refactored existing method to use unified mined info; updated trait associated types.
applications/minotari_node/src/grpc/readiness_grpc_server.rs Added stub implementation of new gRPC method returning unavailable status.
applications/minotari_node/src/grpc_method.rs Added new enum variant SearchPaymentReferencesViaOutputHash with FromStr and iterator updates.
base_layer/core/src/base_node/comms_interface/comms_request.rs Replaced single FetchOutputByPayRef variant with three new variants for mined info requests by payref and output hash.
base_layer/core/src/base_node/comms_interface/comms_response.rs Added new response variants MinedInfo and PayRef; updated Display trait accordingly.
base_layer/core/src/base_node/comms_interface/inbound_handlers.rs Extended request handling to support new mined info request variants and responses.
base_layer/core/src/base_node/comms_interface/local_interface.rs Renamed and added methods for fetching mined info by payref and output hash; updated signatures and error handling.
base_layer/core/src/chain_storage/async_db.rs Removed old fetch by payref method; added async methods for fetching mined info by payref and output hash.
base_layer/core/src/chain_storage/blockchain_backend.rs Replaced single fetch method with two methods for mined info by payref and output hash; updated docs and imports.
base_layer/core/src/chain_storage/blockchain_database.rs Updated method to fetch mined info by payref; added method to fetch mined info by output hash; adjusted locking and documentation.
base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs Overhauled PayRef handling: unified fetching methods returning MinedInfo, fixed migration logic to run multiple versions with replace semantics, improved reorg cleanup and logging, removed explicit PayRef deletion on input insertion, added new methods.
base_layer/core/src/chain_storage/utxo_mined_info.rs Added new MinedInfo struct encapsulating optional input and output mined info.
base_layer/core/src/test_helpers/blockchain.rs Updated test backend to implement new mined info fetch methods replacing old output-by-payref method.
integration_tests/tests/features/WalletTransactions.feature Updated terminology from "mining node MINER" to "SHA3X mining node MINER" in multiple scenarios.
common/config/presets/c_base_node_b_mining_allow_methods.toml Added "search_payment_references_via_output_hash" to allowed gRPC methods list.
common/config/presets/c_base_node_b_non_mining_allow_methods.toml Added commented entry for "search_payment_references_via_output_hash" to allowed methods list.
base_layer/core/src/chain_storage/lmdb_db/lmdb.rs Added clarifying comment on LMDB replace semantics in lmdb_replace function.
base_layer/core/tests/chain_storage_tests/chain_storage.rs Commented out test logger initialization in test_rewind_to_height.
base_layer/core/tests/tests/block_sync.rs Updated commented logger initialization to use trace level.
base_layer/core/tests/tests/block_validation.rs Commented out logger initialization in inputs_are_not_malleable test.
comms/dht/src/connectivity/test.rs Updated comment to change log output filename extension from .txt to .log.
infrastructure/storage/src/lmdb_store/store.rs Changed argument passing in resize_if_required to pass None directly instead of wrapping in Some.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant GRPCServer
    participant LocalNodeComms
    participant BlockchainDB

    Client->>GRPCServer: SearchPaymentReferencesViaOutputHash(request: output_hashes)
    GRPCServer->>LocalNodeComms: fetch_mined_info_by_output_hash(output_hash)
    LocalNodeComms->>BlockchainDB: fetch_mined_info_by_output_hash(output_hash)
    BlockchainDB-->>LocalNodeComms: MinedInfo (input/output details)
    LocalNodeComms-->>GRPCServer: MinedInfo
    GRPCServer-->>Client: stream PaymentReferenceResponse (with spent_timestamp, output_hash, etc.)
Loading

Assessment against linked issues

Objective Addressed Explanation
Prevent loss of outputs after they are spent by ensuring payrefs are not deleted (#7263)

Assessment against linked issues: Out-of-scope changes

No out-of-scope changes detected. All code modifications align with the objectives from the linked issues.

Possibly related PRs

Suggested reviewers

  • SWvheerden

Poem

In the warren of hashes and blocks so bright,
A rabbit found references hidden from sight.
Now with output and payref, the search is a breeze,
Migrations are tidy, the index at ease.
Hop, hop, hooray!—the chain’s future is light!
🐇✨


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3bf5457 and 712432b.

📒 Files selected for processing (1)
  • applications/minotari_app_grpc/proto/base_node.proto (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • applications/minotari_app_grpc/proto/base_node.proto
⏰ Context from checks skipped due to timeout of 90000ms (7)
  • GitHub Check: Cucumber tests / Base Layer
  • GitHub Check: Cucumber tests / FFI
  • GitHub Check: ci
  • GitHub Check: test (mainnet, stagenet)
  • GitHub Check: test (nextnet, nextnet)
  • GitHub Check: test (testnet, esmeralda)
  • GitHub Check: cargo check with stable

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need 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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

🧹 Nitpick comments (1)
applications/minotari_node/src/grpc/readiness_grpc_server.rs (1)

97-97: Remove unused stream type definition.

SearchTxosByOutputHashStream is defined but never used. The method search_txos_by_output_hash returns SearchPaymentReferencesStream instead, which is correct since it streams PaymentReferenceResponse messages.

-    type SearchTxosByOutputHashStream = mpsc::Receiver<Result<tari_rpc::PaymentReferenceResponse, Status>>;
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d3676ef and febd053.

📒 Files selected for processing (7)
  • applications/minotari_app_grpc/proto/base_node.proto (1 hunks)
  • applications/minotari_node/src/grpc/base_node_grpc_server.rs (2 hunks)
  • applications/minotari_node/src/grpc/readiness_grpc_server.rs (2 hunks)
  • base_layer/core/src/base_node/comms_interface/comms_request.rs (1 hunks)
  • base_layer/core/src/base_node/comms_interface/inbound_handlers.rs (1 hunks)
  • base_layer/core/src/base_node/comms_interface/local_interface.rs (1 hunks)
  • integration_tests/tests/features/WalletTransactions.feature (1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: hansieodendaal
PR: tari-project/tari#6963
File: comms/core/src/peer_manager/manager.rs:60-68
Timestamp: 2025-05-26T02:40:23.812Z
Learning: PeerDatabaseSql in the Tari codebase has been specifically refactored to handle concurrent access and mitigate blocking I/O concerns on async executor threads. The implementation has been tested under high load at both system level and through unit tests like test_concurrent_add_or_update_and_get_closest_peers which validates concurrent read/write operations.
applications/minotari_app_grpc/proto/base_node.proto (1)
Learnt from: SolfataraEmit
PR: tari-project/tari#6994
File: docs/src/API_GRPC_Explanation.md:236-244
Timestamp: 2025-04-28T11:41:15.722Z
Learning: The minotari_console_wallet reports a fourth field `timelocked_balance` in the terminal output for the `getBalance` method, which should be reflected in the documentation for accuracy. The `wallet.proto` file confirms that `timelocked_balance` is defined as the fourth field in the `GetBalanceResponse` message.
applications/minotari_node/src/grpc/readiness_grpc_server.rs (1)
Learnt from: ksrichard
PR: tari-project/tari#7129
File: applications/minotari_node/src/http/handler/sync_utxos_by_block.rs:21-29
Timestamp: 2025-05-29T20:59:32.020Z
Learning: In the Tari codebase, request validation for RPC services is handled at the service layer using the `serde_valid` crate rather than at the HTTP handler level. For example, `SyncUtxosByBlockRequest` validates that limit is between 1-5 and page is minimum 0. HTTP handlers should delegate validation to the service layer rather than implementing their own validation.
applications/minotari_node/src/grpc/base_node_grpc_server.rs (3)
Learnt from: SWvheerden
PR: tari-project/tari#6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
Learnt from: SWvheerden
PR: tari-project/tari#6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
Learnt from: ksrichard
PR: tari-project/tari#7129
File: applications/minotari_node/src/http/handler/sync_utxos_by_block.rs:21-29
Timestamp: 2025-05-29T20:59:32.020Z
Learning: In the Tari codebase, request validation for RPC services is handled at the service layer using the `serde_valid` crate rather than at the HTTP handler level. For example, `SyncUtxosByBlockRequest` validates that limit is between 1-5 and page is minimum 0. HTTP handlers should delegate validation to the service layer rather than implementing their own validation.
🧬 Code Graph Analysis (3)
base_layer/core/src/base_node/comms_interface/inbound_handlers.rs (1)
base_layer/core/src/transactions/transaction_components/transaction_input.rs (1)
  • output_hash (434-462)
base_layer/core/src/base_node/comms_interface/local_interface.rs (1)
base_layer/core/src/transactions/transaction_components/transaction_input.rs (1)
  • output_hash (434-462)
applications/minotari_node/src/grpc/readiness_grpc_server.rs (1)
applications/minotari_node/src/grpc/base_node_grpc_server.rs (1)
  • search_txos_by_output_hash (2956-3051)
⏰ Context from checks skipped due to timeout of 90000ms (7)
  • GitHub Check: Cucumber tests / FFI
  • GitHub Check: test (testnet, esmeralda)
  • GitHub Check: test (mainnet, stagenet)
  • GitHub Check: Cucumber tests / Base Layer
  • GitHub Check: test (nextnet, nextnet)
  • GitHub Check: cargo check with stable
  • GitHub Check: ci
🔇 Additional comments (6)
base_layer/core/src/base_node/comms_interface/comms_request.rs (1)

88-88: LGTM! Clean addition of new request variant.

The new FetchOutputMinedInfo(HashOutput) variant follows established patterns and maintains consistency with similar request types like FetchOutputByPayRef.

integration_tests/tests/features/WalletTransactions.feature (1)

188-188: Good clarification in test descriptions.

Specifying "SHA3X mining node" instead of generic "mining node" improves test documentation clarity and aligns with the mining algorithm specificity introduced in this PR.

applications/minotari_app_grpc/proto/base_node.proto (1)

105-106: Well-designed new RPC method for reverse PayRef lookup.

The SearchTxosByOutputHash method appropriately reuses existing message types (FetchMatchingUtxosRequest and PaymentReferenceResponse) and follows established patterns. The streaming response is consistent with similar lookup methods.

base_layer/core/src/base_node/comms_interface/inbound_handlers.rs (1)

461-464: Excellent implementation following established patterns.

The new FetchOutputMinedInfo handler is perfectly consistent with existing patterns, particularly mirroring the FetchOutputByPayRef handler above. The use of blockchain_db.fetch_output() is appropriate for retrieving output information by hash.

base_layer/core/src/base_node/comms_interface/local_interface.rs (1)

376-388: LGTM: Method implementation is correct.

The method correctly follows the established pattern for communication interface methods and properly handles the request/response flow for fetching output mined information by hash.

applications/minotari_node/src/grpc/readiness_grpc_server.rs (1)

372-377: LGTM: Stub method follows established pattern.

The method correctly implements the readiness server pattern by returning an unavailable status, consistent with other unimplemented RPC methods in this server.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Jun 26, 2025

Test Results (Integration tests)

 2 files  + 2  10 suites  +10   1h 3m 53s ⏱️ + 1h 3m 53s
33 tests +33  28 ✅ +28  2 💤 +2  3 ❌ +3 
39 runs  +39  28 ✅ +28  2 💤 +2  9 ❌ +9 

For more details on these failures, see this check.

Results for commit 712432b. ± Comparison against base commit 73e6c9e.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Jun 26, 2025

Test Results (CI)

    3 files    126 suites   41m 18s ⏱️
1 353 tests 1 353 ✅ 0 💤 0 ❌
4 057 runs  4 057 ✅ 0 💤 0 ❌

Results for commit 712432b.

♻️ This comment has been updated with latest results.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

♻️ Duplicate comments (3)
base_layer/core/src/base_node/comms_interface/local_interface.rs (2)

387-400: Fix the documentation comment.

The comment incorrectly describes this method as fetching "mined info by PayRef" when it actually fetches a payment reference by output hash.

-    /// Fetch mined info by PayRef (Payment Reference)
+    /// Fetch payment reference by output hash

372-385: Fix the incorrect request type and documentation comment.

The method has two critical issues:

  1. Line 379 sends the wrong request type - it should be FetchMinedInfoByOutputHash not FetchMinedInfoByPayRef
  2. The documentation comment incorrectly describes this as fetching by PayRef when it fetches by output hash

Apply this diff to fix both issues:

-    /// Fetch mined info by PayRef (Payment Reference)
+    /// Fetch mined info by output hash
 pub async fn fetch_mined_info_by_output_hash(
     &mut self,
     output_hash: &HashOutput,
 ) -> Result<MinedInfo, CommsInterfaceError> {
     match self
         .request_sender
-        .call(NodeCommsRequest::FetchMinedInfoByPayRef(*output_hash))
+        .call(NodeCommsRequest::FetchMinedInfoByOutputHash(*output_hash))
         .await??
     {
         NodeCommsResponse::MinedInfo(mined_info) => Ok(mined_info),
         _ => Err(CommsInterfaceError::UnexpectedApiResponse),
     }
 }
applications/minotari_node/src/grpc/base_node_grpc_server.rs (1)

3061-3061: Fix incorrect trace log message.

The trace log references the wrong method name "FindMatchingUtxos" instead of the actual method being executed.

-            "Sending FindMatchingUtxos response stream to client"
+            "Sending SearchPaymentReferencesViaOutputHash response stream to client"
🧹 Nitpick comments (5)
applications/minotari_node/src/grpc/base_node_grpc_server.rs (2)

2984-3055: Consider providing complete output information for spent outputs.

For spent outputs, the method only returns spent information (spent_height, spent_block_hash, spent_timestamp) while setting mined fields to 0/empty. This creates an API inconsistency where unspent outputs return complete mined information but spent outputs don't.

Consider fetching the original output mined information for spent outputs to provide a consistent response format, or clearly document this limitation in the API.

// Example approach: fetch original output info for spent outputs
Ok(MinedInfo {
    input: Some(input_info),
    ..
}) => {
    // Try to get original output info if available
    match node_service.fetch_output_mined_info(&output_hash).await {
        Ok(Some(original_output)) => {
            let response = tari_rpc::PaymentReferenceResponse {
                payment_reference_hex: payref.to_hex(),
                block_height: original_output.mined_height,
                block_hash: original_output.header_hash.to_vec(),
                mined_timestamp: original_output.mined_timestamp,
                commitment: original_output.output.commitment.to_vec(),
                is_spent: true,
                spent_height: input_info.spent_height,
                spent_block_hash: input_info.header_hash.to_vec(),
                min_value_promise: original_output.output.minimum_value_promise.as_u64(),
                spent_timestamp: input_info.spent_timestamp,
            };
        },
        _ => {
            // Fallback to spent-only information
            // ... existing logic
        }
    }
}

2990-2994: Consider using stored payment reference for consistency.

The method generates payment references using generate_payment_reference for unspent outputs, but for spent outputs it fetches stored payrefs. For consistency, consider using the same approach for both cases.

-                            payment_reference_hex: generate_payment_reference(
-                                &output_info.header_hash,
-                                &output_info.output.hash(),
-                            )
-                            .to_hex(),
+                            payment_reference_hex: output_info.output.features.payment_reference()
+                                .map(|pr| pr.to_hex())
+                                .unwrap_or_else(|| generate_payment_reference(
+                                    &output_info.header_hash,
+                                    &output_info.output.hash(),
+                                ).to_hex()),
base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs (3)

1106-1118: Consider improving error handling visibility.

The code correctly handles the expected case where PayRefs don't exist for certain outputs. However, silently ignoring ValueNotFound errors might hide issues during debugging.

Consider adding trace-level logging for the expected "not found" case to aid in debugging while keeping the info log clean:

                    Err(ChainStorageError::ValueNotFound { .. }) => {
                        // Expected case for outputs that didn't have PayRefs
+                       trace!(target: LOG_TARGET, "PayRef not found during reorg for output {} (expected for some outputs)", output_hash.to_hex());
                    },

1926-1941: Consider returning an enum instead of struct with optional fields.

The current MinedInfo struct with optional input and output fields where only one is populated at a time could be confusing. Consider using an enum to make the mutual exclusivity explicit.

This would make the API clearer and prevent callers from having to check both fields:

enum MinedInfo {
    Output(OutputMinedInfo),
    Input(InputMinedInfo),
    NotFound,
}

3393-3410: Use consistent insert operations for both indexes.

The code uses lmdb_replace for the forward index but lmdb_insert for the reverse index. For consistency and idempotency, consider using lmdb_replace for both.

        if add_reverse_lookup {
-           lmdb_insert(
+           lmdb_replace(
                &write_txn,
                &db.output_to_payref_index,
                output_hash.as_slice(),
                &payref,
-               "output_to_payref_index",
+               None,
            )?;
        }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between febd053 and 85de473.

📒 Files selected for processing (17)
  • applications/minotari_app_grpc/proto/base_node.proto (2 hunks)
  • applications/minotari_node/src/grpc/base_node_grpc_server.rs (5 hunks)
  • applications/minotari_node/src/grpc/readiness_grpc_server.rs (2 hunks)
  • applications/minotari_node/src/grpc_method.rs (4 hunks)
  • base_layer/core/src/base_node/comms_interface/comms_request.rs (2 hunks)
  • base_layer/core/src/base_node/comms_interface/comms_response.rs (3 hunks)
  • base_layer/core/src/base_node/comms_interface/inbound_handlers.rs (1 hunks)
  • base_layer/core/src/base_node/comms_interface/local_interface.rs (2 hunks)
  • base_layer/core/src/chain_storage/async_db.rs (2 hunks)
  • base_layer/core/src/chain_storage/blockchain_backend.rs (2 hunks)
  • base_layer/core/src/chain_storage/blockchain_database.rs (2 hunks)
  • base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs (20 hunks)
  • base_layer/core/src/chain_storage/utxo_mined_info.rs (1 hunks)
  • base_layer/core/src/test_helpers/blockchain.rs (2 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)
  • integration_tests/tests/features/WalletTransactions.feature (1 hunks)
✅ Files skipped from review due to trivial changes (2)
  • common/config/presets/c_base_node_b_mining_allow_methods.toml
  • common/config/presets/c_base_node_b_non_mining_allow_methods.toml
🚧 Files skipped from review as they are similar to previous changes (4)
  • integration_tests/tests/features/WalletTransactions.feature
  • base_layer/core/src/base_node/comms_interface/comms_request.rs
  • applications/minotari_node/src/grpc/readiness_grpc_server.rs
  • applications/minotari_app_grpc/proto/base_node.proto
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: hansieodendaal
PR: tari-project/tari#6963
File: comms/core/src/peer_manager/manager.rs:60-68
Timestamp: 2025-05-26T02:40:23.812Z
Learning: PeerDatabaseSql in the Tari codebase has been specifically refactored to handle concurrent access and mitigate blocking I/O concerns on async executor threads. The implementation has been tested under high load at both system level and through unit tests like test_concurrent_add_or_update_and_get_closest_peers which validates concurrent read/write operations.
base_layer/core/src/base_node/comms_interface/comms_response.rs (2)
Learnt from: hansieodendaal
PR: tari-project/tari#7216
File: applications/minotari_app_grpc/src/conversions/aggregate_body.rs:31-46
Timestamp: 2025-06-14T04:46:31.966Z
Learning: When a type implements the Copy trait (like FixedHash/BlockHash), it can be used multiple times in iterator closures without being consumed or moved. The compiler automatically copies Copy types, so no explicit .clone() is needed and will trigger clippy::clone_on_copy warnings.
Learnt from: hansieodendaal
PR: tari-project/tari#7216
File: base_layer/core/src/base_node/comms_interface/comms_request.rs:146-148
Timestamp: 2025-06-14T04:56:43.247Z
Learning: In the Tari codebase, `HashOutput` is a type alias for `FixedHash`, and `FixedHash` already implements `Display` trait with hex formatting by calling `self.0.to_hex()` internally. This means when formatting `HashOutput` values, there's no need to explicitly call `.to_hex()` as the Display implementation handles hex conversion automatically.
base_layer/core/src/chain_storage/blockchain_backend.rs (1)
Learnt from: hansieodendaal
PR: tari-project/tari#7216
File: applications/minotari_app_grpc/src/conversions/aggregate_body.rs:31-46
Timestamp: 2025-06-14T04:46:31.966Z
Learning: When a type implements the Copy trait (like FixedHash/BlockHash), it can be used multiple times in iterator closures without being consumed or moved. The compiler automatically copies Copy types, so no explicit .clone() is needed and will trigger clippy::clone_on_copy warnings.
base_layer/core/src/chain_storage/async_db.rs (1)
Learnt from: hansieodendaal
PR: tari-project/tari#6963
File: comms/core/src/peer_manager/manager.rs:60-68
Timestamp: 2025-05-26T02:40:23.812Z
Learning: PeerDatabaseSql in the Tari codebase has been specifically refactored to handle concurrent access and mitigate blocking I/O concerns on async executor threads. The implementation has been tested under high load at both system level and through unit tests like test_concurrent_add_or_update_and_get_closest_peers which validates concurrent read/write operations.
base_layer/core/src/base_node/comms_interface/local_interface.rs (2)
Learnt from: SolfataraEmit
PR: tari-project/tari#6994
File: docs/src/API_GRPC_Explanation.md:236-244
Timestamp: 2025-04-28T11:41:15.722Z
Learning: The minotari_console_wallet reports a fourth field `timelocked_balance` in the terminal output for the `getBalance` method, which should be reflected in the documentation for accuracy. The `wallet.proto` file confirms that `timelocked_balance` is defined as the fourth field in the `GetBalanceResponse` message.
Learnt from: SolfataraEmit
PR: tari-project/tari#6994
File: docs/src/API_GRPC_Explanation.md:236-244
Timestamp: 2025-04-28T11:41:15.722Z
Learning: The minotari_console_wallet reports a fourth field `timelocked_balance` in the terminal output for the `getBalance` method, which should be reflected in the documentation for accuracy.
applications/minotari_node/src/grpc/base_node_grpc_server.rs (5)

undefined

<retrieved_learning>
Learnt from: hansieodendaal
PR: #6963
File: comms/core/src/peer_manager/manager.rs:60-68
Timestamp: 2025-05-26T02:40:23.812Z
Learning: PeerDatabaseSql in the Tari codebase has been specifically refactored to handle concurrent access and mitigate blocking I/O concerns on async executor threads. The implementation has been tested under high load at both system level and through unit tests like test_concurrent_add_or_update_and_get_closest_peers which validates concurrent read/write operations.
</retrieved_learning>

<retrieved_learning>
Learnt from: martinserts
PR: #7122
File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0
Timestamp: 2025-06-16T14:49:31.831Z
Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.
</retrieved_learning>

<retrieved_learning>
Learnt from: ksrichard
PR: #7129
File: applications/minotari_node/src/http/handler/sync_utxos_by_block.rs:21-29
Timestamp: 2025-05-29T20:59:32.020Z
Learning: In the Tari codebase, request validation for RPC services is handled at the service layer using the serde_valid crate rather than at the HTTP handler level. For example, SyncUtxosByBlockRequest validates that limit is between 1-5 and page is minimum 0. HTTP handlers should delegate validation to the service layer rather than implementing their own validation.
</retrieved_learning>

<retrieved_learning>
Learnt from: SolfataraEmit
PR: #6994
File: docs/src/API_GRPC_Explanation.md:236-244
Timestamp: 2025-04-28T11:41:15.722Z
Learning: The minotari_console_wallet reports a fourth field timelocked_balance in the terminal output for the getBalance method, which should be reflected in the documentation for accuracy. The wallet.proto file confirms that timelocked_balance is defined as the fourth field in the GetBalanceResponse message.
</retrieved_learning>

<retrieved_learning>
Learnt from: hansieodendaal
PR: #6974
File: base_layer/wallet/src/output_manager_service/service.rs:2026-2031
Timestamp: 2025-04-23T05:56:30.985Z
Learning: When setting the fee parameter in output_to_self method in the Tari wallet, use an accurate fee estimate instead of fee_per_gram. The input_selection.as_final_fee() method provides a good initial estimate, and the final fee can be obtained later from stp.get_fee_amount().
</retrieved_learning>

base_layer/core/src/chain_storage/blockchain_database.rs (1)

undefined

<retrieved_learning>
Learnt from: hansieodendaal
PR: #7216
File: applications/minotari_app_grpc/src/conversions/aggregate_body.rs:31-46
Timestamp: 2025-06-14T04:46:31.966Z
Learning: When a type implements the Copy trait (like FixedHash/BlockHash), it can be used multiple times in iterator closures without being consumed or moved. The compiler automatically copies Copy types, so no explicit .clone() is needed and will trigger clippy::clone_on_copy warnings.
</retrieved_learning>

base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs (1)
Learnt from: martinserts
PR: tari-project/tari#7122
File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0
Timestamp: 2025-06-16T14:49:31.831Z
Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.
🧬 Code Graph Analysis (4)
base_layer/core/src/chain_storage/async_db.rs (5)
base_layer/core/src/chain_storage/blockchain_backend.rs (3)
  • fetch_mined_info_by_payref (113-113)
  • fetch_mined_info_by_output_hash (116-116)
  • fetch_payref_by_output_hash (119-119)
base_layer/core/src/test_helpers/blockchain.rs (3)
  • fetch_mined_info_by_payref (318-320)
  • fetch_mined_info_by_output_hash (322-324)
  • fetch_payref_by_output_hash (326-328)
base_layer/core/src/chain_storage/blockchain_database.rs (3)
  • fetch_mined_info_by_payref (458-461)
  • fetch_mined_info_by_output_hash (464-467)
  • fetch_payref_by_output_hash (470-473)
base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs (3)
  • fetch_mined_info_by_payref (2421-2424)
  • fetch_mined_info_by_output_hash (2426-2429)
  • fetch_payref_by_output_hash (2431-2434)
base_layer/core/src/transactions/transaction_components/transaction_input.rs (1)
  • output_hash (434-462)
base_layer/core/src/test_helpers/blockchain.rs (4)
base_layer/core/src/chain_storage/blockchain_backend.rs (3)
  • fetch_mined_info_by_payref (113-113)
  • fetch_mined_info_by_output_hash (116-116)
  • fetch_payref_by_output_hash (119-119)
base_layer/core/src/chain_storage/blockchain_database.rs (3)
  • fetch_mined_info_by_payref (458-461)
  • fetch_mined_info_by_output_hash (464-467)
  • fetch_payref_by_output_hash (470-473)
base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs (3)
  • fetch_mined_info_by_payref (2421-2424)
  • fetch_mined_info_by_output_hash (2426-2429)
  • fetch_payref_by_output_hash (2431-2434)
base_layer/core/src/transactions/transaction_components/transaction_input.rs (1)
  • output_hash (434-462)
base_layer/core/src/base_node/comms_interface/local_interface.rs (5)
base_layer/core/src/chain_storage/blockchain_backend.rs (3)
  • fetch_mined_info_by_payref (113-113)
  • fetch_mined_info_by_output_hash (116-116)
  • fetch_payref_by_output_hash (119-119)
base_layer/core/src/test_helpers/blockchain.rs (3)
  • fetch_mined_info_by_payref (318-320)
  • fetch_mined_info_by_output_hash (322-324)
  • fetch_payref_by_output_hash (326-328)
base_layer/core/src/chain_storage/blockchain_database.rs (3)
  • fetch_mined_info_by_payref (458-461)
  • fetch_mined_info_by_output_hash (464-467)
  • fetch_payref_by_output_hash (470-473)
base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs (3)
  • fetch_mined_info_by_payref (2421-2424)
  • fetch_mined_info_by_output_hash (2426-2429)
  • fetch_payref_by_output_hash (2431-2434)
base_layer/core/src/transactions/transaction_components/transaction_input.rs (1)
  • output_hash (434-462)
base_layer/core/src/base_node/comms_interface/inbound_handlers.rs (1)
base_layer/core/src/transactions/transaction_components/transaction_input.rs (1)
  • output_hash (434-462)
⏰ Context from checks skipped due to timeout of 90000ms (7)
  • GitHub Check: Cucumber tests / FFI
  • GitHub Check: Cucumber tests / Base Layer
  • GitHub Check: test (mainnet, stagenet)
  • GitHub Check: test (testnet, esmeralda)
  • GitHub Check: test (nextnet, nextnet)
  • GitHub Check: ci
  • GitHub Check: cargo check with stable
🔇 Additional comments (31)
base_layer/core/src/chain_storage/utxo_mined_info.rs (1)

44-48: LGTM! Well-designed unified structure for mined information.

The MinedInfo struct elegantly combines input and output mined information with optional fields, allowing for flexible representation of both spent and unspent outputs. The derive traits are appropriate for serialization and debugging.

base_layer/core/src/base_node/comms_interface/comms_response.rs (4)

30-30: LGTM! Proper import addition for new functionality.

The FixedHash import is correctly added to support the new PayRef response variant.


35-35: LGTM! Consistent import pattern.

The MinedInfo import follows the existing import structure and supports the new response variant.


65-67: LGTM! Well-structured response variants.

The new response variants MinedInfo and PayRef are properly added with appropriate types. The PayRef uses Option<FixedHash> to handle cases where no payment reference exists for a given output hash.


106-108: LGTM! Consistent Display implementation.

The Display trait implementations for the new variants follow the established pattern of simple variant name output.

base_layer/core/src/chain_storage/async_db.rs (2)

33-33: LGTM! Proper import addition.

The MinedInfo import is correctly added to support the new async methods.


279-283: LGTM! Consistent async method implementation.

The three new async methods follow the established pattern using the make_async_fn! macro:

  • fetch_mined_info_by_payref - Fetches unified mined info by payment reference
  • fetch_mined_info_by_output_hash - Fetches unified mined info by output hash
  • fetch_payref_by_output_hash - Reverse lookup to get payment reference from output hash

The method signatures are consistent with the trait definitions and the naming follows established conventions.

base_layer/core/src/chain_storage/blockchain_backend.rs (2)

9-9: LGTM! Proper import addition.

The MinedInfo import is correctly added to support the new trait methods.


112-119: LGTM! Well-documented trait method additions.

The three new trait methods are properly documented and provide a clean interface for:

  • Fetching mined info by payment reference
  • Fetching mined info by output hash (new reverse lookup capability)
  • Fetching payment reference by output hash (new reverse lookup capability)

The method signatures are consistent with the async wrapper implementations and use appropriate return types.

applications/minotari_node/src/grpc_method.rs (4)

69-69: LGTM! Consistent enum variant addition.

The new SearchPaymentReferencesViaOutputHash variant follows the established naming convention and logically extends the payment reference search capabilities.


74-74: LGTM! Comprehensive enum infrastructure updates.

All related constants and type parameters are properly updated:

  • Array size increased from 37 to 38
  • New variant added to ALL_VARIANTS array
  • IntoIterator type parameter updated to match

This ensures consistency across the enum's infrastructure.

Also applies to: 112-112, 117-117


169-169: LGTM! Consistent string mapping.

The FromStr implementation properly maps the snake_case string "search_payment_references_via_output_hash" to the new enum variant, following the established pattern.


267-267: LGTM! Test completeness maintained.

The test is properly updated to include the new variant, ensuring the exhaustiveness check continues to pass and all variants are accounted for.

base_layer/core/src/chain_storage/blockchain_database.rs (1)

62-62: Import addition looks correct.

The MinedInfo import is appropriately added to support the new method return types.

base_layer/core/src/test_helpers/blockchain.rs (2)

64-64: LGTM: Import addition is appropriate.

The MinedInfo import is correctly added to support the new method signatures in the trait implementation.


318-328: LGTM: Proper implementation of new trait methods.

The three new methods correctly implement the updated BlockchainBackend trait interface by delegating to the underlying database. The error handling and return types match the trait signatures.

base_layer/core/src/base_node/comms_interface/inbound_handlers.rs (1)

457-472: LGTM: Correct implementation of new request handlers.

The four new request handlers properly delegate to the blockchain database methods and return the appropriate NodeCommsResponse variants. The error handling follows the established pattern of propagating database errors.

base_layer/core/src/base_node/comms_interface/local_interface.rs (3)

41-41: LGTM: Import addition is appropriate.

The MinedInfo import is correctly added to support the new method signatures.


360-370: LGTM: Correct implementation of payref-based mined info fetch.

The method correctly sends NodeCommsRequest::FetchMinedInfoByPayRef and handles the expected response type.


402-415: LGTM: Correct implementation of output mined info fetch.

The method correctly sends NodeCommsRequest::FetchOutputMinedInfo and handles the expected response type. The documentation comment is also accurate.

applications/minotari_node/src/grpc/base_node_grpc_server.rs (2)

3132-3194: Approve refactored search_payment_references method.

The refactored method now properly uses the new MinedInfo structure and handles both unspent outputs and spent inputs consistently. The error handling and response construction look correct.


2960-2960: We need to locate the actual server implementation and inspect the surrounding RPC method. Let’s find the file and then dump the FetchMatchingUtxosRequest usage with context:

#!/bin/bash
# 1. Locate the gRPC server file
echo "Searching for base_node_grpc_server.rs…"
FILE=$(fd --full-path base_node_grpc_server.rs)
echo "Found: $FILE"

# 2. Show the RPC method where FetchMatchingUtxosRequest is used
echo "Showing surrounding lines for FetchMatchingUtxosRequest:"
rg -n -C5 "FetchMatchingUtxosRequest" "$FILE"
base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs (9)

118-118: LGTM!

The import of MinedInfo is correctly added to support the new functionality for fetching mined information.


161-161: LGTM!

The new reverse PayRef index database is correctly integrated into the LMDB structure, following the existing naming conventions and initialization patterns.

Also applies to: 216-216, 276-276, 334-334, 550-550, 568-568


644-671: LGTM!

The insert_payref_index helper method correctly implements atomic bidirectional indexing, ensuring data consistency between the forward and reverse PayRef mappings.


618-618: LGTM!

Correctly updated to use the insert_payref_index helper method for consistent bidirectional indexing.


1195-1220: LGTM!

The delete_payref_index helper correctly removes both forward and reverse mappings atomically, maintaining index consistency.


1943-1965: LGTM!

The fetch_mined_info_by_output_hash_in_txn method correctly checks both output and input databases to return the appropriate mined information.


1967-1975: LGTM!

The fetch_payref_by_output_hash_in_txn method correctly uses the new reverse index for efficient PayRef lookup.


2421-2434: LGTM!

The public interface methods correctly follow the established pattern of creating read transactions and delegating to the internal transaction-based implementations.


3052-3052: Clarify the migration version jump and error handling.

The migration version jumps from 3 to 5, and there's a comment about fixing a previous error. The use of max() on line 3260 is unusual for migration versioning.

Please clarify:

  1. Why skip version 4?
  2. What was the previous migration error?
  3. Is the max() usage correct here, or should it just increment by 1?

This unusual versioning could cause issues if not handled correctly.

Also applies to: 3067-3070, 3260-3260

@hansieodendaal hansieodendaal marked this pull request as draft June 26, 2025 14:33
@hansieodendaal hansieodendaal marked this pull request as ready for review June 26, 2025 16:06
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (1)
base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs (1)

3241-3248: Consider making the PayRef index clearing more robust.

Clearing the entire payref_to_output_index and output_to_payref_index before rebuilding is risky. If the migration fails partway through, both indexes will be left in an inconsistent state.

This mirrors the previous review suggestion about building in a temporary table first, clearing only after successful rebuild, and adding progress checkpoints.

🧹 Nitpick comments (3)
base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs (3)

550-550: Update array size comment for accuracy.

The array size is now 33 but the method signature still references the old count.

Consider updating any documentation or comments that reference the database count to reflect the new total of 33 databases.


3072-3075: Migration flag indicates previous issues.

The payref_index_done flag and comment about "fixing the previous payref migration error" suggest there were problems with earlier migrations. While this addresses the immediate issue, it adds complexity.

Consider documenting the specific migration error that required this workaround, or implement a more robust migration state tracking mechanism.

Also applies to: 3227-3227


3250-3256: Migration performance could be optimized.

Processing each height individually with separate transactions could be slow for large chains. The migration commits after every single height.

Consider batching multiple heights in a single transaction (e.g., 100-1000 heights per transaction) to improve migration performance while maintaining reasonable transaction sizes.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 85de473 and 55df956.

📒 Files selected for processing (4)
  • applications/minotari_node/src/grpc/base_node_grpc_server.rs (5 hunks)
  • base_layer/core/src/base_node/comms_interface/local_interface.rs (2 hunks)
  • base_layer/core/src/chain_storage/blockchain_database.rs (2 hunks)
  • base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs (19 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • base_layer/core/src/chain_storage/blockchain_database.rs
  • applications/minotari_node/src/grpc/base_node_grpc_server.rs
  • base_layer/core/src/base_node/comms_interface/local_interface.rs
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: hansieodendaal
PR: tari-project/tari#6963
File: comms/core/src/peer_manager/manager.rs:60-68
Timestamp: 2025-05-26T02:40:23.812Z
Learning: PeerDatabaseSql in the Tari codebase has been specifically refactored to handle concurrent access and mitigate blocking I/O concerns on async executor threads. The implementation has been tested under high load at both system level and through unit tests like test_concurrent_add_or_update_and_get_closest_peers which validates concurrent read/write operations.
base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs (2)

undefined

<retrieved_learning>
Learnt from: hansieodendaal
PR: #7266
File: base_layer/core/src/chain_storage/blockchain_database.rs:457-461
Timestamp: 2025-06-26T13:18:55.867Z
Learning: The MinedInfo struct in the Tari blockchain database contains two optional fields: input: Option<InputMinedInfo> and output: Option<OutputMinedInfo>. This design allows methods returning MinedInfo to always return a valid struct rather than wrapping it in Option, with "not found" scenarios represented through the internal optional fields.
</retrieved_learning>

<retrieved_learning>
Learnt from: martinserts
PR: #7122
File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0
Timestamp: 2025-06-16T14:49:31.831Z
Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.
</retrieved_learning>

⏰ Context from checks skipped due to timeout of 90000ms (7)
  • GitHub Check: test (mainnet, stagenet)
  • GitHub Check: test (testnet, esmeralda)
  • GitHub Check: test (nextnet, nextnet)
  • GitHub Check: cargo check with stable
  • GitHub Check: ci
  • GitHub Check: Cucumber tests / Base Layer
  • GitHub Check: Cucumber tests / FFI
🔇 Additional comments (8)
base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs (8)

161-161: LGTM: Consistent naming convention for reverse index.

The constant name follows the established pattern and clearly indicates the reverse mapping direction.


216-216: LGTM: Proper database setup and integration.

The new reverse PayRef database is correctly added to the LMDB builder, struct initialization, and database enumeration.

Also applies to: 334-334, 568-568


644-672: Excellent refactoring: Atomic PayRef index operations.

The helper method ensures both forward and reverse mappings are maintained atomically. Using lmdb_replace instead of lmdb_insert is appropriate here as it handles potential duplicate entries gracefully.


1197-1225: Good error handling in reverse index cleanup.

The method properly handles both forward and reverse mapping deletions with appropriate error propagation.


1932-1981: Well-structured unified mined info retrieval.

The fetch methods follow the MinedInfo pattern from the learnings, returning a struct with optional input/output fields rather than wrapping in Option. The logic correctly handles both unspent (output) and spent (input) scenarios.


2427-2440: Clean public API for mined info queries.

The new BlockchainBackend methods provide a consistent interface for PayRef and output hash lookups.


3058-3058: Migration version jump requires explanation.

The version jumps from 3 to 5, skipping version 4. This suggests either version 4 was problematic or there's an inconsistency in versioning.

Please verify why the migration version jumps from 3 to 5. If version 4 was skipped due to issues, consider documenting this in the code or commit message.


3396-3413: Robust PayRef processing with proper error handling.

The migration correctly rebuilds both forward and reverse mappings with appropriate trace logging and error propagation.

Copy link
Copy Markdown
Collaborator

@SWvheerden SWvheerden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am against this appraoch
We already have all info needed to calculate this, we don't need to store another index,
And on the application layer, you need to do two lookups and not one.

use this function: fetch_output_in_txn
it contains all the info needed for the payref.

@hansieodendaal hansieodendaal marked this pull request as draft June 27, 2025 03:16
@hansieodendaal hansieodendaal marked this pull request as ready for review June 27, 2025 07:16
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (1)
base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs (1)

3246-3268: Consider making the PayRef migration more robust

The current migration clears both payref indexes before rebuilding them completely. If the migration fails partway through, both indexes will be left in an inconsistent state.

Consider implementing a more robust approach:

  1. Build new indexes in temporary tables first
  2. Only clear the original indexes after successful rebuild
  3. Add transaction-level error handling to rollback on failure

This aligns with the previous review feedback about making PayRef index operations more resilient.

🧹 Nitpick comments (1)
base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs (1)

646-674: Clean up commented debugging code

The helper method functionality is correct and uses appropriate lmdb_replace for atomic operations. However, the commented-out lmdb_insert code should be removed as it appears to be leftover debugging code.

-        // Forward mapping: payref -> output_hash
-        // lmdb_insert(
-        //     txn,
-        //     &self.payref_to_output_index,
-        //     payref.as_slice(),
-        //     output_hash,
-        //     "payref_to_output_index",
-        // )?;
         lmdb_replace(txn, &self.payref_to_output_index, payref.as_slice(), &output_hash, None)?;

-        // Reverse mapping: output_hash -> payref
-        // lmdb_insert(
-        //     txn,
-        //     &self.output_to_payref_index,
-        //     output_hash.as_slice(),
-        //     payref,
-        //     "output_to_payref_index",
-        // )?;
         lmdb_replace(txn, &self.output_to_payref_index, output_hash.as_slice(), payref, None)?;
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 55df956 and 2c06ff6.

📒 Files selected for processing (1)
  • base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs (24 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: hansieodendaal
PR: tari-project/tari#6963
File: comms/core/src/peer_manager/manager.rs:60-68
Timestamp: 2025-05-26T02:40:23.812Z
Learning: PeerDatabaseSql in the Tari codebase has been specifically refactored to handle concurrent access and mitigate blocking I/O concerns on async executor threads. The implementation has been tested under high load at both system level and through unit tests like test_concurrent_add_or_update_and_get_closest_peers which validates concurrent read/write operations.
base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs (4)

undefined

<retrieved_learning>
Learnt from: hansieodendaal
PR: #7266
File: base_layer/core/src/chain_storage/blockchain_database.rs:457-461
Timestamp: 2025-06-26T13:18:55.867Z
Learning: The MinedInfo struct in the Tari blockchain database contains two optional fields: input: Option<InputMinedInfo> and output: Option<OutputMinedInfo>. This design allows methods returning MinedInfo to always return a valid struct rather than wrapping it in Option, with "not found" scenarios represented through the internal optional fields.
</retrieved_learning>

<retrieved_learning>
Learnt from: SWvheerden
PR: #6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
</retrieved_learning>

<retrieved_learning>
Learnt from: SWvheerden
PR: #6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
</retrieved_learning>

<retrieved_learning>
Learnt from: martinserts
PR: #7122
File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0
Timestamp: 2025-06-16T14:49:31.831Z
Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.
</retrieved_learning>

⏰ Context from checks skipped due to timeout of 90000ms (7)
  • GitHub Check: Cucumber tests / FFI
  • GitHub Check: Cucumber tests / Base Layer
  • GitHub Check: test (nextnet, nextnet)
  • GitHub Check: test (mainnet, stagenet)
  • GitHub Check: test (testnet, esmeralda)
  • GitHub Check: cargo check with stable
  • GitHub Check: ci
🔇 Additional comments (6)
base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs (6)

39-39: LGTM: Proper database infrastructure additions

The new imports, database constants, and field additions properly support the reverse payref lookup functionality. All necessary components are consistently added throughout the initialization and management code.

Also applies to: 70-70, 120-120, 163-163, 218-218, 277-278, 336-336, 552-552, 570-570


620-620: LGTM: Consistent usage of new helper methods

The usage of insert_payref_index and delete_payref_index helper methods is consistent and maintains the bidirectional mapping integrity throughout the codebase.

Also applies to: 1110-1110, 1185-1185


1199-1227: LGTM: Well-implemented atomic deletion

The delete_payref_index method correctly handles atomic deletion of both forward and reverse mappings with proper error handling. The approach ensures consistency by failing if either deletion fails.


1934-1982: LGTM: Well-designed fetch methods with proper MinedInfo usage

The new fetch methods correctly implement the lookup logic and follow the established pattern of using MinedInfo with optional fields to represent either input or output mined information. The delegation pattern and error handling are appropriate.


2429-2441: LGTM: Standard public interface methods

The public interface methods correctly follow the established pattern of creating read transactions and delegating to internal transaction-specific methods. The return types and error handling are consistent.


3385-3460: LGTM: Well-implemented migration function with proper genesis handling

The process_payref_for_height function correctly handles the special case of genesis block by fetching outputs directly from the genesis block instead of the database. The batch processing approach with transaction commits is efficient and the error handling is appropriate.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs (1)

3167-3188: Comprehensive PayRef migration with proper error correction

The PayRef migration logic correctly handles the historical issue where PayRefs were missing for spent outputs. Key improvements:

  1. Handles versions 2, 3, and 4 collectively to fix the original migration error
  2. Clears the entire PayRef index before rebuilding to ensure consistency
  3. Processes all blocks from genesis to current height
  4. Uses the payref_index_done flag to prevent duplicate work

However, note the existing concern about clearing the entire index before rebuilding.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2c06ff6 and 818c3c8.

📒 Files selected for processing (10)
  • applications/minotari_app_grpc/proto/base_node.proto (2 hunks)
  • applications/minotari_node/src/grpc/base_node_grpc_server.rs (4 hunks)
  • base_layer/core/src/base_node/comms_interface/comms_request.rs (2 hunks)
  • base_layer/core/src/base_node/comms_interface/inbound_handlers.rs (1 hunks)
  • base_layer/core/src/base_node/comms_interface/local_interface.rs (2 hunks)
  • base_layer/core/src/chain_storage/async_db.rs (2 hunks)
  • base_layer/core/src/chain_storage/blockchain_backend.rs (2 hunks)
  • base_layer/core/src/chain_storage/blockchain_database.rs (2 hunks)
  • base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs (16 hunks)
  • base_layer/core/src/test_helpers/blockchain.rs (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
  • base_layer/core/src/test_helpers/blockchain.rs
  • applications/minotari_app_grpc/proto/base_node.proto
  • base_layer/core/src/base_node/comms_interface/comms_request.rs
  • base_layer/core/src/chain_storage/blockchain_backend.rs
  • base_layer/core/src/base_node/comms_interface/local_interface.rs
  • base_layer/core/src/chain_storage/blockchain_database.rs
  • base_layer/core/src/base_node/comms_interface/inbound_handlers.rs
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: hansieodendaal
PR: tari-project/tari#6963
File: comms/core/src/peer_manager/manager.rs:60-68
Timestamp: 2025-05-26T02:40:23.812Z
Learning: PeerDatabaseSql in the Tari codebase has been specifically refactored to handle concurrent access and mitigate blocking I/O concerns on async executor threads. The implementation has been tested under high load at both system level and through unit tests like test_concurrent_add_or_update_and_get_closest_peers which validates concurrent read/write operations.
base_layer/core/src/chain_storage/async_db.rs (2)
Learnt from: hansieodendaal
PR: tari-project/tari#7266
File: base_layer/core/src/chain_storage/blockchain_database.rs:457-461
Timestamp: 2025-06-26T13:18:55.867Z
Learning: The MinedInfo struct in the Tari blockchain database contains two optional fields: `input: Option<InputMinedInfo>` and `output: Option<OutputMinedInfo>`. This design allows methods returning MinedInfo to always return a valid struct rather than wrapping it in Option, with "not found" scenarios represented through the internal optional fields.
Learnt from: hansieodendaal
PR: tari-project/tari#6963
File: comms/core/src/peer_manager/manager.rs:60-68
Timestamp: 2025-05-26T02:40:23.812Z
Learning: PeerDatabaseSql in the Tari codebase has been specifically refactored to handle concurrent access and mitigate blocking I/O concerns on async executor threads. The implementation has been tested under high load at both system level and through unit tests like test_concurrent_add_or_update_and_get_closest_peers which validates concurrent read/write operations.
applications/minotari_node/src/grpc/base_node_grpc_server.rs (6)

undefined

<retrieved_learning>
Learnt from: hansieodendaal
PR: #7266
File: base_layer/core/src/chain_storage/blockchain_database.rs:457-461
Timestamp: 2025-06-26T13:18:55.867Z
Learning: The MinedInfo struct in the Tari blockchain database contains two optional fields: input: Option<InputMinedInfo> and output: Option<OutputMinedInfo>. This design allows methods returning MinedInfo to always return a valid struct rather than wrapping it in Option, with "not found" scenarios represented through the internal optional fields.
</retrieved_learning>

<retrieved_learning>
Learnt from: hansieodendaal
PR: #6963
File: comms/core/src/peer_manager/manager.rs:60-68
Timestamp: 2025-05-26T02:40:23.812Z
Learning: PeerDatabaseSql in the Tari codebase has been specifically refactored to handle concurrent access and mitigate blocking I/O concerns on async executor threads. The implementation has been tested under high load at both system level and through unit tests like test_concurrent_add_or_update_and_get_closest_peers which validates concurrent read/write operations.
</retrieved_learning>

<retrieved_learning>
Learnt from: martinserts
PR: #7122
File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0
Timestamp: 2025-06-16T14:49:31.831Z
Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.
</retrieved_learning>

<retrieved_learning>
Learnt from: ksrichard
PR: #7129
File: applications/minotari_node/src/http/handler/sync_utxos_by_block.rs:21-29
Timestamp: 2025-05-29T20:59:32.020Z
Learning: In the Tari codebase, request validation for RPC services is handled at the service layer using the serde_valid crate rather than at the HTTP handler level. For example, SyncUtxosByBlockRequest validates that limit is between 1-5 and page is minimum 0. HTTP handlers should delegate validation to the service layer rather than implementing their own validation.
</retrieved_learning>

<retrieved_learning>
Learnt from: SolfataraEmit
PR: #6994
File: docs/src/API_GRPC_Explanation.md:236-244
Timestamp: 2025-04-28T11:41:15.722Z
Learning: The minotari_console_wallet reports a fourth field timelocked_balance in the terminal output for the getBalance method, which should be reflected in the documentation for accuracy. The wallet.proto file confirms that timelocked_balance is defined as the fourth field in the GetBalanceResponse message.
</retrieved_learning>

<retrieved_learning>
Learnt from: hansieodendaal
PR: #6974
File: base_layer/wallet/src/output_manager_service/service.rs:2026-2031
Timestamp: 2025-04-23T05:56:30.985Z
Learning: When setting the fee parameter in output_to_self method in the Tari wallet, use an accurate fee estimate instead of fee_per_gram. The input_selection.as_final_fee() method provides a good initial estimate, and the final fee can be obtained later from stp.get_fee_amount().
</retrieved_learning>

base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs (4)

undefined

<retrieved_learning>
Learnt from: hansieodendaal
PR: #7266
File: base_layer/core/src/chain_storage/blockchain_database.rs:457-461
Timestamp: 2025-06-26T13:18:55.867Z
Learning: The MinedInfo struct in the Tari blockchain database contains two optional fields: input: Option<InputMinedInfo> and output: Option<OutputMinedInfo>. This design allows methods returning MinedInfo to always return a valid struct rather than wrapping it in Option, with "not found" scenarios represented through the internal optional fields.
</retrieved_learning>

<retrieved_learning>
Learnt from: SWvheerden
PR: #6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
</retrieved_learning>

<retrieved_learning>
Learnt from: SWvheerden
PR: #6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
</retrieved_learning>

<retrieved_learning>
Learnt from: martinserts
PR: #7122
File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0
Timestamp: 2025-06-16T14:49:31.831Z
Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.
</retrieved_learning>

🧬 Code Graph Analysis (2)
base_layer/core/src/chain_storage/async_db.rs (6)
base_layer/core/src/base_node/comms_interface/local_interface.rs (2)
  • fetch_mined_info_by_payref (361-370)
  • fetch_mined_info_by_output_hash (373-385)
base_layer/core/src/chain_storage/blockchain_backend.rs (2)
  • fetch_mined_info_by_payref (113-113)
  • fetch_mined_info_by_output_hash (116-116)
base_layer/core/src/test_helpers/blockchain.rs (2)
  • fetch_mined_info_by_payref (318-320)
  • fetch_mined_info_by_output_hash (322-324)
base_layer/core/src/chain_storage/blockchain_database.rs (2)
  • fetch_mined_info_by_payref (458-461)
  • fetch_mined_info_by_output_hash (464-467)
base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs (2)
  • fetch_mined_info_by_payref (2355-2358)
  • fetch_mined_info_by_output_hash (2360-2363)
base_layer/core/src/transactions/transaction_components/transaction_input.rs (1)
  • output_hash (434-462)
base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs (5)
base_layer/core/src/blocks/genesis_block.rs (1)
  • get_genesis_block (40-50)
base_layer/core/src/chain_storage/lmdb_db/lmdb.rs (4)
  • lmdb_replace (132-166)
  • lmdb_clear (505-514)
  • lmdb_get (233-256)
  • lmdb_fetch_matching_after (352-366)
base_layer/core/src/chain_storage/blockchain_backend.rs (2)
  • fetch_mined_info_by_payref (113-113)
  • fetch_mined_info_by_output_hash (116-116)
base_layer/core/src/chain_storage/blockchain_database.rs (5)
  • fetch_mined_info_by_payref (458-461)
  • fetch_mined_info_by_output_hash (464-467)
  • rewind_to_height (1290-1293)
  • rewind_to_height (1818-1953)
  • db (1699-1734)
common/src/configuration/network.rs (1)
  • get_current_or_user_setting_or_default (60-71)
⏰ Context from checks skipped due to timeout of 90000ms (7)
  • GitHub Check: Cucumber tests / FFI
  • GitHub Check: Cucumber tests / Base Layer
  • GitHub Check: test (nextnet, nextnet)
  • GitHub Check: test (testnet, esmeralda)
  • GitHub Check: test (mainnet, stagenet)
  • GitHub Check: ci
  • GitHub Check: cargo check with stable
🔇 Additional comments (18)
base_layer/core/src/chain_storage/async_db.rs (2)

33-33: LGTM: Import correctly updated for new functionality.

The addition of MinedInfo to the imports is necessary for the new async functions being added.


279-281: LGTM: New async functions follow established patterns.

The two new async functions fetch_mined_info_by_payref and fetch_mined_info_by_output_hash correctly use the make_async_fn! macro and follow the same pattern as other async functions in this file. The function signatures align with the corresponding backend trait methods and support the payref reverse lookup functionality described in the PR objectives.

The return type MinedInfo is appropriate as it encapsulates both input and output mined information, allowing the caller to determine whether the payref or output hash corresponds to a spent or unspent output through the internal optional fields.

applications/minotari_node/src/grpc/base_node_grpc_server.rs (5)

41-41: LGTM: Import added for new functionality

The generate_payment_reference import is correctly added to support the payment reference generation in the new method.


240-241: LGTM: Associated type correctly defined

The SearchPaymentReferencesViaOutputHashStream type is properly defined for the new gRPC streaming method.


2962-2981: LGTM: Proper method validation and input processing

The method correctly checks permissions and validates the input hashes with appropriate error handling.


2987-3034: LGTM: Comprehensive response handling for both mined and spent outputs

The implementation correctly handles both output and input information from the unified MinedInfo struct:

  • Generates payment reference using output information
  • Sets spent status and timestamps when input information is present
  • Properly populates all response fields including output hash fallback
  • Includes appropriate error handling and logging

This unified approach is an improvement over the previous separate handling.


3110-3138: LGTM: Refactored to use unified MinedInfo approach

The refactoring correctly utilizes the new fetch_mined_info_by_payref method and unified MinedInfo struct, providing consistent handling of both mined outputs and spent inputs. This improves maintainability and ensures consistent response formatting.

base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs (11)

39-39: LGTM: Network import addition

The addition of the Network import is necessary for the genesis block handling in the migration logic.


70-70: LGTM: Genesis block import addition

The genesis block import is needed for the enhanced PayRef migration that handles height 0 specially.


120-120: LGTM: MinedInfo import addition

The MinedInfo import is required for the new unified mined info methods that combine output and input information.


614-614: Excellent improvement: Using lmdb_replace for PayRef consistency

Changing from lmdb_insert to lmdb_replace for PayRef mappings is a crucial fix that allows overwriting existing entries. This ensures consistency during reorgs and prevents potential database corruption from duplicate key errors.


1154-1154: Consistent PayRef handling during reorgs

Good application of lmdb_replace for PayRef entries during reorg processing, maintaining consistency with the insert_output method.


1873-1908: Well-implemented unified MinedInfo methods

The new methods correctly implement the unified MinedInfo pattern:

  1. fetch_mined_info_by_payref_in_txn properly looks up the output hash by PayRef and delegates to the output hash method
  2. fetch_mined_info_by_output_hash_in_txn correctly populates both input and output fields when available
  3. Returns a properly structured MinedInfo with optional fields as per the learning about this design pattern

The error handling is appropriate, and the methods align well with the blockchain backend interface.


2355-2363: Proper public interface for unified MinedInfo access

The public methods correctly delegate to the transaction-specific implementations, maintaining the established pattern in the codebase.


2981-2981: Migration version increment looks correct

The bump to version 5 appears appropriate for the PayRef enhancements being introduced.


2991-2992: Excellent migration logging improvements

The enhanced logging throughout the migration process provides much better visibility into:

  • Current and target migration versions
  • Progress for each migration step
  • Specific operations being performed
  • Completion status

This will significantly help with debugging and monitoring migration progress.

Also applies to: 3036-3037, 3073-3074, 3101-3102, 3121-3125, 3136-3156, 3168-3169, 3179-3180, 3187-3188, 3194-3197


2996-2998: Smart migration loop optimization

The addition of the payref_index_done flag and the updated loop logic (last_migrated_version..MIGRATION_VERSION) ensures:

  1. PayRef migration runs only once even across multiple version increments
  2. Migrations run efficiently from the last completed version
  3. Prevents redundant PayRef rebuilds

This is a good optimization for the migration process.


3304-3373: ```shell
#!/bin/bash

Locate where process_payref_for_height is invoked to understand migration context

rg -n -A 5 -B 5 "process_payref_for_height" --type rust


</details>

</blockquote></details>

</details>

<!-- This is an auto-generated comment by CodeRabbit for review status -->

@hansieodendaal hansieodendaal changed the title feat!: add payref reverse lookup feat!: add grpc method to retrieve output and payref information via output hash Jun 27, 2025
@hansieodendaal hansieodendaal changed the title feat!: add grpc method to retrieve output and payref information via output hash feat!: add grpc method for output and payref information via output hash Jun 27, 2025
Hansie Odendaal added 2 commits June 27, 2025 16:42
[c::cs::lmdb_db::lmdb_db] TRACE Processing payref 668624580bc1d2675de974c444c9ec0b1052bf41f14d673defa01fccfcc36175 and output hash 1a11b329183df05848249ee345ee29e44375505dac1689adc32ac021d0f636f6 for height 26902
[c::cs::lmdb_db::lmdb_db] TRACE Processing payref 5cac218ded22eeb8efdc4dc295a2461666c8d5137abfc02b30b9152f82332e99 and output hash 1a5551e00b270645e3804be945d477840a748bca9f96dd919a3691271c1bef43 for height 26902
[c::cs::lmdb_db::lmdb_db] TRACE Processing payref ae08d3ec581531f503603f419ff458e28eea07669286f7d3c650a0b6bcbc0f40 and output hash 1c0a293ed8682b4f00bfa9d5cb8ee039f1065a8aaff964c91318bfe8e910316f for height 26902
[tonic::transport::server] TRACE signal received, shutting down
[tonic::transport::server] TRACE waiting for 0 connections to close
[minotari::base_node::app] INFO  Stopping GRPC
[minotari::base_node::app] ERROR Exiting with code (114): ExitError { exit_code: DatabaseError, details: Some("Database resize required") }
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (1)
base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs (1)

3167-3193: PayRef migration logic improved but retains some risk.

The migration handles multiple versions efficiently and includes memory management. However, the concern from past comments remains valid - clearing the entire PayRef index before rebuilding (line 3177) could leave the system in an inconsistent state if migration fails partway through.

Consider implementing the suggested approach of building in a temporary table first for better safety.

🧹 Nitpick comments (1)
base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs (1)

3312-3352: Genesis block handling is functionally correct but verbose.

The special case logic for height 0 is necessary to ensure genesis outputs get proper PayRef entries. The manual construction of TransactionOutputRowData ensures consistency, though the implementation is quite verbose.

Consider extracting the genesis block processing into a separate helper method to improve readability.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0600610 and 5c5328a.

📒 Files selected for processing (1)
  • base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs (16 hunks)
🧰 Additional context used
🧠 Learnings (1)
base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs (4)

undefined

<retrieved_learning>
Learnt from: hansieodendaal
PR: #7266
File: base_layer/core/src/chain_storage/blockchain_database.rs:457-461
Timestamp: 2025-06-26T13:18:55.867Z
Learning: The MinedInfo struct in the Tari blockchain database contains two optional fields: input: Option<InputMinedInfo> and output: Option<OutputMinedInfo>. This design allows methods returning MinedInfo to always return a valid struct rather than wrapping it in Option, with "not found" scenarios represented through the internal optional fields.
</retrieved_learning>

<retrieved_learning>
Learnt from: SWvheerden
PR: #6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
</retrieved_learning>

<retrieved_learning>
Learnt from: SWvheerden
PR: #6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
</retrieved_learning>

<retrieved_learning>
Learnt from: martinserts
PR: #7122
File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0
Timestamp: 2025-06-16T14:49:31.831Z
Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.
</retrieved_learning>

⏰ Context from checks skipped due to timeout of 90000ms (7)
  • GitHub Check: test (testnet, esmeralda)
  • GitHub Check: test (mainnet, stagenet)
  • GitHub Check: test (nextnet, nextnet)
  • GitHub Check: cargo check with stable
  • GitHub Check: ci
  • GitHub Check: Cucumber tests / FFI
  • GitHub Check: Cucumber tests / Base Layer
🔇 Additional comments (6)
base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs (6)

39-39: LGTM! Import additions are appropriate.

The new imports support the enhanced mined info functionality and PayRef migration improvements.

Also applies to: 70-70, 120-120


614-614: Good improvement to PayRef handling robustness.

Switching from lmdb_insert to lmdb_replace prevents failures when PayRef entries already exist, which is especially important during reorgs and with the new persistent PayRef semantics.

Also applies to: 1154-1154


1873-1908: Excellent implementation of unified mined info fetching.

The methods correctly follow the MinedInfo pattern with optional fields, always returning a valid struct rather than wrapping in Option. The implementation properly handles both output and input lookups for comprehensive mined information.


2355-2363: LGTM! Public interface methods are correctly implemented.

The methods follow the established pattern of creating read transactions and delegating to internal transaction-based implementations.


2981-2998: Migration version bump to 5 is appropriate.

The jump from version 3 to 5 aligns with fixing the migration counter error mentioned in past comments. The payref_index_done flag efficiently prevents duplicate PayRef rebuilding across multiple migration versions.


3354-3378: PayRef processing logic is well-implemented.

The batch processing approach with per-height commits provides a good balance between performance and recoverability. The use of lmdb_replace ensures safe handling of existing entries during migration.

@hansieodendaal hansieodendaal changed the title feat!: add grpc method for output and payref information via output hash fix!: payref migration and indexes, add grpc query via output hash Jun 29, 2025
@SWvheerden SWvheerden merged commit 3ceea6e into tari-project:development Jul 1, 2025
15 of 16 checks passed
@hansieodendaal hansieodendaal deleted the ho_outputs_by_hash branch July 1, 2025 13:00
SWvheerden added a commit to tari-project/tari-explorer that referenced this pull request Jul 2, 2025
Description
---
Added output hash to combined search
Display not found heights or hashes

**Dependency:** Works in conjunction with
tari-project/tari#7266.

Motivation and Context
---
Commitments are not persisted, but output hashes are. This will enhance
the search capabilities of the text explorer.

How Has This Been Tested?
---
Local system-level testing 

```rust
<!--StartFragment--><h1>Found in output/blocks:</h1>
  
Search | Block Height | Payment Reference | Mined Timestamp | Spent Timestamp | Min Value Promise | Commitment | Output Hash | Block Hash | Spent Block Hash
-- | -- | -- | -- | -- | -- | -- | -- | -- | --
Commitment | 37783 | e00d70aa027b031058224f5026517b54c3c39a810c55cb44845448a9822d304c | 1751034882 |   | 0 | 081e2bf57016f7cea84d4e1ae4510ffb8eff3aa45d504371dc37b9c9b802e875 | d14ac0b2530d327203c47e1367000e9d3c50850276f8e43f809c6c7effa1d83e | 347568607b39224ebd39e57aedb2612a35d400e26248c600ddd9eb5ff68ea42e |  
OutputHash | 37386 | 72569e1d977d0579b1c4bd6b631e6984d5414fa7bb5d933cb4cf0ac846952944 | 1750985978 | 1751022817 | 0 | d20bdbe74f5385f35a07af832b343641648377423d89345adde4b42b71bc5128 | a52f9b8c855f0dff34d0bc69bf0e17f81c4edc13ae5617ed3fcce37beaa38e1b | c4e17fd862100e68b3fb7528f8007929bf56bf06318780a87b99ace65ba6cf01 | 9c1dadf257a00175ee8b8392b1feaae7e6c986b66f9dcff3ee08a889acece46f
OutputHash | 37328 | 2001d4ccceb5c00c90f26df0eee2959a0260febe7827f1bf7dc52651845163a6 | 1750979051 | 1751022817 | 0 | 88741af96e69684c7c0c44f61493f8fa2d095ced241ecf942cd90682f6b00767 | ba9521c7a0b44eb2ec3064593dc2ae49e6fbce9c4ac6ed049c64950be853243c | 6192189cf2b81392c3b5ec4ade0e615b6aa3222bb157b8030a663f0e819a1e2e | 9c1dadf257a00175ee8b8392b1feaae7e6c986b66f9dcff3ee08a889acece46f
OutputHash | 37256 | faaabe19994fb482983aa0ab5c9ff6381bccbae25c5467900a7733a925ef03ab | 1750969957 |   | 0 | d4b4b81515d79845ecda76a7433dd9fd67697de524b6ed6867afe0d9a2722f65 | 6647881db0f63cc48bf950e5d3a05c5c32600d2a0f712fb47e07c36c6065b8cb | d82c3a6a4246a7eabbe20a3487559a7b03913a87197c428a167df1b6934a93d0 |  
OutputHash | 37256 | 21aba95f3eca2f7ea672fb95a1ab4f36f7d0a5d60c9f3f01abd19bb79451614c | 1750969957 | 1750976306 | 0 | d6a7004afee9d54d93ba72d3c04638cfc1f70e5bbf849f782c99bc98e4c7c656 | ab90468335c37536d311e52c2a240d9114c1d435d3b6dfac81e6fe227d455470 | d82c3a6a4246a7eabbe20a3487559a7b03913a87197c428a167df1b6934a93d0 | fe453919e74a2072a3bb17fa95843df982873ded81d7d1e3ee7b8e179586dd8e
OutputHash | 37256 | a418f474b5da97f0056211911209fc20155e435ef792b89ae09c8644c852ff99 | 1750969957 | 1750996120 | 0 | d6bd8643c9f22e969dec05c7146eb0bf3677df5be3df231008756e9ad6521267 | 0e2d463f63bb8ed03193aae3f6f7dd35d8faf550ccb4a404ae454785ea78ce8a | d82c3a6a4246a7eabbe20a3487559a7b03913a87197c428a167df1b6934a93d0 | 77eeebaf395892a1f95cd04a643c56392d13159629d6555da8687c161f6fe79e
OutputHash | 37256 | 6f7879771feedf725f10ebb69fbe1cf16e006339e24a84b004cbf70931da51f0 | 1750969957 | 1750976062 | 0 | d82ca1b13699e4a19378a119fe2585cf259e3e2ed6c769e5d3c0a16679475f27 | 5da0213519e89afe938ce14e2289971e1e84f062ec73bd1caffc3556d5b8c678 | d82c3a6a4246a7eabbe20a3487559a7b03913a87197c428a167df1b6934a93d0 | 199eb4f867593f1ddf532633c8b3869a7903bc2baa92f31e1bde1e864586b3a5
OutputHash | 37256 | 9d32e2ac20600cfb83ac5fe554567c1532adf821fdc924abbcc916ae5286c71d | 1750969957 |   | 0 | da8100ae5f15a359d46a657bafef35d1754ee0ae13f696889f75ba661555ba6c | 18779556b30b148d58ea65cab5828c199f1364567e69fbe995385a8b47bcd2d3 | d82c3a6a4246a7eabbe20a3487559a7b03913a87197c428a167df1b6934a93d0 |  
OutputHash | 37256 | 5ab56febfceba7bf94f10457954d414914f2774c4b64f05b17d7628cf8481ad7 | 1750969957 |   | 0 | dab1f7d5af50168af1725cc6f2fc785dbc3af1c29b815fc152c1e2f07000106e | 8ecbee7a54c51cae53817f3309a8b21ec441641d687aca4b33d29375adfd4937 | d82c3a6a4246a7eabbe20a3487559a7b03913a87197c428a167df1b6934a93d0 |  
#height | 37021 |   | 1750941093 |   |   |   |   | 674a42249a57bc44d51eecedfb4ea846de45ab0171e0994aee540682a683fa0d |  
Payref | 36176 | df6aa0d79205760396bf99690d5cf99e4a5df96236aa855f33bcda4e5e883c26 | 1750841042 | 1750842406 | 0 | 06b23af9ef442a81865ee6a5804d92511659fdcf7c7ea14eac228662c3780506 | 00aa0ea8c8ac86929bca7186b98b7a9f4b0a66e5e2686d968a057851926dc98c | c83229dc0080ea4e38a7515b4bbdcc9be358494bf070664f0d262a169af244f7 | c36c198bf728e9ec6639285ff62607c4f9257bad96ffc63ff1b1375017247b04
Payref | 36173 | c550b97c41fcb1237e4121963dbe2c09c388cd1b99decd11390bcf9fdee884bd | 1750840596 |   | 0 | 044cca475dd322099c5c828c921ac4f89ccf1d27b7f3e256e832cd158d8b6c3a | 20ee12f53c0c09a8082587fe04cfd2317ed945c2e0e6c9787f917a65a7126319 | 2151dc2132645e3e048ac98b25fb51c16ceff93096ca99e6dcdcecd14fb3439d |  
Payref | 35479 | c82bce7aca8270ed45d8af4ccd1d8972693ea313dcd91ed2a3fc26a189873090 | 1750757135 |   | 0 | 0ca37d39fd7a895e4d01dcfd7a1b509f741f7cc3259a174c9ffce4d748bc1f3b | 04cccd40e8fd1083b5b3e968a2a4ed47564021e45b46841196418b1dc9465f75 | 4b4b46404f26cdb3a25a2eb1db5dbdb13075c4984c41fa54161341cecdaffcda |  
Payref | 35479 | d1f0d6e508c2e5c04290dce1d29300ff1b0402a7afa1df4e62538e07cd746237 | 1750757135 | 1750759099 | 0 | 080f4065b10036e9f5020853322bf56d40003f254eff08cdca6049efadcaf332 | 09b5d5331418b7a7dd2a22168d748b1e8d464cc647f342fb2512ed7e0ffa0c8f | 4b4b46404f26cdb3a25a2eb1db5dbdb13075c4984c41fa54161341cecdaffcda | ae452fc40f4d201a6cb232f4c84e609b05bdbe42ca1f8c904255eee016fd099e
Payref | 35240 | 9433fdcaa75af1c01c631ab7363f9db00bc4466c904653a594c44f7302eba712 | 1750726642 | 1750819136 | 0 | 24fc73f0be78a3dff521ef9f2ac9fda008f5b80e3d05ed0b257b13cc3222e332 | 9717f21d442c3b100877f9b76fc311b17a965d64ef1c54467104b83b638e5eee | f7b982654c47b26865642d06878aad6911bc8e5c122f91a1a522c01c808602a3 | 44b08cb2175617891089e6cb058b56c3ae51d5f47301e841ed10711a5ce113f0
Commitment | 35240 | 0a68514be7a1189286b75794cb600b201731bbf75ea62910c32ca06665569077 | 1750726642 |   | 0 | 043e0c466847bf882ebd3613fea84d2e5beaab478ed6126df80f4ead6c441b16 | 1385ee655d766574945c6c97ae18967093f0fff9bca99503625b511fc349f904 | f7b982654c47b26865642d06878aad6911bc8e5c122f91a1a522c01c808602a3 |  
Payref | 35231 | 710246f5f5f3d5b7cf8bdb06d14927c1421d3c1634ecd7a98fe72ccad4e9bd16 | 1750725937 | 1750755141 | 1352742898 | 280ccafa2b53dc290e197112445c27300cf4db45182060ded1c2699306c83d53 | afb560616ac726721418e7f2d0def435eaf2253cc403315a38243e24666e445d | ebdb962b89588c689869abf937c8de042ac9dfb755a6901af78cf85440242e6c | eb5b434dc8c85f90720192137950dfe8b22d93499ccf9d793bdfcb175926fdca
#hash | 35211 |   | 1750723355 |   |   |   |   | daabbb5662981144b8c62948018c9022d718af97b938fe5e44bfaf12fe2f8a62 |  
#hash | 35206 |   | 1750722249 |   |   |   |   | cd003785b9f5124372b9f0211e022b380d4694273d4d61bcfc4a774a55c0f4f7 |  
Payref | 35206 | 267cb7443e11e4fec15f0ce1fd8cec5e759c122c5077bc690bfdff5a2888bf7c | 1750722249 | 1750725922 | 0 | 02743d78eab8a57d4a3314d2a00c48e31991f2fe9467bc0ce3b4d1c4cf2ea877 | 2d64ce448774e6613a427d5d7c0bc8dc98763c6ef5f8089346ab0e5301f3198d | cd003785b9f5124372b9f0211e022b380d4694273d4d61bcfc4a774a55c0f4f7 | 02a6a9caf5740bf8b7733460174fae4c7ee5cb85bf5db8f1bcafa0c48bf393df
OutputHash | 35141 | 37b5b2fba61b9b0d131c9c6920b03f8178e20b78f091f86f1fb323f19b3a2cc9 | 1750714884 | 1751025073 | 0 | 10852412d393e059099c244b2849d1160fd1013fdd2106038150b4519542aa53 | f474bdfcb4090c94ade3a921f79cdfe155969ffd9046c238956898f6bd493e21 | fc9ba60174a9cec77a45a87f74b86b40509a0bfabc3021ffef4d432e90f7215b | 17f22ee1175cabf9ca2bc40846f42019863476c52c64500554abd2bba0e9cf77
#height | 34111 |   | 1750590713 |   |   |   |   | 0f3588d531a64da2c9ad1e8ff4775f27104b62d97e22eac029ddd3a8e6ce8403 |  
OutputHash | 26206 | 0ae07e321f890829d0e1659bd752f3ccfe2b2d9234a243077745ea1b1d6a9fa4 | 1749628944 | 1751025073 | 0 | 3a4c93aece796d3dc093ab0ed59ab6a6f309ec9a12bcd32ecf0cec85912e7d4a | ece6e74fa73390e83e63e56be050dffe8111d2b1b1b5e2040686e0de3db24122 | e284921e78a37dc7c3f4b24966345d8d1f2e5f2550271c0fe48e69ca33971f6b | 17f22ee1175cabf9ca2bc40846f42019863476c52c64500554abd2bba0e9cf77
OutputHash | 18794 | 6b67da2bd16c294c2792680a9e94d8fdaed25419c7b37be05bb4fcfe76e64f2a | 1748725490 | 1751025073 | 0 | 6c8859e32fad63079dcaf934cfd41d87037f5364ac267dd967255057efeda909 | f06d0a65ee12df22530a9c24b4f920801840e3f3802c7a93f8363d579747aca7 | f6e6de699931e492cad6ec6f8d8bca025e6624e33aa2eb4d32545c8c88dbb870 | 17f22ee1175cabf9ca2bc40846f42019863476c52c64500554abd2bba0e9cf77
#height | 222 |   | 1746525773 |   |   |   |   | 3e0ed5f0f6e7d0c0bbbec6cc54e7e1c957bcde69c797fedeb7fc657743ba4d70 |  
#height | 111 |   | 1746519480 |   |   |   |   | 283aa165b859d40dc4de1d0da20001fba1382d074993611a5b89f1e0fdf4d571 |  

<!--EndFragment-->
```

What process can a PR reviewer use to test or verify this change?
---
Local system-level testing 
Code review

<!-- Checklist -->
<!-- 1. Is the title of your PR in the form that would make nice release
notes? The title, excluding the conventional commit
tag, will be included exactly as is in the CHANGELOG, so please think
about it carefully. -->


Breaking Changes
---

- [x] None
- [ ] Requires data directory on base node to be deleted
- [ ] Requires hard fork
- [ ] Other - Please specify

<!-- Does this include a breaking change? If so, include this line as a
footer -->
<!-- BREAKING CHANGE: Description what the user should do, e.g. delete a
database, resync the chain -->
SWvheerden added a commit that referenced this pull request Jul 2, 2025
Description
---
Fixes a migration bug introduced in #7266 

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

* **Bug Fixes**
* Improved error handling during block reorganization to prevent
unnecessary errors when certain entries are missing.

* **Tests**
  * Simplified test code by removing redundant cloning of arguments.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
@coderabbitai coderabbitai bot mentioned this pull request Jul 3, 2025
4 tasks
sdbondi added a commit to sdbondi/tari that referenced this pull request Jul 4, 2025
* development:
  chore: fix regression bug (tari-project#7276)
  feat!: expand gRPC readiness status to contain current processed block info (tari-project#7262)
  fix!: payref migration and indexes, add grpc query via output hash (tari-project#7266)
  feat: auto zero value coinbase reward calculation (tari-project#7259)
  feat!: improve grpc token supply (tari-project#7261)
  chore: new release v4.7.0-pre.0 (tari-project#7268)
  fix: get_all_completed_transactions limit issues (tari-project#7267)
  fix: ledger builds (tari-project#7260)
  feat: offline signing (tari-project#7122)
  test: verify accumulated difficulty (tari-project#7243)
@coderabbitai coderabbitai bot mentioned this pull request Sep 29, 2025
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[base node grpc] Not all requested outputs are returned via payref

3 participants