Skip to content

Conversation

@drewstone
Copy link
Contributor

  • chore: fix clippy warnings for stable2503
  • Fix TxBaseImplication constructor usage in transaction extension tests
  • Add #[allow(dead_code)] for unused test mock utilities
  • Replace manual absolute difference with .abs_diff() method
  • Convert test constants to uppercase (Alice -> ALICE, etc)
  • Add missing 11th parameter (authorization_list) to Evm::call for EIP-7702
  • chore: use FreeEVMExecution in EVM config

Replace () with FreeEVMExecution for OnChargeTransaction type to properly
utilize the mock implementation and eliminate dead code warning.

  • chore: remove unused dead code from rewards mocks

Remove ExtBuilder and MockedEvmRunner that were never used.
These were copy-pasted boilerplate from the original PR but
tests use new_test_ext() directly without needing ExtBuilder,
and nothing references MockedEvmRunner.

  • chore: update remaining mocks for polkadot-sdk stable2503
  • Add DoneSlashHandler to pallet_balances::Config
  • Add EVM config types (AccountProvider, CreateOriginFilter, CreateInnerOriginFilter, GasLimitStorageGrowthRatio)
  • Add Holder type to pallet_assets::Config
  • Add dev_accounts field to GenesisConfig
  • Update migrations to use new storage API
  • Implement DecodeWithMemTracking trait where neededSummary of changes
    Changes introduced in this pull request:

Reference issue to close (if applicable)

Closes

1xstj and others added 19 commits October 2, 2025 23:00
* chore: fix clippy warnings for stable2503

- Fix TxBaseImplication constructor usage in transaction extension tests
- Add #[allow(dead_code)] for unused test mock utilities
- Replace manual absolute difference with .abs_diff() method
- Convert test constants to uppercase (Alice -> ALICE, etc)
- Add missing 11th parameter (authorization_list) to Evm::call for EIP-7702

* chore: use FreeEVMExecution in EVM config

Replace () with FreeEVMExecution for OnChargeTransaction type to properly
utilize the mock implementation and eliminate dead code warning.

* chore: remove unused dead code from rewards mocks

Remove ExtBuilder and MockedEvmRunner that were never used.
These were copy-pasted boilerplate from the original PR but
tests use new_test_ext() directly without needing ExtBuilder,
and nothing references MockedEvmRunner.

* chore: update remaining mocks for polkadot-sdk stable2503

- Add DoneSlashHandler to pallet_balances::Config
- Add EVM config types (AccountProvider, CreateOriginFilter, CreateInnerOriginFilter, GasLimitStorageGrowthRatio)
- Add Holder type to pallet_assets::Config
- Add dev_accounts field to GenesisConfig
- Update migrations to use new storage API
- Implement DecodeWithMemTracking trait where needed
@drewstone
Copy link
Contributor Author

@danielbui12 we'll want to finish this too, lets chat about what you're stuck on.

Cargo.toml Outdated
blueprint-manager = { default-features = false, git = "https://github.com/tangle-network/blueprint", branch = "polkadot-stable2407" }
blueprint-runner = { default-features = false, git = "https://github.com/tangle-network/blueprint", branch = "polkadot-stable2407" }
blueprint-keystore = { default-features = false, git = "https://github.com/tangle-network/blueprint", branch = "polkadot-stable2407" }
# blueprint-manager = { default-features = false, git = "https://github.com/tangle-network/blueprint", branch = "polkadot-stable2503" }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danielbui12 we need to update the blueprint sdk for this to work fully

drewstone and others added 6 commits November 11, 2025 14:47
Uncomments blueprint-manager, blueprint-runner, and blueprint-keystore
dependencies, pointing to the polkadot-stable2503 branch of the blueprint
repository (formerly gadget).

Co-Authored-By: Claude <[email protected]>
Investigation findings:
- The `graph` variable was correctly used by txpool RPC functionality
- However, the txpool feature is broken due to H256 type mismatches:
  * rpc_txpool::TxPool requires B: BlockT<Hash = ethereum_types::H256>
  * Tangle's Block type uses sp_core::H256
  * This causes trait bound failures when trying to use TxPoolServer

The txpool RPC has been temporarily disabled, consistent with other
Ethereum RPC features (Eth, EthFilter, EthPubSub, Trace, Debug) which
are also disabled due to H256 type mismatches in the stable2503 upgrade.

