From eb767a1f4b9b81449fd7546ee9d955bcf2baed0c Mon Sep 17 00:00:00 2001 From: HuangYi Date: Thu, 22 Jun 2023 06:37:50 +0800 Subject: [PATCH 1/3] Problem: halt-height behavior is not deterministic Solution: - make sure in state machine that we don't execute block beyond halt-height --- CHANGELOG.md | 1 + baseapp/abci.go | 14 ++++++++++++++ 2 files changed, 15 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c796a584801f..f972e2cc065c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,6 +41,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Bug Fixes * [#16547](https://github.com/cosmos/cosmos-sdk/pull/16547) Ensure a transaction's gas limit cannot exceed the block gas limit. +* [#]() Make sure we don't execute blocks beyond the halt height. ### Improvements diff --git a/baseapp/abci.go b/baseapp/abci.go index ded22c971933..5f5534962d65 100644 --- a/baseapp/abci.go +++ b/baseapp/abci.go @@ -648,6 +648,20 @@ func (app *BaseApp) VerifyVoteExtension(req *abci.RequestVerifyVoteExtension) (r func (app *BaseApp) FinalizeBlock(req *abci.RequestFinalizeBlock) (*abci.ResponseFinalizeBlock, error) { var events []abci.Event + // don't execute blocks beyond the halt height + var halt bool + switch { + case app.haltHeight > 0 && uint64(req.Height) > app.haltHeight: + halt = true + + case app.haltTime > 0 && req.Time.Unix() > int64(app.haltTime): + halt = true + } + + if halt { + return nil, fmt.Errorf("halt per configuration height %d time %d", app.haltHeight, app.haltTime) + } + if err := app.validateFinalizeBlockHeight(req); err != nil { return nil, err } From eefd7bc3f2db18232528bb4ab0cc14db7e98f672 Mon Sep 17 00:00:00 2001 From: yihuang Date: Thu, 22 Jun 2023 06:40:14 +0800 Subject: [PATCH 2/3] Update CHANGELOG.md --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f972e2cc065c..1d19b6f8427a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,7 +41,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Bug Fixes * [#16547](https://github.com/cosmos/cosmos-sdk/pull/16547) Ensure a transaction's gas limit cannot exceed the block gas limit. -* [#]() Make sure we don't execute blocks beyond the halt height. +* [#16639](https://github.com/cosmos/cosmos-sdk/pull/16639) Make sure we don't execute blocks beyond the halt height. ### Improvements From d9c41610c4de0c4da1787c916c803f3f105b055c Mon Sep 17 00:00:00 2001 From: HuangYi Date: Tue, 27 Jun 2023 11:01:23 +0800 Subject: [PATCH 3/3] cleanup and unit tests --- baseapp/abci.go | 73 ++++++++++++-------------------------------- baseapp/abci_test.go | 40 ++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 53 deletions(-) diff --git a/baseapp/abci.go b/baseapp/abci.go index 5f5534962d65..4edef67455fb 100644 --- a/baseapp/abci.go +++ b/baseapp/abci.go @@ -4,10 +4,8 @@ import ( "context" "crypto/sha256" "fmt" - "os" "sort" "strings" - "syscall" "time" coreheader "cosmossdk.io/core/header" @@ -648,18 +646,8 @@ func (app *BaseApp) VerifyVoteExtension(req *abci.RequestVerifyVoteExtension) (r func (app *BaseApp) FinalizeBlock(req *abci.RequestFinalizeBlock) (*abci.ResponseFinalizeBlock, error) { var events []abci.Event - // don't execute blocks beyond the halt height - var halt bool - switch { - case app.haltHeight > 0 && uint64(req.Height) > app.haltHeight: - halt = true - - case app.haltTime > 0 && req.Time.Unix() > int64(app.haltTime): - halt = true - } - - if halt { - return nil, fmt.Errorf("halt per configuration height %d time %d", app.haltHeight, app.haltTime) + if err := app.checkHalt(req.Height, req.Time); err != nil { + return nil, err } if err := app.validateFinalizeBlockHeight(req); err != nil { @@ -761,6 +749,24 @@ func (app *BaseApp) FinalizeBlock(req *abci.RequestFinalizeBlock) (*abci.Respons }, nil } +// checkHalt checkes if height or time exceeds halt-height or halt-time respectively. +func (app *BaseApp) checkHalt(height int64, time time.Time) error { + var halt bool + switch { + case app.haltHeight > 0 && uint64(height) > app.haltHeight: + halt = true + + case app.haltTime > 0 && time.Unix() > int64(app.haltTime): + halt = true + } + + if halt { + return fmt.Errorf("halt per configuration height %d time %d", app.haltHeight, app.haltTime) + } + + return nil +} + // Commit implements the ABCI interface. It will commit all state that exists in // the deliver state's multi-store and includes the resulting commit ID in the // returned abci.ResponseCommit. Commit will set the check state based on the @@ -812,23 +818,6 @@ func (app *BaseApp) Commit() (*abci.ResponseCommit, error) { app.prepareCheckStater(app.checkState.ctx) } - var halt bool - switch { - case app.haltHeight > 0 && uint64(header.Height) >= app.haltHeight: - halt = true - - case app.haltTime > 0 && header.Time.Unix() >= int64(app.haltTime): - halt = true - } - - if halt { - // Halt the binary and allow CometBFT to receive the ResponseCommit - // response with the commit ID hash. This will allow the node to successfully - // restart and process blocks assuming the halt configuration has been - // reset or moved to a more distant value. - app.halt() - } - go app.snapshotManager.SnapshotIfApplicable(header.Height) return resp, nil @@ -852,28 +841,6 @@ func (app *BaseApp) workingHash() []byte { return commitHash } -// halt attempts to gracefully shutdown the node via SIGINT and SIGTERM falling -// back on os.Exit if both fail. -func (app *BaseApp) halt() { - app.logger.Info("halting node per configuration", "height", app.haltHeight, "time", app.haltTime) - - p, err := os.FindProcess(os.Getpid()) - if err == nil { - // attempt cascading signals in case SIGINT fails (os dependent) - sigIntErr := p.Signal(syscall.SIGINT) - sigTermErr := p.Signal(syscall.SIGTERM) - - if sigIntErr == nil || sigTermErr == nil { - return - } - } - - // Resort to exiting immediately if the process could not be found or killed - // via SIGINT/SIGTERM signals. - app.logger.Info("failed to send SIGINT/SIGTERM; exiting...") - os.Exit(0) -} - func handleQueryApp(app *BaseApp, path []string, req *abci.RequestQuery) *abci.ResponseQuery { if len(path) >= 2 { switch path[1] { diff --git a/baseapp/abci_test.go b/baseapp/abci_test.go index a67c5d1fb8bf..d337ff95775c 100644 --- a/baseapp/abci_test.go +++ b/baseapp/abci_test.go @@ -8,6 +8,7 @@ import ( "strconv" "strings" "testing" + "time" errorsmod "cosmossdk.io/errors" "cosmossdk.io/log" @@ -1465,3 +1466,42 @@ func TestABCI_Proposal_Reset_State_Between_Calls(t *testing.T) { require.Equal(t, abci.ResponseProcessProposal_ACCEPT, resProcessProposal.Status) } } + +func TestABCI_HaltChain(t *testing.T) { + testCases := []struct { + name string + haltHeight uint64 + haltTime uint64 + blockHeight int64 + blockTime int64 + expHalt bool + }{ + {"default", 0, 0, 10, 0, false}, + {"halt-height-edge", 10, 0, 10, 0, false}, + {"halt-height", 10, 0, 11, 0, true}, + {"halt-time-edge", 0, 10, 1, 10, false}, + {"halt-time", 0, 10, 1, 11, true}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + suite := NewBaseAppSuite(t, baseapp.SetHaltHeight(tc.haltHeight), baseapp.SetHaltTime(tc.haltTime)) + suite.baseApp.InitChain(&abci.RequestInitChain{ + ConsensusParams: &cmtproto.ConsensusParams{}, + InitialHeight: tc.blockHeight, + }) + + app := suite.baseApp + _, err := app.FinalizeBlock(&abci.RequestFinalizeBlock{ + Height: tc.blockHeight, + Time: time.Unix(tc.blockTime, 0), + }) + if !tc.expHalt { + require.NoError(t, err) + } else { + require.Error(t, err) + require.True(t, strings.HasPrefix(err.Error(), "halt per configuration")) + } + }) + } +}