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.

Sign up for GitHub

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

Jump to bottom

fix: use deliverState in prepare and processProposal at height 1 #14505

Merged
merged 21 commits into from
Jan 8, 2023
Merged

fix: use deliverState in prepare and processProposal at height 1 #14505

Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
704d4d3
fix: use deliverState in prepare and processProposal at height 1
facundomedica Jan 5, 2023
c0d666c
cache deliverState
facundomedica Jan 5, 2023
2d80482
add test + add panic if prepareProposal gets called with height=0
facundomedica Jan 5, 2023
641b0a5
Merge branch 'main' into facu/fix-prep-prop-2
facundomedica Jan 5, 2023
18e8daf
make the linter happy
facundomedica Jan 5, 2023
dafc7f6
Merge branch 'main' into facu/fix-prep-prop-2
facundomedica Jan 5, 2023
fac4b17
make the linter happy
facundomedica Jan 5, 2023
5f24a29
Merge branch 'facu/fix-prep-prop-2' of https://github.com/cosmos/cosm…
facundomedica Jan 5, 2023
aec9010
add comment
facundomedica Jan 5, 2023
a220784
Revert "make the linter happy"
facundomedica Jan 5, 2023
d5d0bbc
fix a wrongly pushed test app
facundomedica Jan 5, 2023
f9167dc
make the linter happy
facundomedica Jan 5, 2023
223c445
make the linter happy
facundomedica Jan 5, 2023
4ce6f8c
make the linter happy
facundomedica Jan 5, 2023
b0e8f83
Merge branch 'main' into facu/fix-prep-prop-2
facundomedica Jan 5, 2023
68dd679
Merge branch 'main' into facu/fix-prep-prop-2
alexanderbez Jan 5, 2023
161f4b9
Merge branch 'main' into facu/fix-prep-prop-2
JeancarloBarrios Jan 5, 2023
21a874e
Merge branch 'main' into facu/fix-prep-prop-2
tac0turtle Jan 6, 2023
4a3beff
suggestion from @ttl33
facundomedica Jan 6, 2023
ac4c3f2
add cl
facundomedica Jan 6, 2023
ae72588
Merge branch 'main' into facu/fix-prep-prop-2
julienrbrt Jan 8, 2023
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
31 changes: 28 additions & 3 deletions baseapp/abci.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,11 +248,29 @@ func (app *BaseApp) EndBlock(req abci.RequestEndBlock) (res abci.ResponseEndBloc
// Ref: https://github.com/cosmos/cosmos-sdk/blob/main/docs/architecture/adr-060-abci-1.0.md
// Ref: https://github.com/tendermint/tendermint/blob/main/spec/abci/abci%2B%2B_basic_concepts.md
func (app *BaseApp) PrepareProposal(req abci.RequestPrepareProposal) (resp abci.ResponsePrepareProposal) {
ctx := app.getContextForTx(runTxPrepareProposal, []byte{})
if app.prepareProposal == nil {
panic("PrepareProposal method not set")
}

// Tendermint must never call PrepareProposal with a height of 0.
// Ref: https://github.com/tendermint/tendermint/blob/059798a4f5b0c9f52aa8655fa619054a0154088c/spec/core/state.md?plain=1#L37-L38
if req.Height < 1 {
panic("PrepareProposal called with invalid height")
}

ctx := app.prepareProposalState.ctx
// Here we use deliverState on the first block given that we want to be able
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious - why is the state with changes that InitChain made not available to the app?

Copy link
Contributor

@alexanderbez alexanderbez Jan 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

wdym "not available to the app"?

After InitChain, changes are not committed. They're technically staged/cached on the (cached) root multi-store. This isn't what prepareProposal state is based off of until Commit is called.

Copy link
Member Author

Choose a reason for hiding this comment

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

InitChain initializes and applies changes (params, genesis and such) to deliverState, which is not persisted until Commit(): see here.
In Commit() deliverState is set to nil while the other states are set to the latest header (that's why we were seeing the previous block height in PrepareProposal before adding WithBlockHeight(req.Height) in this PR).

So PrepareProposal and ProcessProposal will always be seeing persisted state (from the previous produced block), except for the first block that we feed it with the genesis state. Also it's good to note that we shouldn't be making any persistable state changes during these stages.

Also BeginBlock is the one in charge of setting deliverState; except when we are at the first block: in that case we just want to update the context retaining any state changes made in InitChain. see here

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, thanks for the explanation. That makes sense.

Random alternative thoughts:

  1. Would it be better to dedicate height == 1 to InitChain only? meaning that the at height 1, CosmosSDK just runs InitChain and then calls Commit. No special logic to override the ctx state to be the deliverState which has not been committed. My main reasoning around this is if there would be any weird behavior for the PrepareProposal to be based off of state that has not been committed to state.
  2. Similarly, is it possible to introduce some special height 0 that does the InitChain and Commit only

// to access any state changes made in InitChain.
if req.Height == 1 {
julienrbrt marked this conversation as resolved.
Show resolved Hide resolved
ctx, _ = app.deliverState.ctx.CacheContext()
}

ctx = ctx.WithVoteInfos(app.voteInfos).
WithBlockHeight(req.Height).
WithBlockTime(req.Time).
WithProposer(req.ProposerAddress).
WithConsensusParams(app.GetConsensusParams(ctx))

defer func() {
if err := recover(); err != nil {
app.logger.Error(
Expand Down Expand Up @@ -289,13 +307,20 @@ func (app *BaseApp) ProcessProposal(req abci.RequestProcessProposal) (resp abci.
panic("app.ProcessProposal is not set")
}

ctx := app.processProposalState.ctx.
ctx := app.processProposalState.ctx
// Here we use deliverState on the first block given that we want to be able
// to access any state changes made in InitChain.
if req.Height == 1 {
ctx, _ = app.deliverState.ctx.CacheContext()
}

ctx = ctx.
WithVoteInfos(app.voteInfos).
WithBlockHeight(req.Height).
WithBlockTime(req.Time).
WithHeaderHash(req.Hash).
WithProposer(req.ProposerAddress).
WithConsensusParams(app.GetConsensusParams(app.processProposalState.ctx))
WithConsensusParams(app.GetConsensusParams(ctx))

defer func() {
if err := recover(); err != nil {
Expand Down
58 changes: 54 additions & 4 deletions baseapp/abci_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1323,10 +1323,6 @@ func TestABCI_Proposal_HappyPath(t *testing.T) {
ConsensusParams: &tmproto.ConsensusParams{},
})

suite.baseApp.BeginBlock(abci.RequestBeginBlock{
Header: tmproto.Header{Height: suite.baseApp.LastBlockHeight() + 1},
})

tx := newTxCounter(t, suite.txConfig, 0, 1)
txBytes, err := suite.txConfig.TxEncoder()(tx)
require.NoError(t, err)
Expand All @@ -1347,6 +1343,7 @@ func TestABCI_Proposal_HappyPath(t *testing.T) {

reqPrepareProposal := abci.RequestPrepareProposal{
MaxTxBytes: 1000,
Height: 1,
}
resPrepareProposal := suite.baseApp.PrepareProposal(reqPrepareProposal)
require.Equal(t, 2, len(resPrepareProposal.Txs))
Expand All @@ -1362,13 +1359,62 @@ func TestABCI_Proposal_HappyPath(t *testing.T) {
resProcessProposal := suite.baseApp.ProcessProposal(reqProcessProposal)
require.Equal(t, abci.ResponseProcessProposal_ACCEPT, resProcessProposal.Status)

suite.baseApp.BeginBlock(abci.RequestBeginBlock{
Header: tmproto.Header{Height: suite.baseApp.LastBlockHeight() + 1},
})

res := suite.baseApp.DeliverTx(abci.RequestDeliverTx{Tx: txBytes})
require.Equal(t, 1, pool.CountTx())

require.NotEmpty(t, res.Events)
require.True(t, res.IsOK(), fmt.Sprintf("%v", res))
}

func TestABCI_Proposal_Read_State_PrepareProposal(t *testing.T) {
someKey := []byte("some-key")

setInitChainerOpt := func(bapp *baseapp.BaseApp) {
bapp.SetInitChainer(func(ctx sdk.Context, req abci.RequestInitChain) abci.ResponseInitChain {
ctx.KVStore(capKey1).Set(someKey, []byte("foo"))
return abci.ResponseInitChain{}
})
}

prepareOpt := func(bapp *baseapp.BaseApp) {
bapp.SetPrepareProposal(func(ctx sdk.Context, req abci.RequestPrepareProposal) abci.ResponsePrepareProposal {
value := ctx.KVStore(capKey1).Get(someKey)
// We should be able to access any state written in InitChain
require.Equal(t, "foo", string(value))
return abci.ResponsePrepareProposal{Txs: req.Txs}
})
}

suite := NewBaseAppSuite(t, setInitChainerOpt, prepareOpt)

suite.baseApp.InitChain(abci.RequestInitChain{
ConsensusParams: &tmproto.ConsensusParams{},
})

reqPrepareProposal := abci.RequestPrepareProposal{
MaxTxBytes: 1000,
Height: 1, // this value can't be 0
}
resPrepareProposal := suite.baseApp.PrepareProposal(reqPrepareProposal)
require.Equal(t, 0, len(resPrepareProposal.Txs))

reqProposalTxBytes := [][]byte{}
reqProcessProposal := abci.RequestProcessProposal{
Txs: reqProposalTxBytes,
}

resProcessProposal := suite.baseApp.ProcessProposal(reqProcessProposal)
require.Equal(t, abci.ResponseProcessProposal_ACCEPT, resProcessProposal.Status)

suite.baseApp.BeginBlock(abci.RequestBeginBlock{
Header: tmproto.Header{Height: suite.baseApp.LastBlockHeight() + 1},
})
}

func TestABCI_PrepareProposal_ReachedMaxBytes(t *testing.T) {
anteKey := []byte("ante-key")
pool := mempool.NewSenderNonceMempool()
Expand All @@ -1389,6 +1435,7 @@ func TestABCI_PrepareProposal_ReachedMaxBytes(t *testing.T) {

reqPrepareProposal := abci.RequestPrepareProposal{
MaxTxBytes: 1500,
Height: 1,
}
resPrepareProposal := suite.baseApp.PrepareProposal(reqPrepareProposal)
require.Equal(t, 10, len(resPrepareProposal.Txs))
Expand All @@ -1412,6 +1459,7 @@ func TestABCI_PrepareProposal_BadEncoding(t *testing.T) {

reqPrepareProposal := abci.RequestPrepareProposal{
MaxTxBytes: 1000,
Height: 1,
}
resPrepareProposal := suite.baseApp.PrepareProposal(reqPrepareProposal)
require.Equal(t, 1, len(resPrepareProposal.Txs))
Expand Down Expand Up @@ -1449,6 +1497,7 @@ func TestABCI_PrepareProposal_Failures(t *testing.T) {

req := abci.RequestPrepareProposal{
MaxTxBytes: 1000,
Height: 1,
}
res := suite.baseApp.PrepareProposal(req)
require.Equal(t, 1, len(res.Txs))
Expand All @@ -1468,6 +1517,7 @@ func TestABCI_PrepareProposal_PanicRecovery(t *testing.T) {

req := abci.RequestPrepareProposal{
MaxTxBytes: 1000,
Height: 1,
}

require.NotPanics(t, func() {
Expand Down