Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add config to toggle the TXM #15714

Merged
merged 15 commits into from
Dec 19, 2024
Merged

Add config to toggle the TXM #15714

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
1a6a2ac
Added config to enable or disable the TXM
amit-momin Dec 16, 2024
28f95e7
Added changeset
amit-momin Dec 16, 2024
e29572f
Fixed test
amit-momin Dec 16, 2024
6eadab5
Fixed linting
amit-momin Dec 16, 2024
e6c2fa3
Fixed linting
amit-momin Dec 16, 2024
76364ea
Updated config description
amit-momin Dec 16, 2024
4f5e7d4
Regenerated config doc
amit-momin Dec 16, 2024
f373dff
Updated error message
amit-momin Dec 16, 2024
7f58819
Updated the config name and nolint comment
amit-momin Dec 17, 2024
604da4c
Initialized gas estimator regardless of TXM toggle
amit-momin Dec 17, 2024
ee2bae1
Fixed logger name to match previous naming
amit-momin Dec 17, 2024
8a7be45
Fixed linting error
amit-momin Dec 17, 2024
1d8500a
Addressed feedback
amit-momin Dec 18, 2024
2e0a39a
Moved EVM.TransactionManagerEnabled config to EVM.Transactions.Enabled
amit-momin Dec 19, 2024
e74a2e2
Fixed linting
amit-momin Dec 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/wild-cats-think.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"chainlink": minor
---

Added the `TXMEnabled` config to enable or disable the TXM. #added
4 changes: 4 additions & 0 deletions core/chains/evm/config/chain_scoped.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,3 +191,7 @@ func (e *EVMConfig) FinalizedBlockOffset() uint32 {
func (e *EVMConfig) NoNewFinalizedHeadsThreshold() time.Duration {
return e.C.NoNewFinalizedHeadsThreshold.Duration()
}

func (e *EVMConfig) TXMEnabled() bool {
return *e.C.TXMEnabled
}
1 change: 1 addition & 0 deletions core/chains/evm/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ type EVM interface {
NodeNoNewHeadsThreshold() time.Duration
FinalizedBlockOffset() uint32
NoNewFinalizedHeadsThreshold() time.Duration
TXMEnabled() bool
jmank88 marked this conversation as resolved.
Show resolved Hide resolved

IsEnabled() bool
TOMLString() (string, error)
Expand Down
13 changes: 12 additions & 1 deletion core/chains/evm/config/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,9 +219,20 @@ func TestChainScopedConfig(t *testing.T) {

assert.Equal(t, false, cfg3.EVM().LogBroadcasterEnabled())
})
})

