Switch to lazyledger-core instead of tendermint

[liamsi]

Ensure that this fork uses lazyledger-core instead of tendermint as we will make use of some modifications of ll-core.

This probably means to search and replace go-imports. If a less invasive approach works that's even better (e.g. the replace directive).

[evan-forbes]

I figured I'd post my progress here and see if anyone can find something obvious while I continue to debug. I'm using the evan/replace-tendermint branch, where I have manually replaced tendermint with lazyledger-core, updated the tendermint related proto files, and made a few minor compatibility changes to allow things to compile with the latest master branch of the sdk and lazyledger-core. Any ideas on what to try would be greatly appreciated.

There are still nine tests that won't pass, but I think some are caused by the same issue. Here are the tests and their error, along with the full output of make test

TestInterceptConfigsPreRunHandlerReadsConfigToml - DBPath was not set from config.toml

TestInterceptConfigsPreRunHandlerDoesNotMixConfigFiles - DBPath was not set from config.toml

fixed the above two with fadd156c36eed33ef22c92a0aa80ce65e9bf8a36, which was stopping viper from unmarshalling a config file properly. However, after comparing ll-core, tendermint, and various versions of the sdk, I can't find a difference for why this tests fails when replacing tendermint with ll-core. Both have the same version of relevant dependencies and idendical config tags

TestIntegrationTestSuite/TestGetCmdQueryAnnualProvisions/json_output - panic message redacted to hide potentially sensitive system info: panic

TestIntegrationTestSuite/TestGetCmdQueryAnnualProvisions/text_output - panic message redacted to hide potentially sensitive system info: panic

TestIntegrationTestSuite/TestGetCmdQueryInflation/json_output- panic message redacted to hide potentially sensitive system info: panic

TestIntegrationTestSuite/TestGetCmdQueryInflation/text_output - panic message redacted to hide potentially sensitive system info: panic

TestMintTestSuite/TestGRPCParams - test panicked: invalid Go type types.Dec for field cosmos.mint.v1beta1.QueryInflationResponse.inflation

TestIntegrationTestSuite/TestQueryGRPC/gRPC_request_inflation - Received unexpected error: unknown field "details" in types.QueryInflationResponse

TestIntegrationTestSuite/TestQueryGRPC/gRPC_request_annual_provisions - Received unexpected error: unknown field "details" in types.QueryAnnualProvisionsResponse

The output also repeats this message a few times, which I think might be because the cosmos-sdk is using the cosmos/iavl, which, as discussed here, has dependencies for tendermint, and also caries an extra copy of all the proto files in lazyledger-core. I'm going to try changing the proto package declarations for lazyledger-core next.

2021/01/12 11:08:17 proto: duplicate proto type registered: tendermint.crypto.Proof
2021/01/12 11:08:17 proto: duplicate proto type registered: tendermint.crypto.ValueOp
2021/01/12 11:08:17 proto: duplicate proto type registered: tendermint.crypto.DominoOp
2021/01/12 11:08:17 proto: duplicate proto type registered: tendermint.crypto.ProofOp
2021/01/12 11:08:17 proto: duplicate proto type registered: tendermint.crypto.ProofOps```

[liamsi]

Is there a branch I can checkout?

[evan-forbes]

yeah evan/replace-tendermint

[evan-forbes]

So good (maybe bad...) news. After some more digging, all the remaining errors are due to the same thing. For some reason, protobuf is not recognizing the custom type found in two of the x/mint module's QueryResponse types Inflation and AnnualProvisions. The weird thing is that this custom type is used in many other protobuf message types, including the mint module's own Params message, which does not have same issue as the two mentioned message types, even when ran in the same test

[adlerjohn]

Can you post the actual error? I might want to look into this since I've had to wrestle with protobuf before.

[evan-forbes]

invalid Go type types.Dec for field cosmos.mint.v1beta1.QueryAnnualProvisionsResponse.annual_provisions: panic [cosmos/cosmos-sdk/baseapp/abci.go:395]

[evan-forbes]

Just an update and personal notes on this sneaky bug:
The panic is occurring when protobuf is marshalling the response here. The types.Dec type is being reflected as bytes, which I'm fairly certain is wrong. The weirdest thing is that I still can't figure out why switching to ll-core causes this bug. So far, all the code that has to do with the bug is contained in the cosmos-sdk. I have also been unable to recreate the bug outside of this context. Those things make me think it could be another weird dependency thing, especially considering the multiple versions of google.golang.org/protobuf, github.com/gogo/protobuf, github.com/golang/protobuf that are required in the dependency tree.

Tomorrow, I'll give another go at recreating the bug outside of this context, and then I think I'm going to try #138 porting changes from v0.34.2 to lazyledger-core, and see if that makes a difference.

PS: after diving through the layers of reflection in protobuf, I am now firmly in the "go needs generics" boat.

[liamsi]

Tomorrow, I'll give another go at recreating the bug outside of this context, and then I think I'm going to try #138 porting changes from v0.34.2 to lazyledger-core, and see if that makes a difference.

Yeah, it's probably a good idea if ll-core is on par with the release that version of the SDK uses (independent of if this is causing the described problems or not; but it's likely related).

[liamsi]

2021/01/12 11:08:17 proto: duplicate proto type registered: tendermint.crypto.Proof

You've probably already figured this out but these also originate from tendermint/ll-core:

https://github.com/lazyledger/lazyledger-core/blob/626c19469070df0868e0570d1a016d0a155d17fd/proto/tendermint/crypto/proof.pb.go#L317-L318

Two different versions of this try to get registered (prob. due to the different tendermint versions that were used)

func RegisterType(x Message, name string) {
	if _, ok := protoTypedNils[name]; ok {
		// TODO: Some day, make this a panic.
		log.Printf("proto: duplicate proto type registered: %s", name)
		return
	}
// ...

[evan-forbes]

Huge thank you to @liamsi for finding the source of the bugs, and to everyone else that took the time to help me. It was a dependency issue, although I tried different versions of protobuf and gogo/protobuf, I wasn't changing versions of grpc, which was the problem dependency that got auto updated by switching to lazyledger-core. Since then, lazyledger-core's version of grpc has been downgraded, and a PR submitted to close this issue.

I guess I also owe an apology to protobuf, for all the curses I threw its way while I tried to fix what I thought was a sneaky marshalling bug.

We still might want to look into a fix for the duplicate registry of protobuf types, but that's another issue.