CreateCommitment could be expensive for CheckTx

[mpoke]

Cf. https://docs.cosmos.network/v0.45/basics/tx-lifecycle.html#guideline

Gas is not charged when ValidateBasic is executed

This means that an attacker could DOS the system by sending invalid MsgWirePayForDatas (that fail the check here). The ValidateBasic of each msg of type MsgWirePayForData contains a call to CreateCommitment for every commit in msg.MessageShareCommitment. Every call to CreateCommitment entails computing multiple hashes (see here).

[mpoke]

A few comments regarding celestiaorg/celestia-app#666

• ValidateAllSquareSizesCommitedTo is invoked after invoking CreateCommitment for each MessageShareCommitment. IMO, ValidateAllSquareSizesCommitedTo is the cheaper check.
• The check on this line is redundant as it's done here.
• IIUC from ValidateAllSquareSizesCommitedTo, the expectation is that the users have access to both MinSquareSize and MaxSquareSize, and they MUST submit MsgWirePayForData with a MessageShareCommitment for each possible square size. Is that right?

[adlerjohn]

the expectation is that the users have access to both MinSquareSize and MaxSquareSize, and they MUST submit MsgWirePayForData with a MessageShareCommitment for each possible square size. Is that right?

If by "possible" you mean that the data will fit in the square, then yes. This means square sizes that are too small or too large aren't included.

[mpoke]

It also means that MsgWirePayForData transactions that don't have MessageShareCommitments for all possible square sizes (i.e., 1, 2, 4, 8, 16, 32, 64, and 128) will be rejected. Right?

[adlerjohn]

No, it will only be rejected if it doesn't have all valid and non-redundant square sizes. If a data blob is 16 shares, then only commitments for sizes[4, 8, 16] are needed.

[mpoke]

IIUC, for a message that fits in 16 shares (i.e., MsgSharesUsed(msgSize int) returns 16), the AllSquareSizes method returns [8, 16, 32, 64, 128]. This must match the SquareSize of the MessageShareCommitments (cf. https://github.com/celestiaorg/celestia-app/blob/e088d61fcb6579b4bc797deefd2ceff7601aa079/x/payment/types/wirepayfordata.go#L152).

[adlerjohn]

Then that's either a bug in AllSquareSizes, or a bug in how it's used. If a data blob fits on a single row, then all greater square sizes would have the same commitment and so aren't needed.

[rootulp]

Related: celestiaorg/celestia-app#236 which hasn't been implemented yet

[rootulp]

I think this attack is significantly less expensive after the adoption of square size independent message commitments because validating a MsgWirePayForBlob now only includes one CreateCommitment invocation.

Is it safe to close this issue?