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

feat: override timeout commit via --timeout-commit #4103

Merged
merged 7 commits into from
Dec 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ help: Makefile
build: mod
@cd ./cmd/celestia-appd
@mkdir -p build/
@echo "--> Building build/celestia-appd"
@go build $(BUILD_FLAGS) -o build/ ./cmd/celestia-appd
.PHONY: build

Expand Down
9 changes: 9 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,15 @@ celestia-appd tx blob pay-for-blob 0x00010203040506070809 0x48656c6c6f2c20576f72

If import celestia-app as a Go module, you may need to add some Go module `replace` directives to avoid type incompatibilities. Please see the `replace` directive in [go.mod](./go.mod) for inspiration.

### Usage in tests

If you are running celestia-app in tests, you may want to override the `timeout_commit` to produce blocks faster. By default, a celestia-app chain with app version >= 3 will produce blocks every ~6 seconds. To produce blocks faster, you can override the `timeout_commit` with the `--timeout-commit` flag.

```shell
# Start celestia-appd with a one second timeout commit.
celestia-appd start --timeout-commit 1s
```

## Contributing

This repo attempts to conform to [conventional commits](https://www.conventionalcommits.org/en/v1.0.0/) so PR titles should ideally start with `fix:`, `feat:`, `build:`, `chore:`, `ci:`, `docs:`, `style:`, `refactor:`, `perf:`, or `test:` because this helps with semantic versioning and changelog generation. It is especially important to include an `!` (e.g. `feat!:`) if the PR includes a breaking change.
Expand Down
23 changes: 20 additions & 3 deletions app/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"fmt"
"io"
"slices"
"time"

"github.com/celestiaorg/celestia-app/v3/app/ante"
"github.com/celestiaorg/celestia-app/v3/app/encoding"
Expand Down Expand Up @@ -170,6 +171,10 @@ type App struct {
// upgradeHeightV2 is used as a coordination mechanism for the height-based
// upgrade from v1 to v2.
upgradeHeightV2 int64
// timeoutCommit is used to override the default timeoutCommit. This is
// useful for testing purposes and should not be used on public networks
// (Arabica, Mocha, or Mainnet Beta).
timeoutCommit time.Duration
// MsgGateKeeper is used to define which messages are accepted for a given
// app version.
MsgGateKeeper *ante.MsgVersioningGateKeeper
Expand All @@ -188,6 +193,7 @@ func New(
invCheckPeriod uint,
encodingConfig encoding.Config,
upgradeHeightV2 int64,
timeoutCommit time.Duration,
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add validation for public network protection.

Consider adding validation in the constructor to prevent setting custom timeout commits on public networks (Arabica, Mocha, or Mainnet Beta) as mentioned in the documentation.

 func New(
 	logger log.Logger,
 	db dbm.DB,
 	traceStore io.Writer,
 	invCheckPeriod uint,
 	encodingConfig encoding.Config,
 	upgradeHeightV2 int64,
 	timeoutCommit time.Duration,
 	appOpts servertypes.AppOptions,
 	baseAppOptions ...func(*baseapp.BaseApp),
 ) *App {
+	// Prevent custom timeout commits on public networks
+	chainID := cast.ToString(appOpts.Get("chain-id"))
+	if timeoutCommit != 0 && (chainID == "arabica-11" || chainID == "mocha-4" || chainID == "celestia") {
+		panic("custom timeout commits are not supported on public networks")
+	}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
timeoutCommit time.Duration,
func New(
logger log.Logger,
db dbm.DB,
traceStore io.Writer,
invCheckPeriod uint,
encodingConfig encoding.Config,
upgradeHeightV2 int64,
timeoutCommit time.Duration,
appOpts servertypes.AppOptions,
baseAppOptions ...func(*baseapp.BaseApp),
) *App {
// Prevent custom timeout commits on public networks
chainID := cast.ToString(appOpts.Get("chain-id"))
if timeoutCommit != 0 && (chainID == "arabica-11" || chainID == "mocha-4" || chainID == "celestia") {
panic("custom timeout commits are not supported on public networks")
}
timeoutCommit time.Duration,

appOpts servertypes.AppOptions,
baseAppOptions ...func(*baseapp.BaseApp),
) *App {
Expand All @@ -214,6 +220,7 @@ func New(
tkeys: tkeys,
memKeys: memKeys,
upgradeHeightV2: upgradeHeightV2,
timeoutCommit: timeoutCommit,
}

app.ParamsKeeper = initParamsKeeper(appCodec, encodingConfig.Amino, keys[paramstypes.StoreKey], tkeys[paramstypes.TStoreKey])
Expand Down Expand Up @@ -481,7 +488,7 @@ func (app *App) EndBlocker(ctx sdk.Context, req abci.RequestEndBlock) abci.Respo
app.SignalKeeper.ResetTally(ctx)
}
}
res.Timeouts.TimeoutCommit = appconsts.GetTimeoutCommit(currentVersion)
res.Timeouts.TimeoutCommit = app.getTimeoutCommit(currentVersion)
res.Timeouts.TimeoutPropose = appconsts.GetTimeoutPropose(currentVersion)
return res
}
Expand Down Expand Up @@ -539,8 +546,8 @@ func (app *App) Info(req abci.RequestInfo) abci.ResponseInfo {
app.mountKeysAndInit(resp.AppVersion)
}

resp.Timeouts.TimeoutCommit = app.getTimeoutCommit(resp.AppVersion)
resp.Timeouts.TimeoutPropose = appconsts.GetTimeoutPropose(resp.AppVersion)
resp.Timeouts.TimeoutCommit = appconsts.GetTimeoutCommit(resp.AppVersion)

return resp
}
Expand All @@ -565,7 +572,7 @@ func (app *App) InitChain(req abci.RequestInitChain) (res abci.ResponseInitChain
app.SetInitialAppVersionInConsensusParams(ctx, appVersion)
app.SetAppVersion(ctx, appVersion)
}
res.Timeouts.TimeoutCommit = appconsts.GetTimeoutCommit(appVersion)
res.Timeouts.TimeoutCommit = app.getTimeoutCommit(appVersion)
res.Timeouts.TimeoutPropose = appconsts.GetTimeoutPropose(appVersion)
return res
}
Expand Down Expand Up @@ -849,3 +856,13 @@ func (app *App) OfferSnapshot(req abci.RequestOfferSnapshot) abci.ResponseOfferS
func isSupportedAppVersion(appVersion uint64) bool {
return appVersion == v1 || appVersion == v2 || appVersion == v3
}

