From d905bc013f97da91b9e1d197a3066fb4cf1c005b Mon Sep 17 00:00:00 2001 From: Sergio Mena Date: Wed, 3 Aug 2022 19:39:17 +0200 Subject: [PATCH] Follow-up fixes to main `PrepareProposal` implementation (#9162) * -----start------ * [cherrypicked] state: panic on ResponsePrepareProposal validation error (#8145) * state: panic on ResponsePrepareProposal validation error * lint++ Co-authored-by: Sam Kleinman Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> * [cherrypicked] abci++: remove CheckTx call from PrepareProposal flow (#8176) * [cherrypicked] abci++: correct max-size check to only operate on added and unmodified (#8242) * [cherrypicked] Remove `ModifiedTxStatus` from the spec and the code (#8210) * Outstanding abci-gen changes to 'pb.go' files * Removed modified_tx_status from spec and protobufs * Fix sed for OSX * Regenerated abci protobufs with 'abci-proto-gen' * Code changes. UTs e2e tests passing * Recovered UT: TestPrepareProposalModifiedTxStatusFalse * Adapted UT * Fixed UT * Revert "Fix sed for OSX" This reverts commit e576708c618f0ef732498f4d348503b823b6c9e8. * Update internal/state/execution_test.go Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> * Update abci/example/kvstore/kvstore.go Co-authored-by: M. J. Fromberger * Update internal/state/execution_test.go Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> * Update spec/abci++/abci++_tmint_expected_behavior_002_draft.md Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> * Addressed some comments * Added one test that tests error at the ABCI client + Fixed some mock calls * Addressed remaining comments * Update abci/example/kvstore/kvstore.go Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> * Update abci/example/kvstore/kvstore.go Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> * Update abci/example/kvstore/kvstore.go Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> * Update spec/abci++/abci++_tmint_expected_behavior_002_draft.md Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> * Addressed William's latest comments * Adressed Michael's comment * Fixed UT * Some md fixes * More md fixes * gofmt Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> Co-authored-by: M. J. Fromberger * make proto-gen * Fixed testcase on PrepareProposal error * mockery Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> Co-authored-by: Sam Kleinman Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: M. J. Fromberger --- abci/example/kvstore/kvstore.go | 5 - abci/example/kvstore/persistent_kvstore.go | 30 ++-- abci/types/application.go | 14 +- abci/types/types.pb.go | 170 ++++++++----------- consensus/mempool_test.go | 15 +- consensus/state.go | 6 +- proto/tendermint/abci/types.proto | 3 +- proxy/mocks/client_creator.go | 5 +- state/execution.go | 17 +- state/execution_test.go | 182 +++++++++++++++++++-- state/helpers_test.go | 2 +- state/test/factory/block.go | 2 +- state/validation_test.go | 2 +- test/e2e/app/app.go | 14 +- test/factory/tx.go | 13 +- types/tx.go | 19 +-- types/tx_test.go | 19 +++ 17 files changed, 332 insertions(+), 186 deletions(-) diff --git a/abci/example/kvstore/kvstore.go b/abci/example/kvstore/kvstore.go index d2b3f5d44..8b851ca9a 100644 --- a/abci/example/kvstore/kvstore.go +++ b/abci/example/kvstore/kvstore.go @@ -170,8 +170,3 @@ func (app *Application) Query(reqQuery types.RequestQuery) (resQuery types.Respo return resQuery } - -func (app *Application) PrepareProposal( - req types.RequestPrepareProposal) types.ResponsePrepareProposal { - return types.ResponsePrepareProposal{} -} diff --git a/abci/example/kvstore/persistent_kvstore.go b/abci/example/kvstore/persistent_kvstore.go index fc8caeb8f..e919ff0cb 100644 --- a/abci/example/kvstore/persistent_kvstore.go +++ b/abci/example/kvstore/persistent_kvstore.go @@ -172,7 +172,7 @@ func (app *PersistentKVStoreApplication) ApplySnapshotChunk( func (app *PersistentKVStoreApplication) PrepareProposal( req types.RequestPrepareProposal) types.ResponsePrepareProposal { - return types.ResponsePrepareProposal{TxRecords: app.substPrepareTx(req.Txs)} + return types.ResponsePrepareProposal{TxRecords: app.substPrepareTx(req.Txs, req.MaxTxBytes)} } //--------------------------------------------- @@ -306,28 +306,32 @@ func (app *PersistentKVStoreApplication) execPrepareTx(tx []byte) types.Response } // substPrepareTx substitutes all the transactions prefixed with 'prepare' in the -// proposal for transactions with the prefix strips. +// proposal for transactions with the prefix stripped. // It marks all of the original transactions as 'REMOVED' so that // Tendermint will remove them from its mempool. -func (app *PersistentKVStoreApplication) substPrepareTx(blockData [][]byte) []*types.TxRecord { - trs := make([]*types.TxRecord, len(blockData)) +func (app *PersistentKVStoreApplication) substPrepareTx(blockData [][]byte, maxTxBytes int64) []*types.TxRecord { + trs := make([]*types.TxRecord, 0, len(blockData)) var removed []*types.TxRecord - for i, tx := range blockData { + var totalBytes int64 + for _, tx := range blockData { + txMod := tx + action := types.TxRecord_UNMODIFIED if isPrepareTx(tx) { removed = append(removed, &types.TxRecord{ Tx: tx, Action: types.TxRecord_REMOVED, }) - trs[i] = &types.TxRecord{ - Tx: bytes.TrimPrefix(tx, []byte(PreparePrefix)), - Action: types.TxRecord_ADDED, - } - continue + txMod = bytes.TrimPrefix(tx, []byte(PreparePrefix)) + action = types.TxRecord_ADDED } - trs[i] = &types.TxRecord{ - Tx: tx, - Action: types.TxRecord_UNMODIFIED, + totalBytes += int64(len(txMod)) + if totalBytes > maxTxBytes { + break } + trs = append(trs, &types.TxRecord{ + Tx: txMod, + Action: action, + }) } return append(trs, removed...) diff --git a/abci/types/application.go b/abci/types/application.go index fa88754db..bc244543c 100644 --- a/abci/types/application.go +++ b/abci/types/application.go @@ -93,7 +93,19 @@ func (BaseApplication) ApplySnapshotChunk(req RequestApplySnapshotChunk) Respons } func (BaseApplication) PrepareProposal(req RequestPrepareProposal) ResponsePrepareProposal { - return ResponsePrepareProposal{} + trs := make([]*TxRecord, 0, len(req.Txs)) + var totalBytes int64 + for _, tx := range req.Txs { + totalBytes += int64(len(tx)) + if totalBytes > req.MaxTxBytes { + break + } + trs = append(trs, &TxRecord{ + Action: TxRecord_UNMODIFIED, + Tx: tx, + }) + } + return ResponsePrepareProposal{TxRecords: trs} } //------------------------------------------------------- diff --git a/abci/types/types.pb.go b/abci/types/types.pb.go index c31dae7b8..b6b4a69c3 100644 --- a/abci/types/types.pb.go +++ b/abci/types/types.pb.go @@ -2557,8 +2557,7 @@ func (m *ResponseApplySnapshotChunk) GetRejectSenders() []string { } type ResponsePrepareProposal struct { - ModifiedTx bool `protobuf:"varint,1,opt,name=modified_tx,json=modifiedTx,proto3" json:"modified_tx,omitempty"` - TxRecords []*TxRecord `protobuf:"bytes,2,rep,name=tx_records,json=txRecords,proto3" json:"tx_records,omitempty"` + TxRecords []*TxRecord `protobuf:"bytes,1,rep,name=tx_records,json=txRecords,proto3" json:"tx_records,omitempty"` } func (m *ResponsePrepareProposal) Reset() { *m = ResponsePrepareProposal{} } @@ -2594,13 +2593,6 @@ func (m *ResponsePrepareProposal) XXX_DiscardUnknown() { var xxx_messageInfo_ResponsePrepareProposal proto.InternalMessageInfo -func (m *ResponsePrepareProposal) GetModifiedTx() bool { - if m != nil { - return m.ModifiedTx - } - return false -} - func (m *ResponsePrepareProposal) GetTxRecords() []*TxRecord { if m != nil { return m.TxRecords @@ -3512,7 +3504,7 @@ func init() { func init() { proto.RegisterFile("tendermint/abci/types.proto", fileDescriptor_252557cfdd89a31a) } var fileDescriptor_252557cfdd89a31a = []byte{ - // 3097 bytes of a gzipped FileDescriptorProto + // 3078 bytes of a gzipped FileDescriptorProto 0x1f, 0x8b, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0xff, 0xe4, 0x5a, 0xcb, 0x77, 0x23, 0xc5, 0xd5, 0xd7, 0xfb, 0x71, 0x6d, 0x49, 0xed, 0x9a, 0x61, 0x46, 0x88, 0xc1, 0x1e, 0x7a, 0x0e, 0x30, 0x0c, 0xe0, 0xf9, 0x30, 0x07, 0xbe, 0xe1, 0xf5, 0x81, 0x2d, 0x6b, 0x90, 0xe7, 0x61, 0xf9, 0x2b, @@ -3645,68 +3637,67 @@ var fileDescriptor_252557cfdd89a31a = []byte{ 0x6e, 0xe4, 0x09, 0xb5, 0x0f, 0x49, 0x8f, 0x6a, 0x02, 0x8a, 0xc4, 0xa6, 0x2b, 0x33, 0x35, 0x26, 0xdd, 0x17, 0x42, 0xf5, 0x83, 0x73, 0xcd, 0x65, 0x19, 0xf2, 0xb8, 0xd5, 0xc5, 0xff, 0xaf, 0x64, 0x11, 0x82, 0x2a, 0x2f, 0x6a, 0xfb, 0xbb, 0x9b, 0x7b, 0xfb, 0xed, 0x0e, 0x9b, 0xcb, 0x0b, 0x50, - 0xf3, 0xe7, 0xd2, 0x17, 0xe6, 0x55, 0x0a, 0x97, 0x13, 0x78, 0x1e, 0xa3, 0x49, 0x43, 0xdb, 0x30, - 0x8f, 0x4c, 0x62, 0x68, 0x32, 0xd5, 0x52, 0xc2, 0xe0, 0x8b, 0xba, 0x63, 0x74, 0x0b, 0x80, 0x8e, - 0x35, 0x97, 0xf4, 0x6c, 0xd7, 0xf0, 0x99, 0xc1, 0xf4, 0x26, 0xec, 0x8e, 0x31, 0xd7, 0xc0, 0x65, - 0x2a, 0x4b, 0x9e, 0xfa, 0xcf, 0x34, 0xd4, 0x26, 0x8e, 0x25, 0xda, 0x80, 0xbc, 0x08, 0x3a, 0x92, - 0x7e, 0x1b, 0x71, 0x54, 0x91, 0x67, 0x58, 0xa8, 0xa2, 0x37, 0xa1, 0x44, 0x64, 0xd8, 0x3d, 0xeb, - 0xf8, 0x8b, 0x34, 0x82, 0x1f, 0x98, 0x4b, 0xd3, 0xc0, 0x02, 0xbd, 0x0d, 0xe5, 0x00, 0x5f, 0x64, - 0xa4, 0xfb, 0xd4, 0xb4, 0x79, 0x80, 0x4c, 0xd2, 0x3e, 0xb4, 0x41, 0xaf, 0x85, 0x14, 0x33, 0x37, - 0x1d, 0xea, 0x48, 0x73, 0xa1, 0x20, 0x8d, 0x7d, 0x7d, 0xb5, 0x09, 0x4b, 0x91, 0xf1, 0xa0, 0x27, - 0xa0, 0x3c, 0xd4, 0xfd, 0xac, 0x80, 0xc8, 0x56, 0x95, 0x86, 0xba, 0xc8, 0x09, 0xa0, 0xcb, 0x50, - 0x64, 0x8d, 0xc7, 0xba, 0xc0, 0xb8, 0x2c, 0x2e, 0x0c, 0xf5, 0xf1, 0xbb, 0xba, 0xa7, 0xbe, 0x0f, - 0xd5, 0x78, 0x9e, 0x8f, 0xed, 0x7f, 0xd7, 0x1e, 0x59, 0x06, 0xf7, 0x91, 0xc7, 0xa2, 0x82, 0x5e, - 0x81, 0xfc, 0x89, 0x2d, 0x20, 0x72, 0xf6, 0x1a, 0x3d, 0xb0, 0x29, 0x89, 0xe4, 0x42, 0x84, 0xb6, - 0x6a, 0x02, 0x9a, 0x4e, 0x97, 0x24, 0x74, 0xf1, 0x56, 0xbc, 0x8b, 0xa7, 0x12, 0x13, 0x2f, 0xb3, - 0xbb, 0xfa, 0x04, 0xf2, 0x1c, 0x5d, 0x19, 0x52, 0xf2, 0xe4, 0xa0, 0x64, 0xf2, 0xac, 0x8c, 0xde, - 0x07, 0xd0, 0x29, 0x75, 0xcd, 0xc3, 0x51, 0xd8, 0xc1, 0xda, 0x6c, 0x74, 0xde, 0xf4, 0xf5, 0xb6, - 0xae, 0x48, 0x98, 0xbe, 0x18, 0x9a, 0x46, 0xa0, 0x3a, 0xe2, 0x50, 0xdd, 0x85, 0x6a, 0xdc, 0x36, - 0x9a, 0xa6, 0x5f, 0x9e, 0x91, 0xa6, 0x0f, 0xd8, 0x62, 0xc0, 0x35, 0xb3, 0x22, 0x11, 0xcc, 0x2b, - 0xea, 0xa7, 0x69, 0x28, 0xb1, 0x4d, 0xcf, 0xcf, 0x6d, 0x42, 0x0e, 0x32, 0x34, 0xcd, 0x44, 0x33, - 0x6e, 0x22, 0xa9, 0x99, 0x0d, 0x52, 0xa5, 0xef, 0x04, 0xc8, 0x94, 0x5b, 0x34, 0x9a, 0xf7, 0xd3, - 0x65, 0x12, 0x8d, 0x7f, 0x29, 0x3f, 0x86, 0x9d, 0x3b, 0xf4, 0x3a, 0x14, 0xf4, 0x5e, 0x90, 0x4c, - 0xaa, 0xce, 0x70, 0xe7, 0xab, 0xae, 0x77, 0xc7, 0x9b, 0x5c, 0x13, 0x4b, 0x0b, 0xf9, 0x69, 0x99, - 0x20, 0xdf, 0xfa, 0x36, 0xf3, 0x2b, 0x74, 0xe2, 0xe0, 0x54, 0x05, 0x38, 0xd8, 0xbd, 0xdf, 0xd9, - 0xde, 0xb9, 0xbd, 0xd3, 0xda, 0x96, 0x00, 0xb5, 0xbd, 0xdd, 0xda, 0x56, 0x32, 0x4c, 0x0f, 0xb7, - 0xee, 0x77, 0x1e, 0xb4, 0xb6, 0x95, 0xac, 0xfa, 0x06, 0x94, 0x83, 0xa3, 0xc5, 0x82, 0x35, 0xdd, - 0x30, 0x5c, 0xe2, 0x79, 0x72, 0xd6, 0xfd, 0x2a, 0x4f, 0xb6, 0xdb, 0x1f, 0xcb, 0x6c, 0x63, 0x16, - 0x8b, 0x8a, 0x6a, 0x40, 0x6d, 0x82, 0x31, 0xa0, 0x37, 0xa0, 0xe8, 0x8c, 0x0e, 0x35, 0x7f, 0xe1, - 0x26, 0x10, 0xc4, 0x27, 0xee, 0xa3, 0xc3, 0x81, 0xd9, 0xbb, 0x4b, 0x4e, 0xfd, 0x69, 0x72, 0x46, - 0x87, 0x77, 0xc5, 0xfa, 0x8a, 0x5e, 0x32, 0xd1, 0x5e, 0x7e, 0x9a, 0x86, 0x92, 0xbf, 0x5f, 0xd1, - 0xff, 0x44, 0xd1, 0xc2, 0xff, 0x07, 0x93, 0x48, 0x63, 0xa4, 0xff, 0x08, 0x58, 0xdc, 0x80, 0x15, - 0xcf, 0x3c, 0xb6, 0x88, 0xa1, 0x85, 0xf1, 0x22, 0xef, 0xae, 0x84, 0x6b, 0xa2, 0xe1, 0x9e, 0x1f, - 0x2c, 0xde, 0xc9, 0x95, 0xb2, 0x4a, 0xee, 0x4e, 0xae, 0x94, 0x53, 0xf2, 0xea, 0xaf, 0xd3, 0xa0, - 0x4c, 0x1e, 0x9e, 0x7f, 0xe7, 0xc7, 0xb0, 0xbb, 0x8a, 0x1d, 0x52, 0x8d, 0xb0, 0x8f, 0x08, 0x22, - 0xe6, 0x65, 0x5c, 0x61, 0xd2, 0x96, 0x2f, 0x54, 0xff, 0x91, 0x86, 0x92, 0x0f, 0xb5, 0xe8, 0xa5, - 0xc8, 0x31, 0xae, 0xce, 0x48, 0x04, 0xfa, 0x8a, 0x61, 0x92, 0x3f, 0x3e, 0xa4, 0xcc, 0xf9, 0x87, - 0x94, 0xf4, 0xb7, 0xc6, 0xff, 0x6d, 0x96, 0x3b, 0xf7, 0x6f, 0xb3, 0x17, 0x00, 0x51, 0x9b, 0xea, - 0x03, 0xed, 0xc4, 0xa6, 0xa6, 0x75, 0xac, 0x89, 0x1d, 0x22, 0x18, 0xb8, 0xc2, 0x5b, 0x1e, 0xf0, - 0x86, 0xbd, 0x60, 0xb3, 0x04, 0x5c, 0xea, 0xbc, 0x39, 0xfb, 0x4b, 0x50, 0x90, 0x74, 0x41, 0x24, - 0xed, 0x65, 0x2d, 0xc8, 0x9c, 0xe7, 0x22, 0x99, 0xf3, 0x06, 0x94, 0x86, 0x84, 0xea, 0x9c, 0x50, - 0x8a, 0x34, 0x43, 0x50, 0xbf, 0xf1, 0x1a, 0x2c, 0x45, 0x7e, 0x9f, 0x30, 0x20, 0xdb, 0x6d, 0xbd, - 0xa7, 0xa4, 0x1a, 0xc5, 0x4f, 0x3f, 0xbf, 0x9a, 0xdd, 0x25, 0x1f, 0xb3, 0x83, 0x86, 0x5b, 0xcd, - 0x76, 0xab, 0x79, 0x57, 0x49, 0x37, 0x96, 0x3e, 0xfd, 0xfc, 0x6a, 0x11, 0x13, 0x9e, 0x3f, 0xbc, - 0xd1, 0x86, 0xe5, 0xe8, 0xaa, 0xc4, 0x0f, 0x35, 0x82, 0xea, 0xf6, 0xc1, 0xde, 0xbd, 0x9d, 0xe6, - 0x66, 0xb7, 0xa5, 0x3d, 0xe8, 0x74, 0x5b, 0x4a, 0x1a, 0x5d, 0x86, 0x0b, 0xf7, 0x76, 0xde, 0x6d, - 0x77, 0xb5, 0xe6, 0xbd, 0x9d, 0xd6, 0x6e, 0x57, 0xdb, 0xec, 0x76, 0x37, 0x9b, 0x77, 0x95, 0xcc, - 0xc6, 0x5f, 0x01, 0x6a, 0x9b, 0x5b, 0xcd, 0x1d, 0xc6, 0x96, 0xcc, 0x9e, 0x2e, 0xf3, 0xb3, 0x39, - 0x9e, 0xe5, 0x39, 0xf3, 0x35, 0x48, 0xe3, 0xec, 0xf4, 0x34, 0xba, 0x0d, 0x79, 0x9e, 0x00, 0x42, - 0x67, 0x3f, 0x0f, 0x69, 0xcc, 0xc9, 0x57, 0xb3, 0x8f, 0xe1, 0xa7, 0xe8, 0xcc, 0xf7, 0x22, 0x8d, - 0xb3, 0xd3, 0xd7, 0x08, 0x43, 0x39, 0xcc, 0xe0, 0xcc, 0x7f, 0x3f, 0xd2, 0x58, 0x20, 0xa5, 0xcd, - 0x7c, 0x86, 0x61, 0xe4, 0xfc, 0xf7, 0x14, 0x8d, 0x05, 0xee, 0x03, 0x74, 0x0f, 0x8a, 0x7e, 0xe4, - 0x3f, 0xef, 0x85, 0x47, 0x63, 0x6e, 0xba, 0x99, 0x2d, 0x81, 0xc8, 0xd0, 0x9c, 0xfd, 0x5c, 0xa5, - 0x31, 0x27, 0x77, 0x8e, 0x76, 0xa0, 0x20, 0x63, 0xa3, 0x39, 0xaf, 0x36, 0x1a, 0xf3, 0xd2, 0xc7, - 0x6c, 0xd2, 0xc2, 0xd4, 0xd7, 0xfc, 0x47, 0x38, 0x8d, 0x05, 0x7e, 0x0b, 0xa0, 0x03, 0x80, 0x48, - 0x3e, 0x66, 0x81, 0xd7, 0x35, 0x8d, 0x45, 0xd2, 0xfd, 0xa8, 0x03, 0xa5, 0x20, 0x3c, 0x9e, 0xfb, - 0xd6, 0xa5, 0x31, 0x3f, 0xef, 0x8e, 0x1e, 0x42, 0x25, 0x1e, 0x17, 0x2e, 0xf6, 0x82, 0xa5, 0xb1, - 0x60, 0x42, 0x9d, 0xf9, 0x8f, 0x07, 0x89, 0x8b, 0xbd, 0x68, 0x69, 0x2c, 0x98, 0x5f, 0x47, 0x1f, - 0xc2, 0xca, 0x74, 0x10, 0xb7, 0xf8, 0x03, 0x97, 0xc6, 0x39, 0x32, 0xee, 0x68, 0x08, 0x68, 0x46, - 0xf0, 0x77, 0x8e, 0xf7, 0x2e, 0x8d, 0xf3, 0x24, 0xe0, 0x91, 0x01, 0xb5, 0xc9, 0x88, 0x6a, 0xd1, - 0xf7, 0x2f, 0x8d, 0x85, 0x93, 0xf1, 0x5b, 0xad, 0x2f, 0xbf, 0x59, 0x4d, 0x7f, 0xf5, 0xcd, 0x6a, - 0xfa, 0xcf, 0xdf, 0xac, 0xa6, 0x3f, 0xfb, 0x76, 0x35, 0xf5, 0xd5, 0xb7, 0xab, 0xa9, 0x3f, 0x7c, - 0xbb, 0x9a, 0xfa, 0xd1, 0xf3, 0xc7, 0x26, 0xed, 0x8f, 0x0e, 0xd7, 0x7b, 0xf6, 0xf0, 0x66, 0xf4, - 0xc9, 0xdf, 0xac, 0x67, 0x88, 0x87, 0x05, 0x7e, 0x1d, 0xbe, 0xfc, 0xaf, 0x00, 0x00, 0x00, 0xff, - 0xff, 0xf8, 0x2c, 0x9d, 0xd4, 0xa6, 0x28, 0x00, 0x00, + 0xf3, 0xe7, 0xd2, 0x17, 0xe6, 0xd5, 0x7d, 0xb8, 0x9c, 0xc0, 0xf3, 0xd0, 0x2d, 0x00, 0x3a, 0xd6, + 0x5c, 0xd2, 0xb3, 0x5d, 0x23, 0x79, 0x8f, 0x75, 0xc7, 0x98, 0x6b, 0xe0, 0x32, 0x95, 0x25, 0x4f, + 0xfd, 0x67, 0x1a, 0x6a, 0x13, 0xa7, 0x0e, 0x6d, 0x40, 0x5e, 0xc4, 0x14, 0x49, 0x7f, 0x85, 0x38, + 0x68, 0xc8, 0x23, 0x2a, 0x54, 0xd1, 0x9b, 0x50, 0x22, 0x32, 0xaa, 0x9e, 0x75, 0xba, 0x45, 0x96, + 0xc0, 0x8f, 0xbb, 0xa5, 0x69, 0x60, 0x81, 0xde, 0x86, 0x72, 0x00, 0x1f, 0x32, 0x90, 0x7d, 0x6a, + 0xda, 0x3c, 0x00, 0x1e, 0x69, 0x1f, 0xda, 0xa0, 0xd7, 0x42, 0x06, 0x99, 0x9b, 0x8e, 0x64, 0xa4, + 0xb9, 0x50, 0x90, 0xc6, 0xbe, 0xbe, 0xda, 0x84, 0xa5, 0xc8, 0x78, 0xd0, 0x13, 0x50, 0x66, 0x71, + 0xbf, 0x08, 0xfa, 0x45, 0x32, 0xaa, 0x34, 0xd4, 0x45, 0xc8, 0x8f, 0x2e, 0x43, 0x91, 0x35, 0x1e, + 0xeb, 0x02, 0xc2, 0xb2, 0xb8, 0x30, 0xd4, 0xc7, 0xef, 0xea, 0x9e, 0xfa, 0x3e, 0x54, 0xe3, 0x69, + 0x3c, 0xb6, 0xbd, 0x5d, 0x7b, 0x64, 0x19, 0xdc, 0x47, 0x1e, 0x8b, 0x0a, 0x7a, 0x05, 0xf2, 0x27, + 0xb6, 0x40, 0xc0, 0xd9, 0x6b, 0xf4, 0xc0, 0xa6, 0x24, 0x92, 0xea, 0x10, 0xda, 0xaa, 0x09, 0x68, + 0x3a, 0x1b, 0x92, 0xd0, 0xc5, 0x5b, 0xf1, 0x2e, 0x9e, 0x4a, 0xcc, 0xab, 0xcc, 0xee, 0xea, 0x13, + 0xc8, 0x73, 0xf0, 0x64, 0x40, 0xc8, 0x73, 0x7f, 0x92, 0xa8, 0xb3, 0x32, 0x7a, 0x1f, 0x40, 0xa7, + 0xd4, 0x35, 0x0f, 0x47, 0x61, 0x07, 0x6b, 0xb3, 0xc1, 0x77, 0xd3, 0xd7, 0xdb, 0xba, 0x22, 0x51, + 0xf8, 0x62, 0x68, 0x1a, 0x41, 0xe2, 0x88, 0x43, 0x75, 0x17, 0xaa, 0x71, 0xdb, 0x68, 0x16, 0x7e, + 0x79, 0x46, 0x16, 0x3e, 0x20, 0x83, 0x01, 0x95, 0xcc, 0x8a, 0x3c, 0x2f, 0xaf, 0xa8, 0x9f, 0xa6, + 0xa1, 0xc4, 0x36, 0x3d, 0x3f, 0x96, 0x09, 0x29, 0xc6, 0xd0, 0x34, 0x13, 0x4d, 0xa8, 0x89, 0x9c, + 0x65, 0x36, 0xc8, 0x84, 0xbe, 0x13, 0x00, 0x4f, 0x6e, 0xd1, 0x60, 0xdd, 0xcf, 0x86, 0x49, 0xb0, + 0xfd, 0xa5, 0xfc, 0x18, 0x76, 0xee, 0xd0, 0xeb, 0x50, 0xd0, 0x7b, 0x41, 0xae, 0xa8, 0x3a, 0xc3, + 0x9d, 0xaf, 0xba, 0xde, 0x1d, 0x6f, 0x72, 0x4d, 0x2c, 0x2d, 0xe4, 0xa7, 0x65, 0x82, 0x74, 0xea, + 0xdb, 0xcc, 0xaf, 0xd0, 0x89, 0x63, 0x4f, 0x15, 0xe0, 0x60, 0xf7, 0x7e, 0x67, 0x7b, 0xe7, 0xf6, + 0x4e, 0x6b, 0x5b, 0xe2, 0xcf, 0xf6, 0x76, 0x6b, 0x5b, 0xc9, 0x30, 0x3d, 0xdc, 0xba, 0xdf, 0x79, + 0xd0, 0xda, 0x56, 0xb2, 0xea, 0x1b, 0x50, 0x0e, 0x8e, 0x16, 0x8b, 0xc5, 0x74, 0xc3, 0x70, 0x89, + 0xe7, 0xc9, 0x59, 0xf7, 0xab, 0x3c, 0x97, 0x6e, 0x7f, 0x2c, 0x93, 0x89, 0x59, 0x2c, 0x2a, 0xaa, + 0x01, 0xb5, 0x09, 0x42, 0x80, 0xde, 0x80, 0xa2, 0x33, 0x3a, 0xd4, 0xfc, 0x85, 0x9b, 0x40, 0x10, + 0x9f, 0x97, 0x8f, 0x0e, 0x07, 0x66, 0xef, 0x2e, 0x39, 0xf5, 0xa7, 0xc9, 0x19, 0x1d, 0xde, 0x15, + 0xeb, 0x2b, 0x7a, 0xc9, 0x44, 0x7b, 0xf9, 0x69, 0x1a, 0x4a, 0xfe, 0x7e, 0x45, 0xff, 0x13, 0x45, + 0x0b, 0xff, 0x17, 0x4b, 0x22, 0x4b, 0x91, 0xfe, 0x23, 0x60, 0x71, 0x03, 0x56, 0x3c, 0xf3, 0xd8, + 0x22, 0x86, 0x16, 0x86, 0x83, 0xbc, 0xbb, 0x12, 0xae, 0x89, 0x86, 0x7b, 0x7e, 0x2c, 0x78, 0x27, + 0x57, 0xca, 0x2a, 0xb9, 0x3b, 0xb9, 0x52, 0x4e, 0xc9, 0xab, 0xbf, 0x4e, 0x83, 0x32, 0x79, 0x78, + 0xfe, 0x9d, 0x1f, 0xc3, 0xae, 0x22, 0x76, 0x48, 0x35, 0xc2, 0x3e, 0x22, 0x08, 0x88, 0x97, 0x71, + 0x85, 0x49, 0x5b, 0xbe, 0x50, 0xfd, 0x47, 0x1a, 0x4a, 0x3e, 0xd4, 0xa2, 0x97, 0x22, 0xc7, 0xb8, + 0x3a, 0x23, 0xcf, 0xe7, 0x2b, 0x86, 0x39, 0xfc, 0xf8, 0x90, 0x32, 0xe7, 0x1f, 0x52, 0xd2, 0xcf, + 0x18, 0xff, 0xaf, 0x58, 0xee, 0xdc, 0x7f, 0xc5, 0x5e, 0x00, 0x44, 0x6d, 0xaa, 0x0f, 0xb4, 0x13, + 0x9b, 0x9a, 0xd6, 0xb1, 0x26, 0x76, 0x88, 0x20, 0xd8, 0x0a, 0x6f, 0x79, 0xc0, 0x1b, 0xf6, 0x82, + 0xcd, 0x12, 0x50, 0xa5, 0xf3, 0xa6, 0xe4, 0x2f, 0x41, 0x41, 0xb2, 0x01, 0x91, 0x93, 0x97, 0xb5, + 0x20, 0x31, 0x9e, 0x8b, 0x24, 0xc6, 0x1b, 0x50, 0x1a, 0x12, 0xaa, 0x73, 0xbe, 0x28, 0xb2, 0x08, + 0x41, 0xfd, 0xc6, 0x6b, 0xb0, 0x14, 0xf9, 0x3b, 0xc2, 0x80, 0x6c, 0xb7, 0xf5, 0x9e, 0x92, 0x6a, + 0x14, 0x3f, 0xfd, 0xfc, 0x6a, 0x76, 0x97, 0x7c, 0xcc, 0x0e, 0x1a, 0x6e, 0x35, 0xdb, 0xad, 0xe6, + 0x5d, 0x25, 0xdd, 0x58, 0xfa, 0xf4, 0xf3, 0xab, 0x45, 0x4c, 0x78, 0x7a, 0xf0, 0x46, 0x1b, 0x96, + 0xa3, 0xab, 0x12, 0x3f, 0xd4, 0x08, 0xaa, 0xdb, 0x07, 0x7b, 0xf7, 0x76, 0x9a, 0x9b, 0xdd, 0x96, + 0xf6, 0xa0, 0xd3, 0x6d, 0x29, 0x69, 0x74, 0x19, 0x2e, 0xdc, 0xdb, 0x79, 0xb7, 0xdd, 0xd5, 0x9a, + 0xf7, 0x76, 0x5a, 0xbb, 0x5d, 0x6d, 0xb3, 0xdb, 0xdd, 0x6c, 0xde, 0x55, 0x32, 0x1b, 0x7f, 0x05, + 0xa8, 0x6d, 0x6e, 0x35, 0x77, 0x18, 0x19, 0x32, 0x7b, 0xba, 0x4c, 0xbf, 0xe6, 0x78, 0x12, 0xe7, + 0xcc, 0xc7, 0x1e, 0x8d, 0xb3, 0xb3, 0xcf, 0xe8, 0x36, 0xe4, 0x79, 0x7e, 0x07, 0x9d, 0xfd, 0xfa, + 0xa3, 0x31, 0x27, 0x1d, 0xcd, 0x3e, 0x86, 0x9f, 0xa2, 0x33, 0x9f, 0x83, 0x34, 0xce, 0xce, 0x4e, + 0x23, 0x0c, 0xe5, 0x30, 0x41, 0x33, 0xff, 0x79, 0x48, 0x63, 0x81, 0x8c, 0x35, 0xf3, 0x19, 0x46, + 0x89, 0xf3, 0x9f, 0x4b, 0x34, 0x16, 0xb8, 0x0f, 0xd0, 0x3d, 0x28, 0xfa, 0x81, 0xfd, 0xbc, 0x07, + 0x1c, 0x8d, 0xb9, 0xd9, 0x64, 0xb6, 0x04, 0x22, 0x01, 0x73, 0xf6, 0x6b, 0x94, 0xc6, 0x9c, 0xd4, + 0x38, 0xda, 0x81, 0x82, 0x0c, 0x7d, 0xe6, 0x3c, 0xca, 0x68, 0xcc, 0xcb, 0x0e, 0xb3, 0x49, 0x0b, + 0x33, 0x5b, 0xf3, 0xdf, 0xd8, 0x34, 0x16, 0xc8, 0xfa, 0xa3, 0x03, 0x80, 0x48, 0xba, 0x65, 0x81, + 0xc7, 0x33, 0x8d, 0x45, 0xb2, 0xf9, 0xa8, 0x03, 0xa5, 0x20, 0xfa, 0x9d, 0xfb, 0x94, 0xa5, 0x31, + 0x3f, 0xad, 0x8e, 0x1e, 0x42, 0x25, 0x1e, 0xf6, 0x2d, 0xf6, 0x40, 0xa5, 0xb1, 0x60, 0xbe, 0x9c, + 0xf9, 0x8f, 0xc7, 0x80, 0x8b, 0x3d, 0x58, 0x69, 0x2c, 0x98, 0x3e, 0x47, 0x1f, 0xc2, 0xca, 0x74, + 0x8c, 0xb6, 0xf8, 0xfb, 0x95, 0xc6, 0x39, 0x12, 0xea, 0x68, 0x08, 0x68, 0x46, 0x6c, 0x77, 0x8e, + 0xe7, 0x2c, 0x8d, 0xf3, 0xe4, 0xd7, 0x91, 0x01, 0xb5, 0xc9, 0x80, 0x69, 0xd1, 0xe7, 0x2d, 0x8d, + 0x85, 0x73, 0xed, 0x5b, 0xad, 0x2f, 0xbf, 0x59, 0x4d, 0x7f, 0xf5, 0xcd, 0x6a, 0xfa, 0xcf, 0xdf, + 0xac, 0xa6, 0x3f, 0xfb, 0x76, 0x35, 0xf5, 0xd5, 0xb7, 0xab, 0xa9, 0x3f, 0x7c, 0xbb, 0x9a, 0xfa, + 0xd1, 0xf3, 0xc7, 0x26, 0xed, 0x8f, 0x0e, 0xd7, 0x7b, 0xf6, 0xf0, 0x66, 0xf4, 0x45, 0xdf, 0xac, + 0x57, 0x86, 0x87, 0x05, 0x7e, 0x1d, 0xbe, 0xfc, 0xaf, 0x00, 0x00, 0x00, 0xff, 0xff, 0x38, 0xf1, + 0x86, 0x0d, 0x85, 0x28, 0x00, 0x00, } // Reference imports to suppress errors if they are not otherwise used. @@ -6413,19 +6404,9 @@ func (m *ResponsePrepareProposal) MarshalToSizedBuffer(dAtA []byte) (int, error) i = encodeVarintTypes(dAtA, i, uint64(size)) } i-- - dAtA[i] = 0x12 + dAtA[i] = 0xa } } - if m.ModifiedTx { - i-- - if m.ModifiedTx { - dAtA[i] = 1 - } else { - dAtA[i] = 0 - } - i-- - dAtA[i] = 0x8 - } return len(dAtA) - i, nil } @@ -8086,9 +8067,6 @@ func (m *ResponsePrepareProposal) Size() (n int) { } var l int _ = l - if m.ModifiedTx { - n += 2 - } if len(m.TxRecords) > 0 { for _, e := range m.TxRecords { l = e.Size() @@ -13657,26 +13635,6 @@ func (m *ResponsePrepareProposal) Unmarshal(dAtA []byte) error { } switch fieldNum { case 1: - if wireType != 0 { - return fmt.Errorf("proto: wrong wireType = %d for field ModifiedTx", wireType) - } - var v int - for shift := uint(0); ; shift += 7 { - if shift >= 64 { - return ErrIntOverflowTypes - } - if iNdEx >= l { - return io.ErrUnexpectedEOF - } - b := dAtA[iNdEx] - iNdEx++ - v |= int(b&0x7F) << shift - if b < 0x80 { - break - } - } - m.ModifiedTx = bool(v != 0) - case 2: if wireType != 2 { return fmt.Errorf("proto: wrong wireType = %d for field TxRecords", wireType) } diff --git a/consensus/mempool_test.go b/consensus/mempool_test.go index 183fedefd..09cb4ec8e 100644 --- a/consensus/mempool_test.go +++ b/consensus/mempool_test.go @@ -259,5 +259,18 @@ func (app *CounterApplication) Commit() abci.ResponseCommit { func (app *CounterApplication) PrepareProposal( req abci.RequestPrepareProposal) abci.ResponsePrepareProposal { - return abci.ResponsePrepareProposal{} + + trs := make([]*abci.TxRecord, 0, len(req.Txs)) + var totalBytes int64 + for _, tx := range req.Txs { + totalBytes += int64(len(tx)) + if totalBytes > req.MaxTxBytes { + break + } + trs = append(trs, &abci.TxRecord{ + Action: abci.TxRecord_UNMODIFIED, + Tx: tx, + }) + } + return abci.ResponsePrepareProposal{TxRecords: trs} } diff --git a/consensus/state.go b/consensus/state.go index 8e662ea45..bce301247 100644 --- a/consensus/state.go +++ b/consensus/state.go @@ -1224,7 +1224,11 @@ func (cs *State) createProposalBlock() (*types.Block, error) { proposerAddr := cs.privValidatorPubKey.Address() - return cs.blockExec.CreateProposalBlock(cs.Height, cs.state, commit, proposerAddr, cs.LastCommit.GetVotes()) + ret, err := cs.blockExec.CreateProposalBlock(cs.Height, cs.state, commit, proposerAddr, cs.LastCommit.GetVotes()) + if err != nil { + panic(err) + } + return ret, nil } // Enter: `timeoutPropose` after entering Propose. diff --git a/proto/tendermint/abci/types.proto b/proto/tendermint/abci/types.proto index 90bd19628..04e174239 100644 --- a/proto/tendermint/abci/types.proto +++ b/proto/tendermint/abci/types.proto @@ -284,8 +284,7 @@ message ResponseApplySnapshotChunk { } message ResponsePrepareProposal { - bool modified_tx = 1; - repeated TxRecord tx_records = 2; + repeated TxRecord tx_records = 1; } //---------------------------------------- diff --git a/proxy/mocks/client_creator.go b/proxy/mocks/client_creator.go index 759b17957..fa0bf3605 100644 --- a/proxy/mocks/client_creator.go +++ b/proxy/mocks/client_creator.go @@ -35,13 +35,14 @@ func (_m *ClientCreator) NewABCIClient() (abcicli.Client, error) { return r0, r1 } -type NewClientCreatorT interface { + +type mockConstructorTestingTNewClientCreator interface { mock.TestingT Cleanup(func()) } // NewClientCreator creates a new instance of ClientCreator. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. -func NewClientCreator(t NewClientCreatorT) *ClientCreator { +func NewClientCreator(t mockConstructorTestingTNewClientCreator) *ClientCreator { mock := &ClientCreator{} mock.Mock.Test(t) diff --git a/state/execution.go b/state/execution.go index 0659bbf61..f9875db81 100644 --- a/state/execution.go +++ b/state/execution.go @@ -129,14 +129,10 @@ func (blockExec *BlockExecutor) CreateProposalBlock( // transaction causing an error. // // Also, the App can simply skip any transaction that could cause any kind of trouble. - // Either way, we can not recover in a meaningful way, unless we skip proposing - // this block, repair what caused the error and try again. Hence, we panic on - // purpose for now. - panic(err) - } - - if !rpp.ModifiedTx { - return block, nil + // Either way, we cannot recover in a meaningful way, unless we skip proposing + // this block, repair what caused the error and try again. Hence, we return an + // error for now (the production code calling this function is expected to panic). + return nil, err } txrSet := types.NewTxRecordSet(rpp.TxRecords) @@ -149,11 +145,6 @@ func (blockExec *BlockExecutor) CreateProposalBlock( blockExec.logger.Debug("error removing transaction from the mempool", "error", err, "tx hash", rtx.Hash()) } } - for _, atx := range txrSet.AddedTxs() { - if err := blockExec.mempool.CheckTx(atx, nil, mempool.TxInfo{}); err != nil { - blockExec.logger.Error("error adding tx to the mempool", "error", err, "tx hash", atx.Hash()) - } - } itxs := txrSet.IncludedTxs() return state.MakeBlock(height, itxs, commit, evidence, proposerAddr), nil } diff --git a/state/execution_test.go b/state/execution_test.go index 5cfea42d5..00bb9b2ed 100644 --- a/state/execution_test.go +++ b/state/execution_test.go @@ -2,6 +2,7 @@ package state_test import ( "context" + "errors" "testing" "time" @@ -9,6 +10,7 @@ import ( "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" + abciclientmocks "github.com/tendermint/tendermint/abci/client/mocks" abci "github.com/tendermint/tendermint/abci/types" abcimocks "github.com/tendermint/tendermint/abci/types/mocks" "github.com/tendermint/tendermint/crypto" @@ -20,6 +22,7 @@ import ( tmproto "github.com/tendermint/tendermint/proto/tendermint/types" tmversion "github.com/tendermint/tendermint/proto/tendermint/version" "github.com/tendermint/tendermint/proxy" + pmocks "github.com/tendermint/tendermint/proxy/mocks" sm "github.com/tendermint/tendermint/state" "github.com/tendermint/tendermint/state/mocks" sf "github.com/tendermint/tendermint/state/test/factory" @@ -496,7 +499,7 @@ func TestEndBlockValidatorUpdatesResultingInEmptySet(t *testing.T) { func TestEmptyPrepareProposal(t *testing.T) { const height = 2 - app := &testApp{} + app := abcimocks.NewBaseMock() cc := proxy.NewLocalClientCreator(app) proxyApp := proxy.NewAppConns(cc) err := proxyApp.Start() @@ -532,6 +535,57 @@ func TestEmptyPrepareProposal(t *testing.T) { require.NoError(t, err) } +// TestPrepareProposalErrorOnNonExistingRemoved tests that the block creation logic returns +// an error if the ResponsePrepareProposal returned from the application marks +// a transaction as REMOVED that was not present in the original proposal. +func TestPrepareProposalErrorOnNonExistingRemoved(t *testing.T) { + const height = 2 + + state, stateDB, privVals := makeState(1, height) + stateStore := sm.NewStore(stateDB) + + evpool := &mocks.EvidencePool{} + evpool.On("PendingEvidence", mock.Anything).Return([]types.Evidence{}, int64(0)) + + mp := &mpmocks.Mempool{} + mp.On("ReapMaxBytesMaxGas", mock.Anything, mock.Anything).Return(types.Txs{}) + + // create an invalid ResponsePrepareProposal + rpp := abci.ResponsePrepareProposal{ + TxRecords: []*abci.TxRecord{ + { + Action: abci.TxRecord_REMOVED, + Tx: []byte("new tx"), + }, + }, + } + + app := abcimocks.NewBaseMock() + app.On("PrepareProposal", mock.Anything).Return(rpp) + + cc := proxy.NewLocalClientCreator(app) + proxyApp := proxy.NewAppConns(cc) + err := proxyApp.Start() + require.NoError(t, err) + defer proxyApp.Stop() //nolint:errcheck // ignore for tests + + blockExec := sm.NewBlockExecutor( + stateStore, + log.TestingLogger(), + proxyApp.Consensus(), + mp, + evpool, + ) + pa, _ := state.Validators.GetByIndex(0) + commit, err := makeValidCommit(height, types.BlockID{}, state.Validators, privVals) + require.NoError(t, err) + block, err := blockExec.CreateProposalBlock(height, state, commit, pa, nil) + require.Nil(t, block) + require.ErrorContains(t, err, "new transaction incorrectly marked as removed") + + mp.AssertExpectations(t) +} + // TestPrepareProposalRemoveTxs tests that any transactions marked as REMOVED // are not included in the block produced by CreateProposalBlock. The test also // ensures that any transactions removed are also removed from the mempool. @@ -544,7 +598,7 @@ func TestPrepareProposalRemoveTxs(t *testing.T) { evpool := &mocks.EvidencePool{} evpool.On("PendingEvidence", mock.Anything).Return([]types.Evidence{}, int64(0)) - txs := factory.MakeTenTxs(height) + txs := factory.MakeNTxs(height, 10) mp := &mpmocks.Mempool{} mp.On("ReapMaxBytesMaxGas", mock.Anything, mock.Anything).Return(types.Txs(txs)) @@ -555,9 +609,8 @@ func TestPrepareProposalRemoveTxs(t *testing.T) { app := abcimocks.NewBaseMock() app.On("PrepareProposal", mock.Anything).Return(abci.ResponsePrepareProposal{ - ModifiedTx: true, - TxRecords: trs, - }, nil) + TxRecords: trs, + }) cc := proxy.NewLocalClientCreator(app) proxyApp := proxy.NewAppConns(cc) err := proxyApp.Start() @@ -587,8 +640,7 @@ func TestPrepareProposalRemoveTxs(t *testing.T) { } // TestPrepareProposalAddedTxsIncluded tests that any transactions marked as ADDED -// in the prepare proposal response are included in the block. The test also -// ensures that any transactions added are also checked into the mempool. +// in the prepare proposal response are included in the block. func TestPrepareProposalAddedTxsIncluded(t *testing.T) { const height = 2 @@ -598,10 +650,9 @@ func TestPrepareProposalAddedTxsIncluded(t *testing.T) { evpool := &mocks.EvidencePool{} evpool.On("PendingEvidence", mock.Anything).Return([]types.Evidence{}, int64(0)) - txs := factory.MakeTenTxs(height) + txs := factory.MakeNTxs(height, 10) mp := &mpmocks.Mempool{} mp.On("ReapMaxBytesMaxGas", mock.Anything, mock.Anything).Return(types.Txs(txs[2:])) - mp.On("CheckTx", mock.Anything, mock.Anything, mock.Anything).Return(nil).Twice() trs := txsToTxRecords(types.Txs(txs)) trs[0].Action = abci.TxRecord_ADDED @@ -609,13 +660,13 @@ func TestPrepareProposalAddedTxsIncluded(t *testing.T) { app := abcimocks.NewBaseMock() app.On("PrepareProposal", mock.Anything).Return(abci.ResponsePrepareProposal{ - ModifiedTx: true, - TxRecords: trs, - }, nil) + TxRecords: trs, + }) cc := proxy.NewLocalClientCreator(app) proxyApp := proxy.NewAppConns(cc) err := proxyApp.Start() require.NoError(t, err) + defer proxyApp.Stop() //nolint:errcheck // ignore for tests blockExec := sm.NewBlockExecutor( stateStore, @@ -634,8 +685,6 @@ func TestPrepareProposalAddedTxsIncluded(t *testing.T) { require.Equal(t, txs[1], block.Data.Txs[1]) mp.AssertExpectations(t) - mp.AssertCalled(t, "CheckTx", types.Tx(trs[0].Tx), mock.Anything, mock.Anything) - mp.AssertCalled(t, "CheckTx", types.Tx(trs[1].Tx), mock.Anything, mock.Anything) } // TestPrepareProposalReorderTxs tests that CreateBlock produces a block with transactions @@ -649,7 +698,7 @@ func TestPrepareProposalReorderTxs(t *testing.T) { evpool := &mocks.EvidencePool{} evpool.On("PendingEvidence", mock.Anything).Return([]types.Evidence{}, int64(0)) - txs := factory.MakeTenTxs(height) + txs := factory.MakeNTxs(height, 10) mp := &mpmocks.Mempool{} mp.On("ReapMaxBytesMaxGas", mock.Anything, mock.Anything).Return(types.Txs(txs)) @@ -659,14 +708,14 @@ func TestPrepareProposalReorderTxs(t *testing.T) { app := abcimocks.NewBaseMock() app.On("PrepareProposal", mock.Anything).Return(abci.ResponsePrepareProposal{ - ModifiedTx: true, - TxRecords: trs, - }, nil) + TxRecords: trs, + }) cc := proxy.NewLocalClientCreator(app) proxyApp := proxy.NewAppConns(cc) err := proxyApp.Start() require.NoError(t, err) + defer proxyApp.Stop() //nolint:errcheck // ignore for tests blockExec := sm.NewBlockExecutor( stateStore, @@ -688,6 +737,103 @@ func TestPrepareProposalReorderTxs(t *testing.T) { } +// TestPrepareProposalErrorOnTooManyTxs tests that the block creation logic returns +// an error if the ResponsePrepareProposal returned from the application is invalid. +func TestPrepareProposalErrorOnTooManyTxs(t *testing.T) { + const height = 2 + + state, stateDB, privVals := makeState(1, height) + // limit max block size + state.ConsensusParams.Block.MaxBytes = 60 * 1024 + stateStore := sm.NewStore(stateDB) + + evpool := &mocks.EvidencePool{} + evpool.On("PendingEvidence", mock.Anything).Return([]types.Evidence{}, int64(0)) + + const nValidators = 1 + var bytesPerTx int64 = 3 + maxDataBytes := types.MaxDataBytes(state.ConsensusParams.Block.MaxBytes, 0, nValidators) + txs := factory.MakeNTxs(height, maxDataBytes/bytesPerTx+2) // +2 so that tx don't fit + mp := &mpmocks.Mempool{} + mp.On("ReapMaxBytesMaxGas", mock.Anything, mock.Anything).Return(types.Txs(txs)) + + trs := txsToTxRecords(types.Txs(txs)) + + app := abcimocks.NewBaseMock() + app.On("PrepareProposal", mock.Anything).Return(abci.ResponsePrepareProposal{ + TxRecords: trs, + }) + + cc := proxy.NewLocalClientCreator(app) + proxyApp := proxy.NewAppConns(cc) + err := proxyApp.Start() + require.NoError(t, err) + defer proxyApp.Stop() //nolint:errcheck // ignore for tests + + blockExec := sm.NewBlockExecutor( + stateStore, + log.NewNopLogger(), + proxyApp.Consensus(), + mp, + evpool, + ) + pa, _ := state.Validators.GetByIndex(0) + commit, err := makeValidCommit(height, types.BlockID{}, state.Validators, privVals) + require.NoError(t, err) + + block, err := blockExec.CreateProposalBlock(height, state, commit, pa, nil) + require.Nil(t, block) + require.ErrorContains(t, err, "transaction data size exceeds maximum") + + mp.AssertExpectations(t) +} + +// TestPrepareProposalErrorOnPrepareProposalError tests when the client returns an error +// upon calling PrepareProposal on it. +func TestPrepareProposalErrorOnPrepareProposalError(t *testing.T) { + const height = 2 + + state, stateDB, privVals := makeState(1, height) + stateStore := sm.NewStore(stateDB) + + evpool := &mocks.EvidencePool{} + evpool.On("PendingEvidence", mock.Anything).Return([]types.Evidence{}, int64(0)) + + txs := factory.MakeNTxs(height, 10) + mp := &mpmocks.Mempool{} + mp.On("ReapMaxBytesMaxGas", mock.Anything, mock.Anything).Return(types.Txs(txs)) + + cm := &abciclientmocks.Client{} + cm.On("SetLogger", mock.Anything).Return() + cm.On("Start").Return(nil) + cm.On("Quit").Return(nil) + cm.On("PrepareProposalSync", mock.Anything).Return(nil, errors.New("an injected error")).Once() + cm.On("Stop").Return(nil) + cc := &pmocks.ClientCreator{} + cc.On("NewABCIClient").Return(cm, nil) + proxyApp := proxy.NewAppConns(cc) + err := proxyApp.Start() + require.NoError(t, err) + defer proxyApp.Stop() //nolint:errcheck // ignore for tests + + blockExec := sm.NewBlockExecutor( + stateStore, + log.NewNopLogger(), + proxyApp.Consensus(), + mp, + evpool, + ) + pa, _ := state.Validators.GetByIndex(0) + commit, err := makeValidCommit(height, types.BlockID{}, state.Validators, privVals) + require.NoError(t, err) + + block, err := blockExec.CreateProposalBlock(height, state, commit, pa, nil) + require.Nil(t, block) + require.ErrorContains(t, err, "an injected error") + + mp.AssertExpectations(t) +} + func makeBlockID(hash []byte, partSetSize uint32, partSetHash []byte) types.BlockID { var ( h = make([]byte, tmhash.Size) diff --git a/state/helpers_test.go b/state/helpers_test.go index 8a7311ed9..f9a60845f 100644 --- a/state/helpers_test.go +++ b/state/helpers_test.go @@ -56,7 +56,7 @@ func makeAndCommitGoodBlock( func makeAndApplyGoodBlock(state sm.State, height int64, lastCommit *types.Commit, proposerAddr []byte, blockExec *sm.BlockExecutor, evidence []types.Evidence) (sm.State, types.BlockID, error) { - block := state.MakeBlock(height, factory.MakeTenTxs(height), lastCommit, evidence, proposerAddr) + block := state.MakeBlock(height, factory.MakeNTxs(height, 10), lastCommit, evidence, proposerAddr) partSet, err := block.MakePartSet(types.BlockPartSizeBytes) if err != nil { return state, types.BlockID{}, err diff --git a/state/test/factory/block.go b/state/test/factory/block.go index 35e46787c..42d88d428 100644 --- a/state/test/factory/block.go +++ b/state/test/factory/block.go @@ -42,7 +42,7 @@ func MakeBlocks(n int, state *sm.State, privVal types.PrivValidator) ([]*types.B func MakeBlock(state sm.State, height int64, c *types.Commit) *types.Block { return state.MakeBlock( height, - factory.MakeTenTxs(state.LastBlockHeight), + factory.MakeNTxs(state.LastBlockHeight, 10), c, nil, state.Validators.GetProposer().Address, diff --git a/state/validation_test.go b/state/validation_test.go index f4de55f80..9bc2f3d6c 100644 --- a/state/validation_test.go +++ b/state/validation_test.go @@ -285,7 +285,7 @@ func TestValidateBlockEvidence(t *testing.T) { evidence = append(evidence, newEv) currentBytes += int64(len(newEv.Bytes())) } - block := state.MakeBlock(height, factory.MakeTenTxs(height), lastCommit, evidence, proposerAddr) + block := state.MakeBlock(height, factory.MakeNTxs(height, 10), lastCommit, evidence, proposerAddr) err := blockExec.ValidateBlock(state, block) if assert.Error(t, err) { diff --git a/test/e2e/app/app.go b/test/e2e/app/app.go index bf31a80b4..00dcc6bf9 100644 --- a/test/e2e/app/app.go +++ b/test/e2e/app/app.go @@ -260,7 +260,19 @@ func (app *Application) ApplySnapshotChunk(req abci.RequestApplySnapshotChunk) a func (app *Application) PrepareProposal( req abci.RequestPrepareProposal) abci.ResponsePrepareProposal { // None of the transactions are modified by this application. - return abci.ResponsePrepareProposal{ModifiedTx: false} + trs := make([]*abci.TxRecord, 0, len(req.Txs)) + var totalBytes int64 + for _, tx := range req.Txs { + totalBytes += int64(len(tx)) + if totalBytes > req.MaxTxBytes { + break + } + trs = append(trs, &abci.TxRecord{ + Action: abci.TxRecord_UNMODIFIED, + Tx: tx, + }) + } + return abci.ResponsePrepareProposal{TxRecords: trs} } func (app *Application) Rollback() error { diff --git a/test/factory/tx.go b/test/factory/tx.go index c97aeefc9..725f3c720 100644 --- a/test/factory/tx.go +++ b/test/factory/tx.go @@ -2,15 +2,10 @@ package factory import "github.com/tendermint/tendermint/types" -// MakeTxs is a helper function to generate mock transactions by given the block height -// and the transaction numbers. -func MakeTxs(height int64, num int) (txs []types.Tx) { - for i := 0; i < num; i++ { - txs = append(txs, types.Tx([]byte{byte(height), byte(i)})) +func MakeNTxs(height, n int64) []types.Tx { + txs := make([]types.Tx, n) + for i := range txs { + txs[i] = types.Tx([]byte{byte(height), byte(i / 256), byte(i % 256)}) } return txs } - -func MakeTenTxs(height int64) (txs []types.Tx) { - return MakeTxs(height, 10) -} diff --git a/types/tx.go b/types/tx.go index 64617a96f..ade9c94e4 100644 --- a/types/tx.go +++ b/types/tx.go @@ -172,11 +172,6 @@ func (t TxRecordSet) IncludedTxs() []Tx { return t.included } -// AddedTxs returns the transactions added by the application. -func (t TxRecordSet) AddedTxs() []Tx { - return t.added -} - // RemovedTxs returns the transactions marked for removal by the application. func (t TxRecordSet) RemovedTxs() []Tx { return t.removed @@ -206,13 +201,7 @@ func (t TxRecordSet) Validate(maxSizeBytes int64, otxs Txs) error { // Only the slices are copied, the transaction contents are shared. allCopy := sortedCopy(t.all) - var size int64 for i, cur := range allCopy { - size += int64(len(cur)) - if size > maxSizeBytes { - return fmt.Errorf("transaction data size %d exceeds maximum %d", size, maxSizeBytes) - } - // allCopy is sorted, so any duplicated data will be adjacent. if i+1 < len(allCopy) && bytes.Equal(cur, allCopy[i+1]) { return fmt.Errorf("found duplicate transaction with hash: %x", cur.Hash()) @@ -225,6 +214,14 @@ func (t TxRecordSet) Validate(maxSizeBytes int64, otxs Txs) error { removedCopy := sortedCopy(t.removed) unmodifiedCopy := sortedCopy(t.unmodified) + var size int64 + for _, cur := range append(unmodifiedCopy, addedCopy...) { + size += int64(len(cur)) + if size > maxSizeBytes { + return fmt.Errorf("transaction data size exceeds maximum %d", maxSizeBytes) + } + } + // make a defensive copy of otxs so that the order of // the caller's data is not altered. otxsCopy := sortedCopy(otxs) diff --git a/types/tx_test.go b/types/tx_test.go index efb435b4c..a84b0bf36 100644 --- a/types/tx_test.go +++ b/types/tx_test.go @@ -64,6 +64,25 @@ func TestValidateTxRecordSet(t *testing.T) { err := txrSet.Validate(9, []Tx{}) require.Error(t, err) }) + t.Run("should not error on removed transaction size exceeding max data size", func(t *testing.T) { + trs := []*abci.TxRecord{ + { + Action: abci.TxRecord_ADDED, + Tx: Tx([]byte{1, 2, 3, 4, 5}), + }, + { + Action: abci.TxRecord_ADDED, + Tx: Tx([]byte{6, 7, 8, 9}), + }, + { + Action: abci.TxRecord_REMOVED, + Tx: Tx([]byte{10}), + }, + } + txrSet := NewTxRecordSet(trs) + err := txrSet.Validate(9, []Tx{[]byte{10}}) + require.NoError(t, err) + }) t.Run("should error on duplicate transactions with the same action", func(t *testing.T) { trs := []*abci.TxRecord{ {