R4R: Judge if trust-node predefined before create certifier add verification for getBlock, queryTx and getValidators #2210

abelliumnt · 2018-09-01T18:22:11Z

New solution for the issue in #2209
Besides, add verification in getting blocks, transactions and validator sets.
@alexanderbez @cwgoes

…tion in getting blocks, transactions and validator sets

codecov · 2018-09-02T00:21:29Z

Codecov Report

Merging #2210 into develop will decrease coverage by 0.59%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           develop    #2210     +/-   ##
==========================================
- Coverage    64.23%   63.63%   -0.6%     
==========================================
  Files          140      140             
  Lines         8575     8542     -33     
==========================================
- Hits          5508     5436     -72     
- Misses        2690     2738     +48     
+ Partials       377      368      -9

abelliumnt · 2018-09-02T01:54:34Z

I noticed some todos: getBlock, queryTx and getValidators. The authors want the gaia-lite could verify the proof from response by default. So here I implement the verification functionality and set distrust-node option to true.
However, currently sometimes some errors will be reported, for instance:

lhy@lhy-ubuntu:~$ gaiacli tendermint validator-set 1 --chain-id test-chain-uIS9nF
ERROR: The height of base truststore in gaia-lite is higher than height 1. 
Can't verify blockchain proof at this height. Please set --distrust-node false and try again

The reason is that:

when the gaia-lite is started, it will fetch the latest commit as its base checkpoint. Here we mark the latest commit height as A.
it can only verify proof from height which is higher than A.
if user want to verify blocks or transactions on prior height this error will be reported.

Do you think this is accptable? If not, could you give me some suggestions about how to improve this?

alexanderbez · 2018-09-04T13:49:18Z

@HaoyangLiu, I like the approach here, but before going too in depth into a review, I think the nomenclature of distrust-node is a bit confusing and would lead to a slightly distasteful UX.

I think we should keep trust-node somehow while allowing any commands that do not need to rely on proofs still work as-is (e.g. gaiacli status).

Alternatively, to keep things simple. We could have trust-node as a persistent/root level flag and/or require anything that uses a CLIConext to use trust-node (including gaiacli status).

lmk your thoughts @cwgoes

abelliumnt · 2018-09-04T15:09:26Z

@alexanderbez
Good idea. I never thought that we add a root flag.
I changed the code, please help take a look at this PR again.

abelliumnt · 2018-09-05T07:07:40Z

@alexanderbez
I found a better way to decide whether to create certifier. If --trust-node is not predefined, then it indicates that certifier is not required. So only when --trust-node is predefined and its value is set to false, will the certifie rbe created. And the response proof will be verified too.
How do you think about this?

	flagPredefined := viper.IsSet(client.FlagTrustNode)
	if !flagPredefined {
		return nil
	}
 	trustNode := viper.GetBool(client.FlagTrustNode)
	if trustNode {
		return nil
	}

alexanderbez

@HaoyangLiu I like this approach better. I left some minor feedback. Otherwise, I tested this and it seems to work. Thanks!

alexanderbez · 2018-09-05T14:48:21Z

client/context/context.go

