Protobuf messages registration conflicts

lukasz-zimnoch commented 4 years ago

tss-lib points to the github.com/golang/protobuf v1.3.2 dependency in the go.mod file. However, according to Go modules version selection policy all versions of protobuf >= v1.3.2 and < v2 can be picked if present in the module list.

This can be a problem because tss-lib protobuf messages are registered twice which causes a registration conflict. The first registration occurs in the generated *.pb.go files and then the second registration is performed in messages.go files.

Example for the ecdsa/keygen package:

First registration: https://github.com/binance-chain/tss-lib/blob/abd66f888438ead112891bc3d74d04d09b94446f/ecdsa/keygen/ecdsa-keygen.pb.go#L235
Second registration: https://github.com/binance-chain/tss-lib/blob/abd66f888438ead112891bc3d74d04d09b94446f/ecdsa/keygen/messages.go#L35

The github.com/golang/protobuf v1.3.2 doesn't complain but the newer github.com/golang/protobuf v1.4.0 displays a warning:

2020/08/05 14:43:02 WARNING: proto: message KGRound1Message is already registered
A future release will panic on registration conflicts. See:
https://developers.google.com/protocol-buffers/docs/reference/go/faq#namespace-conflict

omershlo commented 3 years ago

hi @lukasz-zimnoch , thank you for reporting, can you suggest ways to fix this issue?

Shadowfiend commented 3 years ago

Thoughts on fixing this by simply removed from messages.go? If the generated pb.go file takes care of registration, the explicit manual registration in messages.go seems like it should be unnecessary. The init function is used in both cases, and both thepb.go file and messages.go are in the same package which means both registrations run at the same time during package initialization. That should mean the messages.go init function is redundant. I believe this should be true even on earlier versions that may not warn on duplicate registration.

PlamenHristov commented 3 years ago

Hey @omershlo,

We've noticed that when both ecdsa and eddsa are used depending on the intialization order, protobuf can't handle both at runtime because of name clashes.

I've opened a PR to fix that. As a side effect the warnings have been cleared as well.

Let me know what you think.

yycen commented 3 years ago

This PR also fixed the issue. See go.mod ecdsa/signing/messages.go ecdsa/keygen/ecdsa-keygen.pb.go and other related part.

bnb-chain / tss-lib

Protobuf messages registration conflicts #106