Remove Chain's virtual static_validate_header and validate_transaction methods#2049
Remove Chain's virtual static_validate_header and validate_transaction methods#2049goodlyrottenapple wants to merge 1 commit intomainfrom
static_validate_header and validate_transaction methods#2049Conversation
44faa6a to
380cf2a
Compare
| @@ -0,0 +1,70 @@ | |||
| // Copyright (C) 2025 Category Labs, Inc. | |||
There was a problem hiding this comment.
| // Copyright (C) 2025 Category Labs, Inc. | |
| // Copyright (C) 2026 Category Labs, Inc. |
Should we update the year for new files?
There was a problem hiding this comment.
Yep, if new file or wholesale changes then do "2025-26" (checked w/ Peter)
| state.set_code(sender, {}); | ||
| BOOST_OUTCOME_TRY( | ||
| validate_transaction<traits>(enriched_txn, sender, state)); | ||
| BOOST_OUTCOME_TRY(validate_ethereum_transaction<traits>( |
There was a problem hiding this comment.
This seems somewhat surprising? Should we be calling validate_transaction which now includes the logic of validate_monad_transaction for monad revisions (I tried this, but the insufficient_balance test started failing).
There was a problem hiding this comment.
I think this was always using the validate_transaction from the ethereum module, so I think it is correct to use validate_ethereum_transaction here for now.
There was a problem hiding this comment.
I am guessing the test failure that you observe when switching to validate_monad_transaction may be due to reserve balance.
|
|
||
| // Block input validation | ||
| BOOST_OUTCOME_TRY(static_validate_consensus_header(consensus_header)); | ||
| BOOST_OUTCOME_TRY(chain.static_validate_header(block.header)); |
There was a problem hiding this comment.
The logic from EthereumMainnet{}.static_validate_header has been merged into the trait version of static_validate_header which gets called via static_validate_block<traits> below.
static_validate_header and validate_transaction methodsstatic_validate_header and validate_transaction methods
380cf2a to
dd27a93
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors the blockchain validation system by removing virtual methods from the Chain class hierarchy and replacing them with trait-based template dispatch. This completes the architectural goal of using compile-time traits for execution logic instead of runtime polymorphism.
Changes:
- Removed virtual
static_validate_headerandvalidate_transactionmethods from Chain, MonadChain, and EthereumMainnet classes - Introduced trait-based
validate_transactiontemplates in both ethereum and monad namespaces with explicit instantiations for their respective trait types - Extracted
TransactionErrorenum and error mappings to separate header/source files - Moved DAO validation logic from EthereumMainnet to the templated
static_validate_headerfunction - Updated all call sites in runloop files, execute_transaction, RPC executor, and tests to use trait-based dispatch
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| category/execution/ethereum/chain/chain.hpp | Removed virtual method declarations from Chain base class |
| category/execution/ethereum/chain/chain.cpp | Deleted file - no longer needed after removing virtual method implementations |
| category/execution/ethereum/chain/ethereum_mainnet.hpp | Removed virtual method overrides from EthereumMainnet |
| category/execution/ethereum/chain/ethereum_mainnet.cpp | Removed DAO validation and validate_transaction implementations |
| category/execution/monad/chain/monad_chain.hpp | Removed virtual validate_transaction declaration |
| category/execution/monad/chain/monad_chain.cpp | Removed validate_transaction implementation |
| category/execution/ethereum/validate_block.hpp | Removed non-template static_validate_block declaration |
| category/execution/ethereum/validate_block.cpp | Added DAO validation to templated static_validate_header, removed non-template static_validate_block |
| category/execution/ethereum/validate_transaction.hpp | Moved validate_ethereum_transaction inline, added trait-based validate_transaction declaration, extracted TransactionError to separate file |
| category/execution/ethereum/validate_transaction.cpp | Refactored to use trait-based validate_transaction with EVM traits instantiation, removed error mappings |
| category/execution/ethereum/validate_transaction_error.hpp | New file containing TransactionError enum and error domain setup |
| category/execution/ethereum/validate_transaction_error.cpp | New file containing TransactionError value mappings |
| category/execution/monad/validate_monad_transaction.hpp | Updated to trait-based validate_transaction signature |
| category/execution/monad/validate_monad_transaction.cpp | Refactored to trait-based validate_transaction with Monad traits instantiation |
| category/execution/ethereum/transaction_gas.hpp | Removed non-template gas_price declaration |
| category/execution/ethereum/transaction_gas.cpp | Removed non-template gas_price implementation |
| category/execution/ethereum/execute_transaction.hpp | Removed non-template g_star declaration |
| category/execution/ethereum/execute_transaction.cpp | Updated to call trait-based validate_transaction, removed non-template g_star |
| cmd/monad/runloop_ethereum.cpp | Removed chain.static_validate_header call |
| cmd/monad/runloop_monad.cpp | Removed chain.static_validate_header call |
| cmd/monad/runloop_monad_ethblocks.cpp | Removed chain.static_validate_header call |
| category/rpc/monad_executor.cpp | Updated to call validate_ethereum_transaction instead of validate_transaction |
| test/ethereum_test/src/blockchain_test.cpp | Removed virtual method overrides from TraitsMainnet test fixture |
| category/execution/ethereum/test/test_validation.cpp | Updated tests to use validate_ethereum_transaction and made wrong_dao_extra_data test trait-aware |
| category/execution/ethereum/test/test_monad_chain.cpp | Updated test to use new validate_transaction signature |
| category/execution/CMakeLists.txt | Removed chain.cpp, added validate_transaction_error files |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ASSERT_TRUE(result.has_error()); | ||
| EXPECT_EQ(result.error(), BlockError::WrongDaoExtraData); | ||
| if constexpr (is_evm_trait_v<typename TestFixture::Trait>) { | ||
| EXPECT_EQ(result.error(), BlockError::WrongDaoExtraData); | ||
| } |
There was a problem hiding this comment.
The test expects an error for all trait types, but the new DAO validation (lines 79-87) only returns an error for EVM traits (when is_evm_trait_v<traits> is true). For Monad traits, the header will pass validation since gas_limit is valid, causing the test assertion at line 341 to fail. The conditional check at line 342-344 only affects which error code is expected, not whether an error should occur.
Consider either:
- Updating the test to only assert an error for EVM traits
- Adding DAO validation for Monad traits if that's the intended behavior
There was a problem hiding this comment.
The test fails validation for different reason in monad traits. Question is whether we want to:
a) leave as is
b) focus only on etherum behaviour by restricting to EvmTraitsTest
c) add an else case for monad which specifies the alternate validation error
d) make the header pass validation for monad traits
There was a problem hiding this comment.
Is case c the matter of adding a single else, or do we need an else per monad revision?
…ion and instead dispatching via traits
dd27a93 to
df83013
Compare
This PR remove the virtual methods above and Instead dispatches via the trait versions of these functions. This should complete the work of using traits everywhere in execution and switching on timestamp/block only in the core loop.