tendermint: Update to master #6471

tac0turtle · 2020-06-19T13:57:07Z

Description

Updating sdk to latest tendermint (master) in order to start testing things on our side with the sdk.

This PR removes:

verfier from context (no longer present)
--trust-node:

closes: #XXXX

Bumps [github.com/prometheus/client_golang](https://github.com/prometheus/client_golang) from 1.6.0 to 1.7.0. - [Release notes](https://github.com/prometheus/client_golang/releases) - [Changelog](https://github.com/prometheus/client_golang/blob/master/CHANGELOG.md) - [Commits](prometheus/client_golang@v1.6.0...v1.7.0) Signed-off-by: dependabot-preview[bot] <support@dependabot.com>

…s/client_golang-1.7.0

fedekunze · 2020-06-19T14:28:03Z

@marbar3778 you also need to update the makefile to point to the commit in order to update the proto deps

tac0turtle · 2020-06-19T14:47:21Z

@marbar3778 you also need to update the makefile to point to the commit in order to update the proto deps

yup yup i know. There isnt a need for that right now.

tac0turtle · 2020-06-19T17:33:31Z

this is currently blocked on ics23-tendermint. I think it will have to be moved to the SDK or some functions that were removed from Tendermint that have been added to Tendermint will need to be moved elsewhere to avoid SDK importing ics23 and ics23 importing SDK

store/rootmulti/map.go

store/rootmulti/simple_mapt_test.go

tac0turtle · 2020-07-21T13:00:46Z

this may be blocked on a possible bug in tendermint. The bug has been present in all previous versions but only now is known due to the strictness of proto

alexanderbez · 2020-07-27T16:16:33Z

baseapp/abci.go

@@ -552,3 +554,19 @@ func splitPath(requestPath string) (path []string) {

 	return path
 }
+
+func (app *BaseApp) ListSnapshots(abci.RequestListSnapshots) abci.ResponseListSnapshots {


Do we plan on supporting state sync for stargate?

yes, there is a separate PR that integrates it but it's blocked on this PR. anyways these are added as no-ops for now

alexanderbez · 2020-07-27T16:18:32Z

crypto/armor.go

@@ -151,7 +152,7 @@ func encryptPrivKey(privKey crypto.PrivKey, passphrase string) (saltBytes []byte
 	}

 	key = crypto.Sha256(key) // get 32 bytes
-	privKeyBytes := privKey.Bytes()
+	privKeyBytes := legacy.Cdc.Amino.MustMarshalBinaryBare(privKey)


Why use the legacy amino codec here?

bytes before was the amino encoding. I wasn't sure if I should work on the migration of keys in this PR as its already large.

zmanian · 2020-07-27T17:59:19Z

Linking to tendermint/tendermint#5138 but my understanding is that it's blocking.

fedekunze · 2020-07-27T17:44:06Z

crypto/hd/algo.go

-		var bzArr [32]byte
-		copy(bzArr[:], bz)
-		return secp256k1.PrivKeySecp256k1(bzArr)
+		var bzArr = make([]byte, secp256k1.PrivKeySize)


Suggested change

var bzArr = make([]byte, secp256k1.PrivKeySize)

bzArr := make([]byte, secp256k1.PrivKeySize)

fedekunze · 2020-07-27T17:46:12Z

crypto/types/multisig/threshold_pubkey.go

@@ -154,3 +154,5 @@ func (pk PubKeyMultisigThreshold) Equals(other crypto.PubKey) bool {
 	}
 	return true
 }
+
+func (pk PubKeyMultisigThreshold) Type() string { return " PubKeyMultisigThreshold" }


Suggested change

func (pk PubKeyMultisigThreshold) Type() string { return " PubKeyMultisigThreshold" }

func (pk PubKeyMultisigThreshold) Type() string { return "PubKeyMultisigThreshold" }

fedekunze · 2020-07-27T17:46:41Z

go.mod

 	google.golang.org/grpc v1.30.0
 	gopkg.in/yaml.v2 v2.3.0
 )

 replace github.com/gogo/protobuf => github.com/regen-network/protobuf v1.3.2-alpha.regen.4
