From 3d505edf31c27f9f7afae337699a27f60ca29b52 Mon Sep 17 00:00:00 2001 From: David Terpay <35130517+davidterpay@users.noreply.github.com> Date: Thu, 8 Jun 2023 16:23:20 -0400 Subject: [PATCH] feat(BB): Decoding Txs before ProcessLane + IgnoreList (#170) --- abci/proposals.go | 16 ++++++-- blockbuster/abci/abci.go | 15 ++++++-- blockbuster/abci/abci_test.go | 4 +- blockbuster/lane.go | 14 +++++-- blockbuster/lanes/auction/abci.go | 55 +++++++++------------------- blockbuster/lanes/base/abci.go | 20 +++------- blockbuster/lanes/base/lane.go | 17 +++++++-- blockbuster/lanes/terminator/lane.go | 4 +- blockbuster/utils/utils.go | 15 ++++++++ tests/app/app.go | 13 ++++++- tests/e2e/e2e_test.go | 22 +++++------ 11 files changed, 113 insertions(+), 82 deletions(-) diff --git a/abci/proposals.go b/abci/proposals.go index 7c80ef5..99487af 100644 --- a/abci/proposals.go +++ b/abci/proposals.go @@ -11,6 +11,7 @@ import ( "github.com/skip-mev/pob/blockbuster" "github.com/skip-mev/pob/blockbuster/abci" "github.com/skip-mev/pob/blockbuster/lanes/auction" + "github.com/skip-mev/pob/blockbuster/utils" ) const ( @@ -39,8 +40,8 @@ type ( VerifyTx(ctx sdk.Context, tx sdk.Tx) error // ProcessLaneBasic is utilized to verify the rest of the proposal according to - // the preferences of the top of block lane. This is used to verify that no - ProcessLaneBasic(txs [][]byte) error + // the preferences of the top of block lane. + ProcessLaneBasic(txs []sdk.Tx) error } // ProposalHandler contains the functionality and handlers required to\ @@ -127,15 +128,22 @@ func (h *ProposalHandler) ProcessProposalHandler() sdk.ProcessProposalHandler { return cometabci.ResponseProcessProposal{Status: cometabci.ResponseProcessProposal_REJECT} } + // Decode the transactions in the proposal. + decodedTxs, err := utils.GetDecodedTxs(h.txDecoder, proposal[NumInjectedTxs:]) + if err != nil { + h.logger.Error("failed to decode transactions", "err", err) + return cometabci.ResponseProcessProposal{Status: cometabci.ResponseProcessProposal_REJECT} + } + // Do a basic check of the rest of the proposal to make sure no auction transactions // are included. - if err := h.tobLane.ProcessLaneBasic(proposal[NumInjectedTxs:]); err != nil { + if err := h.tobLane.ProcessLaneBasic(decodedTxs); err != nil { h.logger.Error("failed to process proposal", "err", err) return cometabci.ResponseProcessProposal{Status: cometabci.ResponseProcessProposal_REJECT} } // Verify that the rest of the proposal is valid according to each lane's verification logic. - if _, err = h.processLanesHandler(ctx, proposal[auctionInfo.NumTxs:]); err != nil { + if _, err = h.processLanesHandler(ctx, decodedTxs[auctionInfo.NumTxs:]); err != nil { h.logger.Error("failed to process proposal", "err", err) return cometabci.ResponseProcessProposal{Status: cometabci.ResponseProcessProposal_REJECT} } diff --git a/blockbuster/abci/abci.go b/blockbuster/abci/abci.go index 6b9c263..c90212e 100644 --- a/blockbuster/abci/abci.go +++ b/blockbuster/abci/abci.go @@ -14,15 +14,17 @@ type ( // handlers. ProposalHandler struct { logger log.Logger + txDecoder sdk.TxDecoder prepareLanesHandler blockbuster.PrepareLanesHandler processLanesHandler blockbuster.ProcessLanesHandler } ) // NewProposalHandler returns a new abci++ proposal handler. -func NewProposalHandler(logger log.Logger, mempool blockbuster.Mempool) *ProposalHandler { +func NewProposalHandler(logger log.Logger, txDecoder sdk.TxDecoder, mempool blockbuster.Mempool) *ProposalHandler { return &ProposalHandler{ logger: logger, + txDecoder: txDecoder, prepareLanesHandler: ChainPrepareLanes(mempool.Registry()...), processLanesHandler: ChainProcessLanes(mempool.Registry()...), } @@ -67,8 +69,15 @@ func (h *ProposalHandler) ProcessProposalHandler() sdk.ProcessProposalHandler { } }() + // Decode the transactions from the proposal. + decodedTxs, err := utils.GetDecodedTxs(h.txDecoder, req.Txs) + if err != nil { + h.logger.Error("failed to decode transactions", "err", err) + return abci.ResponseProcessProposal{Status: abci.ResponseProcessProposal_REJECT} + } + // Verify the proposal using the verification logic from each lane. - if _, err := h.processLanesHandler(ctx, req.Txs); err != nil { + if _, err := h.processLanesHandler(ctx, decodedTxs); err != nil { h.logger.Error("failed to validate the proposal", "err", err) return abci.ResponseProcessProposal{Status: abci.ResponseProcessProposal_REJECT} } @@ -163,7 +172,7 @@ func ChainProcessLanes(chain ...blockbuster.Lane) blockbuster.ProcessLanesHandle chain = append(chain, terminator.Terminator{}) } - return func(ctx sdk.Context, proposalTxs [][]byte) (sdk.Context, error) { + return func(ctx sdk.Context, proposalTxs []sdk.Tx) (sdk.Context, error) { // Short circuit if there are no transactions to process. if len(proposalTxs) == 0 { return ctx, nil diff --git a/blockbuster/abci/abci_test.go b/blockbuster/abci/abci_test.go index c1d1f10..e4bf6f2 100644 --- a/blockbuster/abci/abci_test.go +++ b/blockbuster/abci/abci_test.go @@ -143,7 +143,7 @@ func (suite *ABCITestSuite) SetupTest() { suite.builderDecorator = ante.NewBuilderDecorator(suite.builderKeeper, suite.encodingConfig.TxConfig.TxEncoder(), suite.tobLane, suite.mempool) // Proposal handler set up - suite.proposalHandler = abci.NewProposalHandler(log.NewNopLogger(), suite.mempool) + suite.proposalHandler = abci.NewProposalHandler(log.NewNopLogger(), suite.encodingConfig.TxConfig.TxDecoder(), suite.mempool) } func (suite *ABCITestSuite) anteHandler(ctx sdk.Context, tx sdk.Tx, _ bool) (sdk.Context, error) { @@ -693,7 +693,7 @@ func (suite *ABCITestSuite) TestPrepareProposal() { } // Create a new proposal handler - suite.proposalHandler = abci.NewProposalHandler(suite.logger, suite.mempool) + suite.proposalHandler = abci.NewProposalHandler(suite.logger, suite.encodingConfig.TxConfig.TxDecoder(), suite.mempool) handler := suite.proposalHandler.PrepareProposalHandler() res := handler(suite.ctx, abcitypes.RequestPrepareProposal{ MaxTxBytes: maxTxBytes, diff --git a/blockbuster/lane.go b/blockbuster/lane.go index f1d2216..f0f8c8c 100644 --- a/blockbuster/lane.go +++ b/blockbuster/lane.go @@ -34,7 +34,7 @@ type ( // ProcessLanesHandler wraps all of the lanes Process functions into a single chained // function. You can think of it like an AnteHandler, but for processing proposals in the // context of lanes instead of modules. - ProcessLanesHandler func(ctx sdk.Context, proposalTxs [][]byte) (sdk.Context, error) + ProcessLanesHandler func(ctx sdk.Context, txs []sdk.Tx) (sdk.Context, error) // BaseLaneConfig defines the basic functionality needed for a lane. BaseLaneConfig struct { @@ -48,6 +48,14 @@ type ( // on the number of transactions that can be included in the block for this // lane (up to maxTxBytes as provided by the request). This is useful for the default lane. MaxBlockSpace sdk.Dec + + // IgnoreList defines the list of lanes to ignore when processing transactions. This + // is useful for when you want lanes to exist after the default lane. For example, + // say there are two lanes: default and free. The free lane should be processed after + // the default lane. In this case, the free lane should be added to the ignore list + // of the default lane. Otherwise, the transactions that belong to the free lane + // will be processed by the default lane. + IgnoreList []Lane } // Lane defines an interface used for block construction @@ -74,12 +82,12 @@ type ( // ProcessLaneBasic validates that transactions belonging to this lane are not misplaced // in the block proposal. - ProcessLaneBasic(txs [][]byte) error + ProcessLaneBasic(txs []sdk.Tx) error // ProcessLane verifies this lane's portion of a proposed block. It inputs the transactions // that may belong to this lane and a function to call the next lane in the chain. The next // lane in the chain will be called with the updated context and filtered down transactions. - ProcessLane(ctx sdk.Context, proposalTxs [][]byte, next ProcessLanesHandler) (sdk.Context, error) + ProcessLane(ctx sdk.Context, proposalTxs []sdk.Tx, next ProcessLanesHandler) (sdk.Context, error) // SetAnteHandler sets the lane's antehandler. SetAnteHandler(antehander sdk.AnteHandler) diff --git a/blockbuster/lanes/auction/abci.go b/blockbuster/lanes/auction/abci.go index 4c5776d..2237d79 100644 --- a/blockbuster/lanes/auction/abci.go +++ b/blockbuster/lanes/auction/abci.go @@ -123,26 +123,23 @@ selectBidTxLoop: // ProcessLane will ensure that block proposals that include transactions from // the top-of-block auction lane are valid. -func (l *TOBLane) ProcessLane(ctx sdk.Context, proposalTxs [][]byte, next blockbuster.ProcessLanesHandler) (sdk.Context, error) { - tx, err := l.Cfg.TxDecoder(proposalTxs[0]) - if err != nil { - return ctx, fmt.Errorf("failed to decode tx in lane %s: %w", l.Name(), err) +func (l *TOBLane) ProcessLane(ctx sdk.Context, txs []sdk.Tx, next blockbuster.ProcessLanesHandler) (sdk.Context, error) { + bidTx := txs[0] + + if !l.Match(bidTx) { + return next(ctx, txs) } - if !l.Match(tx) { - return next(ctx, proposalTxs) - } - - bidInfo, err := l.GetAuctionBidInfo(tx) + bidInfo, err := l.GetAuctionBidInfo(bidTx) if err != nil { return ctx, fmt.Errorf("failed to get bid info for lane %s: %w", l.Name(), err) } - if err := l.VerifyTx(ctx, tx); err != nil { + if err := l.VerifyTx(ctx, bidTx); err != nil { return ctx, fmt.Errorf("invalid bid tx: %w", err) } - return next(ctx, proposalTxs[len(bidInfo.Transactions)+1:]) + return next(ctx, txs[len(bidInfo.Transactions)+1:]) } // ProcessLaneBasic ensures that if a bid transaction is present in a proposal, @@ -150,20 +147,12 @@ func (l *TOBLane) ProcessLane(ctx sdk.Context, proposalTxs [][]byte, next blockb // - all of the bundled transactions are included after the bid transaction in the order // they were included in the bid transaction. // - there are no other bid transactions in the proposal -func (l *TOBLane) ProcessLaneBasic(txs [][]byte) error { - tx, err := l.Cfg.TxDecoder(txs[0]) - if err != nil { - return fmt.Errorf("failed to decode tx in lane %s: %w", l.Name(), err) - } +func (l *TOBLane) ProcessLaneBasic(txs []sdk.Tx) error { + bidTx := txs[0] // If there is a bid transaction, it must be the first transaction in the block proposal. - if !l.Match(tx) { - for _, txBz := range txs[1:] { - tx, err := l.Cfg.TxDecoder(txBz) - if err != nil { - return fmt.Errorf("failed to decode tx in lane %s: %w", l.Name(), err) - } - + if !l.Match(bidTx) { + for _, tx := range txs[1:] { if l.Match(tx) { return fmt.Errorf("misplaced bid transactions in lane %s", l.Name()) } @@ -172,7 +161,7 @@ func (l *TOBLane) ProcessLaneBasic(txs [][]byte) error { return nil } - bidInfo, err := l.GetAuctionBidInfo(tx) + bidInfo, err := l.GetAuctionBidInfo(bidTx) if err != nil { return fmt.Errorf("failed to get bid info for lane %s: %w", l.Name(), err) } @@ -182,17 +171,12 @@ func (l *TOBLane) ProcessLaneBasic(txs [][]byte) error { } // Ensure that the order of transactions in the bundle is preserved. - for i, bundleTxBz := range txs[1 : len(bidInfo.Transactions)+1] { - tx, err := l.WrapBundleTransaction(bundleTxBz) - if err != nil { - return fmt.Errorf("failed to decode bundled tx in lane %s: %w", l.Name(), err) - } - - if l.Match(tx) { + for i, bundleTx := range txs[1 : len(bidInfo.Transactions)+1] { + if l.Match(bundleTx) { return fmt.Errorf("multiple bid transactions in lane %s", l.Name()) } - txBz, err := l.Cfg.TxEncoder(tx) + txBz, err := l.Cfg.TxEncoder(bundleTx) if err != nil { return fmt.Errorf("failed to encode bundled tx in lane %s: %w", l.Name(), err) } @@ -203,12 +187,7 @@ func (l *TOBLane) ProcessLaneBasic(txs [][]byte) error { } // Ensure that there are no more bid transactions in the block proposal. - for _, txBz := range txs[len(bidInfo.Transactions)+1:] { - tx, err := l.Cfg.TxDecoder(txBz) - if err != nil { - return fmt.Errorf("failed to decode tx in lane %s: %w", l.Name(), err) - } - + for _, tx := range txs[len(bidInfo.Transactions)+1:] { if l.Match(tx) { return fmt.Errorf("multiple bid transactions in lane %s", l.Name()) } diff --git a/blockbuster/lanes/base/abci.go b/blockbuster/lanes/base/abci.go index 515fc76..cd62d3a 100644 --- a/blockbuster/lanes/base/abci.go +++ b/blockbuster/lanes/base/abci.go @@ -66,19 +66,14 @@ func (l *DefaultLane) PrepareLane( } // ProcessLane verifies the default lane's portion of a block proposal. -func (l *DefaultLane) ProcessLane(ctx sdk.Context, proposalTxs [][]byte, next blockbuster.ProcessLanesHandler) (sdk.Context, error) { - for index, tx := range proposalTxs { - tx, err := l.Cfg.TxDecoder(tx) - if err != nil { - return ctx, fmt.Errorf("failed to decode tx: %w", err) - } - +func (l *DefaultLane) ProcessLane(ctx sdk.Context, txs []sdk.Tx, next blockbuster.ProcessLanesHandler) (sdk.Context, error) { + for index, tx := range txs { if l.Match(tx) { if err := l.VerifyTx(ctx, tx); err != nil { return ctx, fmt.Errorf("failed to verify tx: %w", err) } } else { - return next(ctx, proposalTxs[index:]) + return next(ctx, txs[index:]) } } @@ -88,16 +83,11 @@ func (l *DefaultLane) ProcessLane(ctx sdk.Context, proposalTxs [][]byte, next bl // ProcessLaneBasic does basic validation on the block proposal to ensure that // transactions that belong to this lane are not misplaced in the block proposal. -func (l *DefaultLane) ProcessLaneBasic(txs [][]byte) error { +func (l *DefaultLane) ProcessLaneBasic(txs []sdk.Tx) error { seenOtherLaneTx := false lastSeenIndex := 0 - for _, txBz := range txs { - tx, err := l.Cfg.TxDecoder(txBz) - if err != nil { - return fmt.Errorf("failed to decode tx in lane %s: %w", l.Name(), err) - } - + for _, tx := range txs { if l.Match(tx) { if seenOtherLaneTx { return fmt.Errorf("the %s lane contains a transaction that belongs to another lane", l.Name()) diff --git a/blockbuster/lanes/base/lane.go b/blockbuster/lanes/base/lane.go index 10c2463..5a849df 100644 --- a/blockbuster/lanes/base/lane.go +++ b/blockbuster/lanes/base/lane.go @@ -36,9 +36,15 @@ func NewDefaultLane(cfg blockbuster.BaseLaneConfig) *DefaultLane { } // Match returns true if the transaction belongs to this lane. Since -// this is the default lane, it always returns true. This means that -// any transaction can be included in this lane. -func (l *DefaultLane) Match(sdk.Tx) bool { +// this is the default lane, it always returns true except for transactions +// that belong to lanes in the ignore list. +func (l *DefaultLane) Match(tx sdk.Tx) bool { + for _, lane := range l.Cfg.IgnoreList { + if lane.Match(tx) { + return false + } + } + return true } @@ -61,3 +67,8 @@ func (l *DefaultLane) SetAnteHandler(anteHandler sdk.AnteHandler) { func (l *DefaultLane) GetMaxBlockSpace() sdk.Dec { return l.Cfg.MaxBlockSpace } + +// GetIgnoreList returns the lane's ignore list. +func (l *DefaultLane) GetIgnoreList() []blockbuster.Lane { + return l.Cfg.IgnoreList +} diff --git a/blockbuster/lanes/terminator/lane.go b/blockbuster/lanes/terminator/lane.go index 972fc9c..b9ddfca 100644 --- a/blockbuster/lanes/terminator/lane.go +++ b/blockbuster/lanes/terminator/lane.go @@ -40,7 +40,7 @@ func (t Terminator) PrepareLane(_ sdk.Context, proposal *blockbuster.Proposal, _ } // ProcessLane is a no-op -func (t Terminator) ProcessLane(ctx sdk.Context, _ [][]byte, _ blockbuster.ProcessLanesHandler) (sdk.Context, error) { +func (t Terminator) ProcessLane(ctx sdk.Context, _ []sdk.Tx, _ blockbuster.ProcessLanesHandler) (sdk.Context, error) { return ctx, nil } @@ -85,7 +85,7 @@ func (t Terminator) Select(context.Context, [][]byte) sdkmempool.Iterator { } // ValidateLaneBasic is a no-op -func (t Terminator) ProcessLaneBasic([][]byte) error { +func (t Terminator) ProcessLaneBasic([]sdk.Tx) error { return nil } diff --git a/blockbuster/utils/utils.go b/blockbuster/utils/utils.go index e3d63ea..ce65f8b 100644 --- a/blockbuster/utils/utils.go +++ b/blockbuster/utils/utils.go @@ -24,6 +24,21 @@ func GetTxHashStr(txEncoder sdk.TxEncoder, tx sdk.Tx) ([]byte, string, error) { return txBz, txHashStr, nil } +// GetDecodedTxs returns the decoded transactions from the given bytes. +func GetDecodedTxs(txDecoder sdk.TxDecoder, txs [][]byte) ([]sdk.Tx, error) { + var decodedTxs []sdk.Tx + for _, txBz := range txs { + tx, err := txDecoder(txBz) + if err != nil { + return nil, fmt.Errorf("failed to decode transaction: %w", err) + } + + decodedTxs = append(decodedTxs, tx) + } + + return decodedTxs, nil +} + // RemoveTxsFromLane removes the transactions from the given lane's mempool. func RemoveTxsFromLane(txs map[sdk.Tx]struct{}, mempool sdkmempool.Mempool) error { for tx := range txs { diff --git a/tests/app/app.go b/tests/app/app.go index 317bb0c..3b3d84e 100644 --- a/tests/app/app.go +++ b/tests/app/app.go @@ -295,7 +295,17 @@ func New( ) // Default lane accepts all other transactions. - defaultLane := base.NewDefaultLane(config) + defaultConfig := blockbuster.BaseLaneConfig{ + Logger: app.Logger(), + TxEncoder: app.txConfig.TxEncoder(), + TxDecoder: app.txConfig.TxDecoder(), + MaxBlockSpace: sdk.ZeroDec(), + IgnoreList: []blockbuster.Lane{ + tobLane, + freeLane, + }, + } + defaultLane := base.NewDefaultLane(defaultConfig) lanes := []blockbuster.Lane{ tobLane, freeLane, @@ -333,6 +343,7 @@ func New( // Set the proposal handlers on the BaseApp along with the custom antehandler. proposalHandlers := abci.NewProposalHandler( app.Logger(), + app.txConfig.TxDecoder(), mempool, ) app.App.SetPrepareProposal(proposalHandlers.PrepareProposalHandler()) diff --git a/tests/e2e/e2e_test.go b/tests/e2e/e2e_test.go index c10c408..b5284d8 100644 --- a/tests/e2e/e2e_test.go +++ b/tests/e2e/e2e_test.go @@ -364,9 +364,6 @@ func (s *IntegrationTestSuite) TestMultipleBids() { bundle2[i] = s.createMsgSendTx(accounts[1], accounts[0].Address.String(), defaultSendAmount, uint64(i), 1000) } - // Wait for a block to ensure all transactions are included in the same block - s.waitForABlock() - // Create a bid transaction that includes the bundle and is valid bid := reserveFee height := s.queryCurrentHeight() @@ -376,6 +373,9 @@ func (s *IntegrationTestSuite) TestMultipleBids() { bid2 := reserveFee.Add(sdk.NewCoin(app.BondDenom, sdk.NewInt(10))) bidTx2 := s.createAuctionBidTx(accounts[3], bid2, bundle2, 0, height+5) + // Wait for a block to ensure all transactions are included in the same block + s.waitForABlock() + // Broadcast the transactions to different validators s.broadcastTx(bidTx, 0) s.broadcastTx(bidTx2, 1) @@ -1066,14 +1066,6 @@ func (s *IntegrationTestSuite) TestFreeLane() { s.waitForABlock() height := s.queryCurrentHeight() - // Ensure that the transaction was executed - balanceAfterFreeTx := s.queryBalanceOf(accounts[0].Address.String(), app.BondDenom) - s.Require().True(balanceAfterFreeTx.Add(defaultStakeAmount).IsGTE(balanceBeforeFreeTx)) - - // The balance must be strictly less than to account for fees - balanceAfterNormalTx := s.queryBalanceOf(accounts[1].Address.String(), app.BondDenom) - s.Require().True(balanceAfterNormalTx.IsLT((balanceBeforeNormalTx.Sub(defaultSendAmount)))) - hashes := s.normalTxsToTxHashes([][]byte{freeTx, normalTx}) expectedExecution := map[string]bool{ hashes[0]: true, @@ -1082,6 +1074,14 @@ func (s *IntegrationTestSuite) TestFreeLane() { // Ensure that the block was built correctly s.verifyBlock(height, hashes, expectedExecution) + + // Ensure that the transaction was executed + balanceAfterFreeTx := s.queryBalanceOf(accounts[0].Address.String(), app.BondDenom) + s.Require().True(balanceAfterFreeTx.Add(defaultStakeAmount).IsGTE(balanceBeforeFreeTx)) + + // The balance must be strictly less than to account for fees + balanceAfterNormalTx := s.queryBalanceOf(accounts[1].Address.String(), app.BondDenom) + s.Require().True(balanceAfterNormalTx.IsLT((balanceBeforeNormalTx.Sub(defaultSendAmount)))) }, }, {