This ensures the code compiles correctly both with and without the
txpool feature flag, preparing it properly for production.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
Replaces manual Default implementation with derived version using
#[default] attribute on the OneMonth variant. This satisfies the
clippy::derivable_impls lint.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
Replaces manual Default implementation with derived version using
#[default] attribute on the Permissioned variant. This satisfies the
clippy::derivable_impls lint.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
Formats additional test files that were missed in the initial nightly rustfmt
run. These files had formatting differences that CI detected.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
@danielbui12 danielbui12 self-requested a review November 12, 2025 15:16
@danielbui12 danielbui12 marked this pull request as ready for review November 12, 2025 15:16
@claude
Copy link

claude bot commented Nov 12, 2025

Claude finished @drewstone's task —— View job


PR Review: Polkadot SDK Upgrade to stable2503

Review Tasks:

  • Read repository guidelines (CLAUDE.md)
  • Analyze core runtime and pallet changes
  • Review EVM integration updates
  • Check migration safety and compatibility
  • Examine security implications
  • Review testing and mock updates
  • Provide comprehensive feedback

Overall Assessment

This is a comprehensive upgrade to Polkadot SDK stable2503 with significant breaking changes. The changes appear well-structured and follow Substrate migration best practices. Here's my detailed feedback:

✅ Positive Aspects

1. Comprehensive Migration Strategy

  • Well-structured migration: pallets/multi-asset-delegation/src/migrations.rs:31-283 shows a thorough migration with proper pre/post upgrade checks
  • Backwards compatibility: Migration preserves existing data while adding new fields like is_nomination
  • Error handling: Proper error handling with detailed logging for failed migrations

2. EVM Integration Updates

  • EIP-7702 Support: Added authorization_list parameter throughout EVM calls for EIP-7702 compatibility
  • Consistent implementation: The parameter is properly added across all runtime call sites and mock implementations
  • Future-proofing: Using _authorization_list prefix indicates awareness it's not yet fully utilized

3. Code Quality Improvements

  • Modern Rust patterns: Replaced manual absolute difference calculations with .abs_diff() method
  • Proper constants: Test constants converted to uppercase following Rust conventions
  • Memory tracking: Added DecodeWithMemTracking implementations where required

⚠️ Areas of Concern

1. Migration Safety - Medium Priority

Location: pallets/multi-asset-delegation/src/migrations.rs:130-158

let raw_value = match unhashed::get_raw(&raw_key) {
    Some(bytes) => bytes,
    None => {
        log::warn!("No raw metadata found for account: {:?}", account_id);
        continue;
    },
};

Issue: Migration continues on missing raw metadata, but this could indicate data corruption or unexpected storage state.
Recommendation: Consider making this a hard error or implementing more robust validation.

2. Potential Data Loss Risk - High Priority

Location: pallets/multi-asset-delegation/src/migrations.rs:205-213

if new_withdraw_requests.try_push(WithdrawRequest { ... }).is_err() {
    log::warn!("Failed to migrate a withdraw request for account {:?}: exceeded capacity", account_id);
    break;
}

Issue: When capacity limits are exceeded, remaining requests are silently dropped without rollback.
Recommendation: Either increase capacity limits or implement a two-phase migration approach.

3. EVM Configuration Concerns - Medium Priority

Location: Various mock_evm.rs files

  • FreeEVMExecution: Using free execution in production could be exploited
  • Mock configurations: Some mock files still contain placeholder/development configurations that should be reviewed for production use

4. Missing Test Updates - Medium Priority

Location: pallets/multi-asset-delegation/src/tests/native_restaking.rs:*
The TxBaseImplication constructor calls suggest tests may need updates for new transaction extension patterns.

🔒 Security Implications

1. Runtime Upgrade Safety

  • Migration weight calculation: Proper weight accounting in migrations prevents DoS attacks
  • Storage version checks: Missing explicit storage version checks could allow re-running migrations

2. EVM Security

  • Authorization lists: EIP-7702 authorization lists not fully implemented but parameter exists - ensure this doesn't create attack vectors
  • Gas accounting: EVM configuration changes maintain proper gas accounting

3. Consensus Critical

  • Balance configuration: Added DoneSlashHandler = () maintains slashing behavior
  • Transaction extensions: Proper integration with new transaction extension system

