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

Aggregate TCP chunks with unmarshal protobuf retry #903

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

zarkone
Copy link

@zarkone zarkone commented Apr 29, 2024

Fixes #729

After successful internal testing on sei-testnet, we've decided to suggest this fix to tmkms upstream.

I've read all the previous discussions and understand the concern that this should belong to tendermint_p2p rather. I'm also agree that tendermint_p2p uses the wrong abstraction in a first place: a stream for a message-oriented protocol.

However, I don't see how it can be fixed inside of tendermint_p2p also: while tendermint / cometbft divides the data by chunks, it doesn't send the full length of data in advance, which means, it is not possible to glue these chunks together.

While his issue was seen only on Sei network, I think that it is rather an issue of tendermint TCP protocol design: it has implicit requirement for length over TCP which was not obvious even for authors of tendermint, it seems. Extending the message size shouldn't break the communication (see expanded below).

Here is the original message from a PR to our fork for the reference:

// tendermint/cometbft proposal:
type Proposal struct {
	Type      SignedMsgType
	Height    int64
	Round     int32
	PolRound  int32
	BlockID   BlockID
	Timestamp time.Time
	Signature []byte
}
// 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.

```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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Protobuf: buffer underflow
1 participant