Problem (Fix #1313): light client doesn't verify the fetched staking state #1511

yihuang · 2020-04-29T09:50:01Z

Solution:

Add staking_root in sync state
Remove the "account" path in favor of the new "staking" path
Client staking state command add wallet name parameter,
and verify staking state and proof against the trusted staking_root in sync state
Need to sync wallet before fetching the new staking state now. (you can still get any staking state without verification by request abci_query API directly)

tomtau

Tx construction in network-ops doesn't verify staking state for now, because the unit tests are hard to mock the env.
Need to sync wallet before fetching the new staking state now.

maybe the verification option could be added as extra argument -- in local setting, one could disable it if one knows it's querying against the user's own full node; in mocked unit tests it could also be disabled?

leejw51crypto · 2020-04-29T11:16:12Z

client-network/src/network_ops/default_network_ops_client.rs

+        address: &StakedStateAddress,
+        verify: bool,
+    ) -> Result<StakedState> {
+        let mstaking = if verify {


instead of mstaking,
how about full word variable name ?

I believe the name style of mxxx is adopted from Haskell, because I've seen this style in our code base several times before, so I keep on using it. Basically in these cases mxxx means Option<xxx> or Result<xxx>.

leejw51crypto · 2020-04-29T11:23:04Z

client-network/src/network_ops/default_network_ops_client.rs

+    ) -> Result<StakedState> {
+        let mstaking = if verify {
+            let sync_state = self.wallet_client.get_sync_state(name)?;
+            let rsp = self.client.query(


full name with "response"

Personally, I think well-known shortcuts should be ok to keep the code short, actually easier to read.

leejw51crypto · 2020-04-29T11:23:58Z

client-network/src/network_ops/default_network_ops_client.rs

+                    format!("Cannot deserialize staked state for address: {}", address)
+                })?;
+
+            let mut proof_bytes = rsp.proof.as_ref().unwrap().ops[0].data.as_slice();


expect and
assert!(ops.len()>0)?

Changed to return proper error for invalid response.

leejw51crypto

lgtm

yihuang · 2020-04-29T16:30:32Z

Tx construction in network-ops doesn't verify staking state for now, because the unit tests are hard to mock the env.
Need to sync wallet before fetching the new staking state now.

maybe the verification option could be added as extra argument -- in local setting, one could disable it if one knows it's querying against the user's own full node; in mocked unit tests it could also be disabled?

Added a verify_staking parameter, unit tests pass false, client-cli and client-rpc passes true.
Hope in the future we'll have more sophisticated unit test mocking ability, then we can remove this parameter.

codecov · 2020-04-30T02:48:21Z

Codecov Report

Merging #1511 into master will decrease coverage by 0.11%.
The diff coverage is 41.32%.

@@            Coverage Diff             @@
##           master    #1511      +/-   ##
==========================================
- Coverage   65.78%   65.66%   -0.12%     
==========================================
  Files         161      161              
  Lines       21818    21892      +74     
==========================================
+ Hits        14353    14376      +23     
- Misses       7465     7516      +51

Impacted Files	Coverage Δ
client-cli/src/command.rs	`0.00% <0.00%> (ø)`
client-cli/src/command/transaction_command.rs	`0.00% <0.00%> (ø)`
client-core/src/wallet.rs	`100.00% <ø> (ø)`
client-core/src/wallet/default_wallet_client.rs	`49.85% <0.00%> (-0.59%)`	⬇️
client-rpc/src/rpc/staking_rpc.rs	`0.00% <0.00%> (ø)`
cro-clib/src/transaction.rs	`0.00% <0.00%> (ø)`
...work/src/network_ops/default_network_ops_client.rs	`74.40% <38.77%> (-3.64%)`	⬇️
client-core/src/wallet/syncer.rs	`71.52% <72.00%> (-0.47%)`	⬇️
client-core/src/service/sync_state_service.rs	`81.25% <83.33%> (-1.17%)`	⬇️
chain-core/src/init/config.rs	`80.00% <100.00%> (ø)`
... and 5 more

tomtau · 2020-05-01T01:19:02Z

bors r+

1511: Problem (Fix #1313): light client doesn't verify the fetched staking state r=tomtau a=yihuang Solution: - Add staking_root in sync state - Remove the "account" path in favor of the new "staking" path - Client staking state command add wallet name parameter, and verify staking state and proof against the trusted staking_root in sync state - Need to sync wallet before fetching the new staking state now. (you can still get any staking state without verification by request abci_query API directly) 1516: Problem (Fix #1515): unbond tx don't subtract fee from bonded r=tomtau a=yihuang Solution: - Fix the bug and add unit test Co-authored-by: yihuang <huang@crypto.com>

bors · 2020-05-01T02:30:08Z

Build failed (retrying...):

drone-hw/push

1511: Problem (Fix #1313): light client doesn't verify the fetched staking state r=tomtau a=yihuang Solution: - Add staking_root in sync state - Remove the "account" path in favor of the new "staking" path - Client staking state command add wallet name parameter, and verify staking state and proof against the trusted staking_root in sync state - Need to sync wallet before fetching the new staking state now. (you can still get any staking state without verification by request abci_query API directly) Co-authored-by: yihuang <huang@crypto.com>

tomtau · 2020-05-01T02:35:25Z

bors r-

bors · 2020-05-01T02:35:26Z

Canceled.

tomtau · 2020-05-01T02:35:45Z

multi-node integration test fails with

jsonrpcclient.exceptions.ReceivedErrorResponseError: Deserialization error: Unable to deserialize global state for wallet with name target

yihuang · 2020-05-01T17:31:17Z

multi-node integration test fails with

jsonrpcclient.exceptions.ReceivedErrorResponseError: Deserialization error: Unable to deserialize global state for wallet with name target

This is actually a separate issue, solved in #1524.

tomtau

looks all right, but perhaps no need to make this a breaking change immediately?

tomtau · 2020-05-04T01:15:44Z

chain-abci/src/app/query.rs

-            "account" => {
-                let account_address = StakedStateAddress::try_from(_req.data.as_slice());
-                if let (Some(state), Ok(address)) = (&self.last_state, account_address) {
-                    let (account, _proof) =
-                        get_with_proof(&self.storage, state.staking_version, &address);
-                    match account {
-                        Some(a) => {
-                            resp.value = a.encode();
-                            // TODO: inclusion proof
-                        }
-                        None => {
-                            resp.log += "account lookup failed: account not exists";
-                            resp.code = 1;
-                        }
-                    }
-                } else {
-                    resp.log += "account lookup failed (either invalid address or node not correctly restored / initialized)";
-                    resp.code = 3;
-                }
-            }


is there a need to do a breaking change? the original default behaviour should identical to "staking" path without any parameters? it could possibly be a non-breaking change with "staking" | "account" => {... pattern match... it could log some deprecation warning for "account" path that it'd be removed in the future

yihuang · 2020-05-04T02:00:52Z

looks all right, but perhaps no need to make this a breaking change immediately?

Do you mean keep the account path and staking path at the same time?

tomtau · 2020-05-04T02:14:27Z

looks all right, but perhaps no need to make this a breaking change immediately?

Do you mean keep the account path and staking path at the same time?

yes, for the time being

…d staking state Solution: - Add staking_root in sync state - Remove the "account" path in favor of the new "staking" path - Client staking state command add wallet name parameter, and verify staking state and proof against the trusted staking_root in sync state - Need to sync wallet before fetching the new staking state now.

yihuang · 2020-05-04T02:25:21Z

looks all right, but perhaps no need to make this a breaking change immediately?

Do you mean keep the account path and staking path at the same time?

yes, for the time being

Ok, done.

tomtau · 2020-05-05T01:54:33Z

bors r+

bors · 2020-05-05T02:41:07Z

Build succeeded:

yihuang requested review from calvinaco, devashishdxt and tomtau as code owners April 29, 2020 09:50

tomtau requested review from leejw51crypto and linfeng-crypto April 29, 2020 10:31

tomtau suggested changes Apr 29, 2020

View reviewed changes

leejw51crypto reviewed Apr 29, 2020

View reviewed changes

leejw51crypto approved these changes Apr 29, 2020

View reviewed changes

yihuang force-pushed the issue1313 branch 2 times, most recently from 69ec821 to a038294 Compare April 29, 2020 16:37

yihuang requested a review from tomtau April 29, 2020 16:38

yihuang force-pushed the issue1313 branch 3 times, most recently from 3810b92 to c8eb7e8 Compare April 30, 2020 02:03

yihuang force-pushed the issue1313 branch from c8eb7e8 to 936e1e6 Compare May 1, 2020 17:29

yihuang force-pushed the issue1313 branch 2 times, most recently from a70190c to 6322057 Compare May 3, 2020 01:20

tomtau suggested changes May 4, 2020

View reviewed changes

yihuang force-pushed the issue1313 branch from 6322057 to aef3d47 Compare May 4, 2020 02:18

yihuang requested a review from tomtau May 4, 2020 09:04

bors bot merged commit 83bdc87 into crypto-com:master May 5, 2020

snyk-bot mentioned this pull request Jan 12, 2022

[Snyk] Security upgrade axios from 0.19.0 to 0.20.0 Itchibon777/chain#16

Open

snyk-bot mentioned this pull request Feb 9, 2022

[Snyk] Security upgrade axios from 0.19.0 to 0.20.0 Itchibon777/chain#17

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Problem (Fix #1313): light client doesn't verify the fetched staking state #1511

Problem (Fix #1313): light client doesn't verify the fetched staking state #1511

yihuang commented Apr 29, 2020 •

edited

tomtau left a comment

leejw51crypto Apr 29, 2020

yihuang Apr 29, 2020 •

edited

leejw51crypto Apr 29, 2020

yihuang Apr 29, 2020

leejw51crypto Apr 29, 2020

yihuang Apr 29, 2020

leejw51crypto left a comment

yihuang commented Apr 29, 2020 •

edited

codecov bot commented Apr 30, 2020 •

edited

tomtau commented May 1, 2020

bors bot commented May 1, 2020

tomtau commented May 1, 2020

bors bot commented May 1, 2020

tomtau commented May 1, 2020

yihuang commented May 1, 2020 •

edited

tomtau left a comment

tomtau May 4, 2020

yihuang commented May 4, 2020

tomtau commented May 4, 2020

yihuang commented May 4, 2020

tomtau commented May 5, 2020

bors bot commented May 5, 2020

Problem (Fix #1313): light client doesn't verify the fetched staking state #1511

Problem (Fix #1313): light client doesn't verify the fetched staking state #1511

Conversation

yihuang commented Apr 29, 2020 • edited

tomtau left a comment

Choose a reason for hiding this comment

leejw51crypto Apr 29, 2020

Choose a reason for hiding this comment

yihuang Apr 29, 2020 • edited

Choose a reason for hiding this comment

leejw51crypto Apr 29, 2020

Choose a reason for hiding this comment

yihuang Apr 29, 2020

Choose a reason for hiding this comment

leejw51crypto Apr 29, 2020

Choose a reason for hiding this comment

yihuang Apr 29, 2020

Choose a reason for hiding this comment

leejw51crypto left a comment

Choose a reason for hiding this comment

yihuang commented Apr 29, 2020 • edited

codecov bot commented Apr 30, 2020 • edited

Codecov Report

tomtau commented May 1, 2020

bors bot commented May 1, 2020

tomtau commented May 1, 2020

bors bot commented May 1, 2020

tomtau commented May 1, 2020

yihuang commented May 1, 2020 • edited

tomtau left a comment

Choose a reason for hiding this comment

tomtau May 4, 2020

Choose a reason for hiding this comment

yihuang commented May 4, 2020

tomtau commented May 4, 2020

yihuang commented May 4, 2020

tomtau commented May 5, 2020

bors bot commented May 5, 2020

yihuang commented Apr 29, 2020 •

edited

yihuang Apr 29, 2020 •

edited

yihuang commented Apr 29, 2020 •

edited

codecov bot commented Apr 30, 2020 •

edited

yihuang commented May 1, 2020 •

edited