Problem (CRO-594): block results and "fast sync" are not verified in client #651

yihuang · 2019-12-04T10:09:43Z

Solution:

Store history hashes to COL_APP_HASHS, history state to COL_APP_STATES
Add query_state_batch to rpc client, and verify block results with it.
Make fast-forward syncing optional
Improve BlockGenerator to pass manual synchronizer test

BlockGenerator not support mocking transaction data yet, maybe save it for another PR.

tomtau · 2019-12-04T10:31:56Z

bors try

bors · 2019-12-04T10:48:07Z

try

Build failed

continuous-integration/drone/push

codecov · 2019-12-04T18:15:55Z

Codecov Report

Merging #651 into master will decrease coverage by 0.72%.
The diff coverage is 72.2%.

@@            Coverage Diff             @@
##           master     #651      +/-   ##
==========================================
- Coverage   70.89%   70.16%   -0.73%     
==========================================
  Files         133      133              
  Lines       16197    16384     +187     
==========================================
+ Hits        11483    11496      +13     
- Misses       4714     4888     +174

Impacted Files	Coverage Δ
client-common/src/tendermint/lite.rs	`100% <ø> (ø)`	⬆️
dev-utils/src/commands/genesis_dev_config.rs	`0% <ø> (ø)`	⬆️
client-rpc/src/rpc/wallet_rpc.rs	`72.29% <ø> (ø)`	⬆️
...lient-common/src/tendermint/unauthorized_client.rs	`0% <ø> (ø)`	⬆️
chain-abci/src/storage/mod.rs	`13.04% <ø> (ø)`	⬆️
...work/src/network_ops/default_network_ops_client.rs	`78.43% <ø> (ø)`	⬆️
...re/src/cipher/mock_abci_transaction_obfuscation.rs	`68.8% <ø> (ø)`	⬆️
client-rpc/src/rpc/multisig_rpc.rs	`33.33% <ø> (ø)`	⬆️
client-rpc/src/program.rs	`0% <0%> (ø)`	⬆️
client-cli/src/command.rs	`0% <0%> (ø)`	⬆️
... and 26 more

yihuang · 2019-12-04T18:50:54Z

Give up on debugging of the compile error of chain-core. so I moved back ChainNodeState and related types to chain-abci, and let client-core depends on chain-abci directly.

tomtau · 2019-12-05T01:12:09Z

bors retry

chain-abci/src/app/app_init.rs

tomtau

looks good -- client libraries depending on abci is a bit of an overkill / extra coupling.
It seems for the lite verification, the client doesn't need the whole node state, but only the top level "app state" / the inputs to computing app hash (staked state merkle trie root hash, rewards pool state, network parameters).

Perhaps extracting this needed information to a struct, e.g. something like:

pub struct ChainState {
  last_account_root_hash: H256,
  rewards_pool: RewardsPool,
  network_params: NetworkParameters,
}

