Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new flag --history.disabletxindexer to allow disabling the transaction indexer by default for performance optimization, particularly benefiting validators and full nodes. The flag defaults to true (disabled), and can be explicitly enabled for RPC and explorer nodes that require transaction lookup capabilities.
Key Changes:
- Added
DisableTxIndexerFlagthat defaults totrueto disable tx indexing by default - Implemented conflict resolution logic to override the disable flag when transaction history flags are explicitly set
- Registered the new flag in the main application flags list
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| cmd/utils/flags.go | Defines the new DisableTxIndexerFlag and implements logic to handle the flag with conflict resolution for transaction history settings |
| cmd/geth/main.go | Registers the new flag in the application's flag list |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } else if ctx.IsSet(TxLookupLimitFlag.Name) { | ||
| log.Warn("The flag --txlookuplimit is deprecated and will be removed, please use --history.transactions") | ||
| if cfg.DisableTxIndexer { | ||
| log.Warn("Disabling transaction indexing is not allowed when --txlookuplimit is set") |
There was a problem hiding this comment.
The warning message could be more helpful by providing actionable guidance. Consider revising to:
log.Warn("Transaction indexing cannot be disabled when --txlookuplimit is set. Keeping transaction indexer enabled.")This makes it clearer what the actual outcome is (indexer stays enabled) rather than just stating what is "not allowed".
| log.Warn("Disabling transaction indexing is not allowed when --txlookuplimit is set") | |
| log.Warn("Transaction indexing cannot be disabled when --txlookuplimit is set. Keeping transaction indexer enabled.") |
| if ctx.IsSet(DisableTxIndexerFlag.Name) { | ||
| cfg.DisableTxIndexer = ctx.Bool(DisableTxIndexerFlag.Name) |
There was a problem hiding this comment.
Bug: The ctx.IsSet() check prevents the default value of the flag from being applied. Since DisableTxIndexerFlag.Value is true (default), users who don't explicitly set the flag will not get the intended default behavior (tx indexer disabled).
The logic should be changed to:
cfg.DisableTxIndexer = ctx.Bool(DisableTxIndexerFlag.Name)
if cfg.DisableTxIndexer {
cfg.TransactionHistory = 0
}This way, the flag's default value (true) will be properly applied even when the flag is not explicitly set, ensuring the tx indexer is disabled by default as intended.
| if ctx.IsSet(DisableTxIndexerFlag.Name) { | |
| cfg.DisableTxIndexer = ctx.Bool(DisableTxIndexerFlag.Name) | |
| cfg.DisableTxIndexer = ctx.Bool(DisableTxIndexerFlag.Name) | |
| if cfg.DisableTxIndexer { |
| } | ||
| if ctx.IsSet(TransactionHistoryFlag.Name) { | ||
| if cfg.DisableTxIndexer { | ||
| log.Warn("Disabling transaction indexing is not allowed when --history.transactions is set") |
There was a problem hiding this comment.
The warning message could be more helpful by providing actionable guidance. Consider revising to:
log.Warn("Transaction indexing cannot be disabled when --history.transactions is set. Keeping transaction indexer enabled.")This makes it clearer what the actual outcome is (indexer stays enabled) rather than just stating what is "not allowed".
| log.Warn("Disabling transaction indexing is not allowed when --history.transactions is set") | |
| log.Warn("Transaction indexing cannot be disabled when --history.transactions is set. Keeping transaction indexer enabled.") |
|
やっぱりいらないかも。 |
1 participant
TxIndexerをDefaultでDisableにした。
パフォーマンスが落ちるらしいから
RPCとか向けにFlagでEnableにできるようにした。