Force upgrade stdlib with v13 for consensus update #4402
Conversation
WalkthroughThis pull request reorganizes and refines the force upgrade functionality. The changes update the version checks (shifting from 12 to 13 in some cases), remove conditional compilation for the Changes
Sequence Diagram(s)sequenceDiagram
participant BC as Block Creator
participant OB as OpenedBlock
participant FU as ForceUpgrade Handler
participant VM as starcoin_vm_runtime
BC->>OB: execute_extra_txn()
OB->>FU: Set standard library version to 13
FU->>VM: Initiate forced upgrade txn
VM-->>FU: Process upgrade txn
FU-->>OB: Return upgrade result
OB-->>BC: Complete txn execution
sequenceDiagram
participant BE as Block Executor
participant CS as Chain State
participant VM as starcoin_vm_runtime
BE->>CS: get_block_metadata_v2()
CS-->>BE: Return block metadata
BE->>VM: get_force_upgrade_block_number()
VM-->>BE: Return force upgrade block number
BE->>BE: Validate txn count and version (12→13)
BE-->>CS: Execute extra forced upgrade txn if conditions met
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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
🔭 Outside diff range comments (1)
vm/vm-runtime/src/force_upgrade_management.rs (1)
75-83:⚠️ Potential issueFix failing test assertions for Dev and Test chains.
The test assertions for Dev and Test chains are failing. The test expects the same address for both chains but uses different chain IDs (254).
Apply this diff to fix the test:
- get_force_upgrade_account(&ChainId::new(254))?.address(), + get_force_upgrade_account(&ChainId::new(255))?.address(),🧰 Tools
🪛 GitHub Actions: Code Coverage Report
[error] 75-75: Assertion
left == rightfailed in test force_upgrade_management::tests::test_get_force_upgrade_account.
🧹 Nitpick comments (5)
executor/src/block_executor.rs (1)
170-170: Address TODO comment regarding execute_extra_txn.The TODO comment indicates a need to check and merge execute_extra_txn functionality with OpenedBlock.
Would you like me to help analyze the OpenedBlock implementation and propose a merged solution?
chain/open-block/src/lib.rs (1)
312-322: Consider making the stdlib version configurable.The stdlib version is hardcoded to 13, which could make future upgrades more difficult. Consider:
- Making the version configurable through a constant or configuration
- Adding a comment explaining why version 13 is specifically required
- let version = on_chain_config::Version { major: 13 }; + // TODO: Consider making this configurable + const STDLIB_VERSION: u64 = 13; + let version = on_chain_config::Version { major: STDLIB_VERSION };chain/open-block/Cargo.toml (1)
1-33: Fix manifest warnings in pipeline.The pipeline reports warnings about unused manifest key:
dependencies.bcs-ext.package. Consider removing thepackagespecification if it's not needed.-bcs-ext = { package = "bcs-ext", workspace = true } +bcs-ext = { workspace = true }🧰 Tools
🪛 GitHub Actions: Code Coverage Report
[warning] 1-1: Unused manifest key: dependencies.bcs-ext.package
🪛 GitHub Actions: benchmark pull requests
[warning] 1-1: Unused manifest key: dependencies.bcs-ext.package
chain/force-upgrade/Cargo.toml (1)
16-16: Fix manifest warnings in pipeline.Similar to
chain/open-block/Cargo.toml, remove the unnecessarypackagespecification to resolve pipeline warnings.-bcs-ext = { package = "bcs-ext", workspace = true } +bcs-ext = { workspace = true }Cargo.toml (1)
1-1: Pipeline Warning: Unused Manifest Key for 'dependencies.bcs-ext.package'
The pipeline reports an unused manifest key (dependencies.bcs-ext.package). Although this issue exists outside the scope of the current PR changes, it may be worth reviewing the configuration forbcs-extto remove any obsolete keys.🧰 Tools
🪛 GitHub Actions: Code Coverage Report
[warning] 1-1: Unused manifest key: dependencies.bcs-ext.package
🪛 GitHub Actions: benchmark pull requests
[warning] 1-1: Unused manifest key: dependencies.bcs-ext.package
🪛 GitHub Actions: Build and Test
[warning] 1-1: unused manifest key: dependencies.bcs-ext.package
📜 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 (14)
Cargo.toml(4 hunks)chain/Cargo.toml(1 hunks)chain/force-upgrade/Cargo.toml(3 hunks)chain/force-upgrade/src/lib.rs(1 hunks)chain/open-block/Cargo.toml(1 hunks)chain/open-block/src/lib.rs(4 hunks)chain/src/chain.rs(2 hunks)chain/tests/test_force_upgrade.rs(7 hunks)cmd/starcoin/Cargo.toml(0 hunks)executor/Cargo.toml(0 hunks)executor/src/block_executor.rs(3 hunks)node/Cargo.toml(0 hunks)vm/vm-runtime/src/force_upgrade_management.rs(2 hunks)vm/vm-runtime/src/lib.rs(1 hunks)
💤 Files with no reviewable changes (3)
- cmd/starcoin/Cargo.toml
- executor/Cargo.toml
- node/Cargo.toml
✅ Files skipped from review due to trivial changes (1)
- vm/vm-runtime/src/lib.rs
🧰 Additional context used
🪛 GitHub Actions: Code Coverage Report
chain/open-block/Cargo.toml
[warning] 1-1: Unused manifest key: dependencies.bcs-ext.package
chain/Cargo.toml
[warning] 1-1: Unused manifest key: dependencies.bcs-ext.package
vm/vm-runtime/src/force_upgrade_management.rs
[error] 75-75: Assertion left == right failed in test force_upgrade_management::tests::test_get_force_upgrade_account.
chain/force-upgrade/Cargo.toml
[warning] 1-1: Unused manifest key: dependencies.bcs-ext.package
Cargo.toml
[warning] 1-1: Unused manifest key: dependencies.bcs-ext.package
🪛 GitHub Actions: benchmark pull requests
chain/open-block/Cargo.toml
[warning] 1-1: Unused manifest key: dependencies.bcs-ext.package
chain/Cargo.toml
[warning] 1-1: Unused manifest key: dependencies.bcs-ext.package
chain/force-upgrade/Cargo.toml
[warning] 1-1: Unused manifest key: dependencies.bcs-ext.package
Cargo.toml
[warning] 1-1: Unused manifest key: dependencies.bcs-ext.package
🪛 GitHub Actions: Build and Test
chain/force-upgrade/Cargo.toml
[warning] 1-1: unused manifest key: dependencies.bcs-ext.package
Cargo.toml
[warning] 1-1: unused manifest key: dependencies.bcs-ext.package
🔇 Additional comments (15)
chain/force-upgrade/src/lib.rs (1)
27-27: LGTM! Package path updated for v13 upgrade.The package file path has been correctly updated to reflect the upgrade from version 12 to version 13.
vm/vm-runtime/src/force_upgrade_management.rs (1)
8-8: Verify the significant reduction in force upgrade block number.The force upgrade block number has been drastically reduced from 17500000 to 100. This could significantly impact the network's upgrade timeline.
Please confirm if this reduction is intentional and aligns with the network's upgrade schedule.
chain/tests/test_force_upgrade.rs (1)
20-20: LGTM! Test cases updated for v12 to v13 upgrade path.The test cases have been correctly updated to use stdlib version 12 as the starting point for the v13 upgrade.
Also applies to: 36-36, 204-204
executor/src/block_executor.rs (2)
133-141: LGTM! Version check updated for v12 to v13 upgrade.The version check has been correctly updated to execute extra transactions only when the stdlib version is 12, preparing for the upgrade to v13.
144-147: LGTM! Improved error handling for metadata retrieval.The code now properly handles metadata retrieval errors using get_block_metadata_v2 with explicit error messaging.
chain/open-block/src/lib.rs (2)
9-9: LGTM! Import changes are well-organized.The new imports are properly structured and align with the force upgrade functionality changes.
Also applies to: 27-27, 31-41
557-563: LGTM! Transaction validation logic is cleaner.The simplified validation logic maintains the same functionality while being more readable and maintainable.
chain/src/chain.rs (1)
756-762: LGTM! Consistent transaction validation.The transaction validation logic matches the changes in
open-block/src/lib.rs, maintaining consistency across the codebase.chain/open-block/Cargo.toml (1)
18-18: LGTM! Required dependency for force upgrade.The addition of
starcoin-vm-runtimeis necessary for the force upgrade functionality.chain/force-upgrade/Cargo.toml (1)
7-7: LGTM! Version bump and dependency addition.The version update to 2.0.2 and addition of
starcoin-vm-runtimealign with the force upgrade changes.Also applies to: 28-28
chain/Cargo.toml (1)
24-24: Dependency Update: 'starcoin-vm-runtime' Now Mandatory
The removal of theoptional = trueflag forstarcoin-vm-runtimeis intentional to ensure that the dependency is always included as part of the consensus update. Please verify that all consuming modules are compatible with this stricter dependency requirement.Cargo.toml (4)
43-43: Workspace Members Update: 'chain/force-upgrade' Module
The workspace members list now includes"chain/force-upgrade"(previously located under"vm/force-upgrade"), reflecting the new module organization. This change improves clarity and aligns with the project’s structural reorganization.
157-157: Default Members Update: 'chain/force-upgrade' Inclusion
Updating the default-members configuration to reference"chain/force-upgrade"ensures that the renamed module is automatically included, which is consistent with the intended restructuring.
457-457: Dependency Revision Update: 'starcoin-framework'
The revision forstarcoin-frameworkhas been bumped to631ddccd5a169f535476a62bd8145782e3d9960a. This update is crucial to maintain compatibility across the codebase as part of the consensus update. Please ensure related tests pass with this new revision.
475-475: Dependency Path Update: 'starcoin-force-upgrade'
The dependency forstarcoin-force-upgradehas been updated to use the new path"chain/force-upgrade". This change correctly reflects the move of the force-upgrade module and ensures path consistency across the project.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
genesis/src/lib.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Generate code coverage report
| //FIXME: consensus config update by genesis init when stdlib upgrade_from_12_to_13 | ||
| /*assert_eq!( | ||
| consensus_config.as_ref().unwrap(), | ||
| &net.genesis_config().consensus_config | ||
| ); | ||
| );*/ |
There was a problem hiding this comment.
❓ Verification inconclusive
Verify the consensus config update during stdlib upgrade.
The commented-out assertion suggests that the consensus config will be updated during the stdlib upgrade from v12 to v13. This change needs careful verification to ensure the new consensus config is correctly applied.
Run the following script to verify the consensus config changes:
Additionally, please verify:
- What are the specific consensus config changes being introduced?
- Are there any migration steps needed for the consensus config update?
- Have all dependent components been updated to handle the new consensus config?
🏁 Script executed:
#!/bin/bash
# Description: Check for consensus config changes in the stdlib upgrade
# Search for consensus config related changes
ast-grep --pattern 'consensus_config'
# Search for stdlib upgrade related changes
rg -A 5 'upgrade.*12.*13|v12.*v13'Length of output: 5077
ACTION: Verify consensus config migration for stdlib upgrade from v12 to v13.
The commented-out assertion indicates that the consensus config verification is intentionally disabled pending the upgrade. Please confirm that:
- The new consensus config values (e.g., gas limits, time targets, difficulty windows, etc.) are being set as expected.
- All necessary migration steps for transitioning from v12 to v13 are implemented and documented.
- All dependent modules (such as genesis initialization and transaction builder logic) correctly handle the updated consensus config.
Once these items are verified, consider adding or re-enabling corresponding tests to ensure the configuration matches post-upgrade requirements.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
chain/src/chain.rs (1)
756-762: Ensure consistent validation logic across block execution methods.The same transaction validation logic is duplicated in both
execute_dag_blockandexecute_block_and_save. This could lead to maintenance issues if the logic needs to be updated in the future.Consider extracting this validation logic into a shared helper method:
+fn validate_block_transactions( + header: &BlockHeader, + chain_id: &ChainId, + transactions_len: usize, + executed_txn_info_len: usize, + with_extra_txn: bool, +) -> bool { + if header.number() == get_force_upgrade_block_number(chain_id) + && with_extra_txn + { + executed_txn_info_len == transactions_len.checked_add(1).unwrap() + } else { + executed_txn_info_len == transactions_len + } +} + // In execute_dag_block and execute_block_and_save: -let valid_txn_num = if header.number() == get_force_upgrade_block_number(&header.chain_id()) - && executed_data.with_extra_txn -{ - vec_transaction_info.len() == transactions.len().checked_add(1).unwrap() -} else { - vec_transaction_info.len() == transactions.len() -}; +let valid_txn_num = validate_block_transactions( + header, + &header.chain_id(), + transactions.len(), + vec_transaction_info.len(), + executed_data.with_extra_txn, +);chain/open-block/src/lib.rs (1)
382-382: Enhance logging for better debugging.The logging has been simplified, which might make debugging harder in production.
Consider adding more context:
-info!("extra txn in opened block ({:?})", extra_txn.id()); +info!( + "Executing stdlib upgrade txn in block {} ({:?})", + self.block_meta.number(), + extra_txn.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 (15)
Cargo.toml(4 hunks)chain/Cargo.toml(1 hunks)chain/force-upgrade/Cargo.toml(1 hunks)chain/force-upgrade/src/lib.rs(1 hunks)chain/open-block/Cargo.toml(1 hunks)chain/open-block/src/lib.rs(4 hunks)chain/src/chain.rs(2 hunks)chain/tests/test_force_upgrade.rs(7 hunks)cmd/starcoin/Cargo.toml(0 hunks)executor/Cargo.toml(0 hunks)executor/src/block_executor.rs(3 hunks)genesis/src/lib.rs(1 hunks)node/Cargo.toml(0 hunks)vm/vm-runtime/src/force_upgrade_management.rs(2 hunks)vm/vm-runtime/src/lib.rs(1 hunks)
💤 Files with no reviewable changes (3)
- node/Cargo.toml
- executor/Cargo.toml
- cmd/starcoin/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (8)
- vm/vm-runtime/src/lib.rs
- chain/force-upgrade/src/lib.rs
- chain/open-block/Cargo.toml
- Cargo.toml
- chain/Cargo.toml
- chain/tests/test_force_upgrade.rs
- executor/src/block_executor.rs
- genesis/src/lib.rs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build and test
🔇 Additional comments (7)
chain/force-upgrade/Cargo.toml (3)
1-11: Ensure Workspace Metadata Exists for Package ConfigurationThe package metadata from lines 1–11 leverages workspace inheritance for several keys (e.g.,
authors,edition,license,publish,homepage,repository, andrust-version). Please verify that the workspace's rootCargo.tomldefines these values so that builds do not end up with missing metadata. This pattern is acceptable if your workspace is set up accordingly.
13-32: Confirm Dependency Version Alignment for Consensus UpdateThe dependencies block correctly declares all dependencies using workspace inheritance. In particular, note the inclusion of
stdlib = { workspace = true }. Since the PR aims to force upgrade the stdlib to v13, please ensure that the workspace configuration enforces or specifies that version. If additional version constraints or overrides are required to meet the consensus update, consider adding them here.
33-35: Dev-Dependencies Declaration Looks CorrectThe dev-dependencies section at lines 33–35 uses workspace-based inheritance for
starcoin-genesisandstest, which is consistent with the project's structure.chain/src/chain.rs (2)
42-42: LGTM: Import change aligns with force upgrade management refactoring.The import statement has been updated to use the new path from
starcoin_vm_runtime::force_upgrade_management, which aligns with the PR objective of upgrading the standard library.
557-563:✅ Verification successful
Verify the transaction validation logic for force upgrade blocks.
The transaction validation logic for force upgrade blocks has been updated to handle an extra transaction. This is a critical check that needs careful verification:
- It checks if the current block is a force upgrade block
- It validates transaction count based on whether an extra transaction was executed
Run the following script to verify the force upgrade block number implementation:
🏁 Script executed:
#!/bin/bash # Description: Check the force upgrade block number implementation # Look for any other usage of force upgrade block number to ensure consistency ast-grep --pattern 'get_force_upgrade_block_number($$$)'Length of output: 878
Force Upgrade Transaction Validation Verified
The verification confirms that the logic for force upgrade blocks is applied consistently throughout the codebase. In particular, the check in
chain/src/chain.rs(lines 557-563) correctly:
- Compares the current header number with the force upgrade block number via
get_force_upgrade_block_number(&header.chain_id())- Appropriately adjusts the expected transaction count based on the
executed_data.with_extra_txnflag using.checked_add(1).unwrap()Additional references in files such as
executor/src/block_executor.rsand in the test suite align with this implementation. No further changes are required.vm/vm-runtime/src/force_upgrade_management.rs (2)
8-8: Verify the significant reduction in FORCE_UPGRADE_BLOCK_NUMBER.The constant has been reduced from 17500000 to 100, which is a drastic change. This could trigger force upgrades much earlier than before.
Please confirm:
- Is this reduction intentional for the v13 stdlib upgrade?
- Have the implications of this early trigger been considered for all chain environments?
42-46:❓ Verification inconclusive
Verify the security of hardcoded account credentials.
New hardcoded private key and address have been added for the fallback case. While this improves chain environment handling, we should ensure:
- The private key is specific to this environment
- The account has appropriate permissions
🏁 Script executed:
#!/bin/bash # Check if this address is used elsewhere in the codebase rg "85e4c2c4859fdbf9c8ce6e046c7e310d" --type rustLength of output: 140
Security Check: Verify Fallback Account Credentials
The hardcoded address appears only in the comment within
vm/vm-runtime/src/force_upgrade_management.rsand is not used elsewhere in the codebase. However, since a new fallback account with hardcoded credentials has been introduced, please ensure that:
- Environment Specificity: The private key is valid only for test/dag-test environments and isn’t inadvertently exposed or misused in production.
- Permission Scope: The account created via
create_account(...)has restricted permissions appropriate solely for its intended testing purpose.Please verify these points to confirm that the security implications are properly mitigated.
| // update stdlib version to 13 directly | ||
| let version_path = on_chain_config::access_path_for_config( | ||
| genesis_address(), | ||
| Identifier::new("Version").unwrap(), | ||
| Identifier::new("Version").unwrap(), | ||
| vec![], | ||
| ); | ||
| let version = on_chain_config::Version { major: 13 }; | ||
| self.state | ||
| .set(&version_path, bcs_ext::to_bytes(&version)?)?; | ||
| } |
There was a problem hiding this comment.
Verify the direct stdlib version update to 13.
The code directly updates the stdlib version to 13 without any validation or fallback mechanism. This is a critical change that:
- Could be irreversible once deployed
- Might affect chain consensus
Consider adding:
- Version validation
- Rollback mechanism in case of issues
// update stdlib version to 13 directly
let version_path = on_chain_config::access_path_for_config(
genesis_address(),
Identifier::new("Version").unwrap(),
Identifier::new("Version").unwrap(),
vec![],
);
-let version = on_chain_config::Version { major: 13 };
+let current_version = self.state.get(&version_path)?
+ .ok_or_else(|| format_err!("Version config not found"))?;
+let new_version = on_chain_config::Version { major: 13 };
+if current_version.major >= new_version.major {
+ info!("Skipping version update as current version is >= 13");
+ return Ok(());
+}
self.state
- .set(&version_path, bcs_ext::to_bytes(&version)?)?;
+ .set(&version_path, bcs_ext::to_bytes(&new_version)?)?;📝 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.
| // update stdlib version to 13 directly | |
| let version_path = on_chain_config::access_path_for_config( | |
| genesis_address(), | |
| Identifier::new("Version").unwrap(), | |
| Identifier::new("Version").unwrap(), | |
| vec![], | |
| ); | |
| let version = on_chain_config::Version { major: 13 }; | |
| self.state | |
| .set(&version_path, bcs_ext::to_bytes(&version)?)?; | |
| } | |
| // update stdlib version to 13 directly | |
| let version_path = on_chain_config::access_path_for_config( | |
| genesis_address(), | |
| Identifier::new("Version").unwrap(), | |
| Identifier::new("Version").unwrap(), | |
| vec![], | |
| ); | |
| let current_version = self.state.get(&version_path)? | |
| .ok_or_else(|| format_err!("Version config not found"))?; | |
| let new_version = on_chain_config::Version { major: 13 }; | |
| if current_version.major >= new_version.major { | |
| info!("Skipping version update as current version is >= 13"); | |
| return Ok(()); | |
| } | |
| self.state | |
| .set(&version_path, bcs_ext::to_bytes(&new_version)?)?; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
chain/src/verifier/mod.rs (1)
87-97: Implementation of blacklist verification.The implementation checks all transactions in a block against the blacklist. This is a critical change that impacts consensus by rejecting blocks containing transactions from blacklisted addresses.
However, there's a potential issue with the error message - it's not very descriptive for debugging purposes.
Consider enhancing the error message to include the blacklisted address that was detected:
verify_block!( VerifyBlockField::Body, !AddressFilter::is_blacklisted(txn, block_number), - "Invalid block: the sender of transaction in block must be not blacklisted" + "Invalid block: transaction from blacklisted sender: {}", txn.sender() );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
chain/open-block/src/lib.rs(4 hunks)chain/src/verifier/mod.rs(3 hunks)
🔇 Additional comments (5)
chain/src/verifier/mod.rs (2)
11-11: New dependency on AddressFilter.This import adds a dependency on the
AddressFilterfrom thestarcoin_open_blockmodule, which is used for the blacklist functionality implementation.
72-72: Verify call sequence in block verification.The new blacklist verification step is now called before any other verification steps, which is the correct approach since there's no need to perform other expensive verifications if the block contains blacklisted transactions.
chain/open-block/src/lib.rs (3)
13-13: New imports for blacklist functionality.These imports support the blacklist implementation -
BlockNumberis used for block-based activation andFromStris used for address parsing.Also applies to: 26-26
141-151: Transaction filtering implementation.The implementation effectively filters out transactions from blacklisted addresses before processing, while collecting them for return as discarded transactions. This helps prevent blacklisted transactions from being included in new blocks.
1-310:❓ Verification inconclusive
Mismatch between PR title and implementation.
The PR is titled "Force upgrade stdlib with v13 for consensus update", but the implementation is about transaction blacklisting with no visible stdlib upgrade code.
This could be due to:
- The code for stdlib upgrade being in other files not included in this review
- The PR title being incorrect or misleading
- The stdlib upgrade being implemented through this blacklisting mechanism in some way
🏁 Script executed:
#!/bin/bash # Search for stdlib upgrade related code in the PR rg -i "stdlib.*upgrade|upgrade.*stdlib|version.*13"Length of output: 22297
PR Title Mismatch: Verify stdlib Upgrade Coverage
The implementation in
chain/open-block/src/lib.rsis focused on transaction processing and the introduction of transaction blacklisting viaAddressFilter—there’s no direct change related to upgrading the stdlib. Our repository search did locate multiple references to stdlib upgrade functionality (e.g., in upgrade tests and Cargo.toml version settings), suggesting that either:
- The stdlib upgrade code is implemented elsewhere.
- The PR title “Force upgrade stdlib with v13 for consensus update” might be misleading for this change.
Please clarify whether:
- The stdlib upgrade changes are part of a different commit or file not shown in this review, or
- The PR title should be updated to accurately reflect that this change mainly deals with transaction blacklisting.
chain/open-block/src/lib.rs
Outdated
| const ACTIVATION_BLOCK_NUMBER: BlockNumber = 16801958; | ||
| pub fn is_blacklisted(raw_txn: &SignedUserTransaction, block_number: BlockNumber) -> bool { | ||
| block_number > Self::ACTIVATION_BLOCK_NUMBER | ||
| && BLACKLIST | ||
| .iter() | ||
| .map(|&s| AccountAddress::from_str(s).expect("account address decode must success")) | ||
| .any(|x| x == raw_txn.sender()) | ||
| } |
There was a problem hiding this comment.
Error handling in address parsing.
The code uses expect() when parsing addresses from strings:
.map(|&s| AccountAddress::from_str(s).expect("account address decode must success"))This will panic if an address in the blacklist is improperly formatted, which could bring down the node.
Implement proper error handling:
impl AddressFilter {
const ACTIVATION_BLOCK_NUMBER: BlockNumber = 16801958;
pub fn is_blacklisted(raw_txn: &SignedUserTransaction, block_number: BlockNumber) -> bool {
+ // Only check after activation block
+ if block_number <= Self::ACTIVATION_BLOCK_NUMBER {
+ return false;
+ }
+
+ // Check blacklist
+ for &address_str in BLACKLIST.iter() {
+ match AccountAddress::from_str(address_str) {
+ Ok(address) => {
+ if address == raw_txn.sender() {
+ return true;
+ }
+ },
+ Err(e) => {
+ // Log the error but don't panic
+ error!("Invalid address in blacklist: {}, error: {}",
+ address_str, e);
+ }
+ }
+ }
+ false
- block_number > Self::ACTIVATION_BLOCK_NUMBER
- && BLACKLIST
- .iter()
- .map(|&s| AccountAddress::from_str(s).expect("account address decode must success"))
- .any(|x| x == raw_txn.sender())
}
}
chain/open-block/src/lib.rs
Outdated
| pub struct AddressFilter; | ||
| static BLACKLIST: [&str; 0] = []; | ||
| impl AddressFilter { | ||
| const ACTIVATION_BLOCK_NUMBER: BlockNumber = 16801958; | ||
| pub fn is_blacklisted(raw_txn: &SignedUserTransaction, block_number: BlockNumber) -> bool { | ||
| block_number > Self::ACTIVATION_BLOCK_NUMBER | ||
| && BLACKLIST | ||
| .iter() | ||
| .map(|&s| AccountAddress::from_str(s).expect("account address decode must success")) | ||
| .any(|x| x == raw_txn.sender()) | ||
| } | ||
| } |
There was a problem hiding this comment.
AddressFilter implementation with empty blacklist.
This implementation introduces:
- An empty blacklist array (
BLACKLIST: [&str; 0] = []) - A hardcoded activation block number (
16801958) - Logic to filter transactions based on sender address
There are several issues with this implementation:
- The blacklist is currently empty, which means no addresses are actually being filtered despite adding the infrastructure
- The hardcoded activation block number is not explained or documented
- The blacklist is static and requires code changes to update
Consider these improvements:
- Load the blacklist from configuration rather than hardcoding
- Add proper documentation explaining the purpose of this feature
- Add logging when transactions are filtered due to blacklisting
pub struct AddressFilter;
-static BLACKLIST: [&str; 0] = [];
+// TODO: Move this to a configuration file that can be updated without code changes
+static BLACKLIST: [&str; 1] = [
+ "0x...", // Add explanation for why this address is blacklisted
+];
impl AddressFilter {
+ /// Block number after which the blacklist functionality becomes active
const ACTIVATION_BLOCK_NUMBER: BlockNumber = 16801958;
pub fn is_blacklisted(raw_txn: &SignedUserTransaction, block_number: BlockNumber) -> bool {
- block_number > Self::ACTIVATION_BLOCK_NUMBER
+ let is_blacklisted = block_number > Self::ACTIVATION_BLOCK_NUMBER
&& BLACKLIST
.iter()
.map(|&s| AccountAddress::from_str(s).expect("account address decode must success"))
- .any(|x| x == raw_txn.sender())
+ .any(|x| x == raw_txn.sender());
+
+ if is_blacklisted {
+ info!("Transaction from blacklisted address {} filtered at block {}",
+ raw_txn.sender(), block_number);
+ }
+
+ is_blacklisted
}
}📝 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.
| pub struct AddressFilter; | |
| static BLACKLIST: [&str; 0] = []; | |
| impl AddressFilter { | |
| const ACTIVATION_BLOCK_NUMBER: BlockNumber = 16801958; | |
| pub fn is_blacklisted(raw_txn: &SignedUserTransaction, block_number: BlockNumber) -> bool { | |
| block_number > Self::ACTIVATION_BLOCK_NUMBER | |
| && BLACKLIST | |
| .iter() | |
| .map(|&s| AccountAddress::from_str(s).expect("account address decode must success")) | |
| .any(|x| x == raw_txn.sender()) | |
| } | |
| } | |
| pub struct AddressFilter; | |
| // TODO: Move this to a configuration file that can be updated without code changes | |
| static BLACKLIST: [&str; 1] = [ | |
| "0x...", // Add explanation for why this address is blacklisted | |
| ]; | |
| impl AddressFilter { | |
| /// Block number after which the blacklist functionality becomes active | |
| const ACTIVATION_BLOCK_NUMBER: BlockNumber = 16801958; | |
| pub fn is_blacklisted(raw_txn: &SignedUserTransaction, block_number: BlockNumber) -> bool { | |
| let is_blacklisted = block_number > Self::ACTIVATION_BLOCK_NUMBER | |
| && BLACKLIST | |
| .iter() | |
| .map(|&s| AccountAddress::from_str(s).expect("account address decode must success")) | |
| .any(|x| x == raw_txn.sender()); | |
| if is_blacklisted { | |
| info!( | |
| "Transaction from blacklisted address {} filtered at block {}", | |
| raw_txn.sender(), | |
| block_number | |
| ); | |
| } | |
| is_blacklisted | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
chain/Cargo.toml (1)
52-54: 💡 Verification agent🧩 Analysis chain
Verify removal of force-deploy feature.
The
force-deployfeature has been removed from the features section. Please ensure that no other part of the codebase still references this feature flag.
🏁 Script executed:
#!/bin/bash # Description: Search for any remaining references to the force-deploy feature flag # Check for any references to force-deploy feature in the codebase echo "Searching for remaining references to force-deploy feature flag..." rg --type rust 'force-deploy|force_deploy' -g '!target/' -g '!*/target/'Length of output: 925
Action Required: Remove remaining force-deploy references
The search output shows that while the Cargo.toml changes correctly removed the feature flag, there are still multiple references to
force-deploy/force_deployin the codebase. Please review and update the following:
- cmd/db-exporter/src/lib.rs: Contains
pub mod force_deploy_output;- cmd/db-exporter/src/force_deploy_output.rs: Shows
"name = "force-deploy"and functions likeforce_deploy_output- cmd/db-exporter/src/main.rs: Uses
force_deploy_outputandForceDeployOutput- chain/force-upgrade/src/lib.rs: References a function named
force_deploy_txn- chain/open-block/src/lib.rs: Calls
ForceUpgrade::force_deploy_txnand comments related to force-deploy transactions- executor/src/block_executor.rs: Also calls
ForceUpgrade::force_deploy_txnPlease ensure these references are updated or removed if the feature is deprecated.
chain/tests/test_force_upgrade.rs (1)
196-197:⚠️ Potential issueOutdated assertion for upgraded version.
This test still asserts that the upgraded version should be 12, but according to the PR changes, it should be verifying version 13.
Update the assertion to match the expected version:
- assert_eq!(upgraded_version, 12); + assert_eq!(upgraded_version, 13);
🧹 Nitpick comments (1)
executor/src/block_executor.rs (1)
144-147: Improved error handling for block metadata retrieval.The code now properly handles the case where block metadata is missing by adding explicit error handling with a descriptive message.
📜 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 (13)
.gitignore(1 hunks)Cargo.toml(4 hunks)chain/Cargo.toml(1 hunks)chain/force-upgrade/Cargo.toml(1 hunks)chain/force-upgrade/src/lib.rs(1 hunks)chain/open-block/Cargo.toml(1 hunks)chain/open-block/src/lib.rs(4 hunks)chain/src/chain.rs(2 hunks)chain/tests/test_force_upgrade.rs(7 hunks)cmd/starcoin/Cargo.toml(0 hunks)executor/Cargo.toml(0 hunks)executor/src/block_executor.rs(3 hunks)genesis/src/lib.rs(1 hunks)
💤 Files with no reviewable changes (2)
- executor/Cargo.toml
- cmd/starcoin/Cargo.toml
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- chain/force-upgrade/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- chain/open-block/src/lib.rs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Generate code coverage report
🔇 Additional comments (14)
chain/open-block/Cargo.toml (1)
18-18: Added dependency is consistent with the PR objectives.The addition of
starcoin-vm-runtimeas a dependency aligns with the force upgrade functionality changes, where functionality has been moved fromstarcoin-force-upgradetostarcoin-vm-runtime.chain/Cargo.toml (1)
24-24: Made vm-runtime dependency mandatory.The
starcoin-vm-runtimedependency is now a mandatory dependency (no longer optional). This is consistent with the broader simplification of the force upgrade functionality and removal of theforce-deployfeature flag.chain/force-upgrade/src/lib.rs (2)
27-27: Updated stdlib version from 12 to 13.The package file path has been updated to reference stdlib version 13 (
"13/12-13/stdlib.blob"), which aligns with the PR objective to upgrade the stdlib version.
26-35:✅ Verification successful
Verify stdlib blob file existence.
Please ensure that the referenced stdlib blob file exists in the expected location.
🏁 Script executed:
#!/bin/bash # Description: Check if the referenced stdlib blob file exists # Check if the file exists in the compiled directory echo "Checking for the existence of the stdlib blob file..." fd -g "13/12-13/stdlib.blob" . # Attempt to find where stdlib files are located if the above fails if [ $? -ne 0 ]; then echo "File not found directly, searching for similar stdlib blob files..." fd -g "*stdlib.blob" . fiLength of output: 165
File Existence Verified:
The stdlib blob file"13/12-13/stdlib.blob"has been successfully located in the expected directory based on the verification script output. No further changes are needed in this section.chain/tests/test_force_upgrade.rs (2)
10-10: Updated import path for force upgrade management.The import for
get_force_upgrade_block_numberhas been correctly moved from the force-upgrade module to the vm-runtime module, consistent with the module restructuring in this PR.
34-34: Test for previous version upgrade is ignored.This test has been marked as ignored because it's specifically for testing the upgrade from version 11 to 12. Consider adding a similar test for the upgrade from version 12 to 13 to maintain test coverage.
Is there a separate test that covers the upgrade from version 12 to 13? If not, the test coverage may be inadequate.
genesis/src/lib.rs (1)
539-543: ACTION: Verify consensus config migration for stdlib upgrade from v12 to v13.The commented-out assertion indicates that the consensus config verification is intentionally disabled pending the upgrade. Please confirm that:
- The new consensus config values (e.g., gas limits, time targets, difficulty windows, etc.) are being set as expected.
- All necessary migration steps for transitioning from v12 to v13 are implemented and documented.
- All dependent modules (such as genesis initialization and transaction builder logic) correctly handle the updated consensus config.
Once these items are verified, consider adding or re-enabling corresponding tests to ensure the configuration matches post-upgrade requirements.
Cargo.toml (2)
43-43: Module relocation fromvm/tochain/.The force-upgrade module has been moved from the VM subsystem to the chain subsystem. This reorganization better aligns with the module's functionality, as force upgrades are more closely related to chain operations than VM operations.
Also applies to: 157-157, 476-476
458-458:❓ Verification inconclusive
Verify the new framework revision compatibility.
The starcoin-framework dependency has been updated to a new revision (
748b28ec62c7133833947dd0e27da1258efe2dd0). Ensure this version is compatible with the consensus changes and stdlib v13 upgrade.
🏁 Script executed:
#!/bin/bash # Check what changes are included in the new framework revision echo "Fetching information about the new framework revision..." git clone https://github.com/starcoinorg/starcoin-framework.git /tmp/starcoin-framework cd /tmp/starcoin-framework git log -n 1 748b28ec62c7133833947dd0e27da1258efe2dd0 --name-statusLength of output: 345
Ensure Compatibility Verification for the New Revision
The dependency now points to commit
748b28ec62c7133833947dd0e27da1258efe2dd0for the starcoin-framework. While the initial clone was successful, the executed script didn’t display the commit details. Please verify that the changes in this commit—especially in the consensus-related components and any updates affecting the stdlib v13 upgrade—are fully compatible with our implementation. For a deeper review, run:cd /tmp/starcoin-framework git show 748b28ec62c7133833947dd0e27da1258efe2dd0 --statReview the output for any pertinent changes and confirm they align with our consensus and stdlib expectations.
chain/src/chain.rs (2)
42-42: Relocated force-upgrade functionality to vm-runtime.The force-upgrade functions have been moved from
starcoin_force_upgradetostarcoin_vm_runtime. This is a good refactoring that centralizes VM-related functionality.
557-563: Simplified transaction validation for force-upgrade blocks.The conditional logic for validating transaction numbers has been consolidated, removing the feature flag dependency. This streamlines the code while maintaining the special handling for force-upgrade blocks.
executor/src/block_executor.rs (3)
25-27: Updated import paths for force-upgrade management.Import paths have been updated to reflect the relocation of force-upgrade functionality to the vm-runtime module, which improves code organization.
133-138: Updated version check from v11 to v12.The stdlib version check has been updated to check for version 12 instead of 11, aligning with the current version before upgrading to v13.
205-213: Stdlib version updated from v12 to v13.The version update logic now directly sets the stdlib version to 13, which is consistent with the PR objective of upgrading to v13 for the consensus update.
| @@ -16,7 +17,7 @@ use test_helper::executor::get_balance; | |||
| #[stest::test] | |||
| pub fn test_force_upgrade_generate_block() -> anyhow::Result<()> { | |||
| let mut genesis_config = BuiltinNetworkID::Test.genesis_config().clone(); | |||
| genesis_config.stdlib_version = StdlibVersion::Version(11); | |||
| genesis_config.stdlib_version = StdlibVersion::Version(12); | |||
There was a problem hiding this comment.
Version mismatch between tests and implementation.
The tests are still configured to use stdlib version 12, but the implementation in chain/force-upgrade/src/lib.rs has been updated to version 13. This appears to be an inconsistency that should be addressed.
Either update these tests to use version 13, or provide documentation explaining why the tests should remain at version 12.
- genesis_config.stdlib_version = StdlibVersion::Version(12);
+ genesis_config.stdlib_version = StdlibVersion::Version(13);Also applies to: 37-37, 53-53, 205-205
| vec![miner.current_header().id()], | ||
| HashValue::zero(), |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
API parameter changes need verification.
The function calls now include a vector of header IDs and a zero hash value instead of None. Ensure that this change is consistently applied throughout the codebase and that all callers have been updated.
Also applies to: 142-143
🏁 Script executed:
#!/bin/bash
# Description: Check for any occurrences of create_block_template with None parameter
# Search for create_block_template calls with None parameter
echo "Searching for create_block_template calls that might need updating..."
rg -A 10 -B 5 "create_block_template\(" --type rust | grep -B 5 -A 2 "None,"Length of output: 29334
API Parameter Consistency Update Required
While the changes in chain/tests/test_force_upgrade.rs (lines 104–105 and the corresponding 142–143 update) correctly replace the old None with vec![miner.current_header().id()] and HashValue::zero(), a grep search across the repository shows that many other invocations of create_block_template still pass None for the affected parameter(s). For example, similar patterns appear in:
- cmd/db-exporter/src/main.rs
- chain/tests/test_txn_info_and_proof.rs
- chain/tests/test_block_chain.rs
- miner/src/create_block_template/test_create_block_template.rs
- sync/src/block_connector/ (multiple test files)
Please verify whether these remaining None values are intentional (e.g., for context-specific calls) or if they should be updated to follow the new API—ensuring all callers consistently provide a vector of header IDs and an explicit hash value. Adjust the calls accordingly if the API change applies universally.
Benchmark for 730595aClick to view benchmark
|
Pull request type
Please check the type of change your PR introduces:
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Other information
Summary by CodeRabbit