Expand proposal gossip and handling validation rules #292
Conversation
masih
requested review from
philipsu522,
sei-will and
stevenlanders
as code owners
masih
force-pushed
the
masih/gossip-validation
branch
from
1db7c27 to
b710371
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #292 +/- ##
==========================================
+ Coverage 56.94% 57.23% +0.28%
==========================================
Files 254 254
Lines 33815 33815
==========================================
+ Hits 19257 19353 +96
+ Misses 13017 12926 -91
+ Partials 1541 1536 -5 🚀 New features to boost your workflow:
|
Compared to the original Tendermint, Sei-Tendermint expands the proposal structure to include a number of additional fields. Update the basic validation logic to take into account additional fields when validating proposals. Add validation to both direct message handling and ambient gossip handling to avoid propagation of an invalid proposal. Expand tests to cover the additional execution paths at unit and reactor integration level.
masih
force-pushed
the
masih/gossip-validation
branch
from
b710371 to
d12a535
Compare
|
|
Message handler calls validation basic on all types that implement it. Thanks to @sei-will for pointing this out! See: https://github.com/sei-protocol/sei-tendermint/blob/c514864f18f013eb4615ac0006cb6448c15c603e/internal/consensus/msgs.go#L587
The contextual assumption across the codebase is remote messages get validated once at transport. Then the rest of the internal implementation implicitly assumes/or selectively does not check for validity. To reduce code churn revert tests that asserted close-to-logic validity check.
CryptoAddress must have a fixed size. Assert the size in Proposal basic validation.
A couple of error handling seem to be missing in proposal instantiation from proto.
By the time we call validation basic it may be too late, in that the decoding process may have allocated many keys unnecessarily. Check earlier on that the list of keys does not reach max. Also assert that each transaction key length meets the expected value.
Some of the *FromProto functions were not validating the constructed wrapper. Consistently validate all.
| }{ | ||
| {"success", vote, true, true}, | ||
| {"fail vote validate basic", &Vote{}, true, false}, | ||
| {"fail vote validate basic", &Vote{}, false, false}, |
There was a problem hiding this comment.
@codchen Can I ask why this test was specifically asserting that an invalid vote is construct-able from Proto?
I have changed the assertion to reject it (now that all *FromProto functions consistently call validation), and would love your eyes on it please! Thanks
# Conflicts: # types/proposal_test.go
types/proposal.go
Outdated
| // included in a proposal. The limit is determined such that the proposal should | ||
| // hit the gas limit before ever reaching the max transaction keys in order to | ||
| // cap the maximum. | ||
| const maxTxKeysPerProposal = 1_000 |
There was a problem hiding this comment.
it may make sense to pull this from config, but then set the default to 1000? we may need that for load testing depending on how we configure the size of the blocks.
There was a problem hiding this comment.
Are you envisioning a load test that exercises some malicious behaviour? If not, I think we will hit the maximum message size before we hit this limit. i.e. the rationale for this constant is to avoid specific attack vectors that a well behaving user should never hit.
Do you think it is possible to fit this many transactions and not hit the max message size?
There was a problem hiding this comment.
I think our max block size is pretty large and the txs are usually very tiny, we are pretty likely to hit this limit before hitting the size limit
There was a problem hiding this comment.
think this should be safe for production, but just not sure if LT would need to bump it or not, and most likely we need because we want to optimize throughput by packing as many txs in a block as possible for parallel exectuion
There was a problem hiding this comment.
Thank you both. I have made this overridable via environment variable SEI_TENDERMINT_MAX_TX_KEYS_PER_PROPOSAL.
I chose env var instead of config because this constant is used in package level functions and places deep in protobuf decentralisation, where no other "tendermint config" is present.
Considering the use of override, which is load test, I think this is a reasonable path of least resistance forward.
Please let me know if there is anything that I might have missed.
# Conflicts: # types/vote.go
Make max tx keys per proposals overridable via env var `SEI_TENDERMINT_MAX_TX_KEYS_PER_PROPOSAL`.
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Compared to the original Tendermint, Sei-Tendermint expands the proposal structure to include a number of additional fields. Update the basic validation logic to take into account additional fields when validating proposals.
Add validation to both direct message handling and ambient gossip handling to avoid propagation of an invalid proposal.
Expand tests to cover the additional execution paths at unit and reactor integration level.