Require MsgWirePayForData and MsgPayForData txs to sign over all relevant square sizes to be considered valid

[evan-forbes]

Currently, someone can easily modify the code to not sign over any square sizes, or only sign over a single square size. Transaction with only a single singed square size will tend to get stuck in mempools, and txs with no signed over square sizes will get stuck in the mempool.

To prevent this, we could add a check to the ValidateBasic methods to ensure that all relevant shares are signed over. This will ensure that WirePayForData transactions do not get stuck in mempools.

more info in relevant square sizes can be found in #236

[rootulp]

I agree that these transactions need to sign over at least one message square to be considered valid. However it seems possible for there to be only one valid square size that a user signs over when the message they send can only fit in the largest possible square k=128 (based on specs and this line). Therefore, it's not clear to me how we should handle the case where only one square size is signed over. I don't think we should throw an error from ValidateBasic() for this scenario because it's valid but unlikely.

[adlerjohn]

There being only one valid configuration isn't contradictory to "we could add a check to the ValidateBasic methods to ensure that all relevant shares are signed over." If it turns out there's only one, then them's the breaks.

[rootulp]

we could add a check to the ValidateBasic methods to ensure that all relevant shares are signed over.

Sorry I don't understand what check needs to be added. Is this not sufficient?

celestia-app/x/payment/types/wirepayfordata.go

Lines 105 to 119 in 1fe223b

	for idx, commit := range msg.MessageShareCommitment {
	// check that each commit is valid
	if !powerOf2(commit.K) {
	return ErrCommittedSquareSizeNotPowOf2.Wrapf("committed to square size: %d", commit.K)
	}
	calculatedCommit, err := CreateCommitment(commit.K, msg.GetMessageNameSpaceId(), msg.Message)
	if err != nil {
	return ErrCalculateCommit.Wrap(err.Error())
	}
	if !bytes.Equal(calculatedCommit, commit.ShareCommitment) {
	return ErrInvalidShareCommit.Wrapf("for square size %d and commit number %v", commit.K, idx)
	}
	}

[evan-forbes]

ideally we would have a function that determines all of the possible square sizes to sign over, not unlike

celestia-app/x/payment/types/square_sizes.go

Lines 24 to 41 in ff2d92a

	// AllSquareSizes calculates all of the square sizes that message could possibly
	// fit in
	func AllSquareSizes(msgSize int) []uint64 {
	allSizes := allSquareSizes
	fitSizes := []uint64{}
	shareCount := MsgSharesUsed(msgSize)
	for _, size := range allSizes {
	// if the number of shares is larger than that in the square, throw an error
	// note, we use k*k-1 here because at least a single share will be reserved
	// for the transaction paying for the message, therefore the max number of
	// shares a message can be is number of shares in square -1.
	if shareCount > (size*size)-1 {
	continue
	}
	fitSizes = append(fitSizes, uint64(size))
	}
	return fitSizes
	}

so the extra check would check that these sizes are signed over

related but separate is an optimized version of this

from #236

This does not account for the fact that commitments do not change if the largest power of two that is smaller than the message can fit on a single row. Meaning that if the largest power of 2 that fits in a message can fit on a single row of square size x, then that commitment is valid for all square sizes >= x.

[rootulp]

Thanks for clarifying @adlerjohn @evan-forbes ! I understand now and think #666 should resolve this issue.