Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sei remote sign hotfix: aggregate tcp chunks before unmarshal proto #1

Closed
wants to merge 3 commits into from

Commits on Apr 25, 2024

  1. Sei remote sign hotfix: aggregate tcp chunks before unmarshal proto

    ```go
    // tendermint/cometbft proposal:
    type Proposal struct {
    	Type      SignedMsgType
    	Height    int64
    	Round     int32
    	PolRound  int32
    	BlockID   BlockID
    	Timestamp time.Time
    	Signature []byte
    }
    ```
    
    ```go
    // vs sei-tendermint proposal
    type Proposal struct {
    	Type            SignedMsgType
    	Height          int64
    	Round           int32
    	PolRound        int32
    	BlockID         BlockID
    	Timestamp       time.Time
    	Signature       []byte
    
            // this is a list, and can be very long...
    	TxKeys          []*TxKey
    	Evidence        *EvidenceList
    	LastCommit      *Commit
    	Header          Header
    	ProposerAddress []byte
    }
    ```
    
    Since Proposal has TxKeys and other lists, Proposal has variable length
    It is easily goes > 1024 bytes if block has big mount of txs. And it
    is not a problem of canonical tendermint/cometbft implementations
    since due to its message structure, it has a fixed max length < 1024 (DATA_MAX_SIZE)
    
    sei-tendermint, when it connects to remote signer over tcp, sends
    proposal divided by chunk of DATA_MAX_SIZE (1024) each, which kind of
    fits the expectation of tmkms. However, tmkms never tries to
    aggregate chunks. In fact, it is impossible for tmkms to implement
    aggregation properly without knowing the length beforehand: which is
    not provided by tendermint protocol.
    
    There might be a confusioon also, because all implementations of
    tendermint send lenght-delimited protobufs, and tmkms also reads with
    a function "length delimited". However, it actually means that the
    protobuf msg is prepended by it's length: so that when tmkms reads
    1024 bytes it knows which zeroes are payload and which a need to be
    cut. Another words, it has nothing to do with multi-chunk payload.
    
    Which means that sei-tendermint just doesn't bother about tcp
    remote signer, and it is impossible to make it work with tmkms without
    rewriting both and adding this custom protocol of "aggregate chunks until
    you get full message length".
    
    --
    This code implements aggregation by trying to unmarshal aggregated
    message each time it gets a new chunk. I don't think it is a good idea
    in a long run, however, the alternative would be to adjust both Sei
    and tmkms, rolling out new length-aware protocol between them -- I'm
    not sure how sufficient it is and definitely needs a
    discussion. Current solution is compartable with both
    cometbft/tendermint and sei-tendermint, however, way less efficient then
    the original `read` implementation of tmkms.
    
    P.S: Apart from custom length-aware protocol, there is another option:
    implement grpc in tmkms, which seem to be supported by sei-tendermint.
    zarkone committed Apr 25, 2024
    Configuration menu
    Copy the full SHA
    5ab22bb View commit details
    Browse the repository at this point in the history

Commits on Jul 18, 2024

  1. Configuration menu
    Copy the full SHA
    39b4013 View commit details
    Browse the repository at this point in the history

Commits on Aug 28, 2024

  1. Configuration menu
    Copy the full SHA
    bf95b8f View commit details
    Browse the repository at this point in the history