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.

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

[Feature]: sdk should be able to provide PrivValidator for cometbft #21064

Closed
islishude opened this issue Jul 25, 2024 · 4 comments
Closed

[Feature]: sdk should be able to provide PrivValidator for cometbft #21064

islishude opened this issue Jul 25, 2024 · 4 comments

Comments

@islishude
Copy link
Contributor

islishude commented Jul 25, 2024

sdk can only provide fixed filepv to cometbft, it can not be updated by apps.

cosmos-sdk/server/start.go

Lines 375 to 378 in 826d4d3

tmNode, err = node.NewNode(
ctx,
cfg,
pvm.LoadOrGenFilePV(cfg.PrivValidatorKeyFile(), cfg.PrivValidatorStateFile()),

cometbft checks if the socket provider configuration is not empty, and use the socket provider then

https://github.com/cometbft/cometbft/blob/24b39c5ae75f6cec77fedd9c3a27a305aa711fcc/node/node.go#L335-L341

I think we can give application access to provide their own PrivValidator.

I propose a change to add a method ValidatorKeyProvider() cmttypes.PrivValidator
to the Application interface.

sdk can have a built-in filepv provider in the baseapp

if apps want to have their own provider, they can overwrite the NodeKeyProvider func.

so sdk doesn't have visible breaking changes.

diff --git a/baseapp/baseapp.go b/baseapp/baseapp.go
index a89b48bcba..7aefdc086f 100644
--- a/baseapp/baseapp.go
+++ b/baseapp/baseapp.go
@@ -24,6 +24,7 @@ import (
        "cosmossdk.io/store/snapshots"
        storetypes "cosmossdk.io/store/types"
 
+       cmttypes "github.com/cometbft/cometbft/types"
        "github.com/cosmos/cosmos-sdk/baseapp/oe"
        "github.com/cosmos/cosmos-sdk/codec"
        codectypes "github.com/cosmos/cosmos-sdk/codec/types"
@@ -60,6 +61,8 @@ var _ servertypes.ABCI = (*BaseApp)(nil)
 
 // BaseApp reflects the ABCI application implementation.
 type BaseApp struct {
+       validatorKeyProvider cmttypes.PrivValidator
+
        // initialized on creation
        logger            log.Logger
        name              string                      // application name from abci.BlockInfo
diff --git a/baseapp/options.go b/baseapp/options.go
index 7218a283e7..b63aefa380 100644
--- a/baseapp/options.go
+++ b/baseapp/options.go
@@ -5,6 +5,7 @@ import (
        "io"
        "math"
 
+       cmttypes "github.com/cometbft/cometbft/types"
        dbm "github.com/cosmos/cosmos-db"
 
        "cosmossdk.io/store/metrics"
@@ -351,6 +352,17 @@ func (app *BaseApp) SetExtendVoteHandler(handler sdk.ExtendVoteHandler) {
        app.extendVote = handler
 }
 
+func (app *BaseApp) SetPrivValidatorProvider(pv cmttypes.PrivValidator) {
+       if app.sealed {
+               panic("SetPrivValidatorProvider() on sealed BaseApp")
+       }
+       app.validatorKeyProvider = pv
+}
+
+func (app *BaseApp) ValidatorKeyProvider() cmttypes.PrivValidator {
+       return app.validatorKeyProvider
+}
+
 func (app *BaseApp) SetVerifyVoteExtensionHandler(handler sdk.VerifyVoteExtensionHandler) {
        if app.sealed {
                panic("SetVerifyVoteExtensionHandler() on sealed BaseApp")
diff --git a/server/start.go b/server/start.go
index c2a812a7b5..a962efcecb 100644
--- a/server/start.go
+++ b/server/start.go
@@ -374,10 +374,15 @@ func startCmtNode(
        }
 
        cmtApp := NewCometABCIWrapper(app)
+
+       pvp := app.ValidatorKeyProvider()
+       if pvp == nil {
+               pvp = pvm.LoadOrGenFilePV(cfg.PrivValidatorKeyFile(), cfg.PrivValidatorStateFile())
+       }
        tmNode, err = node.NewNodeWithContext(
                ctx,
                cfg,
-               pvm.LoadOrGenFilePV(cfg.PrivValidatorKeyFile(), cfg.PrivValidatorStateFile()),
+               pvp,
                nodeKey,
                proxy.NewLocalClientCreator(cmtApp),
                getGenDocProvider(cfg),
diff --git a/server/types/app.go b/server/types/app.go
index 3b5feab3c0..7a4e4b4be0 100644
--- a/server/types/app.go
+++ b/server/types/app.go
@@ -62,6 +62,8 @@ type (
                // Close is called in start cmd to gracefully cleanup resources.
                // Must be safe to be called multiple times.
                Close() error
+
+               ValidatorKeyProvider() cmttypes.PrivValidator
        }
 
        // AppCreator is a function that allows us to lazily initialize an
@github-actions github-actions bot added the needs-triage Issue that needs to be triaged label Jul 25, 2024
@islishude
Copy link
Contributor Author

a use case to me:

I want to use aws ksm for my validator node, and I don't want to use a socket provider due to I have to have a independent process to use it.

@islishude islishude changed the title sdk should be able to provide PrivValidator for cometbft [Feature]: sdk should be able to provide PrivValidator for cometbft Jul 25, 2024
@tac0turtle
Copy link
Member

in 52 this is possible due to comet changes, Ill take on modifying the code, thank you for opening the pr

@tac0turtle tac0turtle self-assigned this Sep 8, 2024
@tac0turtle tac0turtle added C: comet C:server/v2 cometbft and removed needs-triage Issue that needs to be triaged labels Sep 8, 2024
@tac0turtle
Copy link
Member

@islishude do you want to modify the privvalidator somehow or just change the key type?

@tac0turtle
Copy link
Member

we made this work by allowing app devs to set key types.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 🥳 Done
Development

No branches or pull requests

2 participants