search

hyperledger / fabric

Hyperledger Fabric is an enterprise-grade permissioned distributed ledger framework for developing solutions and applications. Its modular and versatile design satisfies a broad range of industry use cases. It offers a unique approach to consensus that enables performance at scale while preserving privacy.
https://wiki.hyperledger.org/display/fabric
Apache License 2.0
15.67k stars 8.82k forks source link

delete shadow copy proto-struct #4962

Closed pfi79 closed 2 weeks ago

pfi79 commented 1 month ago

The new fabric-protos-go-apiv2 has copy protection in the structures. This code is adapted for the transition to the new protos-go-apiv2.

denyeart commented 1 month ago

There is an open issue discussing potential update to protos-go-apiv2. Due to some concerns that were raised in that issue, my latest thinking was to not proceed with the update. Let's continue the discussion in that issue prior to moving forward with the PRs to update to protos-go-apiv2.

pfi79 commented 1 month ago

There is an open issue discussing potential update to protos-go-apiv2. Due to some concerns that were raised in that issue, my latest thinking was to not proceed with the update. Let's continue the discussion in that issue prior to moving forward with the PRs to update to protos-go-apiv2.

Yes, it's a move towards protos-go-apiv2. But independent of it. No new protos, no new protobuf.

I specifically highlighted this code, which is independent of protos-go-apiv2.

I would suggest that it should be considered independent.

denyeart commented 1 month ago

There is an open issue discussing potential update to protos-go-apiv2. Due to some concerns that were raised in that issue, my latest thinking was to not proceed with the update. Let's continue the discussion in that issue prior to moving forward with the PRs to update to protos-go-apiv2.

Yes, it's a move towards protos-go-apiv2. But independent of it. No new protos, no new protobuf.

I specifically highlighted this code, which is independent of protos-go-apiv2.

I would suggest that it should be considered independent.

Yes, it can be considered independent. But in that case the PR description should mention the rationale for the various changes. For example adding proto.Clone() in various places seems to add a dependency where none was required previously. Can you describe the benefits? And some of the other changes seem not related, ideally they would be done in a separate pull request or at least mention the rationale for the changes in the PR description.

pfi79 commented 1 month ago

Yes, it can be considered independent. But in that case the PR description should mention the rationale for the various changes. For example adding proto.Clone() in various places seems to add a dependency where none was required previously. Can you describe the benefits? And some of the other changes seem not related, ideally they would be done in a separate pull request or at least mention the rationale for the changes in the PR description.

I think the developers of the protobuf package added shadow copy protection to the new structures for a reason. It means that they had good reasons for it. And if we can't switch to a new version of protobuf yet, we can at least remove shadow copying in them. And in those places where copying is required make it more explicit for further code maintenance.

But if you don't consider this an important reason, let's wait until I prepare a version with all the changes and present it in the corresponding issue.

bestbeforetoday commented 1 month ago

See this comment describing why it is not safe to make shallow copies of Go protobuf messages, which explains why they are referred to in the protobuf API only as pointers (proto.Message interface):

https://github.com/golang/protobuf/issues/1225#issuecomment-713699536