that would be in chain-core (it'll just need feature guards for serde),
and have this around:

ChainNodeState could have a field say last_top_level_state: ChainState instead of individual components
compute_app_hash could take &ChainState as argument instead of those components
it could be stored for that historical querying and returned to client

any thoughts?

bors · 2019-12-05T02:00:19Z

try

Build failed

continuous-integration/drone/push

leejw51crypto · 2019-12-05T02:30:26Z

if abci and client needs same data, consider separate to another lower level.
abci ---> another lower level
client -->
so that client not dependent abci

leejw51crypto · 2019-12-05T02:35:53Z

chain-abci/src/app/app_init.rs

+    let encoded_height = i64::encode_var_vec(0);
+    inittx.put(COL_APP_HASHS, &encoded_height, &genesis_state.last_apphash);
+    if write_history_states {
+        inittx.put(COL_APP_STATES, &encoded_height, &encoded);


histroy states implies that vector
how about previos state?

It's stored as key value map: height -> state

leejw51crypto · 2019-12-05T02:36:21Z

chain-abci/src/app/commit.rs

+        let encoded_state = new_state.encode();
+        inittx.put(COL_NODE_INFO, LAST_STATE_KEY, &encoded_state);
+        let encoded_height = i64::encode_var_vec(new_state.last_block_height);
+        inittx.put(COL_APP_HASHS, &encoded_height, &new_state.last_apphash);


COL_APP_HASH?
because it's single

It's actually a map of height -> app_hash

leejw51crypto

lgtm
but do some tidy up and naming

yihuang · 2019-12-05T02:53:46Z

looks good -- client libraries depending on abci is a bit of an overkill / extra coupling.
It seems for the lite verification, the client doesn't need the whole node state, but only the top level "app state" / the inputs to computing app hash (staked state merkle trie root hash, rewards pool state, network parameters).

Perhaps extracting this needed information to a struct, e.g. something like:
pub struct ChainState {
  last_account_root_hash: H256,
  rewards_pool: RewardsPool,
  network_params: NetworkParameters,
}
that would be in chain-core (it'll just need feature guards for serde),
and have this around:

ChainNodeState could have a field say last_top_level_state: ChainState instead of individual components

compute_app_hash could take &ChainState as argument instead of those components

it could be stored for that historical querying and returned to client

any thoughts?

I'll try this. we would have to hard code key length of StarlingFixedKey, to avoid bringing starling into chain-core.

yihuang · 2019-12-05T02:55:01Z

if abci and client needs same data, consider separate to another lower level.
abci ---> another lower level
client -->
so that client not dependent abci

Yeah, chain-core is a good place for this, it's just chain-core are more strict because of the no_std thing,.
I'll try to bring it back to chain-core again.

yihuang · 2019-12-05T08:32:09Z

Moved ChainState into chain-core.
Fixed integration testing.
Fixed enclave build.

tomtau · 2019-12-05T08:51:02Z

integration-tests/docker-compose.yml

@@ -48,7 +48,7 @@ services:
      - ${TENDERMINT_ZEROFEE_RPC_PORT:-16657}:26657
  chain-abci-zerofee:
    image: "${CHAIN_DOCKER_IMAGE:-integration-tests-chain}"
-    command: "/usr/bin/chain-abci --host=0.0.0.0 --port=26658 --chain_id=${CHAIN_ID} --genesis_app_hash=${ZEROFEE_APP_HASH} --enclave_server=tcp://chain-tx-enclave-zerofee:25933 --data=/.storage"
+    command: "/usr/bin/chain-abci --host=0.0.0.0 --port=26658 --chain_id=${CHAIN_ID} --genesis_app_hash=${ZEROFEE_APP_HASH} --enclave_server=tcp://chain-tx-enclave-zerofee:25933 --tx_query=tcp://chain-tx-enclave-zerofee:25933 --data=/.storage"


tx-query address != enclave server

just a note to myself, as this will then need to be fixed in #637

tomtau

lgtm

tomtau · 2019-12-05T08:56:46Z

bors r+

651: Problem (CRO-594): block results and "fast sync" are not verified in client r=tomtau a=yihuang Solution: - Store history hashes to `COL_APP_HASHS`, history state to `COL_APP_STATES` - Add `query_state_batch` to rpc client, and verify block results with it. - Make fast-forward syncing optional - Improve `BlockGenerator` to pass manual synchronizer test `BlockGenerator` not support mocking transaction data yet, maybe save it for another PR. Co-authored-by: yihuang <yi.codeplayer@gmail.com>

bors · 2019-12-05T09:26:23Z

Build failed

continuous-integration/drone/push

tomtau · 2019-12-05T09:50:15Z

bors retry

bors · 2019-12-05T10:40:25Z

Merge conflict (retrying...)

bors · 2019-12-05T11:34:50Z

Merge conflict

…client Solution: - Store history hashes to `COL_APP_HASHS`, history state to `COL_APP_STATES` - Add `query_state_batch` to rpc client, and verify block results with it. - Make fast-forward syncing optional - Improve BlockGenerator to pass manual synchronizer test

yihuang · 2019-12-05T12:32:38Z

Rebased.

tomtau · 2019-12-06T01:55:35Z

bors r+

651: Problem (CRO-594): block results and "fast sync" are not verified in client r=tomtau a=yihuang Solution: - Store history hashes to `COL_APP_HASHS`, history state to `COL_APP_STATES` - Add `query_state_batch` to rpc client, and verify block results with it. - Make fast-forward syncing optional - Improve `BlockGenerator` to pass manual synchronizer test `BlockGenerator` not support mocking transaction data yet, maybe save it for another PR. Co-authored-by: yihuang <yi.codeplayer@gmail.com>

bors · 2019-12-06T02:27:39Z

Build succeeded

2484: Bump cbindgen from 0.16.0 to 0.17.0 r=tomtau a=dependabot-preview[bot] Bumps [cbindgen](https://github.com/eqrion/cbindgen) from 0.16.0 to 0.17.0. <details> <summary>Release notes</summary> <p><em>Sourced from <a href="https://github.com/eqrion/cbindgen/releases">cbindgen's releases</a>.</em></p> <blockquote> <h2>v0.17.0</h2> <ul> <li>Add with_parse_extra_bindings to builder. (<a href="https://github-redirect.dependabot.com/eqrion/cbindgen/issues/645">#645</a>)</li> <li>Support NonZero and fix incorrect simplification of Option into ptr. (<a href="https://github-redirect.dependabot.com/eqrion/cbindgen/issues/647">#647</a>)</li> <li>Deal with name conflicts correctly in declaration type resolution. (<a href="https://github-redirect.dependabot.com/eqrion/cbindgen/issues/651">#651</a>)</li> <li>Support pointers to ZSTs. (<a href="https://github-redirect.dependabot.com/eqrion/cbindgen/issues/656">#656</a>)</li> </ul> </blockquote> </details> <details> <summary>Changelog</summary> <p><em>Sourced from <a href="https://github.com/eqrion/cbindgen/blob/master/CHANGES">cbindgen's changelog</a>.</em></p> <blockquote> <h2>0.17.0</h2> <pre><code> * Add with_parse_extra_bindings to builder. ([#645](mozilla/cbindgen#645)) * Support NonZero and fix incorrect simplification of Option<ptr> into ptr. ([#647](mozilla/cbindgen#647)) * Deal with name conflicts correctly in declaration type resolution. ([#651](mozilla/cbindgen#651)) * Support pointers to ZSTs. ([#656](mozilla/cbindgen#656)) </code></pre> </blockquote> </details> <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/eqrion/cbindgen/commit/8236c8267936ae66785cd236c9c119c2f5b95b57"><code>8236c82</code></a> v0.17.0</li> <li><a href="https://github.com/eqrion/cbindgen/commit/3d41d33ffe1b1f811a5e3e3a8a077b8ec7464150"><code>3d41d33</code></a> config: Deal with clippy lints -.-.</li> <li><a href="https://github.com/eqrion/cbindgen/commit/c5c301b08a6a604d15e4ac6f18cb4ad5a916b4f1"><code>c5c301b</code></a> ir: Support pointers to zsts.</li> <li><a href="https://github.com/eqrion/cbindgen/commit/d3cd22bc6f07ccb595b8b82c8765f9023f2ffcc1"><code>d3cd22b</code></a> doc: Document the <code>documentation</code> config entry.</li> <li><a href="https://github.com/eqrion/cbindgen/commit/d0d287f9f32f4b64017250c0054e2637befa1075"><code>d0d287f</code></a> parser: Deal with a new clippy warning.</li> <li><a href="https://github.com/eqrion/cbindgen/commit/750745831af8755790a42f199b7733b37a3d9e0d"><code>7507458</code></a> ir: Deal with name conflicts correctly in declaration type resolution.</li> <li><a href="https://github.com/eqrion/cbindgen/commit/c47ee1516be72f4278b2c5e6b64a1e1edad13faf"><code>c47ee15</code></a> ir: Handle NonZero and simplify Option<NonZero> like we simplify Option<NonNu...</li> <li><a href="https://github.com/eqrion/cbindgen/commit/f922f68531fa44708af7779ffbb4b118d6e189ae"><code>f922f68</code></a> ir: Avoid generating bogus pointer arguments.</li> <li><a href="https://github.com/eqrion/cbindgen/commit/4ba7d1f0833b2c09b1c77a3012460c937b414f52"><code>4ba7d1f</code></a> ir: Make simplify_standard_types less clone-happy.</li> <li><a href="https://github.com/eqrion/cbindgen/commit/2e1be241671a356516cf8c910ba40c014c7b3144"><code>2e1be24</code></a> ir: Give integers a more structured representation.</li> <li>Additional commits viewable in <a href="https://github.com/eqrion/cbindgen/compare/v0.16.0...v0.17.0">compare view</a></li> </ul> </details> <br /> [![Dependabot compatibility score](https://api.dependabot.com/badges/compatibility_score?dependency-name=cbindgen&package-manager=cargo&previous-version=0.16.0&new-version=0.17.0)](https://dependabot.com/compatibility-score/?dependency-name=cbindgen&package-manager=cargo&previous-version=0.16.0&new-version=0.17.0) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) - `@dependabot use these labels` will set the current labels as the default for future PRs for this repo and language - `@dependabot use these reviewers` will set the current reviewers as the default for future PRs for this repo and language - `@dependabot use these assignees` will set the current assignees as the default for future PRs for this repo and language - `@dependabot use this milestone` will set the current milestone as the default for future PRs for this repo and language - `@dependabot badge me` will comment on this PR with code to add a "Dependabot enabled" badge to your readme Additionally, you can set the following in your Dependabot [dashboard](https://app.dependabot.com): - Update frequency (including time of day and day of week) - Pull request limits (per update run and/or open at any time) - Out-of-range updates (receive only lockfile updates, if desired) - Security updates (receive only security updates, if desired) </details> Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>

tomtau requested review from tomtau, devashishdxt, leejw51crypto, calvinaco, lezzokafka and linfeng-crypto and removed request for tomtau December 4, 2019 10:31

bors bot added a commit that referenced this pull request Dec 4, 2019

Try #651:

657538e

yihuang force-pushed the cro-594 branch from 654dab6 to 35b2805 Compare December 4, 2019 17:31

yihuang force-pushed the cro-594 branch from 35b2805 to 18ba3c8 Compare December 4, 2019 18:48

yihuang force-pushed the cro-594 branch from 18ba3c8 to 38da5bb Compare December 4, 2019 18:52

bors bot added a commit that referenced this pull request Dec 5, 2019

Try #651:

818e0c9

tomtau reviewed Dec 5, 2019

View reviewed changes

chain-abci/src/app/app_init.rs Show resolved Hide resolved

tomtau suggested changes Dec 5, 2019

View reviewed changes

leejw51crypto reviewed Dec 5, 2019

View reviewed changes

leejw51crypto approved these changes Dec 5, 2019

View reviewed changes

yihuang force-pushed the cro-594 branch from 38da5bb to e585476 Compare December 5, 2019 08:05

yihuang requested a review from tomtau December 5, 2019 08:32

tomtau reviewed Dec 5, 2019

View reviewed changes

tomtau approved these changes Dec 5, 2019

View reviewed changes

yihuang mentioned this pull request Dec 5, 2019

Problem (CRO-627) council node pubkey format different from tendermint #657

Merged

yihuang force-pushed the cro-594 branch from e585476 to c223624 Compare December 5, 2019 12:31

bors bot merged commit c223624 into crypto-com:master Dec 6, 2019

yihuang deleted the cro-594 branch December 19, 2019 08:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Problem (CRO-594): block results and "fast sync" are not verified in client #651

Problem (CRO-594): block results and "fast sync" are not verified in client #651

yihuang commented Dec 4, 2019 •

edited

Loading

tomtau commented Dec 4, 2019

bors bot commented Dec 4, 2019

codecov bot commented Dec 4, 2019 •

edited

Loading

yihuang commented Dec 4, 2019

tomtau commented Dec 5, 2019

tomtau left a comment

bors bot commented Dec 5, 2019

leejw51crypto commented Dec 5, 2019

leejw51crypto Dec 5, 2019

yihuang Dec 5, 2019

leejw51crypto Dec 5, 2019

yihuang Dec 5, 2019

leejw51crypto left a comment

yihuang commented Dec 5, 2019 •

edited

Loading

yihuang commented Dec 5, 2019

yihuang commented Dec 5, 2019

tomtau Dec 5, 2019

tomtau left a comment

tomtau commented Dec 5, 2019

bors bot commented Dec 5, 2019

tomtau commented Dec 5, 2019

bors bot commented Dec 5, 2019

bors bot commented Dec 5, 2019

yihuang commented Dec 5, 2019

tomtau commented Dec 6, 2019

bors bot commented Dec 6, 2019

Problem (CRO-594): block results and "fast sync" are not verified in client #651

Problem (CRO-594): block results and "fast sync" are not verified in client #651

Conversation

yihuang commented Dec 4, 2019 • edited Loading

tomtau commented Dec 4, 2019

bors bot commented Dec 4, 2019

try

Build failed

codecov bot commented Dec 4, 2019 • edited Loading

Codecov Report

yihuang commented Dec 4, 2019

tomtau commented Dec 5, 2019

tomtau left a comment

Choose a reason for hiding this comment

bors bot commented Dec 5, 2019

try

Build failed

leejw51crypto commented Dec 5, 2019

leejw51crypto Dec 5, 2019

Choose a reason for hiding this comment

yihuang Dec 5, 2019

Choose a reason for hiding this comment

leejw51crypto Dec 5, 2019

Choose a reason for hiding this comment

yihuang Dec 5, 2019

Choose a reason for hiding this comment

leejw51crypto left a comment

Choose a reason for hiding this comment

yihuang commented Dec 5, 2019 • edited Loading

yihuang commented Dec 5, 2019

yihuang commented Dec 5, 2019

tomtau Dec 5, 2019

Choose a reason for hiding this comment

tomtau left a comment

Choose a reason for hiding this comment

tomtau commented Dec 5, 2019

bors bot commented Dec 5, 2019

Build failed

tomtau commented Dec 5, 2019

bors bot commented Dec 5, 2019

Merge conflict (retrying...)

bors bot commented Dec 5, 2019

Merge conflict

yihuang commented Dec 5, 2019

tomtau commented Dec 6, 2019

bors bot commented Dec 6, 2019

Build succeeded

yihuang commented Dec 4, 2019 •

edited

Loading

codecov bot commented Dec 4, 2019 •

edited

Loading

yihuang commented Dec 5, 2019 •

edited

Loading