feat: replacing hashchains with accounts #180
Conversation
WalkthroughThis pull request introduces a significant architectural shift from a hashchain-based system to an account-based system across multiple crates in the Prism project. The primary changes involve replacing Changes
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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}
jns-ps
force-pushed
the
tree-in-own-crate
branch
from
120e664 to
46e2e12
Compare
distractedm1nd
force-pushed
the
no-hashchains
branch
from
8373f81 to
6bf3c97
Compare
distractedm1nd
force-pushed
the
no-hashchains
branch
from
6bf3c97 to
85c7132
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (23)
crates/common/src/transaction.rs (4)
1-3: Consider grouping imports or clarifying usage.
While the imports appear correct, consider whether any of these can be relocated or grouped for clarity. This is not strictly required, but it can help maintain consistent organization across the codebase.
10-23: Validate and document constraints on Transaction fields.
The added fields (operation, nonce, signature, vk) and the transaction ID are logically consistent. However, consider improving the doc comments to state whether any of these fields can be empty, or if they must satisfy domain-specific rules (e.g., ID format).
25-31: Check error context when encoding the transaction to bytes.
The use of anyhow!(e) is acceptable, but you might consider adding more descriptive context about what encoding step failed, especially for downstream debugging.
33-43: Validate multiple signatures scenario.
The method prevents multiple signing attempts by returning an error if already signed. If multi-signature transactions will be needed in the future (e.g., threshold-based), consider extending or refactoring the "already signed" logic to accommodate additional signers.crates/tree/src/proofs.rs (3)
29-37: Naming consistency for InsertProof.
The InsertProof struct's field changed from new_entry to tx (Transaction). This is good, but consider if renaming the field to something more descriptive like inserted_tx might provide better clarity.
53-56: Operation correctness check.
The code calls account.process_transaction(&self.tx)? which processes the transaction. Ensure that new accounts require no pre-existing state that might break or partially bypass the intended account creation path.
89-99: Nonce mismatch or double-spend checks.
While the code verifies existence of old_account, confirm that any logic for preventing double-spend or replay attacks is handled (for example, by incrementing the nonce in the account).crates/common/src/account.rs (5)
1-5: Check high-level architecture.
This file introduces the Account struct and related logic. The layering of validations, post-processing, and transaction creation is a major pivot away from hashchains. Make sure cross-file references (e.g., in proofs.rs) remain in sync with these new account-based workflows.
14-34: Data privacy concerns.
The Account struct stores everything in plaintext, including valid_keys and any signed_data. If any of these fields are sensitive, consider encryption or a future approach that separates private data from publicly accessible data.
36-75: prepare_transaction signature / ID mismatch.
prepare_transaction sets tx.id = account_id (passed in) and tx.nonce = self.nonce. That’s correct, but if the caller accidentally passes a conflicting account_id, it’s not validated here. You might want a final check that theaccount_idis the same asself.idunless it’s a create-from-empty scenario.
89-116: Transaction validation logic.
You check nonce equality, ID match, verifying key membership, and signature validity. This is solid. Further checks might be needed for multi-signature scenarios or advanced operations.
191-194: is_empty definition.
The function checks only if self.nonce == 0. If an account was created but had zero transactions, the account might have valid_keys but still no transactions. Consider adjusting how you define “empty.”crates/node_types/prover/src/webserver.rs (6)
69-69: Updated UserKeyResponse.
Replacing the hashchain field with account: Option is good. Consider including the account nonce or some minimal info for partially uninitialized accounts.
158-160: Validate queued transaction usage.
You call session.validate_and_queue_update(tx_request.0). Ensure that server logs or metrics reflect success/failure, especially if the user needs immediate confirmation.
174-180: Doc comment references old name.
The doc comment mentions “The /get-account endpoint returns all added keys.” That is correct; verify that references to “hashchain” are fully removed across docstrings.
187-189: Error boundary.
The function returns 500 for internal errors but 200 for both found and not found. Though that’s valid, consider whether a 404 might be more appropriate for an absent account, depending on the design constraints.
191-197: Better error detail for membership retrieval.
You print a “Failed to retrieve account…” message. If it’s truly an internal issue, you might want to suppress error details from the user or clarify it’s a server-side failure.Would you like a more refined approach to error handling and user-facing messages?
203-215: Use distinct response codes for found vs. not found.
While returning 200 for both found and not found is permissible, it may be helpful to differentiate these states for client logic. For example, 404 for an absent account might be more standard.crates/tree/src/snarkable_tree.rs (4)
24-24: Clarify doc comments
Consider incorporating examples or usage instructions in the doc comments to help maintainers and newcomers see how these methods should be used in practice.
39-44: Shared logging level check
The debug-level logs here are potentially helpful for debugging. If these operations occur frequently, consider using trace-level logs to avoid noise at scale, or keep them as debug if performance overhead is limited.
136-136: Preserving transaction context
Storing the transaction in the proof might be useful for debugging or verification. Just remember to keep an eye on payload sizes.
168-168: Storing old_account
Retaining the old account helps with rollbacks or debug logs. Ensure you handle large accounts carefully in memory.crates/common/src/transaction_builder.rs (1)
52-53: Document simulated storage
The inline comment is helpful. Consider letting users know about the ephemeral nature of this HashMap (i.e., not persisted in production).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
crates/common/src/account.rs(1 hunks)crates/common/src/hashchain.rs(0 hunks)crates/common/src/lib.rs(1 hunks)crates/common/src/operation.rs(1 hunks)crates/common/src/transaction.rs(1 hunks)crates/common/src/transaction_builder.rs(14 hunks)crates/node_types/prover/src/prover/mod.rs(3 hunks)crates/node_types/prover/src/prover/tests.rs(2 hunks)crates/node_types/prover/src/webserver.rs(7 hunks)crates/tree/src/key_directory_tree.rs(1 hunks)crates/tree/src/lib.rs(1 hunks)crates/tree/src/proofs.rs(2 hunks)crates/tree/src/snarkable_tree.rs(4 hunks)crates/tree/src/tests.rs(9 hunks)
💤 Files with no reviewable changes (1)
- crates/common/src/hashchain.rs
✅ Files skipped from review due to trivial changes (2)
- crates/common/src/lib.rs
- crates/tree/src/key_directory_tree.rs
🧰 Additional context used
📓 Learnings (1)
crates/common/src/operation.rs (1)
Learnt from: distractedm1nd
PR: deltadevsde/prism#141
File: crates/common/src/operation.rs:311-313
Timestamp: 2024-11-12T11:47:59.930Z
Learning: When implementing `AddSignedData` in `crates/common/src/operation.rs`, ensure that signatures cover the entire operation (including metadata like account ID) to prevent replay attacks or unauthorized data modifications. Including a `value_signature: Option<(VerifyingKey, Signature)>` allows for externally signed data while ensuring proper validation.
🔇 Additional comments (44)
crates/common/src/transaction.rs (1)
7-9: Confirm that Operation usage is correct.
It's good to see a strongly typed import for the Operation enum. Ensure all possible variants of Operation are covered in serialization & usage.
crates/tree/src/proofs.rs (4)
6-6: Imports check.
The switch from Hashchain to Account and Transaction is clear. Ensure that no leftover references to old hashchain logic remain in other proof types.
51-52: Confirm nonexistence proof correctness.
The verify_nonexistence call ensures the key doesn't yet exist. Make sure logic is consistent with typical usage for "create account" flows, especially if there's any requirement to validate that the ID is truly unoccupied.
69-81: Ensure that update proofs handle edge cases.
The UpdateProof structure references old_account and a transaction. Verify that partial updates or invalid operation sequences are properly rejected.
113-125: MerkleProof Implementation Considerations.
The rename of MembershipProof to MerkleProof is good from a clarity standpoint. Just confirm usage thoroughly across the codebase. Also, the new verify_nonexistence method is nicely separated, but watch for any boundary cases where the proof might need to confirm partial matches.
crates/common/src/account.rs (4)
6-9: Ensure correct crate references.
The usage of crate::operation::{Operation, ServiceChallenge} and crate::transaction::Transaction is consistent with the new design. Carefully track if any other crates still reference removed hashchain code.
11-13: SignedData usage.
The SignedData struct is introduced here. Double-check that all validation logic (especially around verifying key usage) is either performed in process_transaction or in relevant places to avoid silent acceptance of invalid data.
118-150: Operation-level validation coverage.
You handle AddKey, RevokeKey, AddData, and account creation operations. Good coverage. Keep an eye out for potential future expansions (e.g., removing data, special role-based constraints, etc.).
152-189: Order of validation vs. writing changes.
process_operation first calls validate_operation, then performs the operation. That helps safety. Confirm that the operation references (e.g., creation_gate in RegisterService) are always properly validated if there are ownership or authorization constraints.
crates/node_types/prover/src/webserver.rs (3)
11-15: Imports from prism_common and prism_tree.
Replacing hashchain references with account-based references looks consistent. Double-check if any leftover references to the old Hashchain logic remain in other parts of the crate.
97-97: API endpoints listing.
Paths references have changed: post_transaction, get_account, get_commitment. Confirm that older references or docs to /get-hashchain are updated in all relevant documentation and tests.
122-122: Route changes.
Endpoints now reference /get-account. Good alignment with the shift away from hashchain.
crates/tree/src/snarkable_tree.rs (9)
11-21: Introduction of Account & AccountResponse
These updates are consistent with the transition from Hashchain to Account structures, aligning well with the broader architectural shift.
71-76: Edge case: nonexistent service account
This code properly checks that the service account exists and has a valid challenge. This ensures that you don’t create a user account referencing a nonexistent or invalid service.
83-90: Signature verification logic
Verifying the signature using service_pubkey is a good security measure. Also, logging the account creation event can help trace suspicious operations if needed.
101-103: Consistent flow for new account creation
Registering a new service-based account is cohesive with the rest of the flow. The approach is clear and aligns with typical account creation patterns.
109-128: Insert method logic
This flow looks correct:
- Check for an existing entry,
- Build a non-membership proof,
- Process the transaction to populate an empty account,
- Insert with proof into the JMT.
Nice job!
121-123: Potential advanced validations
While processing the transaction, consider if you need additional validations for edge conditions (e.g., preventing negative balances if your domain involves currency, max key count, etc.).
142-155: Update method clarity
This code is well-structured:
• Retrieves the old account.
• Processes the transaction into a new account state.
• Commits changes and returns proofs.
Clean approach!
175-186: Get method design
• The membership vs non-membership proof dichotomy is clear.
• Perfect for zero-knowledge scenarios.
No issues spotted.
29-31: Method signature alignment
The updated insert, update, and get methods reflect the new transaction-based flow, which appears consistent with the rest of the codebase. Ensure you have updated all call sites to match these signatures.
crates/common/src/transaction_builder.rs (12)
4-6: Transition from Hashchain to Account
Replacing hashchain references and focusing on account-based operations is consistent with the new design. Imports also look correct.
26-29: Maintaining simulation
By updating the builder’s in-memory account and calling process_transaction, the code ensures coherence between the in-memory simulation and final transactions.
62-67: TransactionBuilder default initialization
This straightforward approach for default initialization is good. No immediate concerns.
79-80: Renamed get_account
The simpler naming scheme is clearer than the original get_hashchain approach. Good improvement.
96-103: Register service flow
The new approach using Operation::RegisterService with an externally signed challenge is consistent with the rest of the account-based design.
156-164: create_account logic
Ensuring the challenge signature is valid helps avoid unauthorized account creation.
178-183: Key addition
Using add_random_key with an account signing key fosters secure random key generation.
253-253: add_randomly_signed_data
Creating a new ephemeral key to sign the data helps clarify the difference between data-signer keys vs. account keys. Good approach for minimal coupling.
311-320: add_internally_signed_data
The naming suggests internal signing within the same account. Straightforward logic.
324-336: Data verification
Excellent approach to ensure the correct account key is used for verifying data.
358-367: Partitioning add_data
This method is well-defined for a final step after building up the data and signatures. The Operation::AddData usage looks correct.
204-207: Graceful fallback with unwrap_or_default
If the account doesn’t exist, we create a default. Make sure that default is valid in your domain context. Otherwise, you may store partial or null states.
✅ Verification successful
Let me gather more information about the Account struct and its default implementation.
Let me try a different approach to find the Account struct and its implementation.
Let me check for the Default implementation in the entire account.rs file since it might be defined separately.
Default implementation for Account is safe and appropriate
The Account struct has a #[derive(Default)] attribute and its fields are well-suited for default initialization: empty string for id, 0 for nonce, empty vectors for valid_keys and signed_data, and None for service_challenge. This default state aligns with the account creation flow, as verified by the is_empty() method which checks if nonce is 0, and the validation in process_operation() which only allows CreateAccount/RegisterService operations on empty accounts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Searching for usage of unwrap_or_default() on Account to see if partial states are handled
rg "unwrap_or_default" -A 5
Length of output: 5175
Script:
#!/bin/bash
# Search for Account struct definition and Default implementation
ast-grep --pattern 'struct Account {
$$$
}'
# Also search for Default implementation
ast-grep --pattern 'impl Default for Account {
$$$
}'
Length of output: 105
Script:
#!/bin/bash
# Search for Account related code using ripgrep
rg "struct Account" -A 10
rg "impl Default for Account" -A 5
rg "impl Account" -A 10
Length of output: 1232
Script:
#!/bin/bash
# Get the full content of account.rs to see the Default implementation
cat crates/common/src/account.rs
Length of output: 6126
crates/node_types/prover/src/prover/mod.rs (4)
5-5: Importing Account
Replacing the old Hashchain import is coherent with the broader shift.
13-13: Renamed AccountResponse
It helps unify the code by removing hashchain references. Good naming.
457-457: get_account
This method complements the new account-based architecture, following the pattern established in snarkable_tree.rs.
479-491: validate_and_queue_update
Nice approach to early-validate transactions (checking signatures, existence, etc.) before queueing them. This design helps keep the system more robust against invalid inputs.
crates/tree/src/lib.rs (3)
6-7: Replaced old proofs
Using MerkleProof instead of membership or non-membership proofs with separate types is simpler and consistent with the JMT usage.
11-13: Updated enum variant
Renaming from HashchainResponse to AccountResponse is fully aligned with the architectural change.
15-16: Non-membership proofs
Swapping NonMembershipProof with MerkleProof for the NotFound variant results in a uniform proof type. This reduces code duplication and clarifies usage.
crates/common/src/operation.rs (1)
30-30: LGTM: Enforcing signatures for data operations.
Making data_signature non-optional aligns with security best practices by ensuring all data operations are properly signed, preventing unauthorized modifications.
crates/node_types/prover/src/prover/tests.rs (2)
80-82: LGTM: Cleaner key retrieval approach.
Direct access to the operation's public key through operation.get_public_key() improves code readability.
106-106: Verify key management changes.
The removal of the key index parameter suggests a change in key management. Please ensure this aligns with the new account-based architecture.
✅ Verification successful
Let me gather more information about the key management implementation to verify the changes.
Based on the search results, I can now generate the final response:
Key management changes align with account-based architecture
The removal of the key index parameter is consistent with the new account-based architecture, as evidenced by:
- The
add_random_keyimplementation accepts account ID and signing key parameters, focusing on account-based identification rather than key indices - Test cases show proper usage with account identifiers like "acc_1", "acc_2" instead of numeric indices
- No remaining key index references found in the key management code paths
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify key management implementation across codebase
# Check for any remaining key index references
rg -A 3 "add_random_key.*\d+"
# Check the new key management implementation
ast-grep --pattern 'fn add_random_key($params) {
$$$
}'
Length of output: 2187
Script:
#!/bin/bash
# Search for the implementation of add_random_key and related methods
ast-grep --pattern 'impl $_ {
$$$
add_random_key($$$) {
$$$
}
$$$
}'
# Look for the transaction builder implementation
rg -A 5 "struct.*Builder"
# Check for any key index related code
rg -A 3 "key.*index|index.*key"
Length of output: 66312
crates/tree/src/tests.rs (1)
191-193: Verify signature validation for internally signed data.
The new test case for internally signed data should ensure proper signature validation to prevent unauthorized modifications.
✅ Verification successful
From the search results, I can see that internally signed data goes through proper signature validation in multiple places:
- In
crates/common/src/account.rs, thevalidate_operationmethod verifies signatures for data operations:
if !self.valid_keys().contains(&data_signature.verifying_key) {
data_signature
.verifying_key
.verify_signature(data, &data_signature.signature)?;
}- In
crates/common/src/transaction_builder.rs, theadd_internally_signed_data_verified_with_rootmethod creates a proper signature bundle:
let bundle = SignatureBundle {
verifying_key: account_signing_key.verifying_key(),
signature: account_signing_key.sign(&value),
};- The transaction itself is also validated in
validate_transactionmethod which verifies the transaction signature:
let msg = tx.get_signature_payload()?;
tx.vk.verify_signature(&msg, &tx.signature)?;Signature validation is properly implemented for internally signed data
The codebase shows comprehensive signature validation for internally signed data through multiple layers:
- Data-level signature verification
- Transaction-level signature verification
- Proper key management and validation against account's valid keys
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify signature validation implementation for internally signed data
# Check signature validation implementation
ast-grep --pattern 'fn add_internally_signed_data($params) {
$$$
$verify_expr
$$$
}'
Length of output: 92
Script:
#!/bin/bash
# Let's try a broader search for internally signed data related functions
ast-grep --pattern 'add_internally_signed_data'
# Also search for the implementation with ripgrep for better context
rg "add_internally_signed_data" -A 10
# Let's also check for any verification or signature validation related code
rg "verify|signature|validate" --type rust -A 5
Length of output: 71116
WIP
Summary by CodeRabbit
New Features
Accountstruct for managing account states and transactions.Accountstruct.Transactionstruct with new fields and signing methods.Bug Fixes
Documentation
Refactor
Hashchainrelated structures and methods, replacing them with account-related functionality throughout the codebase.Tests