@@ -69,32 +70,40 @@ func NewCLIContext() CLIContext {
 }

 func createCertifier() tmlite.Certifier {
+	flagPredefined := viper.IsSet(client.FlagTrustNode)


trustNodeDefined?

alexanderbez · 2018-09-05T14:52:23Z

client/tx/search.go

-
-	// TODO: change this to false once proofs built in
-	cmd.Flags().Bool(client.FlagTrustNode, true, "Don't verify proofs for responses")
+	cmd.Flags().Bool(client.FlagTrustNode, false, "Don't verify proofs for query responses")


Seems we're inconsistent with this flag's description. Can we update all occurrences to something like: Trust connected full node (don't verify proofs for responses)?

alexanderbez · 2018-09-05T14:53:14Z

client/tx/query.go

-	// TODO: change this to false when we can
-	cmd.Flags().Bool(client.FlagTrustNode, true, "Don't verify proofs for responses")
+	cmd.Flags().Bool(client.FlagTrustNode, false, "Don't verify proofs for query responses")
+	cmd.Flags().String(client.FlagChainID, "", "The chain ID to connect to")


Chain ID of Tendermint node*

…code reviewer

alexanderbez

tested ACK -- thanks!

jaekwon · 2018-09-12T09:57:49Z

client/context/query.go

@@ -309,7 +309,7 @@ func (ctx CLIContext) query(path string, key cmn.HexBytes) (res []byte, err erro
 		return res, errors.Errorf("query failed: (%d) %s", resp.Code, resp.Log)
 	}

-	// Data from trusted node or subspace query doesn't need verification
+	// data from trusted node or subspace query doesn't need verification


Capitalize and add trailing period please.

I'm sorry. Could you clarify trailing period please?

@HaoyangLiu I think he is stating that it should be reverted back to a sentence with a period.

Got it! Thanks

my pleasure 👍

jaekwon · 2018-09-12T09:58:11Z

client/lcd/lcd_test.go

@@ -177,7 +177,7 @@ func TestBlock(t *testing.T) {

 	// --

-	res, body = Request(t, port, "GET", "/blocks/1", nil)
+	res, body = Request(t, port, "GET", "/blocks/2", nil)


Why did this change?

I have added a comment about the reason above. When gaia-lite is launched, it will fetch the latest height as its trust basement. Then it can only verify blocks or transactions in later height.
Here gaia-lite is launched after a new block is created, please refer to the code. The wait is necessary. If the height of gaia node is zero, gaia-lite will be aborted for fetching block failure. So here gaia-lite will be start later and it will get block 2 as its trust basement. As a result, it can't verify block 1. As I clear here?

OK, we should update gaia lite so that you can easily start it with a separate root-of-trust.

cwgoes

See comments, mostly DRY, we can save other changes for later PRs.

cwgoes · 2018-09-13T05:54:50Z

client/rpc/block.go

@@ -41,6 +44,26 @@ func getBlock(cliCtx context.CLIContext, height *int64) ([]byte, error) {
 		return nil, err
 	}

+	trustNode := viper.GetBool(client.FlagTrustNode)
+	if !trustNode {
+		check, err := tmliteProxy.GetCertifiedCommit(*height, node, cliCtx.Certifier)


Combine this with similar instances below (DRY)

cwgoes · 2018-09-13T05:55:11Z

client/flags.go

@@ -62,7 +61,7 @@ func PostCommands(cmds ...*cobra.Command) []*cobra.Command {
 		c.Flags().Bool(FlagAsync, false, "broadcast transactions asynchronously")
 		c.Flags().Bool(FlagJson, false, "return output in json format")
 		c.Flags().Bool(FlagPrintResponse, true, "return tx response (only works with async = false)")
-		c.Flags().Bool(FlagTrustNode, true, "Don't verify proofs for query responses")
+		c.Flags().Bool(FlagTrustNode, true, "Trust connected full node (don't verify proofs for responses)")


cwgoes · 2018-09-13T05:55:24Z

client/rpc/validators.go

+	trustNode := viper.GetBool(client.FlagTrustNode)
+
+	if !trustNode {
+		check, err := tmliteProxy.GetCertifiedCommit(*height, node, cliCtx.Certifier)


cwgoes · 2018-09-13T05:55:45Z

client/rpc/validators.go

@@ -70,6 +75,21 @@ func getValidators(cliCtx context.CLIContext, height *int64) ([]byte, error) {
 		return nil, err
 	}

+	trustNode := viper.GetBool(client.FlagTrustNode)


This should be fetched from the CLIContext instead

cwgoes · 2018-09-13T05:56:32Z

client/tx/search.go

@@ -93,7 +94,21 @@ func searchTxs(cliCtx context.CLIContext, cdc *wire.Codec, tags []string) ([]Inf
 	if err != nil {
 		return nil, err
 	}
+	if prove {
+		for _, tx := range res.Txs {
+			check, err := tmliteProxy.GetCertifiedCommit(tx.Height, node, cliCtx.Certifier)


DRY with above

This will be cached so the loop is OK I think

cwgoes · 2018-09-13T05:56:51Z

client/tx/query.go

@@ -72,6 +73,20 @@ func queryTx(cdc *wire.Codec, cliCtx context.CLIContext, hashHexStr string, trus
 		return nil, err
 	}

+	if !trustNode {
+		check, err := tmliteProxy.GetCertifiedCommit(info.Height, node, cliCtx.Certifier)


DRY with above/below

cwgoes · 2018-09-13T05:59:22Z

client/lcd/lcd_test.go

@@ -177,7 +177,7 @@ func TestBlock(t *testing.T) {

 	// --

-	res, body = Request(t, port, "GET", "/blocks/1", nil)
+	res, body = Request(t, port, "GET", "/blocks/2", nil)


abelliumnt · 2018-09-13T10:28:33Z

client/tx/query.go

-		// trustNode defaults to true
-		if err != nil {
-			trustNode = true
-		}



I remove this because if rest-server is stared with trust-node option, then we can't verify tx proof even if the request requires proof.

cwgoes

Almost ready, one more suggested change.

cwgoes · 2018-09-13T18:54:56Z

client/tx/query.go

-func formatTxResult(cdc *wire.Codec, res *ctypes.ResultTx) (Info, error) {
-	// TODO: verify the proof if requested
+func formatTxResult(cdc *wire.Codec, cliCtx context.CLIContext, res *ctypes.ResultTx) (Info, error) {
+	if !cliCtx.TrustNode {


This logic is fine but I think it's confusing to put it in a function called formatTxResult - at first I thought you deleted the proof verification because I expected formatTxResult to only do formatting! Can we separate this into a different function - maybe validateTxResult or something?

I added the verification logic here because I noticed the TODO.
How about I create a new function named validateTxResult and call it in formatTxResult, like this:

func formatTxResult(cdc *wire.Codec, cliCtx context.CLIContext, res *ctypes.ResultTx) (Info, error) { if !cliCtx.TrustNode { if err := validateTxResult(cliCtx , res); err !=nil { return ni, err } } .... }

I'm sorry for misunderstanding. I have changed the code move the tx verification out of tx format.

cwgoes · 2018-09-13T18:55:03Z

client/tx/query.go

-		// trustNode defaults to true
-		if err != nil {
-			trustNode = true
-		}



cwgoes · 2018-09-13T18:57:52Z

Also merge develop to fix the file conflicts.

abelliumnt · 2018-09-14T05:15:23Z

The test_sim_modulues failure seems have no connection with my changes.

alexanderbez · 2018-09-14T13:30:29Z

The test_sim_modulues failure seems have no connection with my changes.

Correct 👍

HaoyangLiu added 5 commits September 2, 2018 01:40

Replace trust-node option with distrust-node option, and add varifica…

71625ed

…tion in getting blocks, transactions and validator sets

default value should be false

0681648

Add special error handle for can't find commit

f254d28

Rename error name

d8a951d

Remove todo and change default value to enable proof verification

bed593d

abelliumnt requested review from cwgoes, ebuchman and rigelrozanski as code owners September 1, 2018 18:22

Fix failure in test_lint and test_cover

e8ef342

Refactor error message

582886b

abelliumnt changed the title ~~WIP: Replace trust-node option with distrust-node option, and add verification~~ WIP: Replace trust-node option with distrust-node option, and add verification for getBlock, queryTx and getValidators Sep 2, 2018

alexanderbez added lcd C:CLI T: UX labels Sep 2, 2018

abelliumnt changed the title ~~WIP: Replace trust-node option with distrust-node option, and add verification for getBlock, queryTx and getValidators~~ R4R: Replace trust-node option with distrust-node option, and add verification for getBlock, queryTx and getValidators Sep 3, 2018

HaoyangLiu added 2 commits September 4, 2018 22:57

Rename distrust-node to trust-node and move it to root level flag

61c6d33

Add missed change

4146cb2

HaoyangLiu added 2 commits September 5, 2018 14:54

Judge if turst-node predefined before create certifier

f075e25

Add missing flag for rest-server

9e1bd1a

HaoyangLiu added 2 commits September 5, 2018 15:14

Refactor description for --trust-node

68655ce

Correct error message

5cc7088

abelliumnt changed the title ~~R4R: Replace trust-node option with distrust-node option, and add verification for getBlock, queryTx and getValidators~~ R4R: Judge if turst-node predefined before create certifier add verification for getBlock, queryTx and getValidators Sep 5, 2018

alexanderbez suggested changes Sep 5, 2018

View reviewed changes

Refactor option description for chain-id and trust-node according to …

e1b15c6

…code reviewer

alexanderbez approved these changes Sep 5, 2018

View reviewed changes

rigelrozanski changed the title ~~R4R: Judge if turst-node predefined before create certifier add verification for getBlock, queryTx and getValidators~~ R4R: Judge if trust-node predefined before create certifier add verification for getBlock, queryTx and getValidators Sep 5, 2018

jaekwon reviewed Sep 12, 2018

View reviewed changes

Capitalize comment and add trailing period

bf6b445

cwgoes suggested changes Sep 13, 2018

View reviewed changes

cwgoes mentioned this pull request Sep 13, 2018

SDK 0.25 / Gaia-9000 release checklist #2220

Closed

23 tasks

HaoyangLiu added 4 commits September 13, 2018 15:58

Refactor code, move ceritifer to a function

ca840e6

Remove wrong import from irisnet

abf1a8a

Move tx verification to formatTxResult

253db7f

go fmt tx/query.go

a63ea1c

abelliumnt commented Sep 13, 2018

View reviewed changes

cwgoes suggested changes Sep 13, 2018

View reviewed changes

HaoyangLiu added 2 commits September 14, 2018 12:57

Merge with cosmos develop

7a7c6df

Move tx verification out of tx format

8dcc536

cwgoes approved these changes Sep 14, 2018

View reviewed changes

cwgoes merged commit 7dc09d0 into cosmos:develop Sep 14, 2018

snyk-bot mentioned this pull request Jan 12, 2022

[Snyk] Security upgrade axios from 0.19.0 to 0.20.0 Itchibon777/cosmos-sdk#4

Open

snyk-bot mentioned this pull request Feb 9, 2022

[Snyk] Security upgrade axios from 0.19.0 to 0.20.0 Itchibon777/cosmos-sdk#5

Open

R4R: Judge if trust-node predefined before create certifier add verification for getBlock, queryTx and getValidators #2210

R4R: Judge if trust-node predefined before create certifier add verification for getBlock, queryTx and getValidators #2210

Conversation

abelliumnt commented Sep 1, 2018

codecov bot commented Sep 2, 2018 • edited Loading

Codecov Report

abelliumnt commented Sep 2, 2018

alexanderbez commented Sep 4, 2018 • edited Loading

abelliumnt commented Sep 4, 2018

abelliumnt commented Sep 5, 2018

alexanderbez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abelliumnt Sep 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwgoes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abelliumnt Sep 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwgoes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwgoes commented Sep 13, 2018

abelliumnt commented Sep 14, 2018

alexanderbez commented Sep 14, 2018

codecov bot commented Sep 2, 2018 •

edited

Loading

alexanderbez commented Sep 4, 2018 •

edited

Loading

abelliumnt Sep 12, 2018 •

edited

Loading

abelliumnt Sep 13, 2018 •

edited

Loading