-
Notifications
You must be signed in to change notification settings - Fork 216
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
Added single-step-skipping JSON tests #143
Merged
Merged
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
f9d872a
Added single-step-skipping JSON tests
Shivani912 0a30022
Add a check for the parts.hash in commit.validate impl
liamsi bbb07cf
All tests passing
Shivani912 413b780
cargo fmt and delete comment
liamsi 46a3c9d
clean up
Shivani912 d009ab3
Merge branch 'master' of github.com:Shivani912/tendermint-rs
Shivani912 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@liamsi
parts.hash
is themerkleRoot(partsHeader)
which is different from theheader_hash
. That's why this check is making many other tests to fail too. We want to check that thecommit.BlockID
is equal to each of theprecommit.BlockID
. In reality, there can be precommits for other blocks and that's totally fine. But then, those precommits should not be considered for this block. This means that when we calculate the total voting power i.e.signed_power
, we do not count voting power of validators who signed these precommits. Maybe this check should go under thevoting_power_in
method(?)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Thanks for the clarification! To move forward, I'd suggest to merge this, and open an issue referencing this comment and the failing test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, is this about the bug I raised here: #145 (comment) ? We should be testing (either here or in voting_power_in) that the votes being tallied are for the right block ID