From 7f44470ad53297059a2cf1811c3a8e007abaa4b3 Mon Sep 17 00:00:00 2001 From: Ethan Buchman Date: Tue, 27 Feb 2018 22:23:56 -0500 Subject: [PATCH 1/2] baseapp: save header on commit. fixes #507 --- baseapp/baseapp.go | 26 ++++++++---- baseapp/baseapp_test.go | 90 ++++++++++++++++++++++++++++++++--------- 2 files changed, 89 insertions(+), 27 deletions(-) diff --git a/baseapp/baseapp.go b/baseapp/baseapp.go index cf86e3f7ac..a04a50cdf7 100644 --- a/baseapp/baseapp.go +++ b/baseapp/baseapp.go @@ -16,7 +16,11 @@ import ( sdk "github.com/cosmos/cosmos-sdk/types" ) -var mainHeaderKey = []byte("header") +// Key to store the header in the DB itself. +// Use the db directly instead of a store to avoid +// conflicts with handlers writing to the store +// and to avoid affecting the Merkle root. +var dbHeaderKey = []byte("header") // The ABCI application type BaseApp struct { @@ -126,19 +130,20 @@ func (app *BaseApp) LastBlockHeight() int64 { // initializes the remaining logic from app.cms func (app *BaseApp) initFromStore(mainKey sdk.StoreKey) error { var lastCommitID = app.cms.LastCommitID() - var main = app.cms.GetKVStore(mainKey) var header abci.Header // main store should exist. + // TODO: we don't actually need the main store here + main := app.cms.GetKVStore(mainKey) if main == nil { return errors.New("BaseApp expects MultiStore with 'main' KVStore") } - // if we've committed before, we expect main:// + // if we've committed before, we expect to exist in the db if !lastCommitID.IsZero() { - headerBytes := main.Get(mainHeaderKey) + headerBytes := app.db.Get(dbHeaderKey) if len(headerBytes) == 0 { - errStr := fmt.Sprintf("Version > 0 but missing key %s", mainHeaderKey) + errStr := fmt.Sprintf("Version > 0 but missing key %s", dbHeaderKey) return errors.New(errStr) } err := proto.Unmarshal(headerBytes, &header) @@ -147,7 +152,7 @@ func (app *BaseApp) initFromStore(mainKey sdk.StoreKey) error { } lastVersion := lastCommitID.Version if header.Height != lastVersion { - errStr := fmt.Sprintf("Expected main://%s.Height %v but got %v", mainHeaderKey, lastVersion, header.Height) + errStr := fmt.Sprintf("Expected main://%s.Height %v but got %v", dbHeaderKey, lastVersion, header.Height) return errors.New(errStr) } } @@ -369,6 +374,14 @@ func (app *BaseApp) EndBlock(req abci.RequestEndBlock) (res abci.ResponseEndBloc // Implements ABCI func (app *BaseApp) Commit() (res abci.ResponseCommit) { + // Write the latest Header to the store + header := app.ctxDeliver.BlockHeader() + headerBytes, err := proto.Marshal(&header) + if err != nil { + panic(err) + } + app.db.Set(dbHeaderKey, headerBytes) + // Write the Deliver state and commit the MultiStore app.msDeliver.Write() commitID := app.cms.Commit() @@ -379,7 +392,6 @@ func (app *BaseApp) Commit() (res abci.ResponseCommit) { // Reset the Check state // NOTE: safe because Tendermint holds a lock on the mempool for Commit. // Use the header from this latest block. - header := app.ctxDeliver.BlockHeader() app.msCheck = app.cms.CacheMultiStore() app.ctxCheck = app.NewContext(true, header) diff --git a/baseapp/baseapp_test.go b/baseapp/baseapp_test.go index d228d2761e..3add7a8b21 100644 --- a/baseapp/baseapp_test.go +++ b/baseapp/baseapp_test.go @@ -42,19 +42,82 @@ func TestMountStores(t *testing.T) { app.MountStoresIAVL(capKey1, capKey2) - // both stores are mounted + // stores are mounted err := app.LoadLatestVersion(capKey1) assert.Nil(t, err) - err = app.LoadLatestVersion(capKey2) - assert.Nil(t, err) + + // check both stores + store1 := app.cms.GetCommitKVStore(capKey1) + assert.NotNil(t, store1) + store2 := app.cms.GetCommitKVStore(capKey2) + assert.NotNil(t, store2) } // Test that we can make commits and then reload old versions. // Test that LoadLatestVersion actually does. func TestLoadVersion(t *testing.T) { - // TODO + logger := defaultLogger() + db := dbm.NewMemDB() + name := t.Name() + app := NewBaseApp(name, logger, db) + + // make a cap key and mount the store + capKey := sdk.NewKVStoreKey("main") + app.MountStoresIAVL(capKey) + err := app.LoadLatestVersion(capKey) // needed to make stores non-nil + assert.Nil(t, err) + + emptyCommitID := sdk.CommitID{} + + lastHeight := app.LastBlockHeight() + lastID := app.LastCommitID() + assert.Equal(t, int64(0), lastHeight) + assert.Equal(t, emptyCommitID, lastID) + + // execute some blocks + header := abci.Header{Height: 1} + app.BeginBlock(abci.RequestBeginBlock{Header: header}) + res := app.Commit() + commitID := sdk.CommitID{1, res.Data} + + // reload + app = NewBaseApp(name, logger, db) + app.MountStoresIAVL(capKey) + err = app.LoadLatestVersion(capKey) // needed to make stores non-nil + assert.Nil(t, err) + + lastHeight = app.LastBlockHeight() + lastID = app.LastCommitID() + assert.Equal(t, int64(1), lastHeight) + assert.Equal(t, commitID, lastID) } +// Test that the app hash is static +// TODO: https://github.com/cosmos/cosmos-sdk/issues/520 +/*func TestStaticAppHash(t *testing.T) { + app := newBaseApp(t.Name()) + + // make a cap key and mount the store + capKey := sdk.NewKVStoreKey("main") + app.MountStoresIAVL(capKey) + err := app.LoadLatestVersion(capKey) // needed to make stores non-nil + assert.Nil(t, err) + + // execute some blocks + header := abci.Header{Height: 1} + app.BeginBlock(abci.RequestBeginBlock{Header: header}) + res := app.Commit() + commitID1 := sdk.CommitID{1, res.Data} + + header = abci.Header{Height: 2} + app.BeginBlock(abci.RequestBeginBlock{Header: header}) + res = app.Commit() + commitID2 := sdk.CommitID{2, res.Data} + + assert.Equal(t, commitID1.Hash, commitID2.Hash) +} +*/ + // Test that txs can be unmarshalled and read and that // correct error codes are returned when not func TestTxDecoder(t *testing.T) { @@ -246,7 +309,8 @@ func TestValidatorChange(t *testing.T) { // Create app. app := newBaseApp(t.Name()) - storeKeys := createMounts(app.cms) + capKey := sdk.NewKVStoreKey("key") + app.MountStoresIAVL(capKey) app.SetTxDecoder(func(txBytes []byte) (sdk.Tx, sdk.Error) { var ttx testUpdatePowerTx fromJSON(txBytes, &ttx) @@ -260,7 +324,7 @@ func TestValidatorChange(t *testing.T) { }) // Load latest state, which should be empty. - err := app.LoadLatestVersion(storeKeys["main"]) + err := app.LoadLatestVersion(capKey) assert.Nil(t, err) assert.Equal(t, app.LastBlockHeight(), int64(0)) @@ -369,17 +433,3 @@ func fromJSON(bz []byte, ptr interface{}) { panic(err) } } - -// Mounts stores to CommitMultiStore and returns a map of keys. -func createMounts(ms sdk.CommitMultiStore) map[string]sdk.StoreKey { - dbMain := dbm.NewMemDB() - dbXtra := dbm.NewMemDB() - keyMain := sdk.NewKVStoreKey("main") - keyXtra := sdk.NewKVStoreKey("xtra") - ms.MountStoreWithDB(keyMain, sdk.StoreTypeIAVL, dbMain) - ms.MountStoreWithDB(keyXtra, sdk.StoreTypeIAVL, dbXtra) - return map[string]sdk.StoreKey{ - "main": keyMain, - "xtra": keyXtra, - } -} From 588acc272e1c8120eda73e9f385b4720b26ca253 Mon Sep 17 00:00:00 2001 From: Ethan Buchman Date: Tue, 27 Feb 2018 23:07:54 -0500 Subject: [PATCH 2/2] baseapp: dont save header for now. use DeliverTx state in InitChain (fixes #474) --- baseapp/baseapp.go | 83 ++++++++++++++++++------------- baseapp/baseapp_test.go | 1 + examples/basecoin/app/app_test.go | 2 + 3 files changed, 52 insertions(+), 34 deletions(-) diff --git a/baseapp/baseapp.go b/baseapp/baseapp.go index a04a50cdf7..308aa6edaf 100644 --- a/baseapp/baseapp.go +++ b/baseapp/baseapp.go @@ -4,7 +4,6 @@ import ( "fmt" "runtime/debug" - "github.com/golang/protobuf/proto" "github.com/pkg/errors" abci "github.com/tendermint/abci/types" @@ -129,8 +128,6 @@ func (app *BaseApp) LastBlockHeight() int64 { // initializes the remaining logic from app.cms func (app *BaseApp) initFromStore(mainKey sdk.StoreKey) error { - var lastCommitID = app.cms.LastCommitID() - var header abci.Header // main store should exist. // TODO: we don't actually need the main store here @@ -139,23 +136,35 @@ func (app *BaseApp) initFromStore(mainKey sdk.StoreKey) error { return errors.New("BaseApp expects MultiStore with 'main' KVStore") } + // XXX: Do we really need the header? What does it have that we want + // here that's not already in the CommitID ? If an app wants to have it, + // they can do so in their BeginBlocker. If we force it in baseapp, + // then either we force the AppHash to change with every block (since the header + // will be in the merkle store) or we can't write the state and the header to the + // db atomically without doing some surgery on the store interfaces ... + // if we've committed before, we expect to exist in the db - if !lastCommitID.IsZero() { - headerBytes := app.db.Get(dbHeaderKey) - if len(headerBytes) == 0 { - errStr := fmt.Sprintf("Version > 0 but missing key %s", dbHeaderKey) - return errors.New(errStr) + /* + var lastCommitID = app.cms.LastCommitID() + var header abci.Header + + if !lastCommitID.IsZero() { + headerBytes := app.db.Get(dbHeaderKey) + if len(headerBytes) == 0 { + errStr := fmt.Sprintf("Version > 0 but missing key %s", dbHeaderKey) + return errors.New(errStr) + } + err := proto.Unmarshal(headerBytes, &header) + if err != nil { + return errors.Wrap(err, "Failed to parse Header") + } + lastVersion := lastCommitID.Version + if header.Height != lastVersion { + errStr := fmt.Sprintf("Expected db://%s.Height %v but got %v", dbHeaderKey, lastVersion, header.Height) + return errors.New(errStr) + } } - err := proto.Unmarshal(headerBytes, &header) - if err != nil { - return errors.Wrap(err, "Failed to parse Header") - } - lastVersion := lastCommitID.Version - if header.Height != lastVersion { - errStr := fmt.Sprintf("Expected main://%s.Height %v but got %v", dbHeaderKey, lastVersion, header.Height) - return errors.New(errStr) - } - } + */ // initialize Check state app.msCheck = app.cms.CacheMultiStore() @@ -201,16 +210,14 @@ func (app *BaseApp) InitChain(req abci.RequestInitChain) (res abci.ResponseInitC return } - // make a context for the initialization. - // NOTE: we're writing to the cms directly, without a CacheWrap - ctx := sdk.NewContext(app.cms, abci.Header{}, false, nil) + // Initialize the deliver state + app.msDeliver = app.cms.CacheMultiStore() + app.ctxDeliver = app.NewContext(false, abci.Header{}) - res = app.initChainer(ctx, req) - // TODO: handle error https://github.com/cosmos/cosmos-sdk/issues/468 + app.initChainer(app.ctxDeliver, req) // no error - // XXX this commits everything and bumps the version. - // https://github.com/cosmos/cosmos-sdk/issues/442#issuecomment-366470148 - app.cms.Commit() + // NOTE: we don't commit, but BeginBlock for block 1 + // starts from this state return } @@ -228,8 +235,14 @@ func (app *BaseApp) Query(req abci.RequestQuery) (res abci.ResponseQuery) { // Implements ABCI func (app *BaseApp) BeginBlock(req abci.RequestBeginBlock) (res abci.ResponseBeginBlock) { - app.msDeliver = app.cms.CacheMultiStore() - app.ctxDeliver = app.NewContext(false, req.Header) + // Initialize the DeliverTx state. + // If this is the first block, it was already + // initialized in InitChain. If we're testing, + // then we may not have run InitChain and these could be nil + if req.Header.Height > 1 || app.msDeliver == nil { + app.msDeliver = app.cms.CacheMultiStore() + app.ctxDeliver = app.NewContext(false, req.Header) + } app.valUpdates = nil if app.beginBlocker != nil { res = app.beginBlocker(app.ctxDeliver, req) @@ -374,13 +387,15 @@ func (app *BaseApp) EndBlock(req abci.RequestEndBlock) (res abci.ResponseEndBloc // Implements ABCI func (app *BaseApp) Commit() (res abci.ResponseCommit) { - // Write the latest Header to the store header := app.ctxDeliver.BlockHeader() - headerBytes, err := proto.Marshal(&header) - if err != nil { - panic(err) - } - app.db.Set(dbHeaderKey, headerBytes) + /* + // Write the latest Header to the store + headerBytes, err := proto.Marshal(&header) + if err != nil { + panic(err) + } + app.db.SetSync(dbHeaderKey, headerBytes) + */ // Write the Deliver state and commit the MultiStore app.msDeliver.Write() diff --git a/baseapp/baseapp_test.go b/baseapp/baseapp_test.go index 3add7a8b21..1155ca24b4 100644 --- a/baseapp/baseapp_test.go +++ b/baseapp/baseapp_test.go @@ -175,6 +175,7 @@ func TestInitChainer(t *testing.T) { // set initChainer and try again - should see the value app.SetInitChainer(initChainer) app.InitChain(abci.RequestInitChain{}) + app.Commit() res = app.Query(query) assert.Equal(t, value, res.Value) } diff --git a/examples/basecoin/app/app_test.go b/examples/basecoin/app/app_test.go index b3677301f1..20985fa329 100644 --- a/examples/basecoin/app/app_test.go +++ b/examples/basecoin/app/app_test.go @@ -86,6 +86,7 @@ func TestGenesis(t *testing.T) { vals := []abci.Validator{} bapp.InitChain(abci.RequestInitChain{vals, stateBytes}) + bapp.Commit() // A checkTx context ctx := bapp.BaseApp.NewContext(true, abci.Header{}) @@ -127,6 +128,7 @@ func TestSendMsgWithAccounts(t *testing.T) { // Initialize the chain vals := []abci.Validator{} bapp.InitChain(abci.RequestInitChain{vals, stateBytes}) + bapp.Commit() // A checkTx context (true) ctxCheck := bapp.BaseApp.NewContext(true, abci.Header{})