feat: auto-detect network module, deprecate --attester-mode flag#357
feat: auto-detect network module, deprecate --attester-mode flag#357tac0turtle wants to merge 2 commits intomainfrom
Conversation
Instead of requiring the --attester-mode CLI flag, detect the network module at startup by issuing an ABCI Query for its Params endpoint. If the query succeeds the module is present and attester mode is enabled automatically. The flag is marked deprecated via cobra's MarkDeprecated and kept for backward compatibility but has no effect. Closes #156 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary of ChangesHello @tac0turtle, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors how attester mode is determined within the system. Instead of relying on a manual command-line flag, the system now intelligently detects the presence of the network module via an ABCI query during startup. This change simplifies node configuration, reduces potential for misconfiguration, and ensures that the operational mode aligns dynamically with the deployed application modules. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively implements the auto-detection of the network module, which simplifies configuration by removing the --evnode.attester-mode flag. The flag is correctly deprecated for backward compatibility, and the integration tests are updated accordingly. The new detection logic is well-tested with unit tests.
I've added a few comments with suggestions to improve context handling and code maintainability, which will make the implementation more robust.
server/start.go
Outdated
|
|
||
| // Auto-detect attester mode by probing the network module via ABCI query. | ||
| // This makes the --attester-mode flag obsolete. | ||
| attesterMode := detectNetworkModule(app, sdkLogger) |
There was a problem hiding this comment.
With the suggested change to detectNetworkModule to accept a context, this call site needs to be updated to pass it.
| attesterMode := detectNetworkModule(app, sdkLogger) | |
| attesterMode := detectNetworkModule(ctx, app, sdkLogger) |
server/start.go
Outdated
| func detectNetworkModule(app abciQuerier, logger log.Logger) bool { | ||
| req := &types.QueryParamsRequest{} | ||
| reqBytes, err := req.Marshal() | ||
| if err != nil { | ||
| logger.Error("failed to marshal network module probe request", "error", err) | ||
| return false | ||
| } | ||
|
|
||
| resp, err := app.Query(context.Background(), &abci.RequestQuery{ | ||
| Path: "/evabci.network.v1.Query/Params", | ||
| Data: reqBytes, | ||
| }) | ||
| if err != nil { | ||
| logger.Debug("network module not detected (query error)", "error", err) | ||
| return false | ||
| } | ||
| if resp.Code != 0 { | ||
| logger.Debug("network module not detected (non-zero response code)", "code", resp.Code) | ||
| return false | ||
| } | ||
|
|
||
| logger.Info("network module detected, enabling attester mode automatically") | ||
| return true | ||
| } |
There was a problem hiding this comment.
This function can be improved in two ways:
- Context Propagation: The function currently uses
context.Background()for the ABCI query. It's better to propagate thecontext.Contextfrom the caller (setupNodeAndExecutor) to allow for cancellation and proper lifecycle management. - Magic String: The query path
"/evabci.network.v1.Query/Params"is a magic string. It should be defined as a constant to improve maintainability and avoid typos. For example:const networkParamsQueryPath = "/evabci.network.v1.Query/Params".
Here is a suggested implementation addressing the context propagation. You'll also need to update the call site in setupNodeAndExecutor to pass the context.
func detectNetworkModule(ctx context.Context, app abciQuerier, logger log.Logger) bool {
req := &types.QueryParamsRequest{}
reqBytes, err := req.Marshal()
if err != nil {
logger.Error("failed to marshal network module probe request", "error", err)
return false
}
resp, err := app.Query(ctx, &abci.RequestQuery{
Path: "/evabci.network.v1.Query/Params",
Data: reqBytes,
})
if err != nil {
logger.Debug("network module not detected (query error)", "error", err)
return false
}
if resp.Code != 0 {
logger.Debug("network module not detected (non-zero response code)", "code", resp.Code)
return false
}
logger.Info("network module detected, enabling attester mode automatically")
return true
}
server/start_test.go
Outdated
|
|
||
| for _, tc := range tests { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| result := detectNetworkModule(tc.querier, nopLogger) |
There was a problem hiding this comment.
This test will fail to compile if detectNetworkModule is updated to accept a context.Context as suggested in another comment. Please update this call to pass a context. context.Background() is appropriate for this test case.
| result := detectNetworkModule(tc.querier, nopLogger) | |
| result := detectNetworkModule(context.Background(), tc.querier, nopLogger) |
…tant - Pass context.Context from setupNodeAndExecutor to detectNetworkModule instead of using context.Background() - Extract query path as networkParamsQueryPath constant - Update test to pass context.Background() Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
|
||
| // Deprecated: FlagAttesterMode is no longer needed. Attester mode is now | ||
| // auto-detected by probing the network module at startup. | ||
| const FlagAttesterMode = config.FlagPrefixEvnode + "attester-mode" |
There was a problem hiding this comment.
AFAIK attester needs to be a flag, as not all nodes are attesters
julienrbrt
left a comment
julienrbrt
left a comment
There was a problem hiding this comment.
I checked the issue, we were talking about another flag:
FlagNetworkSoftConfirmation and not FlagAttesterMode
interesting how it totally missed it. |
|
ah i know the flag doesnt exist. the issue should have been closed |
2 participants
Summary
Paramsquery, removing the need for the--evnode.attester-modeCLI flagFlagAttesterModeusing cobra'sMarkDeprecated(kept for backward compatibility, no effect)--evnode.attester-modeflag from integration test start argsHow it works
At startup in
setupNodeAndExecutor,detectNetworkModuleissues an ABCI query to/evabci.network.v1.Query/Params. If the query returns code 0, the network module is registered and attester mode is enabled automatically. If the module is absent (non-zero code or error), sequencer mode is used.Test plan
detectNetworkModulecovering: module present, module absent (non-zero code), query errorserver/andpkg/rpc/tests passCloses #156
🤖 Generated with Claude Code