t.Run("TXMEnabled", func(t *testing.T) {
t.Run("turn on TXMEnabled by default", func(t *testing.T) {
assert.True(t, cfg.EVM().TXMEnabled())
})

t.Run("use Noop logBroadcaster when LogBroadcaster is disabled", func(t *testing.T) {
t.Run("verify TXMEnabled is set correctly", func(t *testing.T) {
val := false
cfg3 := testutils.NewTestChainScopedConfig(t, func(c *toml.EVMConfig) {
c.TXMEnabled = ptr(val)
})

assert.False(t, cfg3.EVM().TXMEnabled())
})
})
}
Expand Down
1 change: 1 addition & 0 deletions core/chains/evm/config/toml/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -386,6 +386,7 @@ type Chain struct {
RPCBlockQueryDelay *uint16
FinalizedBlockOffset *uint32
NoNewFinalizedHeadsThreshold *commonconfig.Duration
TXMEnabled *bool

Transactions Transactions `toml:",omitempty"`
BalanceMonitor BalanceMonitor `toml:",omitempty"`
Expand Down
4 changes: 3 additions & 1 deletion core/chains/evm/config/toml/defaults.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,10 +236,12 @@ func (c *Chain) SetFrom(f *Chain) {
if v := f.FinalizedBlockOffset; v != nil {
c.FinalizedBlockOffset = v
}

if v := f.NoNewFinalizedHeadsThreshold; v != nil {
c.NoNewFinalizedHeadsThreshold = v
}
if v := f.TXMEnabled; v != nil {
c.TXMEnabled = v
}

c.Transactions.setFrom(&f.Transactions)
c.BalanceMonitor.setFrom(&f.BalanceMonitor)
Expand Down
1 change: 1 addition & 0 deletions core/chains/evm/config/toml/defaults/fallback.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ RPCBlockQueryDelay = 1
FinalizedBlockOffset = 0
NoNewFinalizedHeadsThreshold = '0'
LogBroadcasterEnabled = true
TXMEnabled = true

[Transactions]
ForwardersEnabled = false
Expand Down
16 changes: 13 additions & 3 deletions core/chains/legacyevm/chain.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,9 +245,19 @@ func newChain(ctx context.Context, cfg *evmconfig.ChainScoped, nodes []*toml.Nod
}

// note: gas estimator is started as a part of the txm
txm, gasEstimator, err := newEvmTxm(opts.DS, cfg.EVM(), opts.AppConfig.EVMRPCEnabled(), opts.AppConfig.Database(), opts.AppConfig.Database().Listener(), client, l, logPoller, opts, headTracker, clientsByChainID)
if err != nil {
return nil, fmt.Errorf("failed to instantiate EvmTxm for chain with ID %s: %w", chainID.String(), err)
var txm txmgr.TxManager
var gasEstimator gas.EvmFeeEstimator
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we consider creating a gas estimator even if TXM is disabled? Are there any paths in the code that might call it apart from the TXM. If they do, we might panic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could for sanity. Otherwise, the existing code also returned nil for the gas estimator if EVMRPCEnabled=false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the code to initialize the gas estimator regardless of the TXM toggle. I looked around and don't think there was any reference to gas estimator that would cause a panic but made the change anyways in case I missed something or if that changes in the future.

Copy link
Collaborator

Choose a reason for hiding this comment

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

There is an issue with the BlockHistory estimator though. Currently, TXM subscribes to HT and implements OnNewLongestChain, providing BHE with a new head. If TXM is nil, BHE won't be getting updates at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would only be an issue if the gas estimator gets started though right? I only separated out the initialization of the gas estimator from the TXM but it still relies on the TXM to start/stop it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I assumed that since it gets initialized we would also want to start it, but I guess we don't. My only issue was that because it's initialized someone might try to fetch prices causing issues, but I noticed BHE has an IfStarted method for both GetLegacyGas and GetDynamicFee so we're ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could separate out the gas estimator lifecycle from the TXM and truly make it a standalone service but that's probably a separate ticket. For now, I'll still keep the initialization separate just to make sure we don't hit a panic anywhere in case this is called outside of the TXM.

//nolint:gocritic // ignoring styling issue
jmank88 marked this conversation as resolved.
Show resolved Hide resolved
if !opts.AppConfig.EVMRPCEnabled() {
txm = &txmgr.NullTxManager{ErrMsg: fmt.Sprintf("Ethereum is disabled for chain %d", chainID)}
} else if !cfg.EVM().TXMEnabled() {
txm = &txmgr.NullTxManager{ErrMsg: fmt.Sprintf("TXM disabled for chain %d", chainID)}
} else {
var err error
txm, gasEstimator, err = newEvmTxm(opts.DS, cfg.EVM(), opts.AppConfig.Database(), opts.AppConfig.Database().Listener(), client, l, logPoller, opts, headTracker, clientsByChainID)
if err != nil {
return nil, fmt.Errorf("failed to instantiate EvmTxm for chain with ID %s: %w", chainID.String(), err)
jmank88 marked this conversation as resolved.
Show resolved Hide resolved
}
}

headBroadcaster.Subscribe(txm)
Expand Down
5 changes: 0 additions & 5 deletions core/chains/legacyevm/evm_txm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ import (
func newEvmTxm(
ds sqlutil.DataSource,
cfg evmconfig.EVM,
evmRPCEnabled bool,
databaseConfig txmgr.DatabaseConfig,
listenerConfig txmgr.ListenerConfig,
client evmclient.Client,
Expand All @@ -31,10 +30,6 @@ func newEvmTxm(
err error,
) {
chainID := cfg.ChainID()
if !evmRPCEnabled {
txm = &txmgr.NullTxManager{ErrMsg: fmt.Sprintf("Ethereum is disabled for chain %d", chainID)}
return txm, nil, nil
}

lggr = lggr.Named("Txm")
lggr.Infow("Initializing EVM transaction manager",
Expand Down
2 changes: 2 additions & 0 deletions core/config/docs/chains-evm.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,8 @@ LogBroadcasterEnabled = true # Default
#
# Set to zero to disable.
NoNewFinalizedHeadsThreshold = '0' # Default
# TXMEnabled is a feature flag for the Transaction Manager. This flag also enables or disables the gas estimator since it is dependent on the TXM to start it.
TXMEnabled = true # Default

[EVM.Transactions]
# ForwardersEnabled enables or disables sending transactions through forwarder contracts.
Expand Down
2 changes: 2 additions & 0 deletions core/services/chainlink/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -586,6 +586,7 @@ func TestConfig_Marshal(t *testing.T) {
FinalityTagEnabled: ptr[bool](true),
FlagsContractAddress: mustAddress("0xae4E781a6218A8031764928E88d457937A954fC3"),
FinalizedBlockOffset: ptr[uint32](16),
TXMEnabled: ptr(true),

GasEstimator: evmcfg.GasEstimator{
Mode: ptr("SuggestedPrice"),
Expand Down Expand Up @@ -1116,6 +1117,7 @@ RPCDefaultBatchSize = 17
RPCBlockQueryDelay = 10
FinalizedBlockOffset = 16
NoNewFinalizedHeadsThreshold = '1h0m0s'
TXMEnabled = true

[EVM.Transactions]
ForwardersEnabled = true
Expand Down
1 change: 1 addition & 0 deletions core/services/chainlink/testdata/config-full.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,6 +335,7 @@ RPCDefaultBatchSize = 17
RPCBlockQueryDelay = 10
FinalizedBlockOffset = 16
NoNewFinalizedHeadsThreshold = '1h0m0s'
TXMEnabled = true

[EVM.Transactions]
ForwardersEnabled = true
Expand Down
3 changes: 3 additions & 0 deletions core/services/chainlink/testdata/config-multi-chain-effective.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,6 +318,7 @@ RPCDefaultBatchSize = 250
RPCBlockQueryDelay = 1
FinalizedBlockOffset = 12
NoNewFinalizedHeadsThreshold = '9m0s'
TXMEnabled = true

[EVM.Transactions]
ForwardersEnabled = false
Expand Down Expand Up @@ -428,6 +429,7 @@ RPCDefaultBatchSize = 250
RPCBlockQueryDelay = 1
FinalizedBlockOffset = 0
NoNewFinalizedHeadsThreshold = '0s'
TXMEnabled = true

[EVM.Transactions]
ForwardersEnabled = false
Expand Down Expand Up @@ -532,6 +534,7 @@ RPCDefaultBatchSize = 100
RPCBlockQueryDelay = 10
FinalizedBlockOffset = 0
NoNewFinalizedHeadsThreshold = '6m0s'
TXMEnabled = true

[EVM.Transactions]
ForwardersEnabled = false
Expand Down
1 change: 1 addition & 0 deletions core/web/resolver/testdata/config-full.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,6 +335,7 @@ RPCDefaultBatchSize = 17
RPCBlockQueryDelay = 10
FinalizedBlockOffset = 0
NoNewFinalizedHeadsThreshold = '15m0s'
TXMEnabled = true

[EVM.Transactions]
ForwardersEnabled = true
Expand Down
3 changes: 3 additions & 0 deletions core/web/resolver/testdata/config-multi-chain-effective.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,6 +318,7 @@ RPCDefaultBatchSize = 250
RPCBlockQueryDelay = 1
FinalizedBlockOffset = 0
NoNewFinalizedHeadsThreshold = '9m0s'
TXMEnabled = true

[EVM.Transactions]
ForwardersEnabled = false
Expand Down Expand Up @@ -428,6 +429,7 @@ RPCDefaultBatchSize = 250
RPCBlockQueryDelay = 1
FinalizedBlockOffset = 0
NoNewFinalizedHeadsThreshold = '0s'
TXMEnabled = true

[EVM.Transactions]
ForwardersEnabled = false
Expand Down Expand Up @@ -532,6 +534,7 @@ RPCDefaultBatchSize = 100
RPCBlockQueryDelay = 10
FinalizedBlockOffset = 0
NoNewFinalizedHeadsThreshold = '6m0s'
TXMEnabled = true

[EVM.Transactions]
ForwardersEnabled = false
Expand Down
Loading
Loading