+
+replace github.com/tendermint/tendermint => ../tendermint


fedekunze · 2020-07-27T17:47:54Z

server/export.go

 					MaxAgeNumBlocks: cp.Evidence.MaxAgeNumBlocks,
 					MaxAgeDuration:  cp.Evidence.MaxAgeDuration,
+					// TODO: expand to house other params


simapp/test_helpers.go

fedekunze · 2020-07-27T17:51:10Z

store/gaskv/store_test.go

-	t.Cleanup(iterator.Close)
+	t.Cleanup(func() {
+		if err := iterator.Close; err != nil {
+			t.Logf("error cleaning up iterator: %v", err())


is err a function?

fedekunze · 2020-07-27T17:51:19Z

store/gaskv/store_test.go

-	t.Cleanup(reverseIterator.Close)
+	t.Cleanup(func() {
+		if err := reverseIterator.Close; err != nil {
+			t.Logf("error cleaning up iterator: %v", err())


fedekunze · 2020-07-27T17:52:19Z

store/tracekv/store_test.go

-			expectedValue: nil,
-			expectedOut:   "{\"operation\":\"read\",\"key\":\"\",\"value\":\"\",\"metadata\":{\"blockHeight\":64}}\n",
-		},
+		// TODO: key types can not me empty


fedekunze · 2020-07-27T17:52:50Z

testutil/network/util.go

@@ -45,10 +46,12 @@ func startInProcess(cfg Config, val *Validator) error {
 		logger.With("module", val.Moniker),
 	)
 	if err != nil {
+		fmt.Println(err)


Suggested change

fmt.Println(err)

fedekunze · 2020-07-27T17:54:07Z

store/tracekv/store_test.go

-			expectedValue: nil,
-			expectedOut:   "{\"operation\":\"read\",\"key\":\"\",\"value\":\"\",\"metadata\":{\"blockHeight\":64}}\n",
-		},
+		// TODO: key types can not me empty


alexanderbez · 2020-07-29T15:02:22Z

I would like to know how many files diff just based on package renaming compared to logic changes. It might really simplify the review process if we could have 1 PR that is just for pkg renaming changes and separate PRs for logic changes -- no matter how small.

The base PR (pkg renaming) should be the first PR and approved immediately, but not merged. Then subsequent PRs are based against the base PR and are merged after thorough review. This way, we can tackle all logic changes piecemeal.

I know you already put a bunch of work into this @marbar3778, but reviewing a 200+ file diff is just not feasible.

colin-axner

did a first pass mostly over IBC stuff, mostly looks good to me 👍 just left comments on things that stood out

colin-axner · 2020-07-29T16:22:41Z

SECURITY.md

@@ -13,7 +13,7 @@ this [blog post](https://blog.cosmos.network/bug-bounty-program-for-tendermint-c

 The following is a list of examples of the kinds of bugs we're most interested
 in for the Cosmos SDK. See [here](https://github.com/tendermint/tendermint/blob/master/SECURITY.md) for vulnerabilities we are interested
-in for Tendermint and other lower-level libraries (eg. [IAVL](https://github.com/tendermint/iavl)).
+in for Tendermint and other lower-level libraries (eg. [IAVL](https://github.com/cosmos/iavl)).


shouldn't this be removed? I imagine IAVL bugs will now be reported to cosmos and not tendermint

colin-axner · 2020-07-29T16:25:37Z

testutil/network/util.go

 		return err
 	}

 	if err := tmNode.Start(); err != nil {
+		fmt.Println(err)


Suggested change

fmt.Println(err)

colin-axner · 2020-07-29T16:25:52Z

testutil/network/util.go

@@ -1,6 +1,7 @@
 package network

 import (
+	"fmt"


Suggested change

"fmt"

colin-axner · 2020-07-29T16:39:29Z

x/ibc/23-commitment/types/merkle.go

@@ -366,7 +366,7 @@ func convertProofs(mproof MerkleProof) ([]*ics23.CommitmentProof, error) {

 // Empty returns true if the root is empty
 func (proof MerkleProof) Empty() bool {
-	return proof.Proof.Equal(nil) || proof.Equal(MerkleProof{}) || proof.Proof.Equal(nil) || proof.Proof.Equal(merkle.Proof{})
+	return proto.Equal(proof.Proof, nil) || proto.Equal(&proof, &MerkleProof{}) || proto.Equal(proof.Proof, nil) || proto.Equal(proof.Proof, &tmmerkle.ProofOps{})


this is outside the scope of this pr, but there is a duplicate check here with proto.Equal(proof.Proof, nil)

I think this is a bug, I think the first check should be proto.Equal(proof, nil)

colin-axner · 2020-07-29T16:45:49Z

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

@@ -109,7 +109,12 @@ func QueryTendermintHeader(clientCtx client.Context) (ibctmtypes.Header, int64,
 		return ibctmtypes.Header{}, 0, err
 	}

-	validators, err := node.Validators(&height, 0, 10000)
+	page := new(int)
+	*page = 0


is there no way to condense these lines?

colin-axner · 2020-07-29T16:53:36Z

types/utils.go

@@ -91,7 +91,12 @@ func NewLevelDB(name, dir string) (db dbm.DB, err error) {
 			err = fmt.Errorf("couldn't create db: %v", r)
 		}
 	}()
-	return dbm.NewDB(name, backend, dir), err
+
+	if err != nil {


is this necessary? won't the error be returned because the err error definition in the return

I think a direct return of the error in the defer func makes the most sense

colin-axner · 2020-07-29T16:55:39Z

types/events.go

@@ -79,8 +78,8 @@ func (a Attribute) String() string {
 }

 // ToKVPair converts an Attribute object into a Tendermint key/value pair.
-func (a Attribute) ToKVPair() tmkv.Pair {
-	return tmkv.Pair{Key: toBytes(a.Key), Value: toBytes(a.Value)}
+func (a Attribute) ToKVPair() abci.EventAttribute {


shouldn't this be using the cosmos kv instead of returning an abci.EventAttribute? the func name and comment differs with behavior with this update

colin-axner · 2020-07-29T17:06:15Z

I agree with @alexanderbez suggestion, I think it'll lead to a faster review process and not require multiple people to scan 200 files worth of import name changes.

@marbar3778 I can lend a hand with doing import name changes on a base PR tomorrow, we could divide up the work like I could do ibc and some other modules so we can work at the same time

tac0turtle · 2020-07-30T09:42:18Z

breaking this pr into multiple ones

tac0turtle · 2020-07-30T18:42:19Z

closing this in favor of #6892

tac0turtle and others added 6 commits June 14, 2020 12:04

start a branch with needed tm updates

8d258c2

Merge branch 'master' into dependabot/go_modules/github.com/prometheu…

80ec7b3

…s/client_golang-1.7.0

generate new proto files

1398a99

Merge branch 'master' into dependabot/go_modules/github.com/prometheu…

df5ec42

…s/client_golang-1.7.0

more updates

38a0d01

tac0turtle added the WIP label Jun 19, 2020

tac0turtle self-assigned this Jun 19, 2020

tac0turtle added 5 commits June 19, 2020 17:29

some more fixes

012b486

Merge branch 'master' of https://github.com/cosmos/cosmos-sdk

332aa29

Merge branch 'master' into marko/tm_update

c6116b6

moar changes

581dbf4

add simpleproofsfrommap and various fixes

7026c42

tac0turtle added 4 commits June 19, 2020 20:06

add simplemap

a540a84

fix type

735a2e3

move to tendermint tag

c0869ca

Merge branch 'master' into marko/tm_update

c6be83b

fedekunze reviewed Jun 24, 2020

View reviewed changes

store/rootmulti/map.go Outdated Show resolved Hide resolved

fedekunze reviewed Jun 24, 2020

View reviewed changes

store/rootmulti/simple_mapt_test.go Outdated Show resolved Hide resolved

fedekunze and others added 3 commits June 26, 2020 12:59

fix conflicts with master

7ccba89

update tm version for proto update deps

b20f010

Merge branch 'master' into marko/tm_update

13f02f3

tac0turtle changed the title ~~DNM: Tm update for testing~~ DNM: Tm update Jun 29, 2020

tac0turtle added 4 commits June 29, 2020 11:10

fix a lot more issues

910255f

go through fixing modules

4a19534

bring over and update proto deps

f7a79e3

fix more modules

4e1c82e

tac0turtle and others added 7 commits July 20, 2020 16:20

fix imports

436a728

Merge branch 'master' into marko/tm_update

ff815fa

update proto-deps

fb5c2ac

Merge branch 'master' into marko/tm_update

80505e0

fix more tests

96a317c

fix more tests

13af89c

fix conflicts with master

ace32bd

tac0turtle added 8 commits July 23, 2020 08:53

Merge branch 'master' into marko/tm_update

01fd97f

Merge branch 'master' into marko/tm_update

ecf36ed

fix amino useage

d72ad43

fix build

cae7fc9

key change

ca011ee

Merge branch 'master' into marko/tm_update

a780ed0

change test to ed25519

df13158

Merge branch 'master' into marko/tm_update

bf0ead2

alexanderbez reviewed Jul 27, 2020

View reviewed changes

fedekunze reviewed Jul 27, 2020

View reviewed changes

colin-axner reviewed Jul 29, 2020

View reviewed changes

tac0turtle changed the base branch from master to tm_update_1 July 30, 2020 10:06

tac0turtle changed the base branch from tm_update_1 to master July 30, 2020 10:07

colin-axner mentioned this pull request Jul 30, 2020

fix nil check bug #6895

Merged

9 tasks

tac0turtle closed this Jul 30, 2020

tac0turtle deleted the marko/tm_update branch October 21, 2020 15:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tendermint: Update to master #6471

tendermint: Update to master #6471

tac0turtle commented Jun 19, 2020 •

edited

Loading

fedekunze commented Jun 19, 2020

tac0turtle commented Jun 19, 2020

tac0turtle commented Jun 19, 2020

tac0turtle commented Jul 21, 2020

alexanderbez Jul 27, 2020

tac0turtle Jul 27, 2020

alexanderbez Jul 27, 2020

tac0turtle Jul 27, 2020

zmanian commented Jul 27, 2020

fedekunze Jul 27, 2020

fedekunze Jul 27, 2020

fedekunze Jul 27, 2020

fedekunze Jul 27, 2020

fedekunze Jul 27, 2020

fedekunze Jul 27, 2020

fedekunze Jul 27, 2020

fedekunze Jul 27, 2020

fedekunze Jul 27, 2020

alexanderbez commented Jul 29, 2020 •

edited

Loading

colin-axner left a comment

colin-axner Jul 29, 2020

colin-axner Jul 29, 2020

colin-axner Jul 29, 2020

colin-axner Jul 29, 2020

colin-axner Jul 29, 2020

colin-axner Jul 29, 2020

colin-axner Jul 29, 2020

colin-axner Jul 29, 2020

colin-axner Jul 29, 2020

colin-axner commented Jul 29, 2020

tac0turtle commented Jul 30, 2020

tac0turtle commented Jul 30, 2020

	var bzArr = make([]byte, secp256k1.PrivKeySize)
	bzArr := make([]byte, secp256k1.PrivKeySize)

	func (pk PubKeyMultisigThreshold) Type() string { return " PubKeyMultisigThreshold" }
	func (pk PubKeyMultisigThreshold) Type() string { return "PubKeyMultisigThreshold" }

tendermint: Update to master #6471

tendermint: Update to master #6471

Conversation

tac0turtle commented Jun 19, 2020 • edited Loading

Description

fedekunze commented Jun 19, 2020

tac0turtle commented Jun 19, 2020

tac0turtle commented Jun 19, 2020

tac0turtle commented Jul 21, 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

zmanian commented Jul 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

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 commented Jul 29, 2020 • edited Loading

colin-axner 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

Choose a reason for hiding this comment

colin-axner commented Jul 29, 2020

tac0turtle commented Jul 30, 2020

tac0turtle commented Jul 30, 2020

tac0turtle commented Jun 19, 2020 •

edited

Loading

alexanderbez commented Jul 29, 2020 •

edited

Loading