Conversation
WalkthroughThis set of changes refactors the handling of block abortion and block number management in the Starknet Devnet codebase. The Estimated code review effort4 (60–120 minutes) Possibly related PRs
✨ Finishing Touches
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
Documentation and Community
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
crates/starknet-devnet-core/src/starknet/mod.rs (1)
916-1005: Excellent refactoring of block abortion logic.The new implementation correctly removes blocks and transactions from memory instead of marking them as rejected, which aligns with the PR objectives. The code properly handles state reversion, block number updates, and maintains the aborted blocks history.
However, consider improving error handling for edge cases:
- let aborted_block = self.blocks.remove(&next_block_to_abort_hash).ok_or( - Error::UnexpectedInternalError { - msg: format!("Cannot find block for abortion: {next_block_to_abort_hash:#x}"), - }, - )?; + let aborted_block = match self.blocks.remove(&next_block_to_abort_hash) { + Some(block) => block, + None => { + // This could happen if the block was already removed in a previous operation + // Log the error and continue with remaining blocks + error!("Block {next_block_to_abort_hash:#x} not found for abortion"); + continue; + } + };This would make the abortion process more resilient to edge cases where blocks might have been removed by other operations.
🧹 Nitpick comments (1)
crates/starknet-devnet-core/src/starknet/mod.rs (1)
600-613: Good refactoring to reduce repeated method calls.Using a local reference to
gas_pricesimproves readability and reduces redundant calls.Consider extracting the gas price assignment logic into a helper method to reduce duplication with the similar code in
generate_pre_confirmed_block(lines 328-335):+fn set_block_gas_prices(block: &mut StarknetBlock, gas_prices: &GasPrices) { + let header = &mut block.header.block_header_without_hash; + header.l1_gas_price = GasPricePerToken { + price_in_fri: gas_prices.l1_gas_price(&FeeType::Strk).get(), + price_in_wei: gas_prices.l1_gas_price(&FeeType::Eth).get(), + }; + header.l1_data_gas_price = GasPricePerToken { + price_in_fri: gas_prices.l1_data_gas_price(&FeeType::Strk).get(), + price_in_wei: gas_prices.l1_data_gas_price(&FeeType::Eth).get(), + }; + header.l2_gas_price = GasPricePerToken { + price_in_fri: gas_prices.l2_gas_price(&FeeType::Strk).get(), + price_in_wei: gas_prices.l2_gas_price(&FeeType::Eth).get(), + }; +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
crates/starknet-devnet-core/src/blocks/mod.rs(1 hunks)crates/starknet-devnet-core/src/starknet/mod.rs(6 hunks)crates/starknet-devnet-core/src/transactions.rs(1 hunks)crates/starknet-devnet-server/src/api/json_rpc/mod.rs(1 hunks)tests/integration/test_abort_blocks.rs(8 hunks)tests/integration/test_accepting_blocks_on_l1.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: FabijanC
PR: 0xSpaceShard/starknet-devnet#755
File: crates/starknet-devnet-server/test_data/spec/0.8.1/starknet_executables.json:1154-1168
Timestamp: 2025-04-14T09:33:59.391Z
Learning: FabijanC wants to configure CodeRabbit to ignore spec files from review, specifically the JSON schema files in the test_data/spec directory.
tests/integration/test_accepting_blocks_on_l1.rs (1)
Learnt from: FabijanC
PR: #799
File: crates/starknet-devnet-core/src/starknet/mod.rs:240-246
Timestamp: 2025-06-16T07:35:24.725Z
Learning: In the Starknet devnet codebase, get_class_hash_at has different behaviors at different layers: at the internal state level (blockifier), it returns Ok(ClassHash(Felt::ZERO)) for undeployed addresses, while at the RPC layer, this gets converted to Err(Error::ContractNotFound). When checking if a contract is deployed at the internal state level, use is_ok_and(|h| h.0 == Felt::ZERO) to detect undeployed addresses.
crates/starknet-devnet-core/src/blocks/mod.rs (1)
Learnt from: FabijanC
PR: #799
File: crates/starknet-devnet-core/src/starknet/mod.rs:240-246
Timestamp: 2025-06-16T07:35:24.725Z
Learning: In the Starknet devnet codebase, get_class_hash_at has different behaviors at different layers: at the internal state level (blockifier), it returns Ok(ClassHash(Felt::ZERO)) for undeployed addresses, while at the RPC layer, this gets converted to Err(Error::ContractNotFound). When checking if a contract is deployed at the internal state level, use is_ok_and(|h| h.0 == Felt::ZERO) to detect undeployed addresses.
tests/integration/test_abort_blocks.rs (1)
Learnt from: FabijanC
PR: #799
File: crates/starknet-devnet-core/src/starknet/mod.rs:240-246
Timestamp: 2025-06-16T07:35:24.725Z
Learning: In the Starknet devnet codebase, get_class_hash_at has different behaviors at different layers: at the internal state level (blockifier), it returns Ok(ClassHash(Felt::ZERO)) for undeployed addresses, while at the RPC layer, this gets converted to Err(Error::ContractNotFound). When checking if a contract is deployed at the internal state level, use is_ok_and(|h| h.0 == Felt::ZERO) to detect undeployed addresses.
crates/starknet-devnet-core/src/starknet/mod.rs (1)
Learnt from: FabijanC
PR: #799
File: crates/starknet-devnet-core/src/starknet/mod.rs:240-246
Timestamp: 2025-06-16T07:35:24.725Z
Learning: In the Starknet devnet codebase, get_class_hash_at has different behaviors at different layers: at the internal state level (blockifier), it returns Ok(ClassHash(Felt::ZERO)) for undeployed addresses, while at the RPC layer, this gets converted to Err(Error::ContractNotFound). When checking if a contract is deployed at the internal state level, use is_ok_and(|h| h.0 == Felt::ZERO) to detect undeployed addresses.
🧬 Code Graph Analysis (1)
crates/starknet-devnet-core/src/transactions.rs (1)
crates/starknet-devnet-core/src/blocks/mod.rs (1)
remove(186-193)
🔇 Additional comments (16)
crates/starknet-devnet-core/src/transactions.rs (1)
38-40: LGTM! Clean implementation of transaction removal.The
removemethod is well-implemented usingshift_removeto maintain ordering and follows standard Rust patterns withOption<T>return type. This supports the new block abortion behavior where transactions are completely removed rather than marked as rejected.tests/integration/test_accepting_blocks_on_l1.rs (1)
208-208: Correctly updated assertion to reflect new block abortion behavior.The error message change from "Rejected block cannot be accepted on L1" to "No block found" accurately reflects the new behavior where aborted blocks are completely removed from storage rather than being marked as rejected. This ensures the test validates the correct post-abortion semantics.
crates/starknet-devnet-core/src/blocks/mod.rs (2)
186-193: Well-implemented block removal with comprehensive cleanup.The
removemethod correctly removes a block and all associated data (state, state diff, block mapping, and number mapping) from internal collections. Using the?operator ensures atomic behavior - if any removal fails, the entire operation returnsNone. The use ofshift_removemaintains ordering consistency.
195-196: Clean replacement for the removednext_block_numbermethod.The
next_latest_block_numbermethod provides a clear, focused interface for getting the next block number from the pre-confirmed block, which aligns with the refactored block number management approach.crates/starknet-devnet-server/src/api/json_rpc/mod.rs (2)
405-410: LGTM! Proper error handling for the new approach.The error handling correctly addresses the case where
last_aborted_block_hash()returnsNone, which would indicate an unexpected internal state during a reorg scenario.
413-413: LGTM! Correct usage of the last aborted block hash.Using the dereferenced
last_aborted_block_hashas the starting block hash aligns well with the new approach where aborted blocks are completely removed from memory rather than marked as rejected.crates/starknet-devnet-core/src/starknet/mod.rs (5)
23-23: LGTM: Unused import removed.Good cleanup of the unused
ExecutionResultimport.
533-546: Good encapsulation of block number setting logic.The new
set_block_numbermethod ensures consistent updates to bothpre_confirmed_blockandblock_context, which is essential for maintaining state consistency after block abortion.
1007-1010: Clean implementation of last aborted block hash getter.Simple and effective method for accessing the most recently aborted block.
1016-1019: Appropriate error message for deprecated block status.The new error message correctly indicates that rejected blocks should no longer exist in the system and provides helpful guidance to report the issue.
375-375: Approve:next_latest_block_numberrename is valid and implementedThe new
next_latest_block_numbermethod is defined in
• crates/starknet-devnet-core/src/blocks/mod.rs:195
and correctly used at lines 375 and 424 in
• crates/starknet-devnet-core/src/starknet/mod.rsNo further changes needed.
tests/integration/test_abort_blocks.rs (5)
25-44: Well-structured test helpers for the new abortion behavior.The updated helper functions correctly validate that aborted blocks and transactions are no longer queryable, properly expecting "not found" errors instead of rejected statuses.
59-70: Test assertions correctly updated for new abortion semantics.All test assertions have been properly updated to reflect that aborted blocks and their transactions are removed from storage rather than marked as rejected.
Also applies to: 92-93, 111-112, 125-125, 245-247, 269-270, 287-287
138-178: Excellent enhancement of state reversion test.The expanded test with explicit balance checks at each stage provides strong validation that state changes are properly reverted during block abortion.
180-217: Valuable test for block number consistency.This test ensures that block numbers are correctly updated after abortion, which is crucial for maintaining consistency in the blockchain state.
249-271: Important edge case test for multiple abortion operations.This test validates that the aborted blocks list is properly maintained across multiple abortion operations, preventing the bug where the list could be overwritten.
Usage related changes
No block found.REJECTEDDevelopment related changes
Checklist:
./scripts/format.sh./scripts/clippy_check.sh./scripts/check_unused_deps.sh./scripts/check_spelling.sh./website/README.mdSummary by CodeRabbit
New Features
Bug Fixes
Tests