fix(prover): test_prover_fullnode_commitment_sync_with_racing_transactions #356
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Caution Review failedThe pull request is closed. WalkthroughThe test logic in Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Function
participant Prover as Prover Node
participant Fullnode as Fullnode
Test->>Prover: Submit initial transactions
Test->>Test: Sleep 150ms
loop Up to 10 iterations
Test->>Fullnode: Check sync status
Test->>Test: Sleep 500ms
end
loop Until commitments match
Test->>Prover: Get commitment
Test->>Fullnode: Get commitment
alt Commitments match
Test->>Test: Proceed
else
Test->>Test: Sleep 200ms
end
end
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✨ Finishing Touches
🧪 Generate Unit Tests
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: 0
🧹 Nitpick comments (1)
crates/node_types/prover/src/prover/tests/mod.rs (1)
2-2: Remove unused import.The
digest::Digestimport is not used in this file and should be removed to clean up the imports.-use prism_common::{digest::Digest, test_transaction_builder::TestTransactionBuilder}; +use prism_common::test_transaction_builder::TestTransactionBuilder;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/node_types/prover/src/prover/tests/mod.rs(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: jns-ps
PR: deltadevsde/prism#147
File: crates/node_types/prover/src/prover/tests.rs:120-129
Timestamp: 2024-10-25T08:50:21.262Z
Learning: Avoid including debug print statements in test functions in `crates/node_types/prover/src/prover/tests.rs`.
Learnt from: jns-ps
PR: deltadevsde/prism#147
File: crates/common/src/test_ops.rs:27-44
Timestamp: 2024-10-29T08:34:32.445Z
Learning: In `crates/common/src/test_ops.rs`, methods in test code are deliberately kept short for brevity, even if this results in abbreviated method names.
Learnt from: distractedm1nd
PR: deltadevsde/prism#146
File: crates/common/src/tree.rs:114-126
Timestamp: 2024-11-01T07:52:07.324Z
Learning: In `UpdateProof::verify` within `crates/common/src/tree.rs`, the `old_hashchain` and `hashchain_after_update` are different hashchains, so caching their serialized values is not feasible.
crates/node_types/prover/src/prover/tests/mod.rs (5)
Learnt from: jns-ps
PR: deltadevsde/prism#147
File: crates/node_types/prover/src/prover/tests.rs:120-129
Timestamp: 2024-10-25T08:50:21.262Z
Learning: Avoid including debug print statements in test functions in `crates/node_types/prover/src/prover/tests.rs`.
Learnt from: jns-ps
PR: deltadevsde/prism#147
File: crates/common/src/test_ops.rs:27-44
Timestamp: 2024-10-29T08:34:32.445Z
Learning: In `crates/common/src/test_ops.rs`, methods in test code are deliberately kept short for brevity, even if this results in abbreviated method names.
Learnt from: distractedm1nd
PR: deltadevsde/prism#146
File: crates/common/src/tree.rs:114-126
Timestamp: 2024-11-01T07:52:07.324Z
Learning: In `UpdateProof::verify` within `crates/common/src/tree.rs`, the `old_hashchain` and `hashchain_after_update` are different hashchains, so caching their serialized values is not feasible.
Learnt from: jns-ps
PR: deltadevsde/prism#198
File: crates/common/src/transaction_builder.rs:450-483
Timestamp: 2025-01-13T15:35:17.509Z
Learning: In the Prism codebase, transactions are protected by a comprehensive signature that covers the entire transaction including account ID, operation, nonce, and verifying key. This transaction-level signature provides security against replay attacks and unauthorized modifications, making operation-level signatures primarily useful for data authenticity rather than transaction security.
Learnt from: distractedm1nd
PR: deltadevsde/prism#146
File: crates/common/src/hashchain.rs:164-174
Timestamp: 2024-11-01T07:51:25.629Z
Learning: In `crates/common/src/hashchain.rs`, extracting duplicated key validation logic between different operation types is not feasible without refactoring the underlying enum structs to use the same type.
🪛 GitHub Check: clippy
crates/node_types/prover/src/prover/tests/mod.rs
[failure] 2-2:
unused import: digest::Digest
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: unused dependencies
- GitHub Check: coverage
- GitHub Check: build-and-push-image
- GitHub Check: unit-test
- GitHub Check: integration-test
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
crates/node_types/prover/src/prover/tests/mod.rs (4)
369-369: Good optimization of sync loop frequency.Reducing iterations from 50 to 10 while increasing sleep duration maintains the same total timeout but reduces unnecessary polling frequency.
386-386: Appropriate sleep duration adjustment.The increased sleep duration complements the reduced loop iterations, maintaining the same 5-second total timeout while being less resource-intensive.
404-404: Reasonable timing adjustment for transaction processing.The slight increase from 100ms to 150ms provides more buffer time for initial transaction processing before racing transactions are submitted.
422-424: Excellent improvement: replace fixed delay with active polling.Replacing the fixed 5-second sleep with a polling loop that checks for commitment synchronization is a significant improvement. This approach makes the test more reliable and responsive by proceeding as soon as the condition is met rather than waiting for an arbitrary timeout.
Summary by CodeRabbit