-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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: ABCI 1.0 baseapp integration #13453
Changes from all commits
d72d1a2
01ed963
35ff6d6
bbd9405
3cc3fb1
9ffa994
aa103ef
77a9b37
46c34ff
9a4eeac
09f4e6b
a6fb1e5
c8931c9
c3746c7
816dc70
85c5f1e
a10b047
59b2195
aafb4ce
1798d68
6be3812
5f2b9cc
14afa8d
ee97a25
bfda138
ee2ed7e
afc04eb
a9a8fa3
9556eff
87bf3b6
5b647eb
6407c12
629d17b
0bd11ff
01858d2
ed23ead
fc784dc
1758aae
0ec9335
1520b5d
fa7cc9a
c4495fd
f4ff6c6
f57847d
82f701c
16100f9
582e914
a8d58dd
1e1be0e
b749c6f
26eb530
5143d58
90996c7
5be329f
c25385a
1725e8d
7cf3a52
9c44433
15834d7
7457271
b0b8b27
5eac4b7
834f85f
d90ac72
fbe1bb4
ffe535d
056bca1
78b774c
192d421
7adc7bf
bd1b55f
96cd0c7
83dcbc7
e473bcf
613019a
3099039
295703f
47fad32
302f81e
83d6320
873cb74
5849815
e61ceea
48ce41c
c24a152
ad829d4
8d13cc2
869b524
47ba2c2
541f112
bae75b9
af4f5bc
1d5b1d4
de40135
6c15d92
250ea90
5be06eb
2663dc2
c2e7069
42d52c1
4ecf2e2
58bd2ce
00a7a8f
be180e8
9d120c1
4f466a4
abbe016
d2f8b21
63d6606
91e30d7
ea8589d
d02e4a9
7d2fd41
1683445
b9a2e05
a1f2ae0
5bd05f8
0c37d23
19e6e3e
5cc97f4
2bc00a1
e42d6a8
1366a87
f2457fa
d7dc83a
bfee335
b7fe34c
52e10e6
4e429dd
5c80aaf
46fb8c4
a17f506
078168b
59cc71f
e321ec2
db1f4cd
ba57a58
227ea17
522aaab
7021bdd
81c67f7
af919d3
54837e8
e9a7232
f634306
6034696
fb6eb64
838e126
c043a7a
b07a56c
45044bd
5ccf004
0528e05
1cb53a0
cc608b6
2f56b6d
bab393a
b88614e
2afe7b1
1dfe713
5fbee38
94ddfa2
f6c073f
179869e
b1c84cb
e514f7b
c834047
4d11de2
4d295b1
46ebfc4
ffdb934
55d13d7
3f76cb6
cb147a3
b04e051
6f71213
0cc1aaa
732d1b1
6601cf3
96cb90b
657ce24
736306c
e907d77
61bd98a
1f53170
369c622
7a7504a
32899a5
ddec83e
3e6e1a4
9c23184
3f1dcd8
d4234d8
580c8b3
3c8640e
d1a1cf8
0697e88
fc4bccb
659faa3
10cd747
bfe7faf
de876ca
51e0da9
7dc0684
72a5c3c
201d52e
dea715f
0af1899
3134207
9e3048f
41816be
678b555
a75e4ef
215d2a6
587f0fb
88f574d
7180e94
97a9c76
6e5ff1c
e1d5ed4
8d4e97b
99566ce
16e98c9
a15d9f0
cf88b04
5db4724
0696b82
a8dd44d
ef5a625
3ac1d30
deb33f5
74775b6
1e36da8
a3bc195
ccc00f1
5595988
26641b6
061e68e
e646e46
e594b59
40a5b21
e50ef7d
5ce1d3a
97f02e9
c10e857
323b9f7
3575579
348f67e
4576305
61e1b60
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,6 +21,7 @@ import ( | |
"github.com/cosmos/cosmos-sdk/telemetry" | ||
sdk "github.com/cosmos/cosmos-sdk/types" | ||
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" | ||
"github.com/cosmos/cosmos-sdk/types/mempool" | ||
) | ||
|
||
// Supported ABCI Query prefixes | ||
|
@@ -38,6 +39,8 @@ func (app *BaseApp) InitChain(req abci.RequestInitChain) (res abci.ResponseInitC | |
// req.InitialHeight is 1 by default. | ||
initHeader := tmproto.Header{ChainID: req.ChainId, Time: req.Time} | ||
|
||
app.logger.Info("InitChain", "initialHeight", req.InitialHeight, "chainID", req.ChainId) | ||
|
||
// If req.InitialHeight is > 1, then we set the initial version in the | ||
// stores. | ||
if req.InitialHeight > 1 { | ||
|
@@ -49,9 +52,11 @@ func (app *BaseApp) InitChain(req abci.RequestInitChain) (res abci.ResponseInitC | |
} | ||
} | ||
|
||
// initialize the deliver state and check state with a correct header | ||
// initialize states with a correct header | ||
app.setDeliverState(initHeader) | ||
app.setCheckState(initHeader) | ||
app.setPrepareProposalState(initHeader) | ||
app.setProcessProposalState(initHeader) | ||
|
||
// Store the consensus params in the BaseApp's paramstore. Note, this must be | ||
// done after the deliver state and context have been set as it's persisted | ||
|
@@ -182,8 +187,6 @@ func (app *BaseApp) BeginBlock(req abci.RequestBeginBlock) (res abci.ResponseBeg | |
WithHeaderHash(req.Hash). | ||
WithConsensusParams(app.GetConsensusParams(app.deliverState.ctx)) | ||
|
||
// we also set block gas meter to checkState in case the application needs to | ||
// verify gas consumption during (Re)CheckTx | ||
if app.checkState != nil { | ||
app.checkState.ctx = app.checkState.ctx. | ||
WithBlockGasMeter(gasMeter). | ||
|
@@ -238,19 +241,52 @@ func (app *BaseApp) EndBlock(req abci.RequestEndBlock) (res abci.ResponseEndBloc | |
// work in a block before proposing it. | ||
// | ||
// Transactions can be modified, removed, or added by the application. Since the | ||
// application maintains it's own local mempool, it will ignore the transactions | ||
// application maintains its own local mempool, it will ignore the transactions | ||
// provided to it in RequestPrepareProposal. Instead, it will determine which | ||
// transactions to return based on the mempool's semantics and the MaxTxBytes | ||
// provided by the client's request. | ||
// | ||
// Note, there is no need to execute the transactions for validity as they have | ||
// already passed CheckTx. | ||
// | ||
// 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) abci.ResponsePrepareProposal { | ||
// TODO: Implement. | ||
return abci.ResponsePrepareProposal{Txs: req.Txs} | ||
var ( | ||
txsBytes [][]byte | ||
byteCount int64 | ||
) | ||
|
||
ctx := app.getContextForTx(runTxPrepareProposal, []byte{}) | ||
iterator := app.mempool.Select(ctx, req.Txs) | ||
|
||
for iterator != nil { | ||
memTx := iterator.Tx() | ||
|
||
bz, err := app.txEncoder(memTx) | ||
if err != nil { | ||
panic(err) | ||
} | ||
|
||
txSize := int64(len(bz)) | ||
|
||
// NOTE: runTx was already run in CheckTx which calls mempool.Insert so ideally everything in the pool | ||
// should be valid. But some mempool implementations may insert invalid txs, so we check again. | ||
_, _, _, _, err = app.runTx(runTxPrepareProposal, bz) | ||
if err != nil { | ||
err := app.mempool.Remove(memTx) | ||
if err != nil && !errors.Is(err, mempool.ErrTxNotFound) { | ||
panic(err) | ||
} | ||
iterator = iterator.Next() | ||
continue | ||
} else if byteCount += txSize; byteCount <= req.MaxTxBytes { | ||
txsBytes = append(txsBytes, bz) | ||
} else { | ||
break | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Food for thought: This For example, let's say Thinking out loud, to prevent this issue, the app can validate that the txs returned in Thinking out loud, some suggestions:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The reason we don't pass MaxBytes to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We discussed this further offline. #13831 fixes the concern. |
||
} | ||
|
||
iterator = iterator.Next() | ||
} | ||
|
||
return abci.ResponsePrepareProposal{Txs: txsBytes} | ||
} | ||
|
||
// ProcessProposal implements the ProcessProposal ABCI method and returns a | ||
|
@@ -266,8 +302,19 @@ func (app *BaseApp) PrepareProposal(req abci.RequestPrepareProposal) abci.Respon | |
// 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) ProcessProposal(req abci.RequestProcessProposal) abci.ResponseProcessProposal { | ||
// TODO: Implement. | ||
return abci.ResponseProcessProposal{Status: abci.ResponseProcessProposal_ACCEPT} | ||
if app.processProposal == nil { | ||
panic("app.ProcessProposal is not set") | ||
} | ||
|
||
ctx := app.processProposalState.ctx. | ||
WithVoteInfos(app.voteInfos). | ||
WithBlockHeight(req.Height). | ||
WithBlockTime(req.Time). | ||
WithHeaderHash(req.Hash). | ||
WithProposer(req.ProposerAddress). | ||
WithConsensusParams(app.GetConsensusParams(app.processProposalState.ctx)) | ||
|
||
return app.processProposal(ctx, req) | ||
} | ||
|
||
// CheckTx implements the ABCI interface and executes a tx in CheckTx mode. In | ||
|
@@ -367,6 +414,8 @@ func (app *BaseApp) Commit() (res abci.ResponseCommit) { | |
// NOTE: This is safe because Tendermint holds a lock on the mempool for | ||
// Commit. Use the header from this latest block. | ||
app.setCheckState(header) | ||
app.setPrepareProposalState(header) | ||
app.setProcessProposalState(header) | ||
|
||
// empty/reset the deliver state | ||
app.deliverState = nil | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,196 @@ | ||
package baseapp_test | ||
|
||
import ( | ||
"context" | ||
"fmt" | ||
"testing" | ||
|
||
"github.com/stretchr/testify/require" | ||
"github.com/stretchr/testify/suite" | ||
abci "github.com/tendermint/tendermint/abci/types" | ||
tmproto "github.com/tendermint/tendermint/proto/tendermint/types" | ||
|
||
"cosmossdk.io/depinject" | ||
"github.com/cosmos/cosmos-sdk/baseapp" | ||
baseapptestutil "github.com/cosmos/cosmos-sdk/baseapp/testutil" | ||
"github.com/cosmos/cosmos-sdk/client" | ||
"github.com/cosmos/cosmos-sdk/codec" | ||
codectypes "github.com/cosmos/cosmos-sdk/codec/types" | ||
"github.com/cosmos/cosmos-sdk/runtime" | ||
sdk "github.com/cosmos/cosmos-sdk/types" | ||
"github.com/cosmos/cosmos-sdk/types/mempool" | ||
authtx "github.com/cosmos/cosmos-sdk/x/auth/tx" | ||
) | ||
|
||
type NoopCounterServerImpl struct{} | ||
|
||
func (m NoopCounterServerImpl) IncrementCounter( | ||
_ context.Context, | ||
_ *baseapptestutil.MsgCounter, | ||
) (*baseapptestutil.MsgCreateCounterResponse, error) { | ||
return &baseapptestutil.MsgCreateCounterResponse{}, nil | ||
} | ||
|
||
type ABCIv1TestSuite struct { | ||
suite.Suite | ||
baseApp *baseapp.BaseApp | ||
mempool mempool.Mempool | ||
txConfig client.TxConfig | ||
cdc codec.ProtoCodecMarshaler | ||
} | ||
|
||
func TestABCIv1TestSuite(t *testing.T) { | ||
suite.Run(t, new(ABCIv1TestSuite)) | ||
} | ||
|
||
func (s *ABCIv1TestSuite) SetupTest() { | ||
t := s.T() | ||
anteKey := []byte("ante-key") | ||
pool := mempool.NewNonceMempool() | ||
anteOpt := func(bapp *baseapp.BaseApp) { | ||
bapp.SetAnteHandler(anteHandlerTxTest(t, capKey1, anteKey)) | ||
} | ||
|
||
var ( | ||
appBuilder *runtime.AppBuilder | ||
cdc codec.ProtoCodecMarshaler | ||
registry codectypes.InterfaceRegistry | ||
) | ||
err := depinject.Inject(makeMinimalConfig(), &appBuilder, &cdc, ®istry) | ||
require.NoError(t, err) | ||
|
||
app := setupBaseApp(t, anteOpt, baseapp.SetMempool(pool)) | ||
baseapptestutil.RegisterInterfaces(registry) | ||
app.SetMsgServiceRouter(baseapp.NewMsgServiceRouter()) | ||
app.SetInterfaceRegistry(registry) | ||
|
||
baseapptestutil.RegisterKeyValueServer(app.MsgServiceRouter(), MsgKeyValueImpl{}) | ||
baseapptestutil.RegisterCounterServer(app.MsgServiceRouter(), NoopCounterServerImpl{}) | ||
header := tmproto.Header{Height: app.LastBlockHeight() + 1} | ||
|
||
app.InitChain(abci.RequestInitChain{ | ||
ConsensusParams: &tmproto.ConsensusParams{}, | ||
}) | ||
|
||
app.BeginBlock(abci.RequestBeginBlock{Header: header}) | ||
|
||
// patch in TxConfig insted of using an output from x/auth/tx | ||
txConfig := authtx.NewTxConfig(cdc, authtx.DefaultSignModes) | ||
|
||
app.SetTxDecoder(txConfig.TxDecoder()) | ||
app.SetTxEncoder(txConfig.TxEncoder()) | ||
|
||
s.baseApp = app | ||
s.mempool = pool | ||
s.txConfig = txConfig | ||
s.cdc = cdc | ||
} | ||
|
||
func (s *ABCIv1TestSuite) TestABCIv1_HappyPath() { | ||
txConfig := s.txConfig | ||
t := s.T() | ||
|
||
tx := newTxCounter(txConfig, 0, 1) | ||
txBytes, err := txConfig.TxEncoder()(tx) | ||
require.NoError(t, err) | ||
|
||
reqCheckTx := abci.RequestCheckTx{ | ||
Tx: txBytes, | ||
Type: abci.CheckTxType_New, | ||
} | ||
s.baseApp.CheckTx(reqCheckTx) | ||
|
||
tx2 := newTxCounter(txConfig, 1, 1) | ||
|
||
tx2Bytes, err := txConfig.TxEncoder()(tx2) | ||
require.NoError(t, err) | ||
|
||
err = s.mempool.Insert(sdk.Context{}, tx2) | ||
require.NoError(t, err) | ||
reqPreparePropossal := abci.RequestPrepareProposal{ | ||
MaxTxBytes: 1000, | ||
} | ||
resPreparePropossal := s.baseApp.PrepareProposal(reqPreparePropossal) | ||
|
||
require.Equal(t, 2, len(resPreparePropossal.Txs)) | ||
|
||
var reqProposalTxBytes [2][]byte | ||
reqProposalTxBytes[0] = txBytes | ||
reqProposalTxBytes[1] = tx2Bytes | ||
reqProcessProposal := abci.RequestProcessProposal{ | ||
Txs: reqProposalTxBytes[:], | ||
} | ||
|
||
s.baseApp.SetProcessProposal(nil) | ||
require.Panics(t, func() { s.baseApp.ProcessProposal(reqProcessProposal) }) | ||
s.baseApp.SetProcessProposal(s.baseApp.DefaultProcessProposal()) | ||
|
||
resProcessProposal := s.baseApp.ProcessProposal(reqProcessProposal) | ||
require.Equal(t, abci.ResponseProcessProposal_ACCEPT, resProcessProposal.Status) | ||
|
||
res := s.baseApp.DeliverTx(abci.RequestDeliverTx{Tx: txBytes}) | ||
require.Equal(t, 1, s.mempool.CountTx()) | ||
|
||
require.NotEmpty(t, res.Events) | ||
require.True(t, res.IsOK(), fmt.Sprintf("%v", res)) | ||
} | ||
|
||
func (s *ABCIv1TestSuite) TestABCIv1_PrepareProposal_ReachedMaxBytes() { | ||
txConfig := s.txConfig | ||
t := s.T() | ||
|
||
for i := 0; i < 100; i++ { | ||
tx2 := newTxCounter(txConfig, int64(i), int64(i)) | ||
err := s.mempool.Insert(sdk.Context{}, tx2) | ||
require.NoError(t, err) | ||
} | ||
|
||
reqPreparePropossal := abci.RequestPrepareProposal{ | ||
MaxTxBytes: 1500, | ||
} | ||
resPreparePropossal := s.baseApp.PrepareProposal(reqPreparePropossal) | ||
|
||
require.Equal(t, 10, len(resPreparePropossal.Txs)) | ||
} | ||
|
||
func (s *ABCIv1TestSuite) TestABCIv1_PrepareProposal_BadEncoding() { | ||
txConfig := authtx.NewTxConfig(s.cdc, authtx.DefaultSignModes) | ||
|
||
t := s.T() | ||
|
||
tx := newTxCounter(txConfig, 0, 0) | ||
err := s.mempool.Insert(sdk.Context{}, tx) | ||
require.NoError(t, err) | ||
|
||
reqPrepareProposal := abci.RequestPrepareProposal{ | ||
MaxTxBytes: 1000, | ||
} | ||
resPrepareProposal := s.baseApp.PrepareProposal(reqPrepareProposal) | ||
|
||
require.Equal(t, 1, len(resPrepareProposal.Txs)) | ||
} | ||
|
||
func (s *ABCIv1TestSuite) TestABCIv1_PrepareProposal_Failures() { | ||
tx := newTxCounter(s.txConfig, 0, 0) | ||
txBytes, err := s.txConfig.TxEncoder()(tx) | ||
s.NoError(err) | ||
|
||
reqCheckTx := abci.RequestCheckTx{ | ||
Tx: txBytes, | ||
Type: abci.CheckTxType_New, | ||
} | ||
checkTxRes := s.baseApp.CheckTx(reqCheckTx) | ||
s.True(checkTxRes.IsOK()) | ||
|
||
failTx := newTxCounter(s.txConfig, 1, 1) | ||
failTx = setFailOnAnte(s.txConfig, failTx, true) | ||
err = s.mempool.Insert(sdk.Context{}, failTx) | ||
s.NoError(err) | ||
s.Equal(2, s.mempool.CountTx()) | ||
|
||
req := abci.RequestPrepareProposal{ | ||
MaxTxBytes: 1000, | ||
} | ||
res := s.baseApp.PrepareProposal(req) | ||
s.Equal(1, len(res.Txs)) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't necessarily understand the point of forcing the
runTx
behavior on all app-side mempool implementations. If anything, this just makes theSetMempool
API more restrictive. It also forces the performance burden on all custom app-side mempools of running all transactions through the Antehandlers a third time (CheckTx
,DeliverTx
, and nowPrepareProposal
). As stated in the comment here this shouldn't be necessary unless the application developer's custom app-side mempool implementation has a bug or is trying to do something unexpected. They can already break themselves in other ways, for example a developer could add somepanic
to theirSelect
implementation which means no proposal would ever be sent, right?Just a suggestion and not a dealbreaker, but I think I'd prefer to be able to opt out of this somehow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Insert
duringCheckTx
so this ensures any injected txs are still valid.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree that they add no performance burden. Some of the checks read from state which could potentially mean disk reads if the state is not in cache. There is also signature verification which is not free.
A couple of examples of reading from state during Antehandlers:
ConsumeTxSizeGasDecorator
here.SendCoinsFromAccountToModule
inDeductFees
which reads module accounts from state here.There is also signature verifications here which looks to take ~0.15ms per-op for
secp256k1
if I am understanding the benchmarks correctly (Sig/secp256k1-8
). This means while ABCI is globally locked duringPrepareProposal
, you're doing this operation for each transaction serially. So if the block has 100 transactions on average, you're spending ~15ms on average per-block blocking the entire application and verifying transaction signatures that have already been verified.Let me know if I'm misunderstanding something!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, state is read from but not written to. In the grand scheme of things, the performance overhead is near negligible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be super clear, 15 milliseconds of additional global lock time per-block (as in my above signature example) is definitely not negligible for us, especially given this is totally unnecessary computation in our case. It's very likely we're a unique case given our throughput aspirations so therefore your assessment is probably true for most other chains.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree with @prettymuchbryce, on profiles I have run on live networks ante handlers show up quite often and do add a non negligible amount of overhead. For many chains this isn't a problem but it's not something we should over look as we are trying to make the sdk more performant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, we validate the txs (i.e. AnteHandler) since applications can insert txs on
Insert
duringCheckTx
and we need to ensure these are still valid when selecting duringPrepareProposal
.Also, recall:
PrepareProposal
)So if this doesn't suite your needs, your AnteHandler chain can easily do something like "If PrepareProposal, skip XYZ".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a really good point that I had forgotten. I think this would be reasonable solution.