// getTimeoutCommit returns the timeoutCommit if a user has overridden it via the
// --timeout-commit flag. Otherwise, it returns the default timeout commit based
// on the app version.
func (app *App) getTimeoutCommit(appVersion uint64) time.Duration {
if app.timeoutCommit != 0 {
return app.timeoutCommit
}
return appconsts.GetTimeoutCommit(appVersion)
}
12 changes: 8 additions & 4 deletions app/app_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"os"
"path/filepath"
"testing"
"time"

"github.com/celestiaorg/celestia-app/v3/app"
"github.com/celestiaorg/celestia-app/v3/app/encoding"
Expand All @@ -29,9 +30,10 @@ func TestNew(t *testing.T) {
invCheckPeriod := uint(1)
encodingConfig := encoding.MakeConfig(app.ModuleEncodingRegisters...)
upgradeHeight := int64(0)
timeoutCommit := time.Second
appOptions := NoopAppOptions{}

got := app.New(logger, db, traceStore, invCheckPeriod, encodingConfig, upgradeHeight, appOptions)
got := app.New(logger, db, traceStore, invCheckPeriod, encodingConfig, upgradeHeight, timeoutCommit, appOptions)

t.Run("initializes ICAHostKeeper", func(t *testing.T) {
assert.NotNil(t, got.ICAHostKeeper)
Expand Down Expand Up @@ -65,8 +67,9 @@ func TestInitChain(t *testing.T) {
invCheckPeriod := uint(1)
encodingConfig := encoding.MakeConfig(app.ModuleEncodingRegisters...)
upgradeHeight := int64(0)
timeoutCommit := time.Second
appOptions := NoopAppOptions{}
testApp := app.New(logger, db, traceStore, invCheckPeriod, encodingConfig, upgradeHeight, appOptions)
testApp := app.New(logger, db, traceStore, invCheckPeriod, encodingConfig, upgradeHeight, timeoutCommit, appOptions)
genesisState, _, _ := util.GenesisStateWithSingleValidator(testApp, "account")
appStateBytes, err := json.MarshalIndent(genesisState, "", " ")
require.NoError(t, err)
Expand Down Expand Up @@ -103,7 +106,7 @@ func TestInitChain(t *testing.T) {

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
application := app.New(logger, db, traceStore, invCheckPeriod, encodingConfig, upgradeHeight, appOptions)
application := app.New(logger, db, traceStore, invCheckPeriod, encodingConfig, upgradeHeight, timeoutCommit, appOptions)
if tc.wantPanic {
assert.Panics(t, func() { application.InitChain(tc.request) })
} else {
Expand Down Expand Up @@ -160,6 +163,7 @@ func createTestApp(t *testing.T) *app.App {
db := tmdb.NewMemDB()
config := encoding.MakeConfig(app.ModuleEncodingRegisters...)
upgradeHeight := int64(3)
timeoutCommit := time.Second
snapshotDir := filepath.Join(t.TempDir(), "data", "snapshots")
t.Cleanup(func() {
err := os.RemoveAll(snapshotDir)
Expand All @@ -174,7 +178,7 @@ func createTestApp(t *testing.T) *app.App {
snapshotStore, err := snapshots.NewStore(snapshotDB, snapshotDir)
require.NoError(t, err)
baseAppOption := baseapp.SetSnapshot(snapshotStore, snapshottypes.NewSnapshotOptions(10, 10))
testApp := app.New(log.NewNopLogger(), db, nil, 0, config, upgradeHeight, util.EmptyAppOptions{}, baseAppOption)
testApp := app.New(log.NewNopLogger(), db, nil, 0, config, upgradeHeight, timeoutCommit, util.EmptyAppOptions{}, baseAppOption)
require.NoError(t, err)
response := testApp.Info(abci.RequestInfo{})
require.Equal(t, uint64(0), response.AppVersion)
Expand Down
2 changes: 1 addition & 1 deletion app/test/upgrade_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,7 @@ func SetupTestAppWithUpgradeHeight(t *testing.T, upgradeHeight int64) (*app.App,

db := dbm.NewMemDB()
encCfg := encoding.MakeConfig(app.ModuleEncodingRegisters...)
testApp := app.New(log.NewNopLogger(), db, nil, 0, encCfg, upgradeHeight, util.EmptyAppOptions{})
testApp := app.New(log.NewNopLogger(), db, nil, 0, encCfg, upgradeHeight, 0, util.EmptyAppOptions{})
genesis := genesis.NewDefaultGenesis().
WithChainID("test").
WithValidators(genesis.NewDefaultValidator(testnode.DefaultValidatorAccountName)).
Expand Down
2 changes: 1 addition & 1 deletion cmd/celestia-appd/cmd/app_exporter.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ func appExporter(
appOptions servertypes.AppOptions,
) (servertypes.ExportedApp, error) {
config := encoding.MakeConfig(app.ModuleEncodingRegisters...)
application := app.New(logger, db, traceStore, uint(1), config, 0, appOptions)
application := app.New(logger, db, traceStore, uint(1), config, 0, 0, appOptions)
if height != -1 {
if err := application.LoadHeight(height); err != nil {
return servertypes.ExportedApp{}, err
Expand Down
5 changes: 4 additions & 1 deletion cmd/celestia-appd/cmd/app_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,13 @@ func NewAppServer(logger log.Logger, db dbm.DB, traceStore io.Writer, appOptions
}

return app.New(
logger, db, traceStore,
logger,
db,
traceStore,
cast.ToUint(appOptions.Get(server.FlagInvCheckPeriod)),
encoding.MakeConfig(app.ModuleEncodingRegisters...),
cast.ToInt64(appOptions.Get(UpgradeHeightFlag)),
cast.ToDuration(appOptions.Get(TimeoutCommitFlag)),
appOptions,
rootulp marked this conversation as resolved.
Show resolved Hide resolved
baseapp.SetPruning(pruningOpts),
baseapp.SetMinGasPrices(cast.ToString(appOptions.Get(server.FlagMinGasPrices))),
Expand Down
9 changes: 7 additions & 2 deletions cmd/celestia-appd/cmd/root.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@ const (
// UpgradeHeightFlag is the flag to specify the upgrade height for v1 to v2
// application upgrade.
UpgradeHeightFlag = "v2-upgrade-height"

// TimeoutCommit is a flag that can be used to override the timeout_commit.
TimeoutCommitFlag = "timeout-commit"
)

// NewRootCmd creates a new root command for celestia-appd.
Expand Down Expand Up @@ -125,7 +128,7 @@ func initRootCommand(rootCommand *cobra.Command, encodingConfig encoding.Config)
)

// Add the following commands to the rootCommand: start, tendermint, export, version, and rollback.
addCommands(rootCommand, app.DefaultNodeHome, NewAppServer, appExporter, addModuleInitFlags)
addCommands(rootCommand, app.DefaultNodeHome, NewAppServer, appExporter, addStartFlags)
}

// setDefaultConsensusParams sets the default consensus parameters for the
Expand All @@ -136,9 +139,11 @@ func setDefaultConsensusParams(command *cobra.Command) error {
return server.SetCmdServerContext(command, ctx)
}

func addModuleInitFlags(startCmd *cobra.Command) {
// addStartFlags adds flags to the start command.
func addStartFlags(startCmd *cobra.Command) {
crisis.AddModuleInitFlags(startCmd)
startCmd.Flags().Int64(UpgradeHeightFlag, 0, "Upgrade height to switch from v1 to v2. Must be coordinated amongst all validators")
startCmd.Flags().Duration(TimeoutCommitFlag, 0, "Override the application configured timeout_commit. Note: only for testing purposes.")
}

// replaceLogger optionally replaces the logger with a file logger if the flag
Expand Down
4 changes: 4 additions & 0 deletions cmd/celestia-appd/cmd/start.go
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,10 @@ is performed. Note, when enabled, gRPC will also be automatically enabled.
serverCtx.Logger.Info(fmt.Sprintf("No default value exists for the v2 upgrade height when the chainID is %v", clientCtx.ChainID))
}

if contains(appconsts.PublicNetworks, clientCtx.ChainID) && serverCtx.Viper.GetDuration(TimeoutCommitFlag) != 0 {
return fmt.Errorf("the --timeout-commit flag was used on %v but it is unsupported on public networks: %v. The --timeout-commit flag should only be used on private testnets", clientCtx.ChainID, strings.Join(appconsts.PublicNetworks, ", "))
}

withTM, _ := cmd.Flags().GetBool(flagWithTendermint)
if !withTM {
serverCtx.Logger.Info("starting ABCI without Tendermint")
Expand Down
2 changes: 2 additions & 0 deletions pkg/appconsts/chain_ids.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,5 @@ const (
MochaChainID = "mocha-4"
MainnetChainID = "celestia"
)

var PublicNetworks = []string{ArabicaChainID, MochaChainID, MainnetChainID}
1 change: 1 addition & 0 deletions scripts/single-node.sh
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ startCelestiaApp() {
--api.enable \
--grpc.enable \
--grpc-web.enable \
--timeout-commit 1s \
--force-no-bbr # no need to require BBR usage on a local node
}

Expand Down
4 changes: 1 addition & 3 deletions test/tokenfilter/setup.go
Original file line number Diff line number Diff line change
Expand Up @@ -146,9 +146,7 @@ func SetupWithGenesisValSet(t testing.TB, valSet *tmtypes.ValidatorSet, genAccs
db := dbm.NewMemDB()
encCdc := encoding.MakeConfig(app.ModuleEncodingRegisters...)
genesisState := app.NewDefaultGenesisState(encCdc.Codec)
app := app.New(
log.NewNopLogger(), db, nil, 5, encCdc, 0, simapp.EmptyAppOptions{},
)
app := app.New(log.NewNopLogger(), db, nil, 5, encCdc, 0, 0, simapp.EmptyAppOptions{})

// set genesis accounts
authGenesis := authtypes.NewGenesisState(authtypes.DefaultParams(), genAccs)
Expand Down
2 changes: 1 addition & 1 deletion test/util/malicious/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ func New(
appOpts servertypes.AppOptions,
baseAppOptions ...func(*baseapp.BaseApp),
) *App {
goodApp := app.New(logger, db, traceStore, invCheckPeriod, encodingConfig, 0, appOpts, baseAppOptions...)
goodApp := app.New(logger, db, traceStore, invCheckPeriod, encodingConfig, 0, 0, appOpts, baseAppOptions...)
badApp := &App{App: goodApp}

// set the malicious prepare proposal handler if it is set in the app options
Expand Down
3 changes: 2 additions & 1 deletion test/util/test_app.go
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ func NewTestApp() *app.App {
cast.ToUint(emptyOpts.Get(server.FlagInvCheckPeriod)),
encCfg,
0,
0,
emptyOpts,
)
}
Expand Down Expand Up @@ -467,7 +468,7 @@ func SetupTestAppWithUpgradeHeight(t *testing.T, upgradeHeight int64) (*app.App,
db := dbm.NewMemDB()
chainID := "test_chain"
encCfg := encoding.MakeConfig(app.ModuleEncodingRegisters...)
testApp := app.New(log.NewNopLogger(), db, nil, 0, encCfg, upgradeHeight, EmptyAppOptions{})
testApp := app.New(log.NewNopLogger(), db, nil, 0, encCfg, upgradeHeight, 0, EmptyAppOptions{})
genesisState, _, kr := GenesisStateWithSingleValidator(testApp, "account")
stateBytes, err := json.MarshalIndent(genesisState, "", " ")
require.NoError(t, err)
Expand Down
2 changes: 2 additions & 0 deletions test/util/testnode/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,7 @@ func DefaultAppCreator(opts ...AppCreationOptions) srvtypes.AppCreator {
0, // invCheckPerid
encodingConfig,
0, // v2 upgrade height
0, // timeout commit
simapp.EmptyAppOptions{},
baseapp.SetMinGasPrices(fmt.Sprintf("%v%v", appconsts.DefaultMinGasPrice, app.BondDenom)),
)
Expand All @@ -198,6 +199,7 @@ func CustomAppCreator(minGasPrice string) srvtypes.AppCreator {
0, // invCheckPerid
encodingConfig,
0, // v2 upgrade height
0, // timeout commit
simapp.EmptyAppOptions{},
baseapp.SetMinGasPrices(minGasPrice),
)
Expand Down
3 changes: 2 additions & 1 deletion tools/chainbuilder/integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,8 @@ func TestRun(t *testing.T) {
nil,
0,
encCfg,
0,
0, // upgrade height v2
0, // timeout commit
util.EmptyAppOptions{},
baseapp.SetMinGasPrices(fmt.Sprintf("%f%s", appconsts.DefaultMinGasPrice, appconsts.BondDenom)),
)
Expand Down
3 changes: 2 additions & 1 deletion tools/chainbuilder/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,8 @@ func Run(ctx context.Context, cfg BuilderConfig, dir string) error {
nil,
0,
encCfg,
0,
0, // upgrade height v2
0, // timeout commit
Comment on lines +205 to +206
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

⚠️ Potential issue

Add support for configurable timeout commit via command line flag

The hardcoded timeout commit value of 0 contradicts the PR objectives which aim to support custom timeout settings for testing environments.

Consider these changes:

  1. Add a command-line flag for timeout commit:
 func main() {
     rootCmd.Flags().Int("num-blocks", 100, "Number of blocks to generate")
+    rootCmd.Flags().Duration("timeout-commit", 0, "Block timeout commit duration for testing. Only applicable for private testnets")
  1. Update the BuilderConfig struct:
 type BuilderConfig struct {
     NumBlocks     int
     BlockSize     int
     BlockInterval time.Duration
+    TimeoutCommit time.Duration
     ExistingDir   string
     Namespace     share.Namespace
     ChainID       string
     AppVersion    uint64
     UpToTime      bool
 }
  1. Pass the timeout commit to app.New:
-		0, // timeout commit
+		cfg.TimeoutCommit,
  1. Add validation to restrict timeout commit changes to private testnets:
 func Run(ctx context.Context, cfg BuilderConfig, dir string) error {
+    // Validate timeout commit usage
+    if cfg.TimeoutCommit > 0 && !strings.HasPrefix(cfg.ChainID, "private-") {
+        return fmt.Errorf("timeout commit can only be set for private testnets (chain IDs prefixed with 'private-')")
+    }

Committable suggestion skipped: line range outside the PR's diff.

util.EmptyAppOptions{},
baseapp.SetMinGasPrices(fmt.Sprintf("%f%s", appconsts.DefaultMinGasPrice, appconsts.BondDenom)),
)
Expand Down
Loading