📋 Specific Recommendations

1. Immediate Actions

// In migrations.rs, add explicit storage version management:
const MIGRATION_VERSION: StorageVersion = StorageVersion::new(1);

#[cfg(feature = "try-runtime")]
fn pre_upgrade() -> Result<Vec<u8>, sp_runtime::DispatchError> {
    // Verify current version before migration
    ensure!(
        Pallet::<T>::on_chain_storage_version() < MIGRATION_VERSION,
        "Migration already applied"
    );
    // ... rest of pre_upgrade
}

2. Testing Enhancements

  • Add integration tests for the new is_nomination field behavior
  • Test migration edge cases (capacity limits, corrupted data)
  • Verify EVM authorization_list parameter doesn't break existing functionality

3. Documentation Updates

  • Missing: No documentation updates for the breaking changes
  • Recommended: Add migration guide for downstream projects
  • Runtime spec: Update runtime spec version to reflect changes

🚨 Blockers (Must Fix)

1. Missing Storage Version Updates

The migrations don't explicitly update storage versions, which could allow accidental re-execution.

2. Capacity Limit Handling

Migration can lose data when capacity limits are exceeded - this needs robust handling or increased limits.

✅ Nice to Have

  1. Performance: Consider batched migration for large datasets
  2. Monitoring: Add metrics for migration success/failure rates
  3. Rollback: Consider implementing rollback mechanisms for failed migrations

Summary

This is a well-executed Polkadot SDK upgrade with proper attention to migration safety and backwards compatibility. The main concerns are around potential data loss during migration and missing storage version management. With the recommended fixes, this should be safe for deployment.

Approval Status: Conditional - fix the high-priority data loss risk and add storage version management.


danielbui12 and others added 4 commits November 12, 2025 22:32
Fixes clippy::derivable_impls warning by deriving Default instead of
manually implementing it.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
Removed duplicate Default trait in derive macro which was causing
conflicting implementations error (E0119).

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
Restored evm_restaking.rs and services_integration.rs test files that were
disabled. These tests are needed for the e2e CI check.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
@drewstone drewstone force-pushed the drew/bump-polkadot-sdk branch from c7f0bb2 to 3f1e491 Compare November 12, 2025 20:46
drewstone and others added 5 commits November 12, 2025 17:59
Added organized chopsticks/ directory for rigorous runtime migration testing:

- Mainnet and testnet fork configurations (migrated from scripts/chopsticks.yml)
- Automated test-migration.sh script for quick testing
- Comprehensive README with testing workflow
- Directory structure for configs, scripts, db, snapshots

Key features:
- Test runtime upgrades against live chain state safely
- Identify missing migrations before deployment
- Validate storage migrations with try-runtime-cli
- Database caching for faster iteration

Critical findings documented:
- Mainnet migrations currently commented out due to Currency trait issues
- Need to update to fungible traits for stable2503 compatibility
- Multiple Polkadot SDK pallet migrations may be required

Next steps:
1. Install try-runtime-cli and chopsticks (npm)
2. Build runtime with --features try-runtime
3. Run ./chopsticks/scripts/test-migration.sh mainnet
4. Address identified migration issues

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
- Fix System::Account storage format to use proper array syntax
- Update db paths to be relative to chopsticks directory
- Validate chopsticks fork works (successfully forks mainnet on port 8000)
- Update README with validation status and setup instructions
- Start try-runtime-cli installation and testnet runtime build for full e2e testing
Fixed critical Ethereum RPC functionality that was disabled due to H256 type incompatibility.

Changes:
- client/rpc/txpool/src/lib.rs: Removed hardcoded ethereum_types::H256 from trait bounds
  - Changed `B: BlockT<Hash = H256>` to `B: BlockT` on lines 43 and 152
  - Allows type inference to use Block's own hash type (sp_core::H256)

- node/src/rpc/eth.rs: Re-enabled TxPool RPC
  - Updated imports to use our custom rpc_txpool implementation
  - Re-enabled TxPool RPC server initialization (line 252-253)
  - Fixed unused variable warning for `graph` parameter

This follows Frontier's pattern of using type inference (B::Hash) instead of hardcoding
hash types, making it compatible with both sp_core::H256 (Substrate) and
ethereum_types::H256 (Ethereum RPC).

Verified: Compilation succeeds with txpool feature enabled.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants