Closed rootulp closed 1 year ago
I can't explicitly request a reviewer but I'd appreciate @thanethomson 's opinion on if it makes sense to do this.
Merging #1340 (77b6844) into main (c2b5c9e) will increase coverage by
0.3%
. The diff coverage is75.0%
.:exclamation: Current head 77b6844 differs from pull request most recent head eab906d. Consider uploading reports for the commit eab906d to get more accurate results
@@ Coverage Diff @@
## main #1340 +/- ##
=======================================
+ Coverage 59.3% 59.6% +0.3%
=======================================
Files 273 272 -1
Lines 27095 26929 -166
=======================================
+ Hits 16068 16071 +3
+ Misses 11027 10858 -169
Files Changed | Coverage Δ | |
---|---|---|
light-client-detector/src/evidence.rs | 0.0% <0.0%> (ø) |
|
tendermint/src/validator.rs | 72.9% <85.7%> (+0.2%) |
:arrow_up: |
... and 3 files with indirect coverage changes
:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more
Are there any updates on the review status of this PR?
Bump to above ^
As we are looking closer into this change, it raises some deeper questions about the way this field is serialized.
It's not a good idea to denormalize the total voting power value on the validator::Set
domain type. Instead, if the field is not present in the deserialized JSON or protobuf, it should be recomputed from the validators' voting power values. There is also an issue of trust with impications on protocol security: if we accept the total voting power value without checking it against the sum of listed validators' powers, this means validator::Set
cannot be assumed correct by construction.
We had to skip the field to match the data where it is not present in at least one place in #1292.
Unfortunately, validator::Set
has also had Serialize
and Deserialize
derived, the total_voting_power
field included, and this deserialization schema has probably been in use by parties unknown. I think we can relax this into making total_voting_power
optional on deserialization, but this changes the schema of the accepted deserialized data. OTOH taking the total voting value without validating that it equals the sum of validator's voting powers implies trust in the sender computing it right anyway, so I doubt we'd be breaking code that was not broken for other reasons.
Closing to implement a proper fix for #1348.
Motivated by https://github.com/celestiaorg/celestia-core/issues/1052
I'm a Rust noob so would appreciate a thorough review and/or feedback on if this is desirable.
total_voting_power
in docs).changelog/