commit followup #199

liamsi · 2020-03-31T22:32:32Z

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

- add basic validation to validate method (depending on `BlockIDFlag`) - move check for unknown validator to validate method (for CommitSig) rename iter -> signed_votes and add validation checks

tendermint/src/lite_impl/signed_header.rs

Shivani912 · 2020-04-02T00:19:27Z

tendermint/src/lite_impl/signed_header.rs

@@ -48,6 +44,11 @@ impl lite::Commit for block::signed_header::SignedHeader {
    }

    fn validate(&self, vals: &Self::ValidatorSet) -> Result<(), Error> {
+        // TODO: self.commit.block_id cannot be zero in the same way as in go


oh, in which case do we find self.commit.block_id to be zero in Go?

I think I meant, we validate if it is zero the same way as in golang
https://github.com/tendermint/tendermint/blob/ef56e6661121a7f8d054868689707cd817f27b24/types/block.go#L689-L691
because in the Struct here it is not an Option.

Shivani912 · 2020-04-02T00:24:48Z

tendermint/src/lite_impl/signed_header.rs

+}
+
+// TODO: consider moving this into commit_sig.rs instead and making it pub
+impl block::commit_sig::CommitSig {


yes, I think this should be moved to commit_sig.rs

Will do in a followup PR.

tendermint/src/lite_impl/signed_header.rs

Shivani912 · 2020-04-02T00:33:21Z

tendermint/src/lite_impl/signed_header.rs

+                //
+                // returns ImplementationSpecific error if it detects a signer
+                // that is not present in the validator set:
+                if let Some(val_addr) = self.validator_address {


Maybe move this check into a separate function which is only called in case of full verification?

Will do in a followup PR.

Co-Authored-By: Shivani Joshi <[email protected]>

tendermint/src/lite_impl/signed_header.rs

…smail/commit_followup # Conflicts: # tendermint/src/lite_impl/signed_header.rs

* initial new commit struct * update functions * fixed deserialization for new commit struct * updated links to generator + val_set_test file * Ismail/commit struct followup (#182) * initial new commit struct * update functions * fixed deserialization for new commit struct * updated links to generator + val_set_test file * Update tendermint/src/account.rs Co-Authored-By: Ismail Khoffi <[email protected]> * fmt * clippy * rpc tests: grab latest example fro commit: - use https://docs.tendermint.com/master/rpc/#/Info/commit - update chain_id: cosmoshub-1 to cosmoshub-2 everywhere else - manually kept `id` a string in JSONrpc responses * Actually let's go cosmoshub-3: - grabbed from: https://rpc.cosmos.network/commit?height=2 * Fix commit test - regenerated commit.json via `curl -X GET "http://localhost:26657/commit?height=10" -H "accept: application/json"` * Fix block test - "regenerated" block.json via `curl -X GET "http://localhost:26657/block?height=10" -H "accept: application/json"` * Fix first_block test - "regenerated" block.json via `curl -X GET "http://localhost:26657/block?height=1" -H "accept: application/json" ` Co-authored-by: Shivani Joshi <[email protected]> Co-authored-by: Shivani Joshi <[email protected]> * fix error * more json bisection tests * abci_info struct app_version implemented * Fixed fmt * Updated integration tests to tendermint v0.33 node * update block result response * changes to abci_info and block_results * Removed CircleCI reference and added codecov reference to Cargo * removing unwanted Option from DeliverTx struct fields * last_block_app_hash is now Option<Vec<u8>> * AbciInfo.last_block_app_hash is base64 encoded instead of hex * last_block_app_hash changed to Vec<u8> * Cherry-pick from 87e888b (Liamsi) fix /abci_info & /genesis: - add app_version & use #[serde(default)] to deal with omitted fields in JSON - make app_state in /genesis reply optional - fix string in abci_info test (kvstore wont reply with "GaiaApp") verify tests pass via running `tendermint node --proxy_app=kvstore` and `cargo test -- --nocapture --ignored * [88] Fix JSON ID * [192] Fix abci_info compatibility for v0.33 * [4] block_results test fix * [184][120] Tendermint v0.33 compatibility fixes + integration tests for CI * Added one abci_query test * FMT + get rid of warnings * Relaxed genesis struct AppHash requirements from strict hex to anything * Removed unused Protocol struct * Removed unused Protocol struct * IdType removed and JSON ID made as enum * Duration looks better now * fix fmt * Update tendermint/src/lite_impl/signed_header.rs Co-Authored-By: Ismail Khoffi <[email protected]> * Update tendermint/src/lite_impl/signed_header.rs Co-Authored-By: Ismail Khoffi <[email protected]> * Removed AsRef from JSON ID * Update Proof and ProofOp (#206) * manually update Proof and introduce ProofOp to match the actual JSON encoding * 0.33 branch seems out of date with master, committing to switch back * Fix test * Fix rpc endpoint test (how wasn't this detected before by the integration tests)? * Add missing serializer for TrustThresholdFraction fields (#210) * commit followup (#199) * rename iter -> signed_votes and add validation checks - add basic validation to validate method (depending on `BlockIDFlag`) - move check for unknown validator to validate method (for CommitSig) Co-authored-by: Shivani Joshi <[email protected]> Co-authored-by: Ismail Khoffi <[email protected]> Co-authored-by: Shivani Joshi <[email protected]> Co-authored-by: Romain Ruetschi <[email protected]>

* initial new commit struct * update functions * fixed deserialization for new commit struct * updated links to generator + val_set_test file * Ismail/commit struct followup (#182) * initial new commit struct * update functions * fixed deserialization for new commit struct * updated links to generator + val_set_test file * Update tendermint/src/account.rs Co-Authored-By: Ismail Khoffi <[email protected]> * fmt * clippy * rpc tests: grab latest example fro commit: - use https://docs.tendermint.com/master/rpc/#/Info/commit - update chain_id: cosmoshub-1 to cosmoshub-2 everywhere else - manually kept `id` a string in JSONrpc responses * Actually let's go cosmoshub-3: - grabbed from: https://rpc.cosmos.network/commit?height=2 * Fix commit test - regenerated commit.json via `curl -X GET "http://localhost:26657/commit?height=10" -H "accept: application/json"` * Fix block test - "regenerated" block.json via `curl -X GET "http://localhost:26657/block?height=10" -H "accept: application/json"` * Fix first_block test - "regenerated" block.json via `curl -X GET "http://localhost:26657/block?height=1" -H "accept: application/json" ` Co-authored-by: Shivani Joshi <[email protected]> Co-authored-by: Shivani Joshi <[email protected]> * fix error * more json bisection tests * abci_info struct app_version implemented * Fixed fmt * Updated integration tests to tendermint v0.33 node * update block result response * changes to abci_info and block_results * Removed CircleCI reference and added codecov reference to Cargo * removing unwanted Option from DeliverTx struct fields * last_block_app_hash is now Option<Vec<u8>> * AbciInfo.last_block_app_hash is base64 encoded instead of hex * last_block_app_hash changed to Vec<u8> * Cherry-pick from 87e888b (Liamsi) fix /abci_info & /genesis: - add app_version & use #[serde(default)] to deal with omitted fields in JSON - make app_state in /genesis reply optional - fix string in abci_info test (kvstore wont reply with "GaiaApp") verify tests pass via running `tendermint node --proxy_app=kvstore` and `cargo test -- --nocapture --ignored * [88] Fix JSON ID * [192] Fix abci_info compatibility for v0.33 * [4] block_results test fix * [184][120] Tendermint v0.33 compatibility fixes + integration tests for CI * Added one abci_query test * FMT + get rid of warnings * Relaxed genesis struct AppHash requirements from strict hex to anything * Removed unused Protocol struct * Removed unused Protocol struct * IdType removed and JSON ID made as enum * Duration looks better now * fix fmt * Update tendermint/src/lite_impl/signed_header.rs Co-Authored-By: Ismail Khoffi <[email protected]> * Update tendermint/src/lite_impl/signed_header.rs Co-Authored-By: Ismail Khoffi <[email protected]> * Removed AsRef from JSON ID * Update Proof and ProofOp (#206) * manually update Proof and introduce ProofOp to match the actual JSON encoding * 0.33 branch seems out of date with master, committing to switch back * Fix test * Fix rpc endpoint test (how wasn't this detected before by the integration tests)? * Add missing serializer for TrustThresholdFraction fields (#210) * commit followup (#199) * rename iter -> signed_votes and add validation checks - add basic validation to validate method (depending on `BlockIDFlag`) - move check for unknown validator to validate method (for CommitSig) * Initial support for websocket subscriptions * Websocker error * Fix formatting * Intitial integration test * Fmt * Missed adding file * Fix url scheme * fix url scheme * Format * Debugged event subscription with unit tests * Fix clippy lint * Add back in the integration test ignore * Refactor the websocket listener into a seperate struct from the RPC client * Cargo fmt * Cargo fmt * Fix integration tests * Remove the Box<Errors> * Update tendermint/src/rpc/endpoint/subscribe.rs Co-Authored-By: Ismail Khoffi <[email protected]> * Fix the doc comment for subscribe * Moved a TODO comment to correct place * Create a more specific type for JSON RPC TX Result responses * Add a convinience function for extracting the event hashmap * add an an additional query for helping type the event responses * switch to borrowed types * Remove extra borrow * Remove borrow * Fmt and fix clippy issues * Improve the extract events function so that we treat any matching query in the vectors as valid * Derive Clone for the Event type * Prevent panics in extract events * Fix module query * Type events only on message.action * Ensures all that events end up in the hashmap. A little unclear if this was happening * Revert "Type events only on message.action" This reverts commit 97f5c9b. * Revert "Type events only on message.action" This reverts commit 97f5c9b. * accept periods in the chain-id * Fmt and clippy * review comments * fix build: dbg! macro and catch up with renaming * simplify match in integration test * Fix typo in doc comment * Make queries an enum handle block events and tx events * Refactor Enum to remove data field * Update tendermint/src/rpc/event_listener.rs Co-authored-by: Ismail Khoffi <[email protected]> * Explain why we throw away subscription responses * DRY the top level JSON response type * Update tendermint/src/rpc/endpoint/subscribe.rs Co-authored-by: Ismail Khoffi <[email protected]> * DRY some of the Block types * Fmt * return `Box<dyn stdError>` instead of `&'static str` * Fix clippy warnings and errs * Fix typo * DRY Block, BlockData, Evidence, Parts * Remove raw json responses from the public API * Please clippy * Box large variant * Box the other variant * Update the RPCTxResult to the new api * clarify some local var names * use rpc::response::Response and improve err handling - unexport Wrapper fields * Fix hyperlink in doc comment * Update link on error codes * Make error codes symmetric * Fix panic string * Add some debug output * Add some more debug output * Type alias instead of wrapping the wrapper * Revert "DRY Block, BlockData, Evidence, Parts" This reverts commit 3269d58 * DRY Block, BlockData, Evidence, Parts * Let's try an Option * Another Option? * These events are actually optional * typo * Let it be * Fix match arm Co-authored-by: Shivani Joshi <[email protected]> Co-authored-by: Ismail Khoffi <[email protected]> Co-authored-by: Shivani Joshi <[email protected]> Co-authored-by: Greg Szabo <[email protected]> Co-authored-by: Greg Szabo <[email protected]> Co-authored-by: Romain Ruetschi <[email protected]> Co-authored-by: Anca Zamfir <[email protected]>

liamsi force-pushed the ismail/commit_followup branch 2 times, most recently from 7f20c05 to c51b939 Compare March 31, 2020 22:45

rename iter -> signed_votes and add validation checks

c529e69

- add basic validation to validate method (depending on `BlockIDFlag`) - move check for unknown validator to validate method (for CommitSig) rename iter -> signed_votes and add validation checks

liamsi force-pushed the ismail/commit_followup branch from c51b939 to c529e69 Compare March 31, 2020 22:49

liamsi requested a review from Shivani912 April 1, 2020 20:44

Shivani912 reviewed Apr 2, 2020

View reviewed changes

tendermint/src/lite_impl/signed_header.rs Outdated Show resolved Hide resolved

Shivani912 reviewed Apr 2, 2020

View reviewed changes

tendermint/src/lite_impl/signed_header.rs Outdated Show resolved Hide resolved

Shivani912 reviewed Apr 2, 2020

View reviewed changes

greg-szabo mentioned this pull request Apr 2, 2020

Tendermint/v0.33 #196

Merged

5 tasks

liamsi and others added 2 commits April 10, 2020 18:01

Update tendermint/src/lite_impl/signed_header.rs

7191d32

Co-Authored-By: Shivani Joshi <[email protected]>

TryFrom instead of as

d6be3e0

Co-Authored-By: Shivani Joshi <[email protected]>

liamsi marked this pull request as ready for review April 10, 2020 16:10

Fix import

deecf80

liamsi commented Apr 10, 2020

View reviewed changes

tendermint/src/lite_impl/signed_header.rs Show resolved Hide resolved

Merge remote-tracking branch 'remotes/origin/tendermint/v0.33' into i…

4a1ff69

…smail/commit_followup # Conflicts: # tendermint/src/lite_impl/signed_header.rs

liamsi merged commit 0eae723 into tendermint/v0.33 Apr 10, 2020

liamsi deleted the ismail/commit_followup branch April 10, 2020 16:22

liamsi mentioned this pull request Apr 10, 2020

tendermint/0.33 / #196 followup issue #211

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

commit followup #199

commit followup #199

liamsi commented Mar 31, 2020

Shivani912 Apr 2, 2020

liamsi Apr 10, 2020

Shivani912 Apr 2, 2020

liamsi Apr 10, 2020

Shivani912 Apr 2, 2020

liamsi Apr 10, 2020

commit followup #199

commit followup #199

Conversation

liamsi commented Mar 31, 2020

Shivani912 Apr 2, 2020

Choose a reason for hiding this comment

liamsi Apr 10, 2020

Choose a reason for hiding this comment

Shivani912 Apr 2, 2020

Choose a reason for hiding this comment

liamsi Apr 10, 2020

Choose a reason for hiding this comment

Shivani912 Apr 2, 2020

Choose a reason for hiding this comment

liamsi Apr 10, 2020

Choose a reason for hiding this comment