refactor config pkg #958

ceyonur · 2025-05-13T19:07:15Z

Why this should be merged

It was a bit weird to get txpool defaults from VM. This PR removes that in favor of retrieving defaults from libevm along with other refactors.

How this works

This pull request introduces several changes to the transaction pool configuration and the EVM plugin's configuration handling. Key updates include simplifying configuration management by removing deprecated fields and methods, updating default values, and enhancing test coverage. The changes aim to streamline the codebase and ensure consistency in configuration handling.

Transaction Pool Configuration Updates:

Updated the default transaction pool journal file to "transactions.rlp" and increased the default transaction lifetime from 10 minutes to 3 hours in DefaultConfig. (core/txpool/legacypool/legacypool.go, [1] [2]

EVM Plugin Configuration Simplification:

Removed deprecated fields such as CorethAdminAPIEnabled, TxRegossipFrequency, and TxLookupLimit from the Config struct. (plugin/evm/config/config.go, [1] [2] [3]
Removed the TxPoolConfig struct and the SetDefaults method, consolidating default configuration management into the GetDefaultConfig and GetConfig functions. (plugin/evm/config/config.go, plugin/evm/config/config.goL247-L320)

Testing Enhancements:

Added new test cases for GetConfig to validate proper default value assignment and configuration unmarshalling. (plugin/evm/config/config_test.go, plugin/evm/config/config_test.goL130-R162)
Removed outdated test cases for deprecated fields such as tx-lookup-limit. (plugin/evm/config/config_test.go, plugin/evm/config/config_test.goL108-L113)

Code Cleanup:

Removed unused imports and constants, such as those in plugin/evm/config/config.go and plugin/evm/atomic_syncer_test.go. (plugin/evm/config/config.go, [1]; plugin/evm/atomic_syncer_test.go, [2]
Consolidated constants like commitInterval and introduced syncInterval for better readability and maintainability in tests. (plugin/evm/atomic_syncer_test.go, plugin/evm/atomic_syncer_test.goL31-R33)

These changes collectively improve the maintainability and clarity of the transaction pool and EVM plugin configurations, while also ensuring robust testing for future modifications.

How this was tested

UT

Need to be documented?

No

Need to update RELEASES.md?

Yes

Signed-off-by: Ceyhun Onur <[email protected]>

Copilot

Pull Request Overview

This PR refactors configuration handling for the transaction pool and EVM plugin by replacing legacy methods with a new default retrieval mechanism from libevm and removing deprecated configuration fields. Key changes include updating default values (e.g. txpool journal and lifetime), simplifying configuration management by removing legacy structs and methods, and enhancing test coverage for the new configuration functions.

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
plugin/evm/vm_test.go	Removed outdated tests for VM configuration settings
plugin/evm/vm.go	Replaced legacy config initialization with GetConfig function
plugin/evm/config/default_config.go	Updated default configuration values
plugin/evm/config/config_test.go	Added tests for GetConfig and updated deprecation handling
plugin/evm/config/config.go	Removed deprecated fields and SetDefaults method; added deprecate()
plugin/evm/atomic_syncer_test.go	Updated syncer initialization to use new syncInterval constant
core/txpool/legacypool/legacypool.go	Updated default journal file and extended lifetime for transactions
RELEASES.md	Updated release notes to reflect removal of deprecated config flags

Copilot · 2025-05-15T15:01:12Z

plugin/evm/vm.go

-
-	if err := vm.config.Validate(vm.ctx.NetworkID); err != nil {
-		return err
+	cfg, deprecateMsg, err := config.GetConfig(configBytes, vm.ctx.NetworkID)


Consider logging or otherwise handling the deprecation message returned by GetConfig (deprecateMsg) so that users are informed when deprecated configuration flags are provided.

Probably copilot is right, something like, but unless you're trying to maintain some interface, I think it would make sense to change this to
cfg, err := config.GetConfig(configBytes, vm.ctx.NetworkID)
And just log in deprecate call. We can log with varying levels: e.g. we force switch to Firewood we can have log.Crit, but some other minor issues can just have a warning.

Deprecate() does change the config itself (moves from flag to another). I really would like to keep a single call to config to get the config. If this is really weird looking I can move the deprecate call to where we log it

alarso16

I think it's reasonable, just don't love the deprecate message style, but that might just be me

alarso16 · 2025-05-15T15:09:29Z

plugin/evm/vm.go

-
-	if err := vm.config.Validate(vm.ctx.NetworkID); err != nil {
-		return err
+	cfg, deprecateMsg, err := config.GetConfig(configBytes, vm.ctx.NetworkID)


Probably copilot is right, something like, but unless you're trying to maintain some interface, I think it would make sense to change this to
cfg, err := config.GetConfig(configBytes, vm.ctx.NetworkID)
And just log in deprecate call. We can log with varying levels: e.g. we force switch to Firewood we can have log.Crit, but some other minor issues can just have a warning.

alarso16 · 2025-05-15T15:13:34Z

plugin/evm/config/default_config.go

+)
+
+const (
+	defaultCommitInterval = 4096


nit: I don't like how there's just the one constant up here, feels unnecessary.

I kept it bc we use it for both commit interval + sync interval

alarso16 · 2025-05-15T15:18:51Z

core/txpool/legacypool/legacypool.go

@@ -177,7 +175,7 @@ var DefaultConfig = Config{
 	AccountQueue: 64,
 	GlobalQueue:  1024,

-	Lifetime: 10 * time.Minute,
+	Lifetime: 3 * time.Hour,


What's this change for?

To reduce the diff between upstream.

ceyonur · 2025-05-15T14:47:00Z

core/txpool/legacypool/legacypool.go

-	// If we re-enable txpool journaling, we should also add the saved local
-	// transactions to the p2p gossip on startup.
-	Journal:   "",
+	Journal:   "transactions.rlp",


this will be overriden in VM

ceyonur · 2025-05-15T14:47:08Z

core/txpool/legacypool/legacypool.go

@@ -177,7 +175,7 @@ var DefaultConfig = Config{
 	AccountQueue: 64,
 	GlobalQueue:  1024,

-	Lifetime: 10 * time.Minute,
+	Lifetime: 3 * time.Hour,


this will be overriden in config defaults

ceyonur · 2025-05-15T16:11:24Z

core/txpool/legacypool/legacypool.go

@@ -177,7 +175,7 @@ var DefaultConfig = Config{
 	AccountQueue: 64,
 	GlobalQueue:  1024,

-	Lifetime: 10 * time.Minute,
+	Lifetime: 3 * time.Hour,


To reduce the diff between upstream.

ceyonur · 2025-05-15T16:23:42Z

plugin/evm/vm.go

-
-	if err := vm.config.Validate(vm.ctx.NetworkID); err != nil {
-		return err
+	cfg, deprecateMsg, err := config.GetConfig(configBytes, vm.ctx.NetworkID)


Deprecate() does change the config itself (moves from flag to another). I really would like to keep a single call to config to get the config. If this is really weird looking I can move the deprecate call to where we log it

ceyonur added 3 commits May 13, 2025 22:06

refactor config pkg

20da116

get txpool defaults from libevm

709db27

update releases md

284a0e7

ceyonur requested a review from Copilot May 15, 2025 15:00

Merge branch 'master' into ceyonur/move-config

3960f09

Signed-off-by: Ceyhun Onur <[email protected]>

Copilot AI reviewed May 15, 2025

View reviewed changes

ceyonur marked this pull request as ready for review May 15, 2025 15:01

ceyonur requested a review from a team as a code owner May 15, 2025 15:01

alarso16 approved these changes May 15, 2025

View reviewed changes

alarso16 reviewed May 15, 2025

View reviewed changes

use direct values for txpool

4595df8

ceyonur commented May 15, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor config pkg #958

refactor config pkg #958

Uh oh!

ceyonur commented May 13, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI May 15, 2025

Uh oh!

alarso16 May 15, 2025

Uh oh!

ceyonur May 15, 2025

Uh oh!

alarso16 left a comment

Uh oh!

alarso16 May 15, 2025

Uh oh!

alarso16 May 15, 2025

Uh oh!

ceyonur May 21, 2025

Uh oh!

alarso16 May 15, 2025

Uh oh!

ceyonur May 15, 2025

Uh oh!

ceyonur May 15, 2025

Uh oh!

ceyonur May 15, 2025

Uh oh!

ceyonur May 15, 2025

Uh oh!

ceyonur May 15, 2025

Uh oh!

Uh oh!

refactor config pkg #958

Are you sure you want to change the base?

refactor config pkg #958

Uh oh!

Conversation

ceyonur commented May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why this should be merged

How this works

Transaction Pool Configuration Updates:

EVM Plugin Configuration Simplification:

Testing Enhancements:

Code Cleanup:

How this was tested

Need to be documented?

Need to update RELEASES.md?

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI May 15, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alarso16 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ceyonur commented May 13, 2025 •

edited

Loading