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.

Sign up for GitHub

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

Jump to bottom

deleted #902

Closed
wants to merge 1 commit into from
Closed

deleted #902

wants to merge 1 commit into from

Conversation

zarkone
Copy link
Contributor

@zarkone zarkone commented Apr 25, 2024 •
edited
Loading

deleted

@zarkone
Sei remote sign hotfix: aggregate tcp chunks before unmarshal proto
f38ca1c 
```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
Copy link
Contributor Author

zarkone commented Apr 25, 2024

sorry for ping: this was intended for internal testing

@zarkone zarkone closed this Apr 25, 2024
@zarkone zarkone changed the title Sei remote sign hotfix: aggregate tcp chunks before unmarshal proto deleted Apr 25, 2024
@zarkone zarkone deleted the aggregate-messages-from-sei branch April 25, 2024 14:59
@tony-iqlusion
Copy link
Member

FWIW, my understanding is tendermint-p2p is supposed to handle the chunk aggregation.

Really it's a bad API on tendermint-p2p's part, where it's trying to use the stream-oriented io::Read API with a message-oriented abstraction. I've talked about that here: informalsystems/tendermint-rs#1392

@zarkone
Copy link
Contributor Author

zarkone commented Apr 25, 2024

@tony-iqlusion right, I've seen this comment and totally agree 👍

This "fix" in commit is rather for testing: we didn't decide yet how to proceed but it is not something I think should go into upstream of tmkms (apart from DATA_MAX_SIZE change maybe): sei-tendermint implementation, to some degree, breaks protocol here I believe (even though it is not explicitly set maybe, but the intention was clearly to keep the max message size fixed ).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@zarkone @tony-iqlusion