Skip to content

Latest commit

 

History

History
113 lines (92 loc) · 13.1 KB

2022-12-25 Audit - Celestia Payment Module.md

File metadata and controls

113 lines (92 loc) · 13.1 KB
Raw

Code review of the payment module

  • code is not well structured, most of the methods are in x/payment/types not really grouped based on functionality

Code quality

Transaction Lifecycle

CheckTx

Note: Although the desired outcome is only for MsgWirePayForData messages to be submitted to the application via CheckTx, there is nothing stopping MsgPayForData messages to be in the mempool.

Message Source code Brief description UTs Issues found
MsgWirePayForData ValidateBasic Stateless checks of MsgWirePayForData. TestWirePayForData_ValidateBasic - CreateCommitment could be expensive for CheckTx
- WirePayForData with empty Message
MsgPayForData See DeliverTx section below

CreateCommitment

func CreateCommitment(k uint64, namespace, message []byte) {
    // ...
    shares := msg.SplitIntoShares().RawShares()
    // SplitIntoShares() => list of shares: (nid|len|partial-message1, nid), (nid|len|partial-message2, nid), ...
    // .RawShares() => [nid|len|partial-message1, nid|len|partial-message2 ... ]
    if uint64(len(shares)) > (k*k)-1 {
		return nil, fmt.Errorf("message size exceeds max shares for square size %d: max %d taken %d", k, (k*k)-1, len(shares))
	}
    // TODO this check assumes that only this message gets in the square (no txs or evidence)

    // split shares into leafSets, each of size k or a power of two < k
    //  - create an ErasuredNamespacedMerkleTree subtree for every set in leafSets
    //      - for every leaf in set, nsLeaf=namespace|leaf is added as a leaf in the subtree
    //      via ErasuredNamespacedMerkleTree.Push(nsLeaf)
    //          - prefix share with another namespace !!!
    //      - compute root of subtree
    //  - compute hash of all the subtree roots 
    return merkle.HashFromByteSlices(subTreeRoots)
}

func (msgs Messages) SplitIntoShares() NamespacedShares {
    shares := make([]NamespacedShare, 0)
    for _, m := range msgs.MessagesList {
        rawData, err := m.MarshalDelimited()
        // rawData: len | data
        shares = AppendToShares(shares, m.NamespaceID, rawData)
    }
    return shares
}

func AppendToShares(shares []NamespacedShare, nid namespace.ID, rawData []byte) []NamespacedShare {
    if len(rawData) <= 248 {
        // rawShare: nid | len | data
        // paddedShare: zeroPadIfNecessary up to 256
        share := NamespacedShare{paddedShare, nid}
        // !!! a share has the nid twice !!!
        shares = append(shares, share)
    } else {
        shares = append(shares, splitMessage(rawData, nid)...)
	}
	return shares
}

// splitMessage breaks the data in a message into the minimum number of
// namespaced shares
func splitMessage(rawData []byte, nid namespace.ID) NamespacedShares {
    // result: (nid|len|partialData1, nid), (nid|len|partialData2, nid), ...
}

PrepareProposal

Method Invoked by Brief description UTs Issues found
parsedTxs() PrepareProposal Parse TXs in proposed block. estimate_square_size_test.go - MsgWirePayForData messages are validate twice by the proposer
estimateSquareSize() PrepareProposal Estimate square size using the data in the proposed block. Test_estimateSquareSize - nextPowerOfTwo vs. NextHigherPowerOf2
rawShareCount() estimateSquareSize Calculates the number of shares needed by the block data. NA - [Optimization] MsgSharesUsed requires remainder operation
- The number of TX shares is incremented twice
- Unit length missing from estimating txBytes in rawShareCount
- Number of share estimated by rawShareCount is inaccurate
FitsInSquare estimateSquareSize Uses the non interactive default rules to see if messages of some lengths will fit in a square of squareSize starting at share index cursor. TestFitsInSquare - FitsInSquare is incorrect for edge case
prune PrepareProposal Removes txs until the set of txs will fit in the square. Test_pruning - Inconsistency while pruning compact shares
- Redundancy in PrepareProposal
malleateTxs PrepareProposal Process any MsgWirePayForData transactions into MsgPayForData and their respective messages. NA NA
malleate malleateTxs Split MsgWirePayForData txs into MsgPayForData txs and data. NA - Complex logic for estimating the square size
ProcessWirePayForData malleate Parses the MsgWirePayForData to produce the components needed to create a single MsgPayForData. TestProcessMessage NA
calculateCompactShareCount malleateTxs calculates the exact number of compact shares used Test_calculateCompactShareCount - ShareIndex offset in calculateCompactShareCount
NewCompactShareSplitter calculateCompactShareCount Creates a CompactShareSplitter. TestCompactShareWriter NA
Split PrepareProposal converts block data into encoded shares NA - SparseShareSplitter writer can be simplified
ExtendShares PrepareProposal Erasure the data square. Relies on the rsmt2d library. TestExtendShares NA
NewDataAvailabilityHeader PrepareProposal Generates a DataAvailability header using the provided square size and shares. TestNewDataAvailabilityHeader NA

ProcessProposal

Method Invoked by Brief description UTs Issues found
ProcessProposal ABCI++ TestMessageInclusionCheck - Minor: Inconsistency re. number of messages in PFDs
- Commitments checked only in ProcessProposal
Split ProcessProposal converts block data into encoded shares NA See PrepareProposal
NewDataAvailabilityHeader ProcessProposal Generates a DataAvailability header using the provided square size and shares. TestNewDataAvailabilityHeader See PrepareProposal
GetCommit ProcessProposal Get PFD commitment NA NA
calculateCommitPaths GetCommit Calculates all of the paths to subtree roots needed to create the commitment for a given message. Test_calculateSubTreeRootCoordinates NA
getSubTreeRoot GetCommit Traverses the nmt of the selected row and returns the subtree root. TestEDSSubRootCacher NA

DeliverTx

Note: Only MsgPayForData are delivered to the payment module.

Message Source code Brief description UTs Issues found
MsgPayForData ValidateBasic Stateless checks of MsgPayForData. TestValidateBasic - MsgPayForData.ValidateBasic doesn't invalidate reserved namespaces
MsgPayForData MsgServer.PayForData Execute MsgPayForData: Consume msg.MessageSize amount of gas. TestPayForDataGas None