CommitSig refactor #277

greg-szabo · 2020-05-21T02:42:14Z

This is a second - slightly less complex - stab at refactoring CommitSig.

Resolves the following:
#247 - CommitSig refactor
#259 - CommitSig timestamp can be "0000" time instead of null (Go compatibility issue)
#260 - CommitSig validator address not present in Absent votes (Go compatibility issue)

The two compatibility issues had to be included or else the tests would have failed. With that said, the compatibility issues should remain open until they are fixed in the Go code. The fixed code is also included here and it's commented out.

Please review and comment. We can change the way we deal with the compatibility code, if this looks unruly.

- @melekes had two questions in a previous review of this code: where is the validator_address validation (I think that was not implemented before) and why is there a TODO about full verification and what should we do about it. - I would like to clarify these and open issues around them or resolve them in this PR, if we don't need to do anything about them.
Anton's questions were either answered in the PR or a new issue opened about it.

~~Because of the above questions, I'm putting this PR into "draft" mode, although it's ready to be reviewed.~~

Referenced an issue explaining the need for the change
Updated all relevant documentation in docs
Updated all code comments where relevant
Wrote tests
Updated CHANGES.md

greg-szabo · 2020-05-21T02:51:08Z

Note: I just realized the note in #266 that the compatibility issues don't need to be implemented. But the current tests would fail because the incompatible tests are not separated yet.

codecov-commenter · 2020-05-21T02:56:56Z

Codecov Report

Merging #277 into master will increase coverage by 0.7%.
The diff coverage is 43.9%.

@@           Coverage Diff            @@
##           master    #277     +/-   ##
========================================
+ Coverage    26.9%   27.7%   +0.7%     
========================================
  Files         105     106      +1     
  Lines        3844    3861     +17     
  Branches     1217    1223      +6     
========================================
+ Hits         1037    1072     +35     
+ Misses       1963    1917     -46     
- Partials      844     872     +28

