feat: Add namespace id checks for messages (#592)

* add namespace checks for message in prepare/process proposal * add namespace checks for message in prepare/process proposal only in app * Update app/split_shares.go * adds idea to gitignore (#593) * add namespace checks for message in prepare/process proposal * add namespace checks for message in prepare/process proposal only in app * add tail and shares namespace check for wirepayfordata * add process proposal test * Revert "adds idea to gitignore (#593)" This reverts commit f5a0743. * fix process proposal test * add prepare proposal test * update comment * Update app/test/process_proposal_test.go * Update app/process_proposal.go Co-authored-by: Rootul Patel <[email protected]> * Update x/payment/types/wirepayfordata.go Co-authored-by: Rootul Patel <[email protected]> * Update app/test/prepare_proposal_test.go Co-authored-by: Rootul Patel <[email protected]> * Update app/test/prepare_proposal_test.go Co-authored-by: Rootul Patel <[email protected]> * Update x/payment/types/wirepayfordata.go Co-authored-by: Evan Forbes <[email protected]> * Update x/payment/types/wirepayfordata.go * add TestProcessMessageWithParityShareNamespaces * refactor TestProcessMessageWithParityShareNamespaces Co-authored-by: Rootul Patel <[email protected]> Co-authored-by: Evan Forbes <[email protected]>
celestiaorg · Aug 4, 2022 · 5158ca8 · 5158ca8
1 parent f5a0743
commit 5158ca8
Show file tree

Hide file tree

Showing 7 changed files with 175 additions and 23 deletions.
diff --git a/.gitignore b/.gitignore
@@ -6,4 +6,3 @@ build
 coverage.txt
 tools-stamp
 .vscode
-.idea
diff --git a/app/process_proposal.go b/app/process_proposal.go
@@ -63,6 +63,19 @@ func (app *App) ProcessProposal(req abci.RequestProcessProposal) abci.ResponsePr
 	// iterate through all of the messages and ensure that a PFD with the exact
 	// commitment exists
 	for _, msg := range req.BlockData.Messages.MessagesList {
+		if err := types.ValidateMessageNamespaceID(msg.NamespaceId); err != nil {
+			app.Logger().Error(
+				rejectedPropBlockLog,
+				"reason",
+				"found a message that uses an invalid namespace id",
+				"error",
+				err.Error(),
+			)
+			return abci.ResponseProcessProposal{
+				Result: abci.ResponseProcessProposal_REJECT,
+			}
+		}
+
 		commit, err := types.CreateCommitment(req.BlockData.OriginalSquareSize, msg.NamespaceId, msg.Data)
 		if err != nil {
 			app.Logger().Error(

diff --git a/app/split_shares.go b/app/split_shares.go
@@ -60,11 +60,18 @@ func SplitShares(txConf client.TxConfig, squareSize uint64, data *core.Data) ([]
 		}
 
 		msg := authTx.GetMsgs()[0]
+
 		wireMsg, ok := msg.(*types.MsgWirePayForData)
 		if !ok {
 			continue
 		}
 
+		// run basic validation on the message
+		err = wireMsg.ValidateBasic()
+		if err != nil {
+			continue
+		}
+
 		// run basic validation on the transaction (which include the wireMsg
 		// above)
 		err = authTx.ValidateBasic()

diff --git a/app/test/prepare_proposal_test.go b/app/test/prepare_proposal_test.go
@@ -8,12 +8,14 @@ import (
 	"github.com/celestiaorg/celestia-app/app/encoding"
 	"github.com/celestiaorg/celestia-app/testutil"
 	"github.com/celestiaorg/celestia-app/x/payment/types"
+	"github.com/celestiaorg/nmt/namespace"
 	"github.com/cosmos/cosmos-sdk/client"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	authsigning "github.com/cosmos/cosmos-sdk/x/auth/signing"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	abci "github.com/tendermint/tendermint/abci/types"
+	"github.com/tendermint/tendermint/pkg/consts"
 	core "github.com/tendermint/tendermint/proto/tendermint/types"
 )
 
@@ -102,6 +104,39 @@ func TestPrepareProposal(t *testing.T) {
 	}
 }
 
+func TestPrepareMessagesWithReservedNamespaces(t *testing.T) {
+	testApp := testutil.SetupTestAppWithGenesisValSet(t)
+	encCfg := encoding.MakeConfig(app.ModuleEncodingRegisters...)
+
+	signer := testutil.GenerateKeyringSigner(t, testAccName)
+
+	type test struct {
+		name             string
+		namespace        namespace.ID
+		expectedMessages int
+	}
+
+	tests := []test{
+		{"transaction namespace id for message", consts.TxNamespaceID, 0},
+		{"evidence namespace id for message", consts.EvidenceNamespaceID, 0},
+		{"tail padding namespace id for message", consts.TailPaddingNamespaceID, 0},
+		{"parity shares namespace id for message", consts.ParitySharesNamespaceID, 0},
+		{"reserved namespace id for message", namespace.ID{0, 0, 0, 0, 0, 0, 0, 200}, 0},
+		{"valid namespace id for message", namespace.ID{3, 3, 2, 2, 2, 1, 1, 1}, 1},
+	}
+
+	for _, tt := range tests {
+		tx := generateRawTx(t, encCfg.TxConfig, tt.namespace, []byte{1}, signer, 2, 4, 8, 16)
+		input := abci.RequestPrepareProposal{
+			BlockData: &core.Data{
+				Txs: [][]byte{tx},
+			},
+		}
+		res := testApp.PrepareProposal(input)
+		assert.Equal(t, tt.expectedMessages, len(res.BlockData.Messages.MessagesList))
+	}
+}
+
 func generateRawTx(t *testing.T, txConfig client.TxConfig, ns, message []byte, signer *types.KeyringSigner, ks ...uint64) (rawTx []byte) {
 	coin := sdk.Coin{
 		Denom:  app.BondDenom,

diff --git a/app/test/process_proposal_test.go b/app/test/process_proposal_test.go
@@ -9,6 +9,7 @@ import (
 	"github.com/celestiaorg/celestia-app/app/encoding"
 	"github.com/celestiaorg/celestia-app/testutil"
 	"github.com/celestiaorg/celestia-app/x/payment/types"
+	"github.com/celestiaorg/nmt/namespace"
 	"github.com/cosmos/cosmos-sdk/client"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	"github.com/stretchr/testify/assert"
@@ -164,13 +165,99 @@ func TestMessageInclusionCheck(t *testing.T) {
 	}
 }
 
+func TestProcessMessagesWithReservedNamespaces(t *testing.T) {
+	testApp := testutil.SetupTestAppWithGenesisValSet(t)
+	encConf := encoding.MakeConfig(app.ModuleEncodingRegisters...)
+
+	signer := testutil.GenerateKeyringSigner(t, testAccName)
+
+	type test struct {
+		name           string
+		namespace      namespace.ID
+		expectedResult abci.ResponseProcessProposal_Result
+	}
+
+	tests := []test{
+		{"transaction namespace id for message", consts.TxNamespaceID, abci.ResponseProcessProposal_REJECT},
+		{"evidence namespace id for message", consts.EvidenceNamespaceID, abci.ResponseProcessProposal_REJECT},
+		{"tail padding namespace id for message", consts.TailPaddingNamespaceID, abci.ResponseProcessProposal_REJECT},
+		{"namespace id 200 for message", namespace.ID{0, 0, 0, 0, 0, 0, 0, 200}, abci.ResponseProcessProposal_REJECT},
+		{"correct namespace id for message", namespace.ID{3, 3, 2, 2, 2, 1, 1, 1}, abci.ResponseProcessProposal_ACCEPT},
+	}
+
+	for _, tt := range tests {
+		pfd, msg := genRandMsgPayForDataForNamespace(t, signer, 8, tt.namespace)
+		input := abci.RequestProcessProposal{
+			BlockData: &core.Data{
+				Txs: [][]byte{
+					buildTx(t, signer, encConf.TxConfig, pfd),
+				},
+				Messages: core.Messages{
+					MessagesList: []*core.Message{
+						{
+							NamespaceId: pfd.GetMessageNamespaceId(),
+							Data:        msg,
+						},
+					},
+				},
+				OriginalSquareSize: 8,
+			},
+		}
+		data, err := coretypes.DataFromProto(input.BlockData)
+		require.NoError(t, err)
+
+		shares, _, err := data.ComputeShares(input.BlockData.OriginalSquareSize)
+		require.NoError(t, err)
+
+		rawShares := shares.RawShares()
+
+		require.NoError(t, err)
+		eds, err := da.ExtendShares(input.BlockData.OriginalSquareSize, rawShares)
+		require.NoError(t, err)
+		dah := da.NewDataAvailabilityHeader(eds)
+		input.Header.DataHash = dah.Hash()
+		res := testApp.ProcessProposal(input)
+		assert.Equal(t, tt.expectedResult, res.Result)
+	}
+}
+
+func TestProcessMessageWithParityShareNamespaces(t *testing.T) {
+	testApp := testutil.SetupTestAppWithGenesisValSet(t)
+	encConf := encoding.MakeConfig(app.ModuleEncodingRegisters...)
+
+	signer := testutil.GenerateKeyringSigner(t, testAccName)
+
+	pfd, msg := genRandMsgPayForDataForNamespace(t, signer, 8, consts.ParitySharesNamespaceID)
+	input := abci.RequestProcessProposal{
+		BlockData: &core.Data{
+			Txs: [][]byte{
+				buildTx(t, signer, encConf.TxConfig, pfd),
+			},
+			Messages: core.Messages{
+				MessagesList: []*core.Message{
+					{
+						NamespaceId: pfd.GetMessageNamespaceId(),
+						Data:        msg,
+					},
+				},
+			},
+			OriginalSquareSize: 8,
+		},
+	}
+	res := testApp.ProcessProposal(input)
+	assert.Equal(t, abci.ResponseProcessProposal_REJECT, res.Result)
+}
+
 func genRandMsgPayForData(t *testing.T, signer *types.KeyringSigner, squareSize uint64) (*types.MsgPayForData, []byte) {
 	ns := make([]byte, consts.NamespaceSize)
 	_, err := rand.Read(ns)
 	require.NoError(t, err)
+	return genRandMsgPayForDataForNamespace(t, signer, squareSize, ns)
+}
 
+func genRandMsgPayForDataForNamespace(t *testing.T, signer *types.KeyringSigner, squareSize uint64, ns namespace.ID) (*types.MsgPayForData, []byte) {
 	message := make([]byte, randomInt(20))
-	_, err = rand.Read(message)
+	_, err := rand.Read(message)
 	require.NoError(t, err)
 
 	commit, err := types.CreateCommitment(squareSize, ns, message)

diff --git a/x/payment/types/errors.go b/x/payment/types/errors.go
@@ -16,4 +16,6 @@ var (
 	ErrInvalidShareCommit             = sdkerrors.Register(ModuleName, 11116, "invalid commit for share")
 	ErrParitySharesNamespace          = sdkerrors.Register(ModuleName, 11117, "cannot use parity shares namespace ID")
 	ErrTailPaddingNamespace           = sdkerrors.Register(ModuleName, 11118, "cannot use tail padding namespace ID")
+	ErrTxNamespace                    = sdkerrors.Register(ModuleName, 11119, "cannot use transaction namespace ID")
+	ErrEvidenceNamespace              = sdkerrors.Register(ModuleName, 11120, "cannot use evidence namespace ID")
 )
diff --git a/x/payment/types/wirepayfordata.go b/x/payment/types/wirepayfordata.go
@@ -5,6 +5,7 @@ import (
 	"errors"
 	fmt "fmt"
 
+	"github.com/celestiaorg/nmt/namespace"
 	sdkclient "github.com/cosmos/cosmos-sdk/client"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
@@ -84,12 +85,8 @@ func (msg *MsgWirePayForData) Route() string { return RouterKey }
 // commitments, signatures for those share commitments, and fulfills the sdk.Msg
 // interface.
 func (msg *MsgWirePayForData) ValidateBasic() error {
-	// ensure that the namespace id is of length == NamespaceIDSize
-	if nsLen := len(msg.GetMessageNameSpaceId()); nsLen != NamespaceIDSize {
-		return ErrInvalidNamespaceLen.Wrapf("got: %d want: %d",
-			nsLen,
-			NamespaceIDSize,
-		)
+	if err := ValidateMessageNamespaceID(msg.GetMessageNameSpaceId()); err != nil {
+		return err
 	}
 
 	if _, err := sdk.AccAddressFromBech32(msg.Signer); err != nil {
@@ -105,21 +102,6 @@ func (msg *MsgWirePayForData) ValidateBasic() error {
 		)
 	}
 
-	// ensure that a reserved namespace is not used
-	if bytes.Compare(msg.GetMessageNameSpaceId(), consts.MaxReservedNamespace) < 1 {
-		return ErrReservedNamespace.Wrapf("got namespace: %x, want: > %x", msg.GetMessageNameSpaceId(), consts.MaxReservedNamespace)
-	}
-
-	// ensure that ParitySharesNamespaceID is not used
-	if bytes.Equal(msg.GetMessageNameSpaceId(), consts.ParitySharesNamespaceID) {
-		return ErrParitySharesNamespace
-	}
-
-	// ensure that TailPaddingNamespaceID is not used
-	if bytes.Equal(msg.GetMessageNameSpaceId(), consts.TailPaddingNamespaceID) {
-		return ErrTailPaddingNamespace
-	}
-
 	for idx, commit := range msg.MessageShareCommitment {
 		// check that each commit is valid
 		if !powerOf2(commit.K) {
@@ -139,6 +121,33 @@ func (msg *MsgWirePayForData) ValidateBasic() error {
 	return nil
 }
 
+// ValidateMessageNamespaceID returns an error if the provided namespace.ID is an invalid or reserved namespace id.
+func ValidateMessageNamespaceID(ns namespace.ID) error {
+	// ensure that the namespace id is of length == NamespaceIDSize
+	if nsLen := len(ns); nsLen != NamespaceIDSize {
+		return ErrInvalidNamespaceLen.Wrapf("got: %d want: %d",
+			nsLen,
+			NamespaceIDSize,
+		)
+	}
+	// ensure that a reserved namespace is not used
+	if bytes.Compare(ns, consts.MaxReservedNamespace) < 1 {
+		return ErrReservedNamespace.Wrapf("got namespace: %x, want: > %x", ns, consts.MaxReservedNamespace)
+	}
+
+	// ensure that ParitySharesNamespaceID is not used
+	if bytes.Equal(ns, consts.ParitySharesNamespaceID) {
+		return ErrParitySharesNamespace
+	}
+
+	// ensure that TailPaddingNamespaceID is not used
+	if bytes.Equal(ns, consts.TailPaddingNamespaceID) {
+		return ErrTailPaddingNamespace
+	}
+
+	return nil
+}
+
 // GetSigners returns the addresses of the message signers
 func (msg *MsgWirePayForData) GetSigners() []sdk.AccAddress {
 	address, err := sdk.AccAddressFromBech32(msg.Signer)
-Original file line number
+Diff line change
@@ Expand Up / @@ -6,4 +6,3 @@ build @@
     coverage.txt
     tools-stamp
     .vscode
-    .idea