diff --git a/CHANGELOG.md b/CHANGELOG.md index 862d4bf64682..b1c30b41b607 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -78,6 +78,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (server) [#25632](https://github.com/cosmos/cosmos-sdk/pull/25632) Add missing call to close the app on shutdown. * (server) [#25740](https://github.com/cosmos/cosmos-sdk/pull/25740) Add variadic `grpc.DialOption` parameter to `StartGrpcServer` for custom gRPC client connection options. * (blockstm) [#25765](https://github.com/cosmos/cosmos-sdk/pull/25765) Minor code readability improvement in block-stm. +* (blockstm) [#25786](https://github.com/cosmos/cosmos-sdk/pull/25786) Add pre-state checking in transaction state transition. ### Bug Fixes diff --git a/blockstm/README.md b/blockstm/README.md index 5e92cd44a951..d81f57641dca 100644 --- a/blockstm/README.md +++ b/blockstm/README.md @@ -14,6 +14,8 @@ func ExecuteBlock( ) error ``` +Broken internal invariants will cause panics. + The main deviations from the paper are: ### Optimisation diff --git a/blockstm/mvmemory_test.go b/blockstm/mvmemory_test.go index ec7d3939aff1..d7fb9cb8c8d1 100644 --- a/blockstm/mvmemory_test.go +++ b/blockstm/mvmemory_test.go @@ -102,6 +102,7 @@ func TestMVMemoryRecord(t *testing.T) { require.False(t, wroteNewLocation) require.True(t, mv.ValidateReadSet(2)) + scheduler.TryIncarnate(version.Index) scheduler.FinishExecution(version, wroteNewLocation) // wait for dependency to finish diff --git a/blockstm/status.go b/blockstm/status.go index be93c4ce5651..1db1e11a2d17 100644 --- a/blockstm/status.go +++ b/blockstm/status.go @@ -1,6 +1,9 @@ package blockstm -import "sync" +import ( + "fmt" + "sync" +) type Status uint @@ -60,16 +63,29 @@ func (s *StatusEntry) TrySetExecuting() (incarnation Incarnation, ok bool) { return incarnation, ok } -func (s *StatusEntry) setStatus(status Status) { +// setStatus sets the status to the given status if the current status is preStatus. +// preStatus invariant must be held by the caller. +func (s *StatusEntry) setStatus(status, preStatus Status) { s.Lock() + + if s.status != preStatus { + s.Unlock() + panic(fmt.Sprintf("invalid status transition: %v -> %v, current: %v", preStatus, status, s.status)) + } + s.status = status s.Unlock() } func (s *StatusEntry) Resume() { - // status must be SUSPENDED and cond != nil s.Lock() + // status must be SUSPENDED and cond != nil + if s.status != StatusSuspended || s.cond == nil { + s.Unlock() + panic(fmt.Sprintf("invalid resume: status=%v", s.status)) + } + s.status = StatusExecuting s.cond.Notify() s.cond = nil @@ -79,7 +95,7 @@ func (s *StatusEntry) Resume() { func (s *StatusEntry) SetExecuted() { // status must have been EXECUTING - s.setStatus(StatusExecuted) + s.setStatus(StatusExecuted, StatusExecuting) } func (s *StatusEntry) TryValidationAbort(incarnation Incarnation) (ok bool) { @@ -97,8 +113,13 @@ func (s *StatusEntry) TryValidationAbort(incarnation Incarnation) (ok bool) { func (s *StatusEntry) SetReadyStatus() { s.Lock() - s.incarnation++ // status must be ABORTING + if s.status != StatusAborting { + s.Unlock() + panic(fmt.Sprintf("invalid status transition: %v -> %v, current: %v", StatusAborting, StatusReadyToExecute, s.status)) + } + + s.incarnation++ s.status = StatusReadyToExecute s.Unlock() @@ -107,6 +128,11 @@ func (s *StatusEntry) SetReadyStatus() { func (s *StatusEntry) Suspend(cond *Condvar) { s.Lock() + if s.status != StatusExecuting { + s.Unlock() + panic(fmt.Sprintf("invalid suspend: status=%v", s.status)) + } + s.cond = cond s.status = StatusSuspended