From b2617e419280061e33feaa12ff4dcdd9cc612113 Mon Sep 17 00:00:00 2001 From: Evan Forbes <42654277+evan-forbes@users.noreply.github.com> Date: Thu, 5 May 2022 16:34:02 -0500 Subject: [PATCH] Message inclusion check (#216) * convert PreprocessTxs to PrepareProposal * add message inclusion check to ProcessProposal * use the malleated tx decoder to decode malleated transactions * use constant for pay for message URL * switch from PayForMessage to PayForData * fix integration test * address #226 * change missed var name Co-authored-by: Ismail Khoffi * get missed pfm vs pfd Co-authored-by: Ismail Khoffi * pfm -> pfd Co-authored-by: John Adler * better wording Co-authored-by: John Adler * better wording Co-authored-by: John Adler * message -> data Co-authored-by: John Adler * pfm -> pfd Co-authored-by: Ismail Khoffi * remaining pfms -> pfds * get rid of empty space Co-authored-by: John Adler * get rid of empty space Co-authored-by: John Adler * log URL type mismatch * add todo for refactoring to check for subtree roots Co-authored-by: Ismail Khoffi Co-authored-by: John Adler --- app/{abci.go => prepare_proposal.go} | 0 app/process_proposal.go | 88 +++++++++ ...{abci_test.go => prepare_proposal_test.go} | 0 app/test/process_proposal_test.go | 177 ++++++++++++++++++ x/payment/payfordata.go | 9 +- 5 files changed, 270 insertions(+), 4 deletions(-) rename app/{abci.go => prepare_proposal.go} (100%) create mode 100644 app/process_proposal.go rename app/test/{abci_test.go => prepare_proposal_test.go} (100%) create mode 100644 app/test/process_proposal_test.go diff --git a/app/abci.go b/app/prepare_proposal.go similarity index 100% rename from app/abci.go rename to app/prepare_proposal.go diff --git a/app/process_proposal.go b/app/process_proposal.go new file mode 100644 index 0000000000..cc478db48f --- /dev/null +++ b/app/process_proposal.go @@ -0,0 +1,88 @@ +package app + +import ( + "github.com/celestiaorg/celestia-app/x/payment/types" + sdk "github.com/cosmos/cosmos-sdk/types" + abci "github.com/tendermint/tendermint/abci/types" +) + +const ( + rejectedPropBlockLog = "Rejected proposal block:" +) + +func (app *App) ProcessProposal(req abci.RequestProcessProposal) abci.ResponseProcessProposal { + // Check for message inclusion: + // - each MsgPayForData included in a block should have a corresponding data also in the block body + // - the commitment in each PFD should match that of its corresponding data + // - there should be no unpaid-for data + + // extract the commitments from any MsgPayForDatas in the block + commitments := make(map[string]struct{}) + // we have a separate counter so that identical data also get counted + // also see https://github.com/celestiaorg/celestia-app/issues/226 + commitmentCounter := 0 + for _, rawTx := range req.BlockData.Txs { + tx, err := MalleatedTxDecoder(app.txConfig.TxDecoder())(rawTx) + if err != nil { + continue + } + + for _, msg := range tx.GetMsgs() { + if sdk.MsgTypeURL(msg) != types.URLMsgPayForData { + continue + } + + pfd, ok := msg.(*types.MsgPayForData) + if !ok { + app.Logger().Error("Msg type does not match MsgPayForData URL") + continue + } + + commitments[string(pfd.MessageShareCommitment)] = struct{}{} + commitmentCounter++ + } + } + + // quickly compare the number of PFDs and messages, if they aren't + // identical, then we already know this block is invalid + if commitmentCounter != len(req.BlockData.Messages.MessagesList) { + app.Logger().Error( + rejectedPropBlockLog, + "reason", + "varying number of messages and payForData txs in the same block", + ) + return abci.ResponseProcessProposal{ + Result: abci.ResponseProcessProposal_REJECT, + } + } + + // iterate through all of the messages and ensure that a PFD with the exact + // commitment exists + for _, msg := range req.BlockData.Messages.MessagesList { + commit, err := types.CreateCommitment(app.SquareSize(), msg.NamespaceId, msg.Data) + if err != nil { + app.Logger().Error( + rejectedPropBlockLog, + "reason", + "failure to create commitment for included message", + "error", + err.Error(), + ) + return abci.ResponseProcessProposal{ + Result: abci.ResponseProcessProposal_REJECT, + } + } + + // TODO: refactor to actually check for subtree roots instead of simply inclusion see issues #382 and #383 + if _, has := commitments[string(commit)]; !has { + app.Logger().Info(rejectedPropBlockLog, "reason", "missing MsgPayForData for included message") + return abci.ResponseProcessProposal{ + Result: abci.ResponseProcessProposal_REJECT, + } + } + } + + return abci.ResponseProcessProposal{ + Result: abci.ResponseProcessProposal_ACCEPT, + } +} diff --git a/app/test/abci_test.go b/app/test/prepare_proposal_test.go similarity index 100% rename from app/test/abci_test.go rename to app/test/prepare_proposal_test.go diff --git a/app/test/process_proposal_test.go b/app/test/process_proposal_test.go new file mode 100644 index 0000000000..f8036fef03 --- /dev/null +++ b/app/test/process_proposal_test.go @@ -0,0 +1,177 @@ +package app_test + +import ( + "crypto/rand" + "testing" + + "github.com/celestiaorg/celestia-app/app" + "github.com/celestiaorg/celestia-app/testutil" + "github.com/celestiaorg/celestia-app/x/payment/types" + "github.com/cosmos/cosmos-sdk/client" + sdk "github.com/cosmos/cosmos-sdk/types" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/tendermint/spm/cosmoscmd" + abci "github.com/tendermint/tendermint/abci/types" + tmrand "github.com/tendermint/tendermint/libs/rand" + "github.com/tendermint/tendermint/pkg/consts" + core "github.com/tendermint/tendermint/proto/tendermint/types" +) + +func TestMessageInclusionCheck(t *testing.T) { + signer := testutil.GenerateKeyringSigner(t, testAccName) + info := signer.GetSignerInfo() + + testApp := testutil.SetupTestApp(t, info.GetAddress()) + + encConf := cosmoscmd.MakeEncodingConfig(app.ModuleBasics) + + firstValidPFD, msg1 := genRandMsgPayForData(t, signer) + secondValidPFD, msg2 := genRandMsgPayForData(t, signer) + + invalidCommitmentPFD, msg3 := genRandMsgPayForData(t, signer) + invalidCommitmentPFD.MessageShareCommitment = tmrand.Bytes(32) + + // block with all messages included + validData := core.Data{ + Txs: [][]byte{ + buildTx(t, signer, encConf.TxConfig, firstValidPFD), + buildTx(t, signer, encConf.TxConfig, secondValidPFD), + }, + Messages: core.Messages{ + MessagesList: []*core.Message{ + { + NamespaceId: firstValidPFD.MessageNamespaceId, + Data: msg1, + }, + { + NamespaceId: secondValidPFD.MessageNamespaceId, + Data: msg2, + }, + }, + }, + } + + // block with a missing message + missingMessageData := core.Data{ + Txs: [][]byte{ + buildTx(t, signer, encConf.TxConfig, firstValidPFD), + buildTx(t, signer, encConf.TxConfig, secondValidPFD), + }, + Messages: core.Messages{ + MessagesList: []*core.Message{ + { + NamespaceId: firstValidPFD.MessageNamespaceId, + Data: msg1, + }, + }, + }, + } + + // block with all messages included, but the commitment is changed + invalidData := core.Data{ + Txs: [][]byte{ + buildTx(t, signer, encConf.TxConfig, firstValidPFD), + buildTx(t, signer, encConf.TxConfig, secondValidPFD), + }, + Messages: core.Messages{ + MessagesList: []*core.Message{ + { + NamespaceId: firstValidPFD.MessageNamespaceId, + Data: msg1, + }, + { + NamespaceId: invalidCommitmentPFD.MessageNamespaceId, + Data: msg3, + }, + }, + }, + } + + // block with all messages included + extraMessageData := core.Data{ + Txs: [][]byte{ + buildTx(t, signer, encConf.TxConfig, firstValidPFD), + }, + Messages: core.Messages{ + MessagesList: []*core.Message{ + { + NamespaceId: firstValidPFD.MessageNamespaceId, + Data: msg1, + }, + { + NamespaceId: secondValidPFD.MessageNamespaceId, + Data: msg2, + }, + }, + }, + } + + type test struct { + input abci.RequestProcessProposal + expectedResult abci.ResponseProcessProposal_Result + } + + tests := []test{ + { + input: abci.RequestProcessProposal{ + BlockData: &validData, + }, + expectedResult: abci.ResponseProcessProposal_ACCEPT, + }, + { + input: abci.RequestProcessProposal{ + BlockData: &missingMessageData, + }, + expectedResult: abci.ResponseProcessProposal_REJECT, + }, + { + input: abci.RequestProcessProposal{ + BlockData: &invalidData, + }, + expectedResult: abci.ResponseProcessProposal_REJECT, + }, + { + input: abci.RequestProcessProposal{ + BlockData: &extraMessageData, + }, + expectedResult: abci.ResponseProcessProposal_REJECT, + }, + } + + for _, tt := range tests { + res := testApp.ProcessProposal(tt.input) + assert.Equal(t, tt.expectedResult, res.Result) + } + +} + +func genRandMsgPayForData(t *testing.T, signer *types.KeyringSigner) (*types.MsgPayForData, []byte) { + ns := make([]byte, consts.NamespaceSize) + _, err := rand.Read(ns) + require.NoError(t, err) + + message := make([]byte, tmrand.Intn(3000)) + _, err = rand.Read(message) + require.NoError(t, err) + + commit, err := types.CreateCommitment(consts.MaxSquareSize, ns, message) + require.NoError(t, err) + + pfd := types.MsgPayForData{ + MessageShareCommitment: commit, + MessageNamespaceId: ns, + } + + return &pfd, message +} + +func buildTx(t *testing.T, signer *types.KeyringSigner, txCfg client.TxConfig, msg sdk.Msg) []byte { + tx, err := signer.BuildSignedTx(signer.NewTxBuilder(), msg) + require.NoError(t, err) + + rawTx, err := txCfg.TxEncoder()(tx) + require.NoError(t, err) + + return rawTx +} diff --git a/x/payment/payfordata.go b/x/payment/payfordata.go index b243fa8679..34c4b71b05 100644 --- a/x/payment/payfordata.go +++ b/x/payment/payfordata.go @@ -2,6 +2,7 @@ package payment import ( "context" + "google.golang.org/grpc" sdk "github.com/cosmos/cosmos-sdk/types" @@ -60,7 +61,7 @@ func BuildPayForData( message []byte, gasLim uint64, ) (*types.MsgWirePayForData, error) { - // create the raw WirePayForMessage transaction + // create the raw WirePayForData transaction wpfd, err := types.NewWirePayForData(nID, message, shareSizes...) if err != nil { return nil, err @@ -72,7 +73,7 @@ func BuildPayForData( return nil, err } - // generate the signatures for each `MsgPayForMessage` using the `KeyringSigner`, + // generate the signatures for each `MsgPayForData` using the `KeyringSigner`, // then set the gas limit for the tx gasLimOption := types.SetGasLimit(gasLim) err = wpfd.SignShareCommitments(signer, gasLimOption) @@ -89,8 +90,8 @@ func SignPayForData( pfd *types.MsgWirePayForData, opts ...types.TxBuilderOption, ) (signing.Tx, error) { - // Build and sign the final `WirePayForMessage` tx that now contains the signatures - // for potential `MsgPayForMessage`s + // Build and sign the final `WirePayForData` tx that now contains the signatures + // for potential `MsgPayForData`s builder := signer.NewTxBuilder() for _, opt := range opts { opt(builder)