-
Notifications
You must be signed in to change notification settings - Fork 189
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
hotfix(katana): make sure validator state is synced with block producer #2353
Conversation
WalkthroughOhayo, sensei! The recent changes involve substantial restructuring of the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TransactionPool
participant Validator
participant StarknetApi
User->>TransactionPool: Submit Transaction
TransactionPool->>Validator: Validate Transaction
Validator-->>TransactionPool: Validation Result
TransactionPool-->>User: Transaction Status
sequenceDiagram
participant NodeService
participant BlockProducer
participant TxValidator
NodeService->>BlockProducer: Update Block
BlockProducer->>TxValidator: Validate Transactions
TxValidator-->>BlockProducer: Validation Result
BlockProducer-->>NodeService: Block Update Confirmation
Recent review detailsConfiguration used: .coderabbit.yaml Files ignored due to path filters (1)
Files selected for processing (2)
Additional comments not posted (10)
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 as PR comments)
Additionally, you can add Documentation and Community
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
let account = Arc::new(sequencer.account()); | ||
|
||
// setup test contract to interact with. | ||
abigen_legacy!(Contract, "crates/katana/rpc/rpc/tests/test_data/erc20.json"); | ||
let contract = Contract::new(DEFAULT_FEE_TOKEN_ADDRESS.into(), &account); | ||
|
||
// function call params | ||
let recipient = Felt::ONE; | ||
let amount = Uint256 { low: Felt::ONE, high: Felt::ZERO }; | ||
|
||
const N: usize = 10; | ||
let mut txs = IndexSet::with_capacity(N); | ||
let initial_nonce = | ||
provider.get_nonce(BlockId::Tag(BlockTag::Pending), sequencer.account().address()).await?; | ||
|
||
const N: usize = 100; | ||
let nonce = Arc::new(Mutex::new(initial_nonce)); | ||
let txs = Arc::new(Mutex::new(IndexSet::with_capacity(N))); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohayo sensei! Use tokio::sync::RwLock
for better read performance.
Consider using tokio::sync::RwLock
instead of Mutex
for nonce
and txs
to improve read performance, as multiple reads can occur concurrently without blocking each other.
Apply this diff to use RwLock
:
-use tokio::sync::Mutex;
+use tokio::sync::{Mutex, RwLock};
-let nonce = Arc::new(Mutex::new(initial_nonce));
-let txs = Arc::new(Mutex::new(IndexSet::with_capacity(N)));
+let nonce = Arc::new(RwLock::new(initial_nonce));
+let txs = Arc::new(RwLock::new(IndexSet::with_capacity(N)));
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.
let account = Arc::new(sequencer.account()); | |
// setup test contract to interact with. | |
abigen_legacy!(Contract, "crates/katana/rpc/rpc/tests/test_data/erc20.json"); | |
let contract = Contract::new(DEFAULT_FEE_TOKEN_ADDRESS.into(), &account); | |
// function call params | |
let recipient = Felt::ONE; | |
let amount = Uint256 { low: Felt::ONE, high: Felt::ZERO }; | |
const N: usize = 10; | |
let mut txs = IndexSet::with_capacity(N); | |
let initial_nonce = | |
provider.get_nonce(BlockId::Tag(BlockTag::Pending), sequencer.account().address()).await?; | |
const N: usize = 100; | |
let nonce = Arc::new(Mutex::new(initial_nonce)); | |
let txs = Arc::new(Mutex::new(IndexSet::with_capacity(N))); | |
let account = Arc::new(sequencer.account()); | |
// setup test contract to interact with. | |
abigen_legacy!(Contract, "crates/katana/rpc/rpc/tests/test_data/erc20.json"); | |
// function call params | |
let recipient = Felt::ONE; | |
let amount = Uint256 { low: Felt::ONE, high: Felt::ZERO }; | |
let initial_nonce = | |
provider.get_nonce(BlockId::Tag(BlockTag::Pending), sequencer.account().address()).await?; | |
const N: usize = 100; | |
let nonce = Arc::new(RwLock::new(initial_nonce)); | |
let txs = Arc::new(RwLock::new(IndexSet::with_capacity(N))); |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2353 +/- ##
==========================================
+ Coverage 67.27% 67.56% +0.29%
==========================================
Files 357 359 +2
Lines 46649 46960 +311
==========================================
+ Hits 31383 31729 +346
+ Misses 15266 15231 -35 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores