Switch mock proofs to real proofs; debug #5711

cwgoes · 2020-02-27T14:29:14Z

cwgoes · 2020-02-27T14:55:32Z

The handshake problems can be clearly reproduced now:

go test ./x/ibc/03-connection/keeper/...

cwgoes · 2020-02-27T15:09:12Z

I fixed one minor mistake in a test-case.

Right now there are two errors:

--- FAIL: TestKeeperTestSuite (0.41s)
    --- FAIL: TestKeeperTestSuite/TestConnOpenAck (0.06s)
        --- FAIL: TestKeeperTestSuite/TestConnOpenAck/Case_success (0.01s)
            handshake_test.go:204: 
                	Error Trace:	handshake_test.go:204
                	            				suite.go:77
                	Error:      	Received unexpected error:
                	            	self consensus state not found
                	Test:       	TestKeeperTestSuite/TestConnOpenAck/Case_success
                	Messages:   	valid test case 0 failed: success

I tried to fix this with 850e6c3#diff-b1978f573dd726c3b9b0b8bb6f72b7ffR180 but this didn't seem to work.

The other problem is the one we've observed in the handshakes:

    --- FAIL: TestKeeperTestSuite/TestConnOpenConfirm (0.04s)
        --- FAIL: TestKeeperTestSuite/TestConnOpenConfirm/Case_success (0.01s)
            handshake_test.go:255: 
                	Error Trace:	handshake_test.go:255
                	            				suite.go:77
                	Error:      	Received unexpected error:
                	            	
                	            	github.com/cosmos/cosmos-sdk/x/ibc/07-tendermint/types.ClientState.VerifyConnectionState
                	            		/home/cwgoes/working/tendermint/go/src/github.com/cosmos/cosmos-sdk/x/ibc/07-tendermint/types/client_state.go:160
                	            	github.com/cosmos/cosmos-sdk/x/ibc/03-connection/keeper.Keeper.VerifyConnectionState
                	            		/home/cwgoes/working/tendermint/go/src/github.com/cosmos/cosmos-sdk/x/ibc/03-connection/keeper/verify.go:59
                	            	github.com/cosmos/cosmos-sdk/x/ibc/03-connection/keeper.Keeper.ConnOpenConfirm
                	            		/home/cwgoes/working/tendermint/go/src/github.com/cosmos/cosmos-sdk/x/ibc/03-connection/keeper/handshake.go:222
                	            	github.com/cosmos/cosmos-sdk/x/ibc/03-connection/keeper_test.(*KeeperTestSuite).TestConnOpenConfirm.func6
                	            		/home/cwgoes/working/tendermint/go/src/github.com/cosmos/cosmos-sdk/x/ibc/03-connection/keeper/handshake_test.go:252
                	            	github.com/stretchr/testify/suite.(*Suite).Run.func1
                	            		/home/cwgoes/working/tendermint/go/pkg/mod/github.com/stretchr/testify@v1.5.1/suite/suite.go:77
                	            	connection state verification failed: calculated root hash is invalid: expected [65 202 250 227 28 199 15 88 1 250 16 22 162 221 84 169 188 184 32 27 91 56 153 25 254 153 118 118 37 50 197 22] but got [96 48 99 120 144 155 117 126 20 167 25 162 81 126 86 115 253 133 118 85 75 108 219 45 193 246 196 136 24 26 203 198]
                	Test:       	TestKeeperTestSuite/TestConnOpenConfirm/Case_success
                	Messages:   	valid test case 0 failed: success

This recurs across all of the handshake / verification tests.

x/ibc/03-connection/keeper/handshake_test.go

x/ibc/types/mock.go

golangcibot · 2020-02-27T15:16:54Z

x/ibc/04-channel/keeper/keeper_test.go

+		(path.String() == proof.path.String()) &&
+		bytes.Equal(value, proof.value) {
+		return nil
+	} else {


if block ends with a return statement, so drop this else and outdent its block (from golint)

cwgoes · 2020-02-27T15:28:55Z

I have altered the test-cases to return the consensus heights they should be using, no luck yet, I suspect there is a single underlying problem that we need to isolate.

go test ./x/ibc/03-connection/keeper/... -run TestKeeperTestSuite/TestConnOpenTry

--- FAIL: TestKeeperTestSuite (0.06s)
    --- FAIL: TestKeeperTestSuite/TestConnOpenTry (0.06s)
        --- FAIL: TestKeeperTestSuite/TestConnOpenTry/Case_success (0.01s)
            handshake_test.go:125: 
                	Error Trace:	handshake_test.go:125
                	            				suite.go:77
                	Error:      	Received unexpected error:
                	            	
                	            	github.com/cosmos/cosmos-sdk/x/ibc/07-tendermint/types.ClientState.VerifyConnectionState
                	            		/home/cwgoes/working/tendermint/go/src/github.com/cosmos/cosmos-sdk/x/ibc/07-tendermint/types/client_state.go:160
                	            	github.com/cosmos/cosmos-sdk/x/ibc/03-connection/keeper.Keeper.VerifyConnectionState
                	            		/home/cwgoes/working/tendermint/go/src/github.com/cosmos/cosmos-sdk/x/ibc/03-connection/keeper/verify.go:59
                	            	github.com/cosmos/cosmos-sdk/x/ibc/03-connection/keeper.Keeper.ConnOpenTry
                	            		/home/cwgoes/working/tendermint/go/src/github.com/cosmos/cosmos-sdk/x/ibc/03-connection/keeper/handshake.go:82
                	            	github.com/cosmos/cosmos-sdk/x/ibc/03-connection/keeper_test.(*KeeperTestSuite).TestConnOpenTry.func8
                	            		/home/cwgoes/working/tendermint/go/src/github.com/cosmos/cosmos-sdk/x/ibc/03-connection/keeper/handshake_test.go:118
                	            	github.com/stretchr/testify/suite.(*Suite).Run.func1
                	            		/home/cwgoes/working/tendermint/go/pkg/mod/github.com/stretchr/testify@v1.5.1/suite/suite.go:77
                	            	connection state verification failed: calculated root hash is invalid: expected [65 202 250 227 28 199 15 88 1 250 16 22 162 221 84 169 188 184 32 27 91 56 153 25 254 153 118 118 37 50 197 22] but got [222 160 240 36 202 50 93 224 173 12 40 103 181 198 119 229 212 218 118 71 53 182 161 92 177 58 140 184 151 245 148 127]
                	Test:       	TestKeeperTestSuite/TestConnOpenTry/Case_success
                	Messages:   	valid test case 0 failed with consensus height 3: success
FAIL
FAIL	github.com/cosmos/cosmos-sdk/x/ibc/03-connection/keeper	0.087s
FAIL

I'm also confused by the fact that the expected root hash seems to be constant but the real one changes each time I re-run the tests - is the test-suite supposed to be deterministic?

golangcibot · 2020-02-27T15:35:26Z

x/ibc/03-connection/keeper/handshake_test.go

 			)

 			if tc.expPass {
-				suite.Require().NoError(err, "valid test case %d failed: %s", i, tc.msg)
+				suite.Require().NoError(err, "valid test case %d failed with consensus height %d: %s", i, consensusHeight, tc.msg)


Using the variable on range scope i in function literal (from scopelint)

golangcibot · 2020-02-27T15:35:28Z

x/ibc/03-connection/keeper/handshake_test.go

 			} else {
-				suite.Require().Error(err, "invalid test case %d passed: %s", i, tc.msg)
+				suite.Require().Error(err, "invalid test case %d passed with consensus height %d: %s", i, consensusHeight, tc.msg)


Using the variable on range scope i in function literal (from scopelint)

x/ibc/03-connection/keeper/handshake_test.go

golangcibot · 2020-02-27T15:35:29Z

x/ibc/03-connection/keeper/handshake_test.go

-				connection.GetCompatibleVersions(), tc.proofInit, tc.proofConsensus,
-				uint64(proofHeight), uint64(consensusHeight),
+				connection.GetCompatibleVersions(), proofInit, proofConsensus,
+				uint64(proofHeight), consensusHeight,


unnecessary conversion (from unconvert)

golangcibot · 2020-02-27T15:35:29Z

x/ibc/03-connection/keeper/handshake_test.go


-			// consensusKey := ibctypes.KeyConsensusState(testClientIDB, uint64(proofHeight))
-			// proofConsensus, consensusHeight := suite.queryProof(consensusKey)
+			consensusKey := ibctypes.KeyConsensusState(testClientIDB, uint64(consensusHeight))


unnecessary conversion (from unconvert)

golangcibot · 2020-02-27T15:35:29Z

x/ibc/03-connection/keeper/handshake_test.go

-				suite.chainA.GetContext(), testConnectionIDA, tc.version, tc.proofTry, tc.proofConsensus,
-				uint64(proofHeight), uint64(consensusHeight),
+				suite.chainA.GetContext(), testConnectionIDA, tc.version, proofTry, proofConsensus,
+				uint64(proofHeight), consensusHeight,


unnecessary conversion (from unconvert)

…usState

cwgoes · 2020-03-02T09:49:36Z

Issues fixed in 69cf142:

Off-by-one height error (app state is from last commit)
verifyClientConsensusState needs to use counterparty client identifier
Need two header updates to make counterparty consensus states accurate

Still debugging

Mistakes in identiifier handling in ICS 7 (chain ID & client ID are not the same thing)

cwgoes · 2020-03-02T10:24:21Z

Still some issues with expected consensus state & self consensus state mis-matching.

cwgoes · 2020-03-02T11:24:03Z

TestConnOpenConfirm and the connection state verification in general now works, no luck on the consensus state verification mismatch yet.

cwgoes · 2020-03-02T18:50:42Z

To run the ConnOpenTry test (still failing):

go test ./x/ibc/03-connection/keeper/... -run TestKeeperTestSuite/TestConnOpenTry

resulting in

root: {[216 137 244 20 97 87 138 53 114 6 173 141 41 248 245 15 217 3 64 210 56 147 140 117 121 232 178 151 123 196 76 13]} at height 4
path: /ibc/consensusState%2Ftestclientida%2F3
--- FAIL: TestKeeperTestSuite (0.02s)
    --- FAIL: TestKeeperTestSuite/TestConnOpenTry (0.02s)
        --- FAIL: TestKeeperTestSuite/TestConnOpenTry/Case_success (0.01s)
            handshake_test.go:133: 
                	Error Trace:	handshake_test.go:133
                	            				suite.go:77
                	Error:      	Received unexpected error:
                	            	
                	            	github.com/cosmos/cosmos-sdk/x/ibc/07-tendermint/types.ClientState.VerifyClientConsensusState
                	            		/home/cwgoes/working/tendermint/go/src/github.com/cosmos/cosmos-sdk/x/ibc/07-tendermint/types/client_state.go:132
                	            	github.com/cosmos/cosmos-sdk/x/ibc/03-connection/keeper.Keeper.VerifyClientConsensusState
                	            		/home/cwgoes/working/tendermint/go/src/github.com/cosmos/cosmos-sdk/x/ibc/03-connection/keeper/verify.go:29
                	            	github.com/cosmos/cosmos-sdk/x/ibc/03-connection/keeper.Keeper.ConnOpenTry
                	            		/home/cwgoes/working/tendermint/go/src/github.com/cosmos/cosmos-sdk/x/ibc/03-connection/keeper/handshake.go:93
                	            	github.com/cosmos/cosmos-sdk/x/ibc/03-connection/keeper_test.(*KeeperTestSuite).TestConnOpenTry.func8
                	            		/home/cwgoes/working/tendermint/go/src/github.com/cosmos/cosmos-sdk/x/ibc/03-connection/keeper/handshake_test.go:126
                	            	github.com/stretchr/testify/suite.(*Suite).Run.func1
                	            		/home/cwgoes/working/tendermint/go/pkg/mod/github.com/stretchr/testify@v1.5.1/suite/suite.go:77
                	            	client consensus state verification failed: verifying value: leaf value hash not same: invalid proof
                	Test:       	TestKeeperTestSuite/TestConnOpenTry/Case_success
                	Messages:   	valid test case 0 failed with consensus height 3 and proof height 3: success
FAIL
FAIL	github.com/cosmos/cosmos-sdk/x/ibc/03-connection/keeper	0.048s

AdityaSripal · 2020-03-02T21:44:18Z

x/ibc/03-connection/keeper/keeper_test.go

 	// Create target ctx
 	ctxTarget := chain.GetContext()

 	// create client
-	clientState, err := ibctmtypes.Initialize(client.ClientID, trustingPeriod, ubdPeriod, client.Header)
+	clientState, err := ibctmtypes.Initialize(chain.ClientID, trustingPeriod, ubdPeriod, client.Header)


Hmm so looks like my code caused some confusion here.

I believe I should rename this field to client.ChainID and document clearly that the clientID that other chains store for the given client is the chainID of the client chain for simplicity in the tests.

So TestChainA may have a chainID field called: testchainida.

If TestChainB creates a client for TestChainA, it will give it the clientID testchainida

OK, that's alright too, it was just inconsistent in some places. The key question is why consensus state verification is failing though.

Hmm working through the test cases now, and I'm getting a failure on Connection state verification before i hit consensus state verification

Did you comment out the connection state issue to hit consensus state?

No, it works for me, can you provide replication instructions?

Sorry i meant on my own branch that reverts your identifier issue

OK, great! Feel free to revise then.

cwgoes · 2020-03-03T18:11:14Z

Step 1: Get these tests working. Step 2: Test relayer again, get it working.

x/ibc/03-connection/keeper/handshake_test.go

cwgoes · 2020-03-04T10:08:49Z

All the handshake tests pass locally for me & the changes make sense, left a comment about a height offset we need to remember to deal with correctly in the relayer. Hopefully we can turn these into more end-to-end tests in the future, but for now fixing the verification tests and moving back to the relayer to get the connection handshake working there makes sense to me as an order of priorities.

…s-sdk into cwgoes/debug-proofs

fedekunze

ACK. Minor cleanup comments

fedekunze · 2020-03-05T13:35:30Z

x/ibc/03-connection/keeper/keeper_test.go

 	client.Header = nextHeader(client)
+
+	/*


remove comment?

fedekunze · 2020-03-05T13:36:51Z

x/ibc/04-channel/keeper/keeper_test.go

 	invalidProof struct{}
 )

 func (validProof) GetCommitmentType() commitment.Type {
 	return commitment.Merkle
 }

-func (validProof) VerifyMembership(
+func (proof validProof) VerifyMembership(


are we still using these proofs?

I think we shouldn't be, maybe we can delete them altogether?

fedekunze · 2020-03-05T13:36:59Z

x/ibc/04-channel/keeper/keeper_test.go

+		(path.String() == proof.path.String()) &&
+		bytes.Equal(value, proof.value) {
+		return nil
+	} else {


fedekunze · 2020-03-05T13:37:37Z

x/ibc/07-tendermint/types/client_state.go

@@ -105,12 +105,15 @@ func (cs ClientState) IsFrozen() bool {
 // Tendermint client stored on the target machine.
 func (cs ClientState) VerifyClientConsensusState(
 	cdc *codec.Codec,
+	provingRoot commitment.RootI,


is this the counterparty commitment root? add comment

fedekunze · 2020-03-05T13:37:47Z

x/ibc/07-tendermint/types/client_state.go

@@ -156,6 +159,8 @@ func (cs ClientState) VerifyConnectionState(
 		return err
 	}

+	fmt.Printf("verify against consensus state %v\n", consensusState)


Suggested change

fmt.Printf("verify against consensus state %v\n", consensusState)

x/ibc/types/mock.go

cwgoes · 2020-03-05T15:58:52Z

Thanks @fedekunze!

Tested locally, everything except ICS 04 tests passes for me now. Those look like they also need to be updated to use queryProof and do the correct connection initialisation, channel initialisation, and client update logic. Up to you @AdityaSripal as to whether you want to debug those now or try to get the connection handshake working in the relayer first; I think either is fine.

Also, it would be nice to share code between the ICS 03 and ICS 04 tests somehow, there seems to be a lot of duplicate logic in the test setup that we have to edit in multiple places right now.

jackzampolin · 2020-03-06T14:44:38Z

x/ibc/02-client/keeper/keeper.go

-		Timestamp:    ctx.BlockTime(),
+		Height: height,
+		// FIXME: Currently commented out due to time normalisation issues.
+		//Timestamp:    histInfo.Header.Time,


This is going to get fixed in this PR?

I wasn't able to figure it out, some sort of normalisation (maybe in Tendermint?).

We should either fix it or open an issue to track it.

What feature/guarantee/check are we missing by not having this timestamp?

cwgoes

tACK (can't approve since I created the PR) modulo comments & #5761. Thanks @AdityaSripal!

cwgoes · 2020-03-07T14:18:25Z

x/ibc/02-client/keeper/keeper.go

-		Timestamp:    ctx.BlockTime(),
+		Height: height,
+		// FIXME: Currently commented out due to time normalisation issues.
+		//Timestamp:    histInfo.Header.Time,


I wasn't able to figure it out, some sort of normalisation (maybe in Tendermint?).

We should either fix it or open an issue to track it.

x/ibc/03-connection/keeper/handshake_test.go

cwgoes · 2020-03-07T14:30:36Z

x/ibc/04-channel/keeper/keeper_test.go

 	invalidProof struct{}
 )

 func (validProof) GetCommitmentType() commitment.Type {
 	return commitment.Merkle
 }

-func (validProof) VerifyMembership(
+func (proof validProof) VerifyMembership(


I think we shouldn't be, maybe we can delete them altogether?

jackzampolin · 2020-03-07T16:11:07Z

Failing sim and proto checks. Lets get those cleaned up before we merge this?

jackzampolin · 2020-03-07T18:04:45Z

Failing simulation:

Simulation complete; Final height (blocks): 51, final time (seconds): 3861-10-29 02:54:35 -0800 PST, operations ran: 14700
{
 "bank": {
  "multisend": {
   "ok": 151
  },
  "no-operation": {
   "failure": 99
  },
  "send": {
   "ok": 1449
  }
 },
 "begin_block": {
  "evidence": {
   "ok": 3
  },
  "signing": {
   "missed": 523,
   "signed": 2588
  }
 },
 "distribution": {
  "fund_community_pool": {
   "ok": 690
  },
  "no-operation": {
   "failure": 917
  },
  "set_withdraw_address": {
   "ok": 736
  },
  "withdraw_delegator_reward": {
   "ok": 375
  },
  "withdraw_validator_commission": {
   "ok": 165
  }
 },
 "end_block": {
  "validator_updates": {
   "added": 189,
   "kicked": 189,
   "updated": 379
  }
 },
 "gov": {
  "deposit": {
   "ok": 290
  },
  "no-operation": {
   "failure": 1874
  },
  "submit_proposal": {
   "ok": 185
  },
  "vote": {
   "ok": 277
  }
 },
 "slashing": {
  "no-operation": {
   "failure": 1481
  },
  "unjail": {
   "failure": 1
  }
 },
 "staking": {
  "begin_redelegate": {
   "ok": 1410
  },
  "begin_unbonding": {
   "ok": 1564
  },
  "create_validator": {
   "ok": 978
  },
  "delegate": {
   "ok": 1469
  },
  "edit_validator": {
   "ok": 35
  },
  "no-operation": {
   "failure": 554
  }
 }
}
exporting app state...
--- FAIL: TestAppImportExport (164.12s)
    sim_test.go:110: 
        	Error Trace:	sim_test.go:110
        	Error:      	Received unexpected error:
        	            	open /tmp/sim-logs-323511574/sim_state-491163361.json: no such file or directory
        	Test:       	TestAppImportExport
FAIL

cwgoes added 2 commits February 27, 2020 15:23

Add key, path, value to mock proofs

61a3e08

Also alter mock types (why are there duplicates)

a1e6bc6

cwgoes requested review from alessio, alexanderbez, fedekunze and jackzampolin as code owners February 27, 2020 14:29

cwgoes added 2 commits February 27, 2020 15:42

Remove mock proofs from handshake_test.go

d7dab89

Use actual proofs

70afe8d

Try to fix historical info, no luck

850e6c3

golangcibot reviewed Feb 27, 2020

View reviewed changes

Have test-cases produce consensus heights

3e42663

cwgoes changed the title ~~Debug mock proofs~~ Switch mock proofs to real proofs; debug Feb 27, 2020

golangcibot reviewed Feb 27, 2020

View reviewed changes

Fix consensus height / proof height difference in verifyClientConsens…

32ee4a9

…usState

cwgoes mentioned this pull request Feb 27, 2020

ATTN IBC working group: agenda for February 27th at 17 CET cosmos/ibc#385

Closed

Bug fixes contd.

69cf142

Fix some identifier issues

c38d60a

TestConnOpenConfirm now works

c81d691

AdityaSripal reviewed Mar 2, 2020

View reviewed changes

AdityaSripal added 3 commits March 3, 2020 07:53

further on proof

587ca1f

fix debugger print statement

c4ecf34

IT PASSES

e8d346f

AdityaSripal and others added 4 commits March 3, 2020 11:26

revert identifier changes

046a8bf

refactor query proof to generate proofs from either chain

2def886

fix ack and confirm

c1e5cfd

Remove temporary break

f3db3c3

cwgoes commented Mar 4, 2020

View reviewed changes

x/ibc/03-connection/keeper/handshake_test.go Show resolved Hide resolved

AdityaSripal added 4 commits March 4, 2020 16:29

fix connection and channel verify tests

c293d99

Merge branch 'cwgoes/debug-proofs' of https://github.com/cosmos/cosmo…

d53b452

…s-sdk into cwgoes/debug-proofs

fix everything but verify client consensusstate

a681130

fix all verify tests

9418881

fedekunze approved these changes Mar 5, 2020

View reviewed changes

fix conflicts

445286e

cwgoes requested review from hschoenburg and nylira as code owners March 5, 2020 14:06

fix ics07 tests

412a77b

cwgoes mentioned this pull request Mar 6, 2020

Alter mock proofs to actually assert the correct key & value #5693

Closed

jackzampolin reviewed Mar 6, 2020

View reviewed changes

AdityaSripal added 3 commits March 6, 2020 12:23

fix handshake tests

b1a4631

fix packet tests

486ff78

fix timeout tests

7ebc311

AdityaSripal mentioned this pull request Mar 7, 2020

Test Cleanup followup from #5711 #5761

Closed

4 tasks

cwgoes commented Mar 7, 2020

View reviewed changes

jackzampolin merged commit 1818d98 into ibc-alpha Mar 7, 2020

jackzampolin deleted the cwgoes/debug-proofs branch March 7, 2020 18:13

jackzampolin mentioned this pull request Apr 28, 2020

Common test harness code for IBC tests #5558

Closed

5 tasks

Switch mock proofs to real proofs; debug #5711

Switch mock proofs to real proofs; debug #5711

Conversation

cwgoes commented Feb 27, 2020 • edited by fedekunze Loading

cwgoes commented Feb 27, 2020

cwgoes commented Feb 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwgoes commented Feb 27, 2020

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 Mar 2, 2020

cwgoes commented Mar 2, 2020

cwgoes commented Mar 2, 2020

cwgoes commented Mar 2, 2020

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

cwgoes commented Mar 3, 2020

cwgoes commented Mar 4, 2020

fedekunze 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

cwgoes commented Mar 5, 2020

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 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackzampolin commented Mar 7, 2020

jackzampolin commented Mar 7, 2020

cwgoes commented Feb 27, 2020 •

edited by fedekunze

Loading

cwgoes commented Feb 27, 2020 •

edited

Loading

cwgoes left a comment •

edited

Loading