fix: Patch all PFD signature bugs (#488)

* main changes

* fix overwrite bug

* improve comments surrounding ExtractMsgWirePayForData and HasWirePayForData

app/prepare_proposal.go

@@ -94,19 +94,13 @@ func estimateMsgShares(txConf client.TxConfig, txs [][]byte) int {
 			continue
 		}
 
-		// skip txs that don't contain messages
-		if !hasWirePayForData(authTx) {
-			continue
-		}
-
-		// only support malleated transactions that contain a single sdk.Msg
-		if len(authTx.GetMsgs()) != 1 {
-			continue
-		}
-
-		msg := authTx.GetMsgs()[0]
-		wireMsg, ok := msg.(*types.MsgWirePayForData)
-		if !ok {
+		wireMsg, err := types.ExtractMsgWirePayForData(authTx)
+		if err != nil {
+			// we catch this error because it means that there are no
+			// potentially valid MsgWirePayForData messages in this tx. If the
+			// tx doesn't have a wirePFD, then it won't contribute any message
+			// shares to the block, and since we're only estimating here, we can
+			// move on without handling or bubbling the error.
 			continue
 		}
 

app/split_shares.go

@@ -7,7 +7,6 @@ import (
 
 	"github.com/celestiaorg/celestia-app/x/payment/types"
 	"github.com/cosmos/cosmos-sdk/client"
-	sdk "github.com/cosmos/cosmos-sdk/types"
 	"github.com/cosmos/cosmos-sdk/x/auth/signing"
 	"github.com/tendermint/tendermint/pkg/consts"
 	core "github.com/tendermint/tendermint/proto/tendermint/types"
@@ -42,7 +41,7 @@ func SplitShares(txConf client.TxConfig, squareSize uint64, data *core.Data) ([]
 		}
 
 		// skip txs that don't contain messages
-		if !hasWirePayForData(authTx) {
+		if !types.HasWirePayForData(authTx) {
 			success, err := sqwr.writeTx(rawTx)
 			if err != nil {
 				continue
@@ -273,13 +272,3 @@ func (sqwr *shareSplitter) export() [][]byte {
 
 	return shares
 }
-
-func hasWirePayForData(tx sdk.Tx) bool {
-	for _, msg := range tx.GetMsgs() {
-		msgName := sdk.MsgTypeURL(msg)
-		if msgName == types.URLMsgWirePayForData {
-			return true
-		}
-	}
-	return false
-}

app/test/block_size_test.go

@@ -16,6 +16,7 @@ import (
 	"github.com/celestiaorg/celestia-app/app"
 	"github.com/celestiaorg/celestia-app/app/encoding"
 	"github.com/celestiaorg/celestia-app/testutil/network"
+	"github.com/celestiaorg/celestia-app/x/payment"
 	"github.com/celestiaorg/celestia-app/x/payment/types"
 	"github.com/celestiaorg/nmt/namespace"
 	abci "github.com/tendermint/tendermint/abci/types"
@@ -54,11 +55,14 @@ func (s *IntegrationTestSuite) SetupSuite() {
 
 	net := network.New(s.T(), s.cfg, s.accounts...)
 
+	err := network.GRPCConn(net)
+	s.Require().NoError(err)
+
 	s.network = net
 	s.kr = net.Validators[0].ClientCtx.Keyring
 	s.encCfg = encoding.MakeEncodingConfig(app.ModuleEncodingRegisters...)
 
-	_, err := s.network.WaitForHeight(1)
+	_, err = s.network.WaitForHeight(1)
 	s.Require().NoError(err)
 }
 
@@ -67,7 +71,7 @@ func (s *IntegrationTestSuite) TearDownSuite() {
 	s.network.Cleanup()
 }
 
-func (s *IntegrationTestSuite) TestSubmitWirePayForData() {
+func (s *IntegrationTestSuite) TestMaxBlockSize() {
 	require := s.Require()
 	assert := s.Assert()
 	val := s.network.Validators[0]
@@ -127,7 +131,7 @@ func (s *IntegrationTestSuite) TestSubmitWirePayForData() {
 				// shares, we should switch to proving txs existence in the block
 				resp, err := queryWithOutProof(val.ClientCtx, hash)
 				assert.NoError(err)
-				assert.Equal(uint32(0), abci.CodeTypeOK)
+				assert.Equal(abci.CodeTypeOK, resp.TxResult.Code)
 				if resp.TxResult.Code == abci.CodeTypeOK {
 					heights[resp.Height]++
 				}
@@ -156,6 +160,63 @@ func (s *IntegrationTestSuite) TestSubmitWirePayForData() {
 		})
 		require.NoError(s.network.WaitForNextBlock())
 	}
+}
+
+func (s *IntegrationTestSuite) TestSubmitWirePayForData() {
+	require := s.Require()
+	assert := s.Assert()
+	val := s.network.Validators[0]
+
+	type test struct {
+		name    string
+		ns      []byte
+		message []byte
+		opts    []types.TxBuilderOption
+	}
+
+	tests := []test{
+		{
+			"small random typical",
+			[]byte{1, 2, 3, 4, 5, 6, 7, 8},
+			tmrand.Bytes(3000),
+			[]types.TxBuilderOption{
+				types.SetFeeAmount(sdk.NewCoins(sdk.NewCoin(app.BondDenom, sdk.NewInt(1)))),
+			},
+		},
+		{
+			"large random typical",
+			[]byte{2, 3, 4, 5, 6, 7, 8, 9},
+			tmrand.Bytes(900000),
+			[]types.TxBuilderOption{
+				types.SetFeeAmount(sdk.NewCoins(sdk.NewCoin(app.BondDenom, sdk.NewInt(10)))),
+			},
+		},
+		{
+			"medium random with memo",
+			[]byte{2, 3, 4, 5, 6, 7, 8, 9},
+			tmrand.Bytes(100000),
+			[]types.TxBuilderOption{
+				types.SetMemo("lol I could stick the rollup block here if I wanted to"),
+			},
+		},
+		{
+			"medium random with timeout height",
+			[]byte{2, 3, 4, 5, 6, 7, 8, 9},
+			tmrand.Bytes(100000),
+			[]types.TxBuilderOption{
+				types.SetTimeoutHeight(1000),
+			},
+		},
+	}
+	for _, tc := range tests {
+		s.Run(tc.name, func() {
+			signer := types.NewKeyringSigner(s.kr, s.accounts[0], val.ClientCtx.ChainID)
+			res, err := payment.SubmitPayForData(context.TODO(), signer, val.ClientCtx.GRPCClient, tc.ns, tc.message, 10000000, tc.opts...)
+			assert.NoError(err)
+			assert.Equal(abci.CodeTypeOK, res.Code)
+			require.NoError(s.network.WaitForNextBlock())
+		})
+	}
 
 }
 

testutil/network/network.go

@@ -2,6 +2,7 @@ package network
 
 import (
 	"fmt"
+	"strings"
 	"testing"
 	"time"
 
@@ -19,6 +20,8 @@ import (
 	banktypes "github.com/cosmos/cosmos-sdk/x/bank/types"
 	tmrand "github.com/tendermint/tendermint/libs/rand"
 	tmdb "github.com/tendermint/tm-db"
+	"google.golang.org/grpc"
+	"google.golang.org/grpc/credentials/insecure"
 )
 
 func New(t *testing.T, config network.Config, genAccNames ...string) *network.Network {
@@ -50,6 +53,15 @@ func New(t *testing.T, config network.Config, genAccNames ...string) *network.Ne
 	return net
 }
 
+// GRPCConn creates and connects a grpc client to the first validator in the
+// network. The resulting grpc client connection is stored in the client context
+func GRPCConn(net *network.Network) error {
+	nodeGRPCAddr := strings.Replace(net.Validators[0].AppConfig.GRPC.Address, "0.0.0.0", "localhost", 1)
+	conn, err := grpc.Dial(nodeGRPCAddr, grpc.WithTransportCredentials(insecure.NewCredentials()))
+	net.Validators[0].ClientCtx.GRPCClient = conn
+	return err
+}
+
 // DefaultConfig will initialize config for the network with custom application,
 // genesis and single validator. All other parameters are inherited from
 // cosmos-sdk/testutil/network.DefaultConfig

testutil/test_app.go

@@ -293,7 +293,7 @@ func genesisStateWithValSet(t *testing.T,
 // the provided accounts
 func GenerateKeyringSigner(t *testing.T, acct string) *types.KeyringSigner {
 	encCfg := encoding.MakeEncodingConfig(app.ModuleEncodingRegisters...)
-	kr := generateKeyring(t, encCfg.Codec)
+	kr := generateKeyring(t, encCfg.Codec, acct)
 	return types.NewKeyringSigner(kr, acct, testChainID)
 }
 

x/payment/payfordata.go

@@ -24,12 +24,14 @@ func SubmitPayForData(
 	gasLim uint64,
 	opts ...types.TxBuilderOption,
 ) (*sdk.TxResponse, error) {
-	pfd, err := BuildPayForData(ctx, signer, conn, nID, data, gasLim)
+	opts = append(opts, types.SetGasLimit(gasLim))
+
+	pfd, err := BuildPayForData(ctx, signer, conn, nID, data, opts...)
 	if err != nil {
 		return nil, err
 	}
 
-	signed, err := SignPayForData(signer, pfd, append(opts, types.SetGasLimit(gasLim))...)
+	signed, err := SignPayForData(signer, pfd, opts...)
 	if err != nil {
 		return nil, err
 	}
@@ -53,7 +55,7 @@ func BuildPayForData(
 	conn *grpc.ClientConn,
 	nID namespace.ID,
 	message []byte,
-	gasLim uint64,
+	opts ...types.TxBuilderOption,
 ) (*types.MsgWirePayForData, error) {
 	// create the raw WirePayForData transaction
 	wpfd, err := types.NewWirePayForData(nID, message, types.AllSquareSizes(len(message))...)
@@ -69,8 +71,7 @@ func BuildPayForData(
 
 	// 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)
+	err = wpfd.SignShareCommitments(signer, opts...)
 	if err != nil {
 		return nil, err
 	}

x/payment/types/builder_options.go

@@ -3,6 +3,8 @@ package types
 import (
 	sdkclient "github.com/cosmos/cosmos-sdk/client"
 	sdk "github.com/cosmos/cosmos-sdk/types"
+	"github.com/cosmos/cosmos-sdk/types/tx"
+	authsigning "github.com/cosmos/cosmos-sdk/x/auth/signing"
 )
 
 type TxBuilderOption func(builder sdkclient.TxBuilder) sdkclient.TxBuilder
@@ -20,3 +22,78 @@ func SetFeeAmount(fees sdk.Coins) TxBuilderOption {
 		return builder
 	}
 }
+
+func SetMemo(memo string) TxBuilderOption {
+	return func(builder sdkclient.TxBuilder) sdkclient.TxBuilder {
+		builder.SetMemo(memo)
+		return builder
+	}
+}
+
+func SetFeePayer(feePayer sdk.AccAddress) TxBuilderOption {
+	return func(builder sdkclient.TxBuilder) sdkclient.TxBuilder {
+		builder.SetFeePayer(feePayer)
+		return builder
+	}
+}
+
+func SetTip(tip *tx.Tip) TxBuilderOption {
+	return func(builder sdkclient.TxBuilder) sdkclient.TxBuilder {
+		builder.SetTip(tip)
+		return builder
+	}
+}
+
+func SetTimeoutHeight(height uint64) TxBuilderOption {
+	return func(builder sdkclient.TxBuilder) sdkclient.TxBuilder {
+		builder.SetTimeoutHeight(height)
+		return builder
+	}
+}
+
+func SetFeeGranter(feeGranter sdk.AccAddress) TxBuilderOption {
+	return func(builder sdkclient.TxBuilder) sdkclient.TxBuilder {
+		builder.SetFeeGranter(feeGranter)
+		return builder
+	}
+}
+
+// InheritTxConfig sets all of the accessible configurations from a given tx
+// into a a give client.TxBuilder
+func InheritTxConfig(builder sdkclient.TxBuilder, tx authsigning.Tx) sdkclient.TxBuilder {
+	if gas := tx.GetGas(); gas != 0 {
+		builder.SetGasLimit(gas)
+	}
+
+	if feeAmmount := tx.GetFee(); !feeAmmount.AmountOf("utia").Equal(sdk.NewInt(0)) {
+		builder.SetFeeAmount(tx.GetFee())
+	}
+
+	if memo := tx.GetMemo(); memo != "" {
+		builder.SetMemo(tx.GetMemo())
+	}
+
+	if tip := tx.GetTip(); tip != nil {
+		builder.SetTip(tip)
+	}
+
+	if timeoutHeight := tx.GetTimeoutHeight(); timeoutHeight != 0 {
+		builder.SetTimeoutHeight(timeoutHeight)
+	}
+
+	signers := tx.GetSigners()
+	// Note: if there are multiple signers in a PFD, then this could create an
+	// invalid signature. This is not an issue at this time because we currently
+	// ignore pfds with multiple signers
+	if len(signers) == 1 {
+		if feePayer := tx.FeeGranter(); !feePayer.Equals(signers[0]) {
+			builder.SetFeeGranter(tx.FeeGranter())
+		}
+	}
+
+	if feeGranter := tx.FeeGranter(); !feeGranter.Empty() {
+		builder.SetFeeGranter(tx.FeeGranter())
+	}
+
+	return builder
+}

x/payment/types/payfordata.go

@@ -81,8 +81,7 @@ func BuildPayForDataTxFromWireTx(
 	if err != nil {
 		return nil, err
 	}
-	builder.SetGasLimit(origTx.GetGas())
-	builder.SetFeeAmount(origTx.GetFee())
+	builder = InheritTxConfig(builder, origTx)
 
 	origSigs, err := origTx.GetSignaturesV2()
 	if err != nil {