fix(common): reject incorrectly signed genesis transactions #245
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
WalkthroughThe changes refine transaction validation within the Changes
Sequence Diagram(s)sequenceDiagram
participant T as Transaction
participant A as Account
T->>A: Call validate_transaction(tx)
A->>A: Validate transaction ID
alt Invalid Transaction ID
A->>A: Use bail! macro for error reporting
A-->>T: Return error
else Valid Transaction ID
A->>A: Validate signing key
alt Invalid Key
A->>A: Use bail! macro for error reporting
A-->>T: Return error
else Valid Key
A->>A: Destructure operation & compare fields
A-->>T: Return result (success/error)
end
end
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}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
crates/common/src/api/mod.rs (2)
32-32: Consider returning more contextual info.
post_transactionreturnsResult<(), Self::Error>. Depending on usage, returning a transaction ID or an object tracking the posted transaction might be more useful for debugging or chaining.
34-40: Method name clarity.
post_transaction_and_waitreturns aPendingTransactionrather than waiting synchronously. A more descriptive name, e.g.post_transaction_and_track, may reduce confusion for users expecting a fully blocking call.crates/client/src/timer.rs (1)
1-11: Consider consolidating duplicate timer implementations.This implementation is identical to
ProverTokioTimer. Consider moving the timer implementation to a common location to avoid code duplication.Potential approaches:
- Move to
prism_common::timermodule- Create a macro to generate the implementation
crates/common/src/api/noop.rs (2)
28-32: Consider preserving error details in From implementation.The current implementation discards the original
TransactionErrordetails. Consider preserving these details for better error reporting during testing.-impl From<TransactionError> for NoopPrismError { - fn from(_: TransactionError) -> Self { - NoopPrismError - } -} +#[derive(Debug)] +pub enum NoopPrismError { + Transaction(TransactionError), + Api(&'static str), +} + +impl From<TransactionError> for NoopPrismError { + fn from(err: TransactionError) -> Self { + NoopPrismError::Transaction(err) + } +}
15-15: Add documentation explaining testing use case.Please add documentation to explain how these no-op implementations should be used in tests. This will help other developers understand the intended usage pattern.
+/// A no-op implementation of PrismApi used for testing. +/// All API methods return errors, allowing tests to verify error handling paths. pub struct NoopPrismApi {} +/// A no-op timer implementation that does nothing when sleep is called. pub struct NoopTimer;Also applies to: 34-34, 40-56
crates/node_types/prover/src/prover/mod.rs (1)
493-520: Consider additional transaction validation checks.While the current validation is good, consider adding:
- Rate limiting to prevent DoS attacks
- Transaction size limits
- Input sanitization for the transaction ID
📜 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 (21)
Cargo.toml(0 hunks)crates/api/Cargo.toml(0 hunks)crates/api/src/lib.rs(0 hunks)crates/client/Cargo.toml(1 hunks)crates/client/src/http_client.rs(3 hunks)crates/client/src/lib.rs(1 hunks)crates/client/src/timer.rs(1 hunks)crates/common/Cargo.toml(1 hunks)crates/common/src/account.rs(3 hunks)crates/common/src/api/mod.rs(6 hunks)crates/common/src/api/noop.rs(1 hunks)crates/common/src/api/types.rs(1 hunks)crates/common/src/builder.rs(9 hunks)crates/common/src/lib.rs(1 hunks)crates/common/src/tests.rs(1 hunks)crates/common/src/transaction.rs(2 hunks)crates/node_types/prover/Cargo.toml(0 hunks)crates/node_types/prover/src/prover/mod.rs(3 hunks)crates/node_types/prover/src/prover/timer.rs(1 hunks)crates/node_types/prover/src/webserver.rs(1 hunks)crates/tree/src/proofs.rs(0 hunks)
💤 Files with no reviewable changes (5)
- crates/api/src/lib.rs
- crates/api/Cargo.toml
- crates/node_types/prover/Cargo.toml
- Cargo.toml
- crates/tree/src/proofs.rs
✅ Files skipped from review due to trivial changes (2)
- crates/common/src/api/types.rs
- crates/node_types/prover/src/webserver.rs
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: unit-test
- GitHub Check: integration-test
- GitHub Check: clippy
- GitHub Check: build-and-push-image
- GitHub Check: unused dependencies
🔇 Additional comments (49)
crates/common/src/api/mod.rs (11)
1-2: Exporting submodules is a good organizational step.
By creating separate modules (noopandtypes), you keep your API concerns neatly separated.
6-6: Ensure consistent usage of time and futures.
Usingstd::future::Futureandstd::time::Durationis correct. Verify that you consistently rely on these standard library imports versustokioor other async runtimes everywhere.
9-9: New imports streamline references to core Prism domain objects.
These imports properly referenceAccount,SignatureBundle,Transaction, andTransactionError, along with new response types intypes. Everything appears consistent with the updated architecture.Also applies to: 11-11, 12-12, 14-14
16-18: Timer trait addition looks solid.
Introducing thePrismApiTimertrait provides a nice abstraction for sleeping and is conducive to testing.
25-26: Expanded type bounds ensure concurrency safety.
Requiring theErrortype to beSend + Sync + 'staticimproves async compatibility. Specifyingtype Timer: PrismApiTimer;clarifies usage of the new timer abstraction.
42-42: Updated builder invocation is consistent.
Switching toRequestBuilder::new_with_prism(self)aligns well with the optional builder pattern introduced elsewhere.
86-86: Renamed method call is clearer.
Changing from something likemodify_existing()toto_modify_account()clarifies intent. Nice improvement in method naming.
100-100: Consistent rename for revoke path.
Using.to_modify_account(account)consistently maintains uniform naming across account-modifying methods.
115-115: Uniform account-modification approach.
Adopting.to_modify_account(account)for adding data continues the tidy rename pattern.
130-130: Same rename for set_data.
The.to_modify_accountfunction usage remains consistent, ensuring clarity across all account modifications.
171-171: Leverage the trait timer abstraction correctly.
Replacing direct calls totokio::time::sleepwithP::Timer::sleepaids in testing and custom runtime support.crates/common/src/builder.rs (22)
1-1: Imports are aligned with your updated domain.
Bringing inSigningKeyandVerifyingKeyfromprism_keysis consistent with the rest of the changes in this builder.
3-3: Imports from crate remain organized.
The consolidated references toaccount,api,digest, etc., keep dependencies well structured.
5-5: Integration withNoopPrismApiandPrismApiis well-defined.
Letting the builder handle both a no-op and a real API scenario is flexible.
11-12: Default type parameter and optional reference approach.
DefiningRequestBuilder<'a, P = NoopPrismApi>withprism: Option<&'a P>fosters a simpler fallback scenario. Just watch for accidental usage of the default in production code.
19-21: Allowing creation of a bare builder is logical.
pub fn new() -> Self { prism: None }paves the way for usage without a real API. This can be beneficial in unit tests or placeholders.
23-24: Dedicated constructor for real references.
new_with_prismclarifies usage when an actualPrismApiis available.
35-37: Refined method name provides clarity.
to_modify_accountbetter communicates the intention to perform an account-modifying operation.
40-48: Default implementation aligns withnew().
Falling back on the bare constructor in theimpl Defaultblock ensures consistent instantiation.
53-53: Optionalprismfield is consistent.
AdoptingOption<&'a P>across related builders unifies handling for both real and no-op APIs.
63-63: Creation pattern with optional references remains uniform.
CreateAccountRequestBuilder::new(prism: Option<&'a P>)is straightforward.
128-128: Same optional approach for RegisterService.
Ensures cohesive usage ofprism: Option<&'a P>in all builders.
137-137: Named constructor matches pattern.
Maintains code symmetry acrossCreateAccountRequestBuilder,RegisterServiceRequestBuilder, and so on.
187-187: Adhering to optional APIs for ModifyAccount.
Continuing the same optional reference pattern fosters consistency.
196-200: Retrieving account details for the operation.
Populatingidandnoncefromaccountis convenient. The rename toto_modify_accountaligns with other methods.
303-303: SigningTransaction builder continues the optional pattern.
Havingprism: Option<&'a P>is consistent with prior changes.
311-311: Dedicated constructor for the signing builder.
The pattern remains uniform across all builder variants.
326-328: Public getter for unsigned transactions improves usability.
Exposingtransaction()is handy for logging or debugging.
335-335: Same optional reference approach for sending transactions.
Seamless integration with the previously introduced builder flow.
343-343: Constructor ensures a uniform API.
Keeping the setup consistent across all builder layers.
348-350: Graceful error when no API is provided.
This early return withTransactionError::MissingSenderis user-friendly and explicit.
352-352: Chaining topost_transaction_and_wait.
Ties right back into your updatedPrismApilogic.
355-357: Public getter for the final transaction.
Provides easy access to the signed transaction if needed downstream.crates/common/src/lib.rs (2)
2-3: Exposingapiandbuilderfrom the common crate.
Bringing these modules in centralizes functionality previously scattered in separate crates, simplifying your workspace structure.
11-12: Integrated test module.
The test module under#[cfg(test)]ensures this crate’s extended functionality is validated in situ.crates/node_types/prover/src/prover/timer.rs (1)
1-11: LGTM! Clean and efficient timer implementation.The implementation is minimal, focused, and correctly uses tokio's async primitives. The
Sendbound ensures thread safety for the returned future.crates/client/src/lib.rs (1)
2-2: LGTM! Clean module organization.The changes align well with the refactoring to remove the prism_api crate. The module organization is clean and the imports are properly updated.
Also applies to: 7-8
crates/common/src/transaction.rs (1)
87-87: LGTM! New error variant for missing sender.The addition of the
MissingSendervariant with its display implementation enhances error handling for genesis transactions.Also applies to: 98-98
crates/client/src/http_client.rs (2)
73-73: Verify the necessity of Timer type addition.The addition of
Timerassociated type seems unrelated to the PR's objective of fixing incorrectly signed genesis transactions.Could you explain how this change contributes to the PR's goal?
112-112: LGTM! Error trait implementation.Implementing
Errortrait forPrismHttpClientErrorimproves error handling capabilities.crates/common/src/tests.rs (2)
5-68: LGTM! Comprehensive test coverage for service registration.Test cases thoroughly verify transaction processing including:
- Successful registration
- Invalid nonce handling
- ID mismatch detection
- Invalid signature rejection
70-137: LGTM! Comprehensive test coverage for account creation.Test cases thoroughly verify transaction processing including:
- Successful account creation
- Invalid nonce handling
- ID mismatch detection
- Invalid signature rejection
crates/common/src/account.rs (3)
73-84: LGTM! Builder methods for request creation.The addition of
builderandbuilder_via_apimethods provides clear separation between local testing and API-based request creation.
88-99: LGTM! Account modification builders.The addition of
modifyandmodify_via_apimethods provides consistent patterns for account modifications.
120-129: LGTM! Improved transaction validation.The updated validation logic for
CreateAccountandRegisterServiceoperations ensures proper ID and key matching.crates/node_types/prover/src/prover/mod.rs (2)
1-14: LGTM! Clean refactoring of imports.The reorganization of imports and addition of the timer module improves the code structure.
523-527: LGTM! Timer type implementation looks good.The implementation of
TimerasProverTokioTimeraligns well with the async architecture.crates/client/Cargo.toml (1)
19-20: LGTM! Tokio workspace dependency added.The addition of tokio as a workspace dependency aligns with the async architecture improvements.
crates/common/Cargo.toml (1)
13-15: LGTM! Async-trait dependency added.The addition of async-trait as a workspace dependency is necessary for the async trait implementations.
Without that fix, genesis transactions can be signed with a key different to the one being added. Also, the equality of operation and transaction ids is can be checked here. Currently, it is checked in snarkable_tree, but we have all required information for the check in this Account method too - so adding it here.
There are similar tests in the tree crate, but these are testing tree specific functions (e.g. proof generation). Introducing a file for tests restricted to the scope of the common crate here (e.g. tests that do not care about database state, or proving).
jns-ps
force-pushed
the
reject-incorrectly-signed-transactions
branch
from
946ca79 to
f18a733
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
crates/common/src/tests.rs (3)
5-68: LGTM! Consider enhancing error messages and edge cases.The test function provides good coverage of service registration transaction validation, including the critical case of invalid signatures.
Consider these improvements:
- Add descriptive error messages to assertions:
- assert!(Account::default().process_transaction(&invalid_tx).is_err()); + assert!(Account::default().process_transaction(&invalid_tx).is_err(), + "Transaction with invalid nonce should be rejected");
- Add edge cases:
- Empty service ID
- Very long service ID
- Null/empty keys
70-137: LGTM! Consider test refactoring and additional cases.The test function provides good coverage of account creation transaction validation, maintaining consistency with the service registration tests.
Consider these improvements:
- Extract common test setup into helper functions to reduce duplication:
fn create_test_keys() -> (SigningKey, SigningKey) { (SigningKey::new_ed25519(), SigningKey::new_ed25519()) } fn create_invalid_nonce_tx(acc_key: &SigningKey, service_key: &SigningKey) -> Transaction { let mut tx = Account::builder() .create_account() // ... rest of the builder chain ... .transaction(); tx.nonce = 1; tx.sign(acc_key).unwrap() }
- Add edge cases:
- Empty account ID
- Very long account ID
- Non-existent service ID
- Invalid service challenge signature
1-138: Consider organizing tests into sub-modules.The test file would benefit from organizing related tests into sub-modules for better maintainability.
Consider this structure:
#[cfg(test)] mod transaction_validation_tests { mod service_registration { #[test] fn test_process_register_service_transactions() { // existing test } // Additional specific service registration tests } mod account_creation { #[test] fn test_process_create_account_transactions() { // existing test } // Additional specific account creation tests } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/common/src/account.rs(2 hunks)crates/common/src/lib.rs(1 hunks)crates/common/src/tests.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/common/src/lib.rs
- crates/common/src/account.rs
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: unit-test
- GitHub Check: unused dependencies
- GitHub Check: integration-test
- GitHub Check: Analyze (javascript-typescript)
jns-ps
changed the title
Merge after #244
Summary by CodeRabbit
Refactor
Tests