chore: update grovedb dependency to new revision and add logging for genesis block proof #2640
Conversation
…genesis block proof
WalkthroughThe changes add conditional logging in the proposal handler to retrieve and log a root tree proof when the proposal height is 1. A new private module and method for generating the root tree proof are introduced. Additionally, GroveDB dependencies are updated to a new revision hash in relevant Cargo.toml files. Changes
Sequence Diagram(s)sequenceDiagram
participant Handler as process_proposal
participant Drive as Drive
participant Logger as Logger
Handler->>Handler: Check if request.height == 1
alt Height is 1
Handler->>Drive: root_tree_proof(transaction, platform_version)
Drive-->>Handler: Vec<u8> (root tree proof)
Handler->>Logger: Log info with tx count, height, round, elapsed, root proof
else Height != 1
Handler->>Logger: Log info with tx count, height, round, elapsed
end
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (19)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
packages/rs-drive-abci/src/abci/handler/process_proposal.rs(1 hunks)packages/rs-drive/Cargo.toml(1 hunks)packages/rs-drive/src/drive/system/internal_tree_hashes/mod.rs(1 hunks)packages/rs-drive/src/drive/system/mod.rs(1 hunks)packages/rs-platform-version/Cargo.toml(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: QuantumExplorer
PR: dashpay/platform#2257
File: packages/rs-drive-abci/src/mimic/test_quorum.rs:159-164
Timestamp: 2024-11-20T16:16:01.830Z
Learning: QuantumExplorer prefers not to receive auto-generated messages asking to post on social media.
packages/rs-platform-version/Cargo.toml (1)
Learnt from: QuantumExplorer
PR: dashpay/platform#2431
File: packages/rs-drive/Cargo.toml:55-60
Timestamp: 2025-01-19T07:36:46.042Z
Learning: The grovedb dependencies in packages/rs-drive/Cargo.toml and related files are intentionally kept at specific revisions rather than using the latest stable version, with plans to update them at a later time.
packages/rs-drive/Cargo.toml (1)
Learnt from: QuantumExplorer
PR: dashpay/platform#2431
File: packages/rs-drive/Cargo.toml:55-60
Timestamp: 2025-01-19T07:36:46.042Z
Learning: The grovedb dependencies in packages/rs-drive/Cargo.toml and related files are intentionally kept at specific revisions rather than using the latest stable version, with plans to update them at a later time.
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: Rust packages (dpp) / Formatting
- GitHub Check: Rust packages (dpp) / Unused dependencies
- GitHub Check: Rust packages (dpp) / Tests
- GitHub Check: Rust packages (dpp) / Linting
- GitHub Check: Rust packages (wasm-dpp) / Linting
- GitHub Check: Rust packages (wasm-dpp) / Tests
- GitHub Check: Rust packages (dash-sdk) / Check each feature
- GitHub Check: Rust packages (dash-sdk) / Linting
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (rs-dapi-client) / Linting
- GitHub Check: Rust packages (rs-dapi-client) / Tests
- GitHub Check: Rust packages (rs-dapi-client) / Check each feature
- GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
- GitHub Check: Build Docker images (DAPI, dapi, dapi) / Build DAPI image
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build JS packages / Build JS
- GitHub Check: Rust crates security audit
🔇 Additional comments (4)
packages/rs-platform-version/Cargo.toml (1)
14-14: LGTM! Dependency revision update is consistent.The update to the
grovedb-versiondependency revision aligns with the coordinated updates across other packages and supports the new root tree proof functionality.packages/rs-drive/src/drive/system/mod.rs (1)
6-6: LGTM! Module addition is well-organized.The new
internal_tree_hashesmodule is appropriately placed and follows the existing module organization pattern.packages/rs-drive/Cargo.toml (1)
55-60: LGTM! Coordinated grovedb dependency updates.All grovedb-related dependencies have been consistently updated to the same revision, ensuring compatibility across the codebase. This aligns with the project's practice of maintaining specific dependency revisions.
packages/rs-drive/src/drive/system/internal_tree_hashes/mod.rs (1)
7-20: LGTM! Well-implemented root tree proof method.The
root_tree_proofmethod correctly implements proof retrieval for the entire root tree using an empty path and full range query. The implementation properly handles transactions and follows the existing patterns in the codebase.
| if request.height == 0 { | ||
| let root_proof = app | ||
| .platform() | ||
| .drive | ||
| .root_tree_proof(Some(&transaction), platform_version)?; | ||
| tracing::info!( | ||
| invalid_tx_count, | ||
| valid_tx_count, | ||
| elapsed_time_ms, | ||
| storage_fees, | ||
| processing_fees, | ||
| "Processed proposal with {} transactions for height: {}, round: {} in {} ms, root proof is {}", | ||
| valid_tx_count + invalid_tx_count, | ||
| request.height, | ||
| request.round, | ||
| elapsed_time_ms, | ||
| hex::encode(&root_proof), | ||
| ); | ||
| } else { | ||
| tracing::info!( | ||
| invalid_tx_count, | ||
| valid_tx_count, | ||
| elapsed_time_ms, | ||
| storage_fees, | ||
| processing_fees, | ||
| "Processed proposal with {} transactions for height: {}, round: {} in {} ms", | ||
| valid_tx_count + invalid_tx_count, | ||
| request.height, | ||
| request.round, | ||
| elapsed_time_ms, | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Eliminate code duplication by consolidating the logging logic.
The logging statements are duplicated between the original code (lines 337-348) and the new conditional branches (lines 356-367 and 369-381). This violates the DRY principle and makes maintenance harder.
Consider refactoring to eliminate duplication:
- tracing::info!(
- invalid_tx_count,
- valid_tx_count,
- elapsed_time_ms,
- storage_fees,
- processing_fees,
- "Processed proposal with {} transactions for height: {}, round: {} in {} ms",
- valid_tx_count + invalid_tx_count,
- request.height,
- request.round,
- elapsed_time_ms,
- );
-
if request.height == 0 {
let root_proof = app
.platform()
.drive
.root_tree_proof(Some(&transaction), platform_version)?;
tracing::info!(
invalid_tx_count,
valid_tx_count,
elapsed_time_ms,
storage_fees,
processing_fees,
"Processed proposal with {} transactions for height: {}, round: {} in {} ms, root proof is {}",
valid_tx_count + invalid_tx_count,
request.height,
request.round,
elapsed_time_ms,
hex::encode(&root_proof),
);
} else {
tracing::info!(
invalid_tx_count,
valid_tx_count,
elapsed_time_ms,
storage_fees,
processing_fees,
"Processed proposal with {} transactions for height: {}, round: {} in {} ms",
valid_tx_count + invalid_tx_count,
request.height,
request.round,
elapsed_time_ms,
);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if request.height == 0 { | |
| let root_proof = app | |
| .platform() | |
| .drive | |
| .root_tree_proof(Some(&transaction), platform_version)?; | |
| tracing::info!( | |
| invalid_tx_count, | |
| valid_tx_count, | |
| elapsed_time_ms, | |
| storage_fees, | |
| processing_fees, | |
| "Processed proposal with {} transactions for height: {}, round: {} in {} ms, root proof is {}", | |
| valid_tx_count + invalid_tx_count, | |
| request.height, | |
| request.round, | |
| elapsed_time_ms, | |
| hex::encode(&root_proof), | |
| ); | |
| } else { | |
| tracing::info!( | |
| invalid_tx_count, | |
| valid_tx_count, | |
| elapsed_time_ms, | |
| storage_fees, | |
| processing_fees, | |
| "Processed proposal with {} transactions for height: {}, round: {} in {} ms", | |
| valid_tx_count + invalid_tx_count, | |
| request.height, | |
| request.round, | |
| elapsed_time_ms, | |
| ); | |
| } | |
| if request.height == 0 { | |
| let root_proof = app | |
| .platform() | |
| .drive | |
| .root_tree_proof(Some(&transaction), platform_version)?; | |
| tracing::info!( | |
| invalid_tx_count, | |
| valid_tx_count, | |
| elapsed_time_ms, | |
| storage_fees, | |
| processing_fees, | |
| "Processed proposal with {} transactions for height: {}, round: {} in {} ms, root proof is {}", | |
| valid_tx_count + invalid_tx_count, | |
| request.height, | |
| request.round, | |
| elapsed_time_ms, | |
| hex::encode(&root_proof), | |
| ); | |
| } else { | |
| tracing::info!( | |
| invalid_tx_count, | |
| valid_tx_count, | |
| elapsed_time_ms, | |
| storage_fees, | |
| processing_fees, | |
| "Processed proposal with {} transactions for height: {}, round: {} in {} ms", | |
| valid_tx_count + invalid_tx_count, | |
| request.height, | |
| request.round, | |
| elapsed_time_ms, | |
| ); | |
| } |
🤖 Prompt for AI Agents
In packages/rs-drive-abci/src/abci/handler/process_proposal.rs between lines 350
and 382, the logging code is duplicated in both branches of the if-else
statement, which violates the DRY principle. Refactor by creating a single
logging statement after the conditional block, setting a variable for the
message or additional info if needed, and then calling the log function once.
This will eliminate redundancy and improve maintainability.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/rs-drive/src/drive/system/root_tree_proof/mod.rs (1)
7-7: Consider expanding the documentation to be more descriptive.The current documentation is minimal. Consider adding more details about what the root tree proof contains, when it should be used, and what the returned bytes represent.
- /// Returns a tree proof of the root of the tree. + /// Returns a tree proof of the root of the tree. + /// + /// This proof contains all items at the root level of the tree and can be used + /// to verify the integrity of the root tree structure. The returned bytes represent + /// a serialized proof that can be verified against the root hash.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/rs-drive/src/drive/system/mod.rs(1 hunks)packages/rs-drive/src/drive/system/root_tree_proof/mod.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/rs-drive/src/drive/system/mod.rs
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: QuantumExplorer
PR: dashpay/platform#2257
File: packages/rs-drive-abci/src/mimic/test_quorum.rs:159-164
Timestamp: 2024-11-20T16:16:01.830Z
Learning: QuantumExplorer prefers not to receive auto-generated messages asking to post on social media.
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: Rust packages (wasm-dpp) / Tests
- GitHub Check: Rust packages (wasm-dpp) / Detect immutable structure changes
- GitHub Check: Rust packages (dpp) / Formatting
- GitHub Check: Rust packages (dpp) / Linting
- GitHub Check: Rust packages (dpp) / Tests
- GitHub Check: Rust packages (dpp) / Unused dependencies
- GitHub Check: Rust packages (dpp) / Check each feature
- GitHub Check: Rust packages (dash-sdk) / Linting
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (dash-sdk) / Check each feature
- GitHub Check: Rust packages (dash-sdk) / Unused dependencies
- GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
- GitHub Check: Build Docker images (DAPI, dapi, dapi) / Build DAPI image
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build JS packages / Build JS
- GitHub Check: Rust crates security audit
🔇 Additional comments (2)
packages/rs-drive/src/drive/system/root_tree_proof/mod.rs (2)
1-5: LGTM! Imports are appropriate for the implementation.All imported types are properly utilized in the method implementation.
13-19:✅ Verification successful
What is the purpose of the mutable empty vector parameter?
The code passes
&mut vec![]togrove_get_proved_path_query. This creates a mutable reference to an empty vector that appears to be discarded after the call. Could you clarify the purpose of this parameter?
🏁 Script executed:
#!/bin/bash # Description: Find the signature of grove_get_proved_path_query to understand the purpose of the mutable vector parameter # Search for the method definition ast-grep --pattern 'fn grove_get_proved_path_query($$$)' # Also search with ripgrep for more context rg -A 10 "fn grove_get_proved_path_query"Length of output: 5703
🏁 Script executed:
#!/bin/bash # Inspect usage of drive_operations within grove_get_proved_path_query_v0 rg -n "drive_operations" -A 5 -B 5 packages/rs-drive/src/util/grove_operations/grove_get_proved_path_query/v0/mod.rsLength of output: 1118
Clarify
drive_operationsparameter usageThe
&mut Vec<LowLevelDriveOperation>you pass in is used to collect low-level drive operations—in this case aCalculatedCostOperation(cost)pushed at line 27 ofgrove_get_proved_path_query_v0. You’re creating an empty vector because you don’t need to retain those operations (e.g., cost) here.No changes are strictly required, but you may consider:
- Naming the vector (
let mut drive_ops = Vec::new()) if you later intend to inspect the cost.- Leaving a brief comment to explain that an empty vec is passed when operation recording isn’t needed.
|
Should we do a new GroveDB release? |
|
No longer need this. |
2 participants
Issue being fixed or feature implemented
This PR updates the
grovedbdependency to a new revision and adds logging for the genesis block proof.What was done?
grovedbdependency version across multiple files to the new revision.process_proposal.rsto include root proof information when processing proposals at height 0.How Has This Been Tested?
The changes have been tested through existing unit tests and integration tests that cover the proposal processing functionality.
Breaking Changes
None
Checklist
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Chores