Problem: client can't trust validators field in genesis api (CRO-101) by yihuang · Pull Request #584 · crypto-com/thaler

yihuang · 2019-11-13T07:38:03Z

Solution:
Validate validators in RequestInitChain, make sure the consistency.

tomtau

bors r+

bors · 2019-11-13T09:02:27Z

Merge conflict (retrying...)

bors · 2019-11-13T09:44:31Z

Canceled

yihuang · 2019-11-13T09:47:16Z

Fixed the failed tests.

bors · 2019-11-13T10:30:15Z

🔒 Permission denied

Existing reviewers: click here to make yihuang a reviewer

codecov · 2019-11-13T10:44:34Z

Codecov Report

Merging #584 into master will decrease coverage by 0.1%.
The diff coverage is 93.61%.

@@            Coverage Diff             @@
##           master     #584      +/-   ##
==========================================
- Coverage   67.42%   67.32%   -0.11%     
==========================================
  Files         125      125              
  Lines       14700    14671      -29     
==========================================
- Hits         9912     9877      -35     
- Misses       4788     4794       +6

Impacted Files	Coverage Δ
chain-abci/src/app/jail_account.rs	`100% <ø> (+1.07%)`	⬆️
chain-abci/src/app/app_init.rs	`66.28% <90%> (-0.25%)`	⬇️
chain-abci/tests/abci_app.rs	`94.09% <91.66%> (-0.06%)`	⬇️
chain-abci/tests/punishment.rs	`96.71% <96%> (-0.06%)`	⬇️
chain-abci/src/app/mod.rs	`84.92% <0%> (-0.4%)`	⬇️
chain-core/src/common/merkle_tree.rs	`98.55% <0%> (-0.25%)`	⬇️

chain-core/src/state/tendermint.rs

tomtau · 2019-11-14T00:28:55Z

bors r+