Impacted Files	Coverage Δ
tendermint/src/serializers/custom.rs	`20.6% <ø> (+7.6%)`	⬆️
tendermint/src/block/commit_sig.rs	`20.5% <19.4%> (-6.2%)`	⬇️
tendermint/src/serializers/raw_commit_sig.rs	`26.6% <30.7%> (ø)`
tendermint/src/lite_impl/signed_header.rs	`52.8% <54.5%> (+9.2%)`	⬆️
tendermint/src/serializers/tests.rs	`53.8% <64.7%> (+6.4%)`	⬆️
tendermint/src/rpc/event_listener.rs	`0.0% <0.0%> (ø)`
tendermint/src/time.rs	`38.2% <0.0%> (+4.2%)`	⬆️
tendermint/src/signature.rs	`38.0% <0.0%> (+9.5%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d28c4b1...dee63d2. Read the comment docs.

greg-szabo · 2020-05-21T10:53:55Z

I'm scratching my head at the test failure, it worked on my machine. I'll look into it in a second.

As for @melekes 's question, I think I realize what he was asking: the validator_address is an account::Id which is deserialized from a hexstring into a 20 long byte array. That is restricted during deserialization. Anything else will panic. (Except in the case of CommitSig, where the Go code enables empty strings as no-address values.)

If that was the question and the answer is good enough, I'd like to remove the comment about it in the source code.

greg-szabo · 2020-05-21T10:54:34Z

tendermint/src/block/commit_sig.rs

+impl TryFrom<RawCommitSig> for CommitSig {
+    type Error = &'static str;
+
+    // Todo: @melekes asked: where's validator_address validation?


I think I realize what he was asking: the validator_address is an account::Id which is deserialized from a hexstring into a 20 long byte array. That is restricted during deserialization. Anything else will panic. (Except in the case of CommitSig, where the Go code enables empty strings as no-address values.)

If that was the question and the answer is good enough, I'd like to remove the comment about it.

liamsi · 2020-05-21T12:28:53Z

But the current tests would fail because the incompatible tests are not separated yet.

Let's jump on a call for this. I think they are separated in the sense that you can skip single JSON files to make an exception and treat them differently (e.g expect a panic, or make sure that they already fail on deserialisation instead of later <- these exceptions aren't written yet, only the possibly to skip and treat them separately)

…ification

greg-szabo · 2020-05-25T15:10:24Z

Good news, tests succeed now, no need for a call. I've resolved all outstanding questions one way or another. Feel free to review and comment.

liamsi

Left a few (minor) comments. My understanding that in a few places the TryFroms can panic instead of error'ing. We should rather return an error in this case.

As a general remark: I find the many comments of the form (in the production code as well as the tests):

// <<< Compatibility code for https://github.com/informalsystems/tendermint-rs/issues/nnn

// some lines of used rust code here

// === Real code after compatibility issue is resolved
/*
// Some few lines of commented out Rust code here
*/
// >>> end of real code

make the code difficult to read. I'm wondering if we should rather track these changes in an issue instead? Or in an ADR? I think that would be more appropriate. On the other hand the comments won't stay in the code for a long time (let's say a few days or weeks only) I assume and then it's super cool to switch to that implementation easily 🤔

tendermint/src/block/commit_sig.rs

liamsi · 2020-05-26T00:06:43Z

tendermint/src/block/commit_sig.rs

+                if value.timestamp.is_none() {
+                    Err("timestamp is null for BlockIDFlagNil CommitSig")
+                } else if value.signature.is_none() {
+                    Err("signature is null for BlockIDFlagNil CommitSig")


s/missing/null/g

liamsi · 2020-05-26T00:08:51Z

tendermint/src/block/commit_sig.rs

+                /*
+                Ok(CommitSig::BlockIDFlagAbsent {
+                    validator_address: value.validator_address,
+                })


Did we actually clarify if that will be changed on the go-side too? (will the validator_address be added back in the case BlockIDFlagAbsent? Or is the ADR simply out of date? Sorry, I can't remember if that was answered somewhere else.

It seems to me that the issue was acknowledged on the Go side. It was tagged as a consensus-related issue and there's 73 of that open. Also, based on the note, the fix for this might be postponed or fully cancelled because of discussions on refactoring messaging.

tendermint/tendermint#4797

it's tricky to get ValidatorAddress from a nil vote. So, if we decide to have ValidatorAddress for absent votes, we'll need to remove the concept of nil votes in Tendermint, which may be a good idea on its own.

tendermint/src/serializers/raw_commit_sig.rs

liamsi · 2020-05-26T00:19:56Z

tendermint/src/block/commit_sig.rs

+                if value.timestamp.is_some() {
+                    // <<< Compatibility code for https://github.com/informalsystems/tendermint-rs/issues/259
+                    if value.timestamp.unwrap()
+                        != Time::parse_from_rfc3339("0001-01-01T00:00:00Z").unwrap()


liamsi · 2020-05-26T00:20:50Z

tendermint/src/block/commit_sig.rs

+                if value.timestamp.is_none() {
+                    Err("timestamp is null for BlockIDFlagCommit CommitSig")
+                } else if value.signature.is_none() {
+                    Err("signature is null for BlockIDFlagCommit CommitSig")


s/null/missing

Co-authored-by: Ismail Khoffi <Ismail.Khoffi@gmail.com>

greg-szabo · 2020-05-26T04:04:32Z

So, based on the priorities in the Tendermint Go code, I wouldn't hold my breath that issue #260 (or #259) will be resolved soon. It's not a real problem for us either, the workarounds are ok. But if you want readability, we should just cut our losses ("the real code") and get on with life.

I'll leave a note where the code needs changing that points to the issues. If we close the issues we should fix/remove those comments.

…O messages with issue number

greg-szabo · 2020-05-26T05:03:00Z

So, all compatibility remarks removed. They were already in issues and the issues were mentioned in that code. The problem is that some of the compatibility changes require changes in multiple files and we're littering our codebase with sub-optimal code because of Go incompatibility things. Right now Issue #260 requires changes in 4 files and a total of 6 places. The code was written for it but now the right code is removed and workarounds were put in place. (I guess whoever fixes #260 can come back to this PR and see what was done.)

If there's a good way to keep track of compatibility decisions in code (things like "why is this an Option<>?" or "why is this check here?"), it would be nice to somehow do it. My example was really just testing if this would be ok.

All above remarks are addressed, let's try another review.

liamsi · 2020-05-26T10:32:09Z

If there's a good way to keep track of compatibility decisions in code (things like "why is this an Option<>?" or "why is this check here?")

If we followed it consequently, ADRs instead of in the are the best place for longer descriptions IMO. Other than that, regular comments in the code can explain why certain things are as they are.

liamsi

👍 Thanks Greg!

CommitSig refactor

f381650

greg-szabo requested a review from liamsi May 21, 2020 02:42

greg-szabo marked this pull request as draft May 21, 2020 02:42

greg-szabo added 2 commits May 20, 2020 22:44

Cargo fmt and clippy

f35cf8e

CHANGES.md fix

e6eb53e

greg-szabo mentioned this pull request May 21, 2020

CommitSig Serialization refactor #258

Closed

5 tasks

CommitSig internal tests

b91bfaf

greg-szabo commented May 21, 2020

View reviewed changes

Removed comment asking for clarificiation about validator_address ver…

3125b77

…ification

greg-szabo mentioned this pull request May 25, 2020

Resolve "this last check is only necessary if we do full verification (2/3)" comment #281

Closed

Opened issue and tagged it about the code comment

d214e9d

greg-szabo marked this pull request as ready for review May 25, 2020 15:03

Merge branch 'master' into greg/commitsig

a23d019

liamsi requested changes May 26, 2020

View reviewed changes

greg-szabo and others added 2 commits May 25, 2020 23:40

Update tendermint/src/block/commit_sig.rs

c634764

Co-authored-by: Ismail Khoffi <Ismail.Khoffi@gmail.com>

null -> missing in error messages

ad16064

greg-szabo added 2 commits May 26, 2020 00:55

Removed commented code that fixes compatibility workarounds. Left TOD…

fe15f62

…O messages with issue number

Typo fix

dee63d2

liamsi approved these changes May 26, 2020

View reviewed changes

liamsi merged commit afb11bf into master May 26, 2020

liamsi deleted the greg/commitsig branch May 26, 2020 10:33

greg-szabo mentioned this pull request May 29, 2020

[pending Go changes] CommitSig validator address missing in Absent vote #260

Closed

This was referenced Jun 3, 2020

commit followup #199

Merged

CommitSig Timestamp can be zero time #259

Closed

tendermint/0.33 / #196 followup issue #211

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CommitSig refactor #277

CommitSig refactor #277

greg-szabo commented May 21, 2020 •

edited

Loading

greg-szabo commented May 21, 2020

codecov-commenter commented May 21, 2020 •

edited

Loading

greg-szabo commented May 21, 2020

greg-szabo May 21, 2020

liamsi commented May 21, 2020

greg-szabo commented May 25, 2020

liamsi left a comment

liamsi May 26, 2020

greg-szabo May 26, 2020

liamsi May 26, 2020

greg-szabo May 26, 2020

liamsi May 26, 2020

liamsi May 26, 2020

greg-szabo May 26, 2020

greg-szabo commented May 26, 2020

greg-szabo commented May 26, 2020

liamsi commented May 26, 2020

liamsi left a comment

CommitSig refactor #277

CommitSig refactor #277

Conversation

greg-szabo commented May 21, 2020 • edited Loading

greg-szabo commented May 21, 2020

codecov-commenter commented May 21, 2020 • edited Loading

Codecov Report

greg-szabo commented May 21, 2020

Choose a reason for hiding this comment

liamsi commented May 21, 2020

greg-szabo commented May 25, 2020

liamsi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg-szabo commented May 26, 2020

greg-szabo commented May 26, 2020

liamsi commented May 26, 2020

liamsi left a comment

Choose a reason for hiding this comment

greg-szabo commented May 21, 2020 •

edited

Loading

codecov-commenter commented May 21, 2020 •

edited

Loading