chore: rust upgrade #4436
chore: rust upgrade #4436
Conversation
WalkthroughThis pull request introduces new feature flags in multiple Cargo.toml files, adding placeholders for "print_gas_info" and "testing". It updates conditional compilation attributes from configuration flags (e.g., Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Executor
participant Logger
User->>Executor: Trigger transaction execution
Executor->>Executor: Check if "testing" feature is enabled
alt "testing" enabled
Executor->>Logger: Log transaction details (gas usage, txn hash)
else "testing" disabled
Executor-->>Executor: Skip logging
end
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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 (
|
nkysg
requested review from
welbon,
simonjiao and
jackzhhuang
and removed request for
jolestar
![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 (4)
miner/src/create_block_template/metrics.rs (1)
6-6: Suppressing Dead Code Warning in BlockBuilderMetrics
Adding#[allow(dead_code)]here is useful to suppress compiler warnings for potentially unused code in theBlockBuilderMetricsstruct. Just ensure that this attribute remains temporary or is revisited when the code becomes active to avoid masking dead code issues.network-p2p/src/metrics.rs (1)
7-7: Suppressing Dead Code Warning in Metrics Struct
The addition of#[allow(dead_code)]effectively suppresses warnings for the unused portions of theMetricsstruct. It’s a pragmatic short-term measure; however, consider scheduling a review later to either integrate or remove dead code as the project evolves.sync/src/parallel/executor.rs (1)
19-19: Suppress Dead Code Warning for ExecuteState Enum
The new#[allow(dead_code)]attribute for theExecuteStateenum helps avoid unnecessary warnings during compilation. Verify that this enum is indeed planned for future use, or consider cleaning it up if it remains unused to keep the codebase lean.vm/vm-runtime/Cargo.toml (1)
49-49: New Feature Flag: print_gas_info
The addition of theprint_gas_infofeature flag is a good move towards modularizing optional functionality. Consider updating the corresponding documentation and tests to explain when and how this feature should be enabled, ensuring clarity for future maintenance.
📜 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)
executor/Cargo.toml(1 hunks)executor/src/block_executor.rs(0 hunks)executor/src/lib.rs(1 hunks)miner/src/create_block_template/metrics.rs(1 hunks)network-p2p/src/metrics.rs(1 hunks)node/src/crash_handler.rs(1 hunks)rust-toolchain.toml(1 hunks)sync/src/parallel/executor.rs(1 hunks)vm/starcoin-gas-meter/Cargo.toml(1 hunks)vm/starcoin-gas-meter/src/gas_meter.rs(27 hunks)vm/types/Cargo.toml(1 hunks)vm/vm-runtime/Cargo.toml(1 hunks)vm/vm-runtime/src/starcoin_vm.rs(3 hunks)
💤 Files with no reviewable changes (1)
- executor/src/block_executor.rs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run benchmark
🔇 Additional comments (39)
rust-toolchain.toml (1)
5-5: Rust Toolchain Upgrade to 1.82.0
Updating the Rust toolchain channel to1.82.0should bring in the latest improvements and features. Please ensure that this upgrade has been thoroughly validated with your CI build and that all modules are compatible with the new toolchain release.vm/types/Cargo.toml (1)
48-48: Addition ofprint_gas_infofeature looks goodThis new feature flag provides a clean way to conditionally compile gas information printing functionality, which is helpful for debugging and development. The empty array indicates it doesn't automatically enable any dependencies.
vm/starcoin-gas-meter/Cargo.toml (1)
12-14: Addition of features section withtestingfeature is appropriateAdding a dedicated
testingfeature flag provides a standard way to conditionally compile test-only code. This follows modern Rust practices for organizing conditional compilation and aligns with similar changes in other packages within this PR.executor/Cargo.toml (1)
40-41: Addition ofprint_gas_infoandtestingfeatures is appropriateAdding these feature flags provides a standard way to enable optional functionality, consistent with modern Rust practices. These features align with similar changes in other packages (vm/types, starcoin-gas-meter) and are part of a broader effort to standardize conditional compilation across the project.
executor/src/lib.rs (1)
4-4: Update to standard feature flag syntax is appropriateChanging from
#[cfg(testing)]to#[cfg(feature = "testing")]follows modern Rust conventions for feature flags. This approach properly ties conditional compilation to the feature declared in Cargo.toml, making the codebase more maintainable and aligned with Rust ecosystem standards.vm/vm-runtime/src/starcoin_vm.rs (3)
951-952: Conditional compilation attribute updated to use feature flag.The change from
#[cfg(testing)]to#[cfg(feature = "testing")]switches from using a configuration flag to using an explicit feature flag, which provides better control over enabling the logging functionality during testing.
1010-1011: Conditional compilation attribute updated to use feature flag.This change ensures consistent usage of the
testingfeature flag across the codebase, matching the pattern on line 951.
1576-1581: Conditional compilation attribute updated to use feature flag.Similar to the previous changes, this updates the conditional compilation attribute for the debugging information logged during
charge_global_write_gas_usage. This change aligns with the Rust feature flagging system.node/src/crash_handler.rs (3)
7-7: Updated panic info type import.The import has been updated from
PanicInfotoPanicHookInfo, which appears to be part of the Rust upgrade mentioned in the PR title.
13-13: Panic hook parameter type updated for compatibility.The closure parameter type has been changed from
PanicInfotoPanicHookInfoto match the updated import. This is a necessary change to maintain compatibility with the newer Rust version.
19-19: Function parameter type updated for consistency.The parameter type for the
handle_panicfunction has been updated to usePanicHookInfoinstead ofPanicInfo, maintaining consistency with the changes above.vm/starcoin-gas-meter/src/gas_meter.rs (28)
22-23: Updated conditional compilation attribute for logger import.The import statement for
starcoin_logger::prelude::*now uses#[cfg(feature = "testing")]instead of what was likely#[cfg(testing)]. This aligns with modern Rust practices of using feature flags for conditional compilation.
190-194: Updated conditional logging to use feature flag.Switched from configuration flag to feature flag for conditional compilation of log statements during testing. This provides more explicit control over when debugging logs are generated.
295-301: Updated conditional logging to use feature flag.Similar to the previous change, the debugging output for instruction costs now depends on the "testing" feature being enabled.
308-312: Updated conditional logging to use feature flag.Consistent update to use feature flags for logging statements.
327-331: Updated conditional logging to use feature flag.Continuing the pattern of updating all logging statements to use the "testing" feature flag.
350-362: Updated conditional logging to use feature flag.Both logging statements for CALL_GENERIC cost information now depend on the "testing" feature.
369-370: Consistent update to use feature flag for logging.Changed LD_CONST logging to use the "testing" feature flag.
388-389: Updated conditional logging to use feature flag.COPY_LOC logging now uses the feature flag system.
400-401: Updated conditional logging to use feature flag.MOVE_LOC logging now uses the feature flag system.
412-413: Updated conditional logging to use feature flag.ST_LOC logging now uses the feature flag system.
432-442: Updated conditional logging to use feature flag.PACK/PACK_GENERIC logging now uses the feature flag system.
452-472: Updated conditional logging to use feature flag.UNPACK opcodes logging now uses the feature flag system.
491-492: Updated conditional logging to use feature flag.READ_REF logging now uses the feature flag system.
506-507: Updated conditional logging to use feature flag.WRITE_REF logging now uses the feature flag system.
518-519: Updated conditional logging to use feature flag.EQ operation logging now uses the feature flag system.
530-531: Updated conditional logging to use feature flag.NEQ operation logging now uses the feature flag system.
554-563: Updated conditional logging to use feature flag.Borrow global operation logging now uses the feature flag system.
584-594: Updated conditional logging to use feature flag with improved syntax.The conditional logging for EXISTS operations now uses the feature flag system with improved block syntax for multiple statements.
612-622: Updated conditional logging to use feature flag with improved syntax.The conditional logging for MOVE_FROM operations now uses the feature flag system with improved block syntax.
646-656: Updated conditional logging to use feature flag with improved syntax.The conditional logging for MOVE_TO operations now uses the feature flag system with improved block syntax.
669-670: Updated conditional logging to use feature flag.VEC_PACK operation logging now uses the feature flag system.
677-678: Updated conditional logging to use feature flag.VEC_LEN operation logging now uses the feature flag system.
698-708: Updated conditional logging to use feature flag with improved syntax.Vector borrow operations logging now uses the feature flag system with improved block syntax.
722-726: Updated conditional logging to use feature flag.VEC_PUSH_BACK operation logging now uses the feature flag system.
737-741: Updated conditional logging to use feature flag.VEC_POP_BACK operation logging now uses the feature flag system.
756-757: Updated conditional logging to use feature flag.VEC_UNPACK operation logging now uses the feature flag system.
764-765: Updated conditional logging to use feature flag.VEC_SWAP operation logging now uses the feature flag system.
783-787: Updated conditional logging to use feature flag.NATIVE_FUNCTION operation logging now uses the feature flag system.
Benchmark for 5d0621cClick to view benchmark
|
| @@ -16,6 +16,7 @@ use tokio::{ | |||
|
|
|||
| const MAX_TOTAL_WAITING_TIME: u64 = 3600000; // an hour | |||
|
|
|||
| #[allow(dead_code)] | |||
| @@ -7,6 +7,7 @@ use starcoin_node::NodeHandle; | |||
| use starcoin_types::block::BlockHeader; | |||
| use std::sync::Arc; | |||
|
|
|||
| #[allow(dead_code)] | |||
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
Cargo.toml (1)
226-228: Lint Rule Configuration Change:
The removal of theuse_selflint and the addition ofdoc_lazy_continuation = "allow"reflects a deliberate update to the project's Clippy configuration. Please ensure that this change is documented in your coding guidelines so that contributors understand the rationale behind this adjustment.scripts/check_commit.sh (1)
12-12: Clippy Lint Flags Update in Commit Check Script:
The updated commandcargo clippy --all-targets -- -Aclippy::doc_lazy_continuation -D warningsaligns with the new lint configuration in Cargo.toml. Verify that this change does not inadvertently suppress any warnings that could indicate real issues in the code.
.github/workflows/build_test.yml (1)
32-32: Workflow Lint Flags Consistency:
The change to setRUSTFLAGSas-Aclippy::doc_lazy_continuation -D warningsensures that both local checks and CI enforce the updated linting rules consistently. It would be beneficial to update any related developer documentation or onboarding guides to reflect this change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (39)
.github/workflows/build_test.yml(1 hunks)Cargo.toml(1 hunks)chain/tests/block_test_utils.rs(1 hunks)chain/tests/test_block_chain.rs(3 hunks)cmd/miner_client/src/cpu_solver.rs(1 hunks)cmd/starcoin/src/account/nft_cmd.rs(2 hunks)cmd/starcoin/src/account/show_cmd.rs(1 hunks)cmd/starcoin/src/state/list_cmd.rs(1 hunks)commons/accumulator/src/accumulator_test.rs(1 hunks)commons/accumulator/src/node_index.rs(6 hunks)commons/accumulator/src/tree.rs(1 hunks)commons/stream-task/src/collector.rs(1 hunks)consensus/src/difficulty.rs(1 hunks)consensus/src/lib.rs(2 hunks)network-p2p/peerset/src/lib.rs(2 hunks)network-p2p/peerset/tests/fuzz.rs(1 hunks)network-p2p/src/discovery.rs(1 hunks)network-p2p/src/out_events.rs(2 hunks)network-p2p/src/protocol.rs(1 hunks)network-p2p/src/protocol/generic_proto/upgrade/notifications.rs(2 hunks)network-p2p/src/request_responses.rs(2 hunks)network-p2p/src/service_test.rs(1 hunks)rpc/api/src/state/mod.rs(1 hunks)rpc/api/src/types.rs(2 hunks)rpc/api/src/types/pubsub.rs(1 hunks)scripts/check_commit.sh(1 hunks)stratum/src/lib.rs(2 hunks)sync/src/announcement/mod.rs(1 hunks)sync/src/store/tests.rs(0 hunks)sync/tests/common_test_sync_libs.rs(1 hunks)txpool/src/pool.rs(1 hunks)txpool/src/tx_pool_service_impl.rs(2 hunks)types/uint/src/tests.rs(1 hunks)vm/parallel-executor/src/scheduler.rs(1 hunks)vm/transaction-builder-generator/src/python3.rs(1 hunks)vm/types/src/token/token_value.rs(1 hunks)vm/types/src/transaction/mod.rs(3 hunks)vm/types/src/transaction/tests.rs(1 hunks)vm/vm-runtime/src/starcoin_vm.rs(5 hunks)
💤 Files with no reviewable changes (1)
- sync/src/store/tests.rs
✅ Files skipped from review due to trivial changes (29)
- rpc/api/src/state/mod.rs
- sync/tests/common_test_sync_libs.rs
- txpool/src/pool.rs
- network-p2p/src/out_events.rs
- cmd/starcoin/src/state/list_cmd.rs
- cmd/starcoin/src/account/show_cmd.rs
- rpc/api/src/types/pubsub.rs
- consensus/src/difficulty.rs
- sync/src/announcement/mod.rs
- vm/types/src/token/token_value.rs
- cmd/starcoin/src/account/nft_cmd.rs
- chain/tests/block_test_utils.rs
- stratum/src/lib.rs
- network-p2p/src/service_test.rs
- network-p2p/peerset/tests/fuzz.rs
- commons/accumulator/src/node_index.rs
- network-p2p/src/protocol.rs
- consensus/src/lib.rs
- rpc/api/src/types.rs
- cmd/miner_client/src/cpu_solver.rs
- vm/parallel-executor/src/scheduler.rs
- network-p2p/peerset/src/lib.rs
- network-p2p/src/request_responses.rs
- vm/types/src/transaction/tests.rs
- commons/accumulator/src/accumulator_test.rs
- vm/types/src/transaction/mod.rs
- chain/tests/test_block_chain.rs
- network-p2p/src/discovery.rs
- commons/accumulator/src/tree.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- vm/vm-runtime/src/starcoin_vm.rs
🧰 Additional context used
🧬 Code Definitions (1)
txpool/src/tx_pool_service_impl.rs (2)
txpool/src/pool.rs (1)
sender(211-213)vm/types/src/transaction/mod.rs (2)
sender(257-259)sender(523-525)
🔇 Additional comments (8)
vm/transaction-builder-generator/src/python3.rs (1)
464-464: Updated string splitting syntax to a more idiomatic formThe code now uses array syntax
['.',]instead of a closure|c| c == '.'for specifying the delimiter in thesplitnmethod. This change maintains the same functionality while adopting more modern Rust syntax.commons/stream-task/src/collector.rs (1)
86-86: Modernized code with improved clarity.Changed from
u64::max_value()tou64::MAX, which is the more idiomatic approach in modern Rust. This improves code readability while maintaining the same functionality.types/uint/src/tests.rs (1)
12-12: Consistent modernization of U256 maximum value references.All instances of
U256::max_value()have been replaced with the more idiomaticU256::MAX. This change follows modern Rust conventions and improves code readability while maintaining the same test functionality.Also applies to: 14-14, 19-21
network-p2p/src/protocol/generic_proto/upgrade/notifications.rs (2)
159-159: Good modernization: Usingusize::MAXinstead ofusize::max_value()This change modernizes the code by using the
usize::MAXconstant instead of the deprecatedusize::max_value()function. This is in line with Rust's evolution, as constants are preferred over function calls for these values.
373-373: Good modernization: Usingusize::MAXinstead ofusize::max_value()Similar to the earlier change, this replaces the deprecated
usize::max_value()with the modernusize::MAXconstant. Both changes are part of the Rust upgrade effort and represent current best practices in Rust development.txpool/src/tx_pool_service_impl.rs (3)
207-207: Modernized Rust syntax for maximum valueThe code has been updated to use
usize::MAXinstead ofusize::max_value(). This is a good modernization that uses the more concise constant syntax introduced in Rust 1.43.0. Both represent the same maximum value, so this is a non-functional change that improves code readability.
292-292: Updated to use u64::MAX constantThe code has been updated to use
u64::MAXinstead ofu64::max_value()when setting theblock_numberfield of thePendingSettingsstruct. This is part of a consistent effort to modernize the codebase by using the more concise constant syntax introduced in Rust 1.43.0.
297-298: Modernized parameter in inner_status callThe code has been updated to use
u64::MAXinstead ofu64::max_value()when calling theinner_statusmethod. This change is consistent with the other updates in this file and across the codebase, standardizing on the newer Rust syntax for maximum values.
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
New Features
Chores
Refactor
Style
MAXinstead ofmax_value()).Documentation