Add config to toggle the TXM #15714
Add config to toggle the TXM #15714
Conversation
AER Report: CI Core ran successfully ✅AER Report: Operator UI CI ran successfully ✅ |
amit-momin
force-pushed
the
MERC-6516-Allow-disabling-TXM-completely-substitute-null-txm-for-mercury-where-it-isn-t-needed
branch
from
3b65ac2 to
e29572f
Compare
core/chains/legacyevm/chain.go
Outdated
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I could for sanity. Otherwise, the existing code also returned nil for the gas estimator if EVMRPCEnabled=false.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
core/chains/legacyevm/chain.go
Outdated
| var gasEstimator gas.EvmFeeEstimator | ||
| if gasEstimator, err = newGasEstimator(cfg.EVM(), client, l, opts, clientsByChainID); err != nil { | ||
| return nil, fmt.Errorf("failed to instantiate gas estimator for chain with ID %s: %w", chainID, err) | ||
| } |
There was a problem hiding this comment.
Since you're returning, there is no point in splitting the declaration from the initialization
| var gasEstimator gas.EvmFeeEstimator | |
| if gasEstimator, err = newGasEstimator(cfg.EVM(), client, l, opts, clientsByChainID); err != nil { | |
| return nil, fmt.Errorf("failed to instantiate gas estimator for chain with ID %s: %w", chainID, err) | |
| } | |
| gasEstimator, err := newGasEstimator(cfg.EVM(), client, l, opts, clientsByChainID) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to instantiate gas estimator for chain with ID %s: %w", chainID, err) | |
| } |
There was a problem hiding this comment.
Weird... I was getting a shadow declaration on err which is why I did this but maybe my other changes fixed it. Fixed in the latest commit
| // 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) | ||
| // initialize gas estimator | ||
| gasEstimator, err := newGasEstimator(cfg.EVM(), client, l, opts, clientsByChainID) |
There was a problem hiding this comment.
We always initialize a gas estimator even if its never going to be used?
There was a problem hiding this comment.
As far as I can tell it isn't used as of now but we don't have a NullGasEstimator to use instead. It didn't seem like an issue to just initialize an actual instance of it though since it wouldn't be started if the TXM was disabled. But at least initializing it would prevent panic if anyone were to refer to it with the TXM disabled. They'd just get a gas estimator not started error instead.
|
samsondav
deleted the
MERC-6516-Allow-disabling-TXM-completely-substitute-null-txm-for-mercury-where-it-isn-t-needed
branch
MERC-6516
Added the
TransactionManagerEnabledconfig to enable or disable the TXM. This toggle was added so a product that only needs read-only access (i.e client, head tracker, log poller) can disable the unused component.