579: Problem:(CRO-574) integration test cannot run on linux r=tomtau a=leejw51crypto Solution: add fixes for linux fix env error add sudo 580: Bump unicase from 2.5.1 to 2.6.0 r=tomtau a=dependabot-preview[bot] Bumps [unicase](https://github.com/seanmonstar/unicase) from 2.5.1 to 2.6.0. <details> <summary>Release notes</summary> *Sourced from [unicase's releases](https://github.com/seanmonstar/unicase/releases).* > ## v2.6.0 > - Fix `UniCase::eq` in Unicode mode so that it doesn't equal a substring of the other. > - Make crate `no_std`. </details> <details> <summary>Commits</summary> - [`7b116bc`](seanmonstar/unicase@7b116bc) v2.6.0 - [`c14856b`](seanmonstar/unicase@c14856b) Fix Unicode::eq to not equal when one side is a substring of the other ([#39](https://github-redirect.dependabot.com/seanmonstar/unicase/issues/39)) - [`4788cba`](seanmonstar/unicase@4788cba) update version_check - [`fbab380`](seanmonstar/unicase@fbab380) Implement no_std support ([#34](https://github-redirect.dependabot.com/seanmonstar/unicase/issues/34)) - See full diff in [compare view](seanmonstar/unicase@v2.5.1...v2.6.0) </details> <br /> [![Dependabot compatibility score](https://api.dependabot.com/badges/compatibility_score?dependency-name=unicase&package-manager=cargo&previous-version=2.5.1&new-version=2.6.0)](https://dependabot.com/compatibility-score.html?dependency-name=unicase&package-manager=cargo&previous-version=2.5.1&new-version=2.6.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> 584: Problem: client can't trust validators field in genesis api (CRO-101) r=tomtau a=yihuang Solution: Validate validators in ``RequestInitChain``, make sure the consistency. Co-authored-by: jongwhan lee <leejw51@gmail.com> Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com> Co-authored-by: yihuang <huang@crypto.com>

bors · 2019-11-14T02:04:27Z

Build failed (retrying...)

continuous-integration/travis-ci/push

580: Bump unicase from 2.5.1 to 2.6.0 r=tomtau a=dependabot-preview[bot] Bumps [unicase](https://github.com/seanmonstar/unicase) from 2.5.1 to 2.6.0. <details> <summary>Release notes</summary> *Sourced from [unicase's releases](https://github.com/seanmonstar/unicase/releases).* > ## v2.6.0 > - Fix `UniCase::eq` in Unicode mode so that it doesn't equal a substring of the other. > - Make crate `no_std`. </details> <details> <summary>Commits</summary> - [`7b116bc`](seanmonstar/unicase@7b116bc) v2.6.0 - [`c14856b`](seanmonstar/unicase@c14856b) Fix Unicode::eq to not equal when one side is a substring of the other ([#39](https://github-redirect.dependabot.com/seanmonstar/unicase/issues/39)) - [`4788cba`](seanmonstar/unicase@4788cba) update version_check - [`fbab380`](seanmonstar/unicase@fbab380) Implement no_std support ([#34](https://github-redirect.dependabot.com/seanmonstar/unicase/issues/34)) - See full diff in [compare view](seanmonstar/unicase@v2.5.1...v2.6.0) </details> <br /> [![Dependabot compatibility score](https://api.dependabot.com/badges/compatibility_score?dependency-name=unicase&package-manager=cargo&previous-version=2.5.1&new-version=2.6.0)](https://dependabot.com/compatibility-score.html?dependency-name=unicase&package-manager=cargo&previous-version=2.5.1&new-version=2.6.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> 584: Problem: client can't trust validators field in genesis api (CRO-101) r=tomtau a=yihuang Solution: Validate validators in ``RequestInitChain``, make sure the consistency. Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com> Co-authored-by: yihuang <huang@crypto.com>

bors · 2019-11-14T02:24:41Z

Build failed (retrying...)

continuous-integration/drone/push

584: Problem: client can't trust validators field in genesis api (CRO-101) r=tomtau a=yihuang Solution: Validate validators in ``RequestInitChain``, make sure the consistency. Co-authored-by: yihuang <huang@crypto.com>

bors · 2019-11-14T03:10:55Z

Build failed

continuous-integration/drone/push

tomtau

seems the chain-abci crashes in integration test due to validators not being consistent -- i think it's because tendermint init is called multiple times to create "genesis.json", from the first one, it takes the pubkey value used in app_data... and it appends it the same app_data (+ copies the validator key). it's ok if the node was the where tendermint init was called, but it'd fail with a different node, as the validators field may be different.
so i guess updating the prepare.sh of integration tests may be necessary -- @calvinaco @calvinlauco is that correct?

592: Problem: duplicate jailing/slashing test setup code. CRO-573 r=tomtau a=yihuang Solution: Refactored into more modular code. Currently removed checking on validators of `ResponseInitChain`, because #584 will change that. Co-authored-by: yihuang <huang@crypto.com>

calvinlauyh · 2019-11-15T12:51:13Z

seems the chain-abci crashes in integration test due to validators not being consistent -- i think it's because tendermint init is called multiple times to create "genesis.json", from the first one, it takes the pubkey value used in app_data... and it appends it the same app_data (+ copies the validator key). it's ok if the node was the where tendermint init was called, but it'd fail with a different node, as the validators field may be different.
so i guess updating the prepare.sh of integration tests may be necessary -- @calvinaco @calvinlauco is that correct?

prepare script run tendermint init only once. It then clones into two sets (zerofee and withfee) and updates the genesis with different fee and other configs. So the validator keys should be kept consistent all the time.

yihuang · 2019-11-15T13:46:37Z

Moved tests in chain-abci/src/app/jail_account.rs to chain-abci/tests/punishment.rs to use the encapsulated TestEnv, there seems to be no easy way to share code between tests.

calvinlauyh · 2019-11-15T13:55:10Z

I think the inconsistent validator is because of the voting power difference:

// validator
[
    pub_key {
      type: "ed25519"
      data: "\362JiJ\245\347\332\253\260k\351\357\263\304:\340`\264\251\323\234E\220E\3550\030e\213\250\367z"
    }
    power: 12500000000
    ,
]

// req_validator
[
    pub_key {
      type: "ed25519"
      data: "\362JiJ\245\347\332\253\260k\351\357\263\304:\340`\264\251\323\234E\220E\3550\030e\213\250\367z"
    }
    power: 10
    ,
]

Solution: Validate validators in RequestInitChain, make sure the consistency.

yihuang · 2019-11-15T15:37:04Z

Integration testing prepare.sh need to update genesis.conf to set validator voting power to correct value.

calvinlauyh · 2019-11-15T15:46:53Z

Integration testing prepare.sh need to update genesis.conf to set validator voting power to correct value.

@yihuang
Yes, 10 is the default value from tendermint init. I will create a fix now.

calvinlauyh · 2019-11-15T15:53:14Z

Integration testing prepare.sh need to update genesis.conf to set validator voting power to correct value.

@yihuang
Yes, 10 is the default value from tendermint init. I will create a fix now.

Actually, it may be even better to update in dev-utils? Because the initial council node information is processed there. What do you think?

yihuang · 2019-11-15T16:09:41Z

Integration testing prepare.sh need to update genesis.conf to set validator voting power to correct value.

@yihuang
Yes, 10 is the default value from tendermint init. I will create a fix now.

Actually, it may be even better to update in dev-utils? Because the initial council node information is processed there. What do you think?

But currently, dev-utils only output app_hash and app_state, which get appended to genesis.json.

calvinlauyh · 2019-11-15T16:25:29Z

Integration testing prepare.sh need to update genesis.conf to set validator voting power to correct value.

@yihuang
Yes, 10 is the default value from tendermint init. I will create a fix now.

Actually, it may be even better to update in dev-utils? Because the initial council node information is processed there. What do you think?

But currently, dev-utils only output app_hash and app_state, which get appended to genesis.json.

Yes, this requires user to update power in genesis.json manually now. Perhaps we should update the setup doc first @lezzokafka . A better solution should make this more automatic.

But the quick fix I am working on will update the prepare.sh first.

yihuang · 2019-11-15T16:31:14Z

Integration testing prepare.sh need to update genesis.conf to set validator voting power to correct value.

@yihuang
Yes, 10 is the default value from tendermint init. I will create a fix now.

Actually, it may be even better to update in dev-utils? Because the initial council node information is processed there. What do you think?

But currently, dev-utils only output app_hash and app_state, which get appended to genesis.json.

Yes, this requires user to update power in genesis.json manually now. Perhaps we should update the setup doc first @lezzokafka . A better solution should make this more automatic.

But the quick fix I am working on will update the prepare.sh first.

I just wrote a python script, i don't know how to do this in bash ;D

$ cat integration-tests/fix_genesis.py
import sys
import base64
import hashlib
import json

GENESIS = json.load(open(sys.argv[1]))


def get_voting_power(state):
    cointype, coin = state
    assert cointype == 'Bonded'
    return str(int(int(coin) / (10 ** 8)))


def validator_addr(pubkey_base64):
    return hashlib.sha256(base64.b64decode(pubkey_base64)).hexdigest().upper()[:40]


GENESIS['validators'] = [
    {
        'address': validator_addr(node[2]['consensus_pubkey_b64']),
        'pub_key': {
            'type': 'tendermint/PubKeyEd25519',
            'value': node[2]['consensus_pubkey_b64'],
        },
        'power': get_voting_power(GENESIS['app_state']['distribution'][addr]),
    }
    for addr, node in GENESIS['app_state']['council_nodes'].items()
]

json.dump(GENESIS, open(sys.argv[1], 'w'), indent=4)

calvinlauyh · 2019-11-15T16:43:01Z

Integration testing prepare.sh need to update genesis.conf to set validator voting power to correct value.

@yihuang
Yes, 10 is the default value from tendermint init. I will create a fix now.

Actually, it may be even better to update in dev-utils? Because the initial council node information is processed there. What do you think?

But currently, dev-utils only output app_hash and app_state, which get appended to genesis.json.

Yes, this requires user to update power in genesis.json manually now. Perhaps we should update the setup doc first @lezzokafka . A better solution should make this more automatic.
But the quick fix I am working on will update the prepare.sh first.

I just wrote a python script ;D
$ cat integration-tests/fix_genesis.py
import sys
import base64
import hashlib
import json

GENESIS = json.load(open(sys.argv[1]))


def get_voting_power(state):
    cointype, coin = state
    assert cointype == 'Bonded'
    return str(int(int(coin) / (10 ** 8)))


def validator_addr(pubkey_base64):
    return hashlib.sha256(base64.b64decode(pubkey_base64)).hexdigest().upper()[:40]


GENESIS['validators'] = [
    {
        'address': validator_addr(node[2]['consensus_pubkey_b64']),
        'pub_key': {
            'type': 'tendermint/PubKeyEd25519',
            'value': node[2]['consensus_pubkey_b64'],
        },
        'power': get_voting_power(GENESIS['app_state']['distribution'][addr]),
    }
    for addr, node in GENESIS['app_state']['council_nodes'].items()
]

json.dump(GENESIS, open(sys.argv[1], 'w'), indent=4)

Awesome! Let's use this script first 😜

Solution: Add a python script to modify validates according to app_state.

calvinaco · 2019-11-15T17:02:15Z

bors try

bors · 2019-11-15T17:48:49Z

try

Build succeeded

tomtau

bors r+

584: Problem: client can't trust validators field in genesis api (CRO-101) r=tomtau a=yihuang Solution: Validate validators in ``RequestInitChain``, make sure the consistency. Co-authored-by: yihuang <huang@crypto.com>

bors · 2019-11-18T00:52:44Z

Build failed

continuous-integration/drone/push

tomtau · 2019-11-18T01:05:39Z

the failed build seems related to this: #595

tomtau · 2019-11-18T04:47:01Z

bors retry

584: Problem: client can't trust validators field in genesis api (CRO-101) r=tomtau a=yihuang Solution: Validate validators in ``RequestInitChain``, make sure the consistency. 586: Problem:(CRO-540) syncing does not work in HD wallet r=tomtau a=leejw51 Solution: add private, public keys to storage Co-authored-by: yihuang <huang@crypto.com> Co-authored-by: jongwhan lee <leejw51@gmail.com>

bors · 2019-11-18T05:31:39Z

Build succeeded

yihuang requested review from calvinaco, devashishdxt, leejw51crypto, linfeng-crypto and tomtau November 13, 2019 07:41

devashishdxt approved these changes Nov 13, 2019

View reviewed changes

yihuang force-pushed the cro-101-validate-validators branch 2 times, most recently from 3a275f7 to 4ca1a18 Compare November 13, 2019 08:09

tomtau approved these changes Nov 13, 2019

View reviewed changes

yihuang force-pushed the cro-101-validate-validators branch from 4ca1a18 to d0d5aab Compare November 13, 2019 09:44

tomtau suggested changes Nov 13, 2019

View reviewed changes

chain-core/src/state/tendermint.rs Outdated Show resolved Hide resolved

yihuang force-pushed the cro-101-validate-validators branch from d0d5aab to 0c40b8e Compare November 13, 2019 11:04

yihuang requested a review from tomtau November 13, 2019 11:17

tomtau suggested changes Nov 14, 2019

View reviewed changes

leejw51crypto approved these changes Nov 14, 2019

View reviewed changes

yihuang mentioned this pull request Nov 15, 2019

Problem: duplicate jailing/slashing test setup code. CRO-573 #592

Merged

yihuang force-pushed the cro-101-validate-validators branch from 0c40b8e to 1221188 Compare November 15, 2019 13:44

Problem: client can't trust validators field in genesis api (CRO-101)

93a809b

Solution: Validate validators in RequestInitChain, make sure the consistency.

yihuang force-pushed the cro-101-validate-validators branch from 1221188 to 93a809b Compare November 15, 2019 14:03

Problem: prepare.sh don't modify validators field in genesis.conf

16526d5

Solution: Add a python script to modify validates according to app_state.

calvinlauyh approved these changes Nov 15, 2019

View reviewed changes

bors bot added a commit that referenced this pull request Nov 15, 2019

Try #584:

e806f7c

tomtau approved these changes Nov 18, 2019

View reviewed changes

bors bot merged commit 16526d5 into crypto-com:master Nov 18, 2019

yihuang deleted the cro-101-validate-validators branch December 19, 2019 08:23

Conversation

yihuang commented Nov 13, 2019

Uh oh!

tomtau left a comment

Choose a reason for hiding this comment

Uh oh!

bors bot commented Nov 13, 2019

Merge conflict (retrying...)

Uh oh!

bors bot commented Nov 13, 2019

Canceled

Uh oh!

yihuang commented Nov 13, 2019

Uh oh!

bors bot commented Nov 13, 2019

Uh oh!

codecov bot commented Nov 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

tomtau commented Nov 14, 2019

Uh oh!

bors bot commented Nov 14, 2019

Build failed (retrying...)

Uh oh!

bors bot commented Nov 14, 2019

Build failed (retrying...)

Uh oh!

bors bot commented Nov 14, 2019

Build failed

Uh oh!

tomtau left a comment

Choose a reason for hiding this comment

Uh oh!

calvinlauyh commented Nov 15, 2019

Uh oh!

yihuang commented Nov 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

calvinlauyh commented Nov 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yihuang commented Nov 15, 2019

Uh oh!

calvinlauyh commented Nov 15, 2019

Uh oh!

calvinlauyh commented Nov 15, 2019

Uh oh!

yihuang commented Nov 15, 2019

Uh oh!

calvinlauyh commented Nov 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yihuang commented Nov 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

calvinlauyh commented Nov 15, 2019

Uh oh!

calvinaco commented Nov 15, 2019

Uh oh!

bors bot commented Nov 15, 2019

try

Build succeeded

Uh oh!

tomtau left a comment

Choose a reason for hiding this comment

Uh oh!

bors bot commented Nov 18, 2019

Build failed

Uh oh!

tomtau commented Nov 18, 2019

Uh oh!

tomtau commented Nov 18, 2019

Uh oh!

bors bot commented Nov 18, 2019

Build succeeded

Uh oh!

Reviewers

Assignees

codecov bot commented Nov 13, 2019 •

edited

Loading

yihuang commented Nov 15, 2019 •

edited

Loading

calvinlauyh commented Nov 15, 2019 •

edited

Loading

calvinlauyh commented Nov 15, 2019 •

edited

Loading

yihuang commented Nov 15, 2019 •

edited

Loading