Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Pull request overview
This PR cleans up multiple test suites to run against an EIP-1559-enabled params.TestChainConfig and updates test chain generation/helpers accordingly, reducing ad-hoc per-test config mutations.
Changes:
- Enable EIP-1559 from genesis in
params.TestChainConfigand remove many test-localEip1559Blockoverrides. - Update multiple tests to use
params.TestChainConfigdirectly and to fund accounts / set gas pricing compatible with base fee. - Refactor chain maker helpers to support “with genesis” generation and adjust core chain tests to use the updated helpers.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| params/config.go | Enables EIP-1559 at block 0 in TestChainConfig, aligning tests with base-fee semantics. |
| eth/tracers/api_test.go | Removes per-test config cloning; uses params.TestChainConfig directly in tracer tests. |
| eth/helper_test.go | Adjusts funded balance in genesis for protocol manager tests. |
| eth/handler_test.go | Makes test tx generation base-fee aware by providing a fee value to legacy txs. |
| eth/filters/filter_test.go | Adjusts genesis commit/chain generation flow to avoid GenerateChainWithGenesis DB behavior and uses TestChainConfig directly. |
| eth/filters/filter_system_test.go | Updates tx gas price selection to be compatible with base fee. |
| eth/fetcher/fetcher_test.go | Updates genesis funding and ensures legacy txs use a non-nil fee/gas price. |
| eth/downloader/testchain_test.go | Removes local config override and uses TestChainConfig consistently for chain/signing. |
| eth/downloader/queue_test.go | Ensures generated txs use non-nil fee/gas price derived from base fee. |
| eth/downloader/downloader_test.go | Removes redundant Eip1559Block override in returned test config. |
| core/txpool/locals/tx_tracker_test.go | Updates funds and gas pricing to align with base fee behavior. |
| core/txpool/legacypool/legacypool_test.go | Removes redundant fork-block overrides now covered by TestChainConfig. |
| core/chain_makers.go | Adds “with genesis” helper variants; threads chain config into header/block chain helpers. |
| core/blockchain_test.go | Refactors canonical-chain test setup to return genesis spec and adjust generation/insertion paths. |
| core/block_validator_test.go | Switches to GenerateChainWithGenesis for test chain setup and uses fresh DB for blockchain. |
| core/bench_test.go | Switches benchmark chain generation to GenerateChainWithGenesis (separate DB from insertion DB). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func newCanonical(engine consensus.Engine, n int, full bool) (ethdb.Database, *Genesis, *BlockChain, error) { | ||
| var ( | ||
| db = rawdb.NewMemoryDatabase() | ||
| gspec = &Genesis{ | ||
| genesis = &Genesis{ | ||
| BaseFee: big.NewInt(params.InitialBaseFee), | ||
| Config: params.AllEthashProtocolChanges, | ||
| } | ||
| genesis = gspec.MustCommit(db) | ||
| ) | ||
|
|
||
| // Initialize a fresh chain with only a genesis block | ||
| blockchain, _ := NewBlockChain(db, nil, gspec, engine, vm.Config{}) | ||
| blockchain, _ := NewBlockChain(rawdb.NewMemoryDatabase(), nil, genesis, engine, vm.Config{}) | ||
|
|
||
| // Create and inject the requested chain | ||
| if n == 0 { | ||
| return db, blockchain, nil | ||
| return rawdb.NewMemoryDatabase(), genesis, blockchain, nil | ||
| } |
There was a problem hiding this comment.
newCanonical builds the BlockChain with a fresh in-memory DB, but when n == 0 it returns a different freshly-created DB that does not contain the committed genesis state. Callers pass the returned genDb into GenerateChain/makeBlockChain, which will panic when trying to open state at parent.Root() (genesis trie nodes are missing in that returned DB).
Return the same DB that backs blockchain (or commit the genesis into the returned DB) so subsequent chain generation has access to the parent state.
| gspec := &Genesis{ | ||
| Alloc: types.GenesisAlloc{benchRootAddr: {Balance: benchRootFunds}}, | ||
| Config: params.TestChainConfig, |
There was a problem hiding this comment.
params.TestChainConfig now has EIP-1559 enabled from block 0, which means legacy transactions must have a non-nil gas price (and typically >= base fee). The benchmark generators used by benchInsertChain (e.g. genValueTx and genTxRing) still create legacy txs with gasPrice == nil, which will panic when tx.GasPrice() is evaluated during block processing.
Update the benchmark generators to always set a concrete gas price (e.g. use gen.BaseFee() with a fallback to params.InitialBaseFee) so the benchmarks run under the updated test chain config.
| gspec := &Genesis{ | |
| Alloc: types.GenesisAlloc{benchRootAddr: {Balance: benchRootFunds}}, | |
| Config: params.TestChainConfig, | |
| cfg := *params.TestChainConfig | |
| cfg.LondonBlock = nil | |
| gspec := &Genesis{ | |
| Alloc: types.GenesisAlloc{benchRootAddr: {Balance: benchRootFunds}}, | |
| Config: &cfg, |
gzliudan
force-pushed
the
cleanup-tests
branch
7 times, most recently
from
5a25812 to
b2b33e2
Compare
gzliudan
requested review from
AnilChinchawale,
anunay-xin,
benjamin202410 and
liam-lai
Proposed changes
Ref: ethereum#25641
Types of changes
What types of changes does your code introduce to XDC network?
Put an
✅in the boxes that applyImpacted Components
Which parts of the codebase does this PR touch?
Put an
✅in the boxes that applyChecklist
Put an
✅in the boxes once you have confirmed below actions (or provide reasons on not doing so) that