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.68k stars 8.83k forks source link

Use new (non-deprecated) protocol buffer bindings #3650

Closed bestbeforetoday closed 2 weeks ago

bestbeforetoday commented 2 years ago

As a user of Fabric I want Fabric v3 to use fabric-protos Go bindings based on protocol buffer APIv2 So that Fabric is using actively maintained and non-deprecated protobuf APIs

In early 2020, a new Go API for protocol buffers was released, and the older protocol buffer API was deprecated.

The two API versions produce 100% wire-level compatible protobuf messages so they can be used interchangeably with no compatibility implications for client applications, SDKs or other Fabric components. This has already been proven in the Fabric Gateway client API, which migrated seamlessly from the old to new protocol buffer API and maintained complete compatibility with Fabric.

Pre-built Go language bindings for the Fabric protobufs are already published based on both the old (fabric-protos-go) and new (fabric-protos-go-apiv2). The only work required is to change Fabric code to use the new protos package.

denyeart commented 1 year ago

Per https://github.com/hyperledger/fabric-protos/blob/main/RELEASING.md fabric-protos-go-apiv2 v0.3.0 is ready for use by fabric main branch.

bestbeforetoday commented 1 year ago

I briefly looked at this and it seems more involved than I'd hoped, particularly since migrating fabric-gateway was very easy (see commit). Other Fabric packages like fabric-config expose the older/deprecated protobuf package in their public API, which is a headache and likely a breaking change.

Since the old protobuf package got reimplemented using the newer protobuf package internally, you can translate between V1 and V2 protobuf Message structs, which might allow at least some changes to be made without breaking any public APIs. See this utility that used to be in fabric-gateway before it was migrated over entirely, which provided a few utility functions that can operate on both V1 and V2 messages by using the GeneratedMessage interface:

hyperledger/fabric-gateway/pkg/internal/util/protobuf.go

Ideally it would be good to entirely replace usage of the old protobuf API with the new one, and avoid juggling between the two message types, but taking advantage of the ability to transform message types might make it easier to migrate step-by-step instead of in one big bang.

denyeart commented 8 months ago

I would be in favor of updating all fabric repositories to APIv2 before Fabric v3 gets released (including Go chaincode repositories), if somebody tests and successfully proves that having a mixed set of Fabric v2.x orderers and peers (old protobuf) and new v3.x orderers and peers (new APIv2) works seamlessly. Any volunteers?

jt-nti commented 8 months ago

Getting everything on APIv2 for Fabric v3 would be great, although none of the modules involved follow the main Fabric versioning! :)

Working out what to do about versions for fabric-chaincode-go (currently pseudo-version numbers) and fabric-contract-api-go (currently v1.2.2) would be good. It seems like these would be breaking changes, so in theory v1 and v2 respectively would make sense...

https://go.dev/doc/modules/version-numbers

davidkhala commented 8 months ago

fabric-chaincode-go

I can help on fabric-chaincode-go v1 change dev.

tobigiwa commented 8 months ago

The sync.Mutex that was dragged into the apiV2 is the problem... function that takes it type and return it... are being flagged by go vet as copy of locked values.

tobigiwa commented 8 months ago

The solution was to introduce pointers (on such functions signatures, 2 of 'em actually, if I remember correctly), the effect cascaded and some other function signatures had to take pointers too (no internal logic was touched) ... everything was well in the patch(PR) I included...test passed, go vet laid to bed... BUT... it broke an Interface definition.

tobigiwa commented 8 months ago

@denyeart mentioned once a solution... I didn't quite figure it... Buh I'll try take another look at it again this week, hopefully it clicks.

davidkhala commented 8 months ago

While I also see the sync.Mutex warning issue, I guess it is inevitable to change some interface definition. I am fine with that since it will only affects v3 and onwards versions

tobigiwa commented 8 months ago

@davidkhala this would require a large community vote... maybe maintainers like @denyeart @bestbeforetoday @ryjones can shed light to that better.

denyeart commented 8 months ago

@tobigiwa Can you provide more details of the Interface definition that gets broken? If it impacts interoperability between v2 and v3 components then it would be a no-go.

tobigiwa commented 8 months ago

Sorry it took this long to respond, I'll get on it including a solution I have in mind. Pardon me again.

denyeart commented 5 months ago

@tobigiwa Any update?

tobigiwa commented 5 months ago

@denyeart Sincerely speaking, I thought I have responded to this already, Please pardon me.

So to the problem at hand, the correction would take place in fabric-chaincode-go itself, which then can be cascaded to fabric-contract-api-go then to fabric. I have updated my fix on fabric-chaincode-go to catch up with main branch.

Regarding the changes to the interfaces, here they are: So it breaks to two interface, 3 method signature in total, by introduction of a pointer to the pb.Response (in the shim/interface.go file).

// Chaincode interface must be implemented by all chaincodes. The fabric runs
// the transactions by calling these functions as specified.
type Chaincode interface {
    // Init is called during Instantiate transaction after the chaincode container
    // has been established for the first time, allowing the chaincode to
    // initialize its internal data
    Init(stub ChaincodeStubInterface) *pb.Response

    // Invoke is called to update or query the ledger in a proposal transaction.
    // Updated state variables are not committed to the ledger until the
    // transaction is committed.
    Invoke(stub ChaincodeStubInterface) *pb.Response
}

type ChaincodeStubInterface interface {
InvokeChaincode(chaincodeName string, args [][]byte, channel string) *pb.Response
}

This alongside some function signatures too, I should add. Regarding the fix I was thinking, could not make it work, it was essentially a plan to fool go vet using generics regarding the copy lock values complaint. Using the new library as intended should be our solution, so no escaping those pointers.

bestbeforetoday commented 5 months ago

It seems like this might have highlighted an underlying issue with how protobuf messages are handled. There are cases where they are passed by value, whereas the protobuf packages are designed for message to be referred to with pointers, and are not safe to pass by value. Perhaps the right thing to do is bite the bullet and make the breaking API change for Fabric v3.

There were some good observations on breaking changes and versioning above. However, although fabric-contract-api-go already has a v1.x version and so seems like a more obvious problem, I'm not sure these changes significantly impact the public API of that module. Typically smart contract implementors are embedding the Contract type, which is not impacted. It does impact ContractChaincode, but this is typically just created with NewChaincode() and then has its Start() method called, neither of which are changed. People should generally be using this higher level API and not the lower level fabric-chaincode-go API so hopefully the end-user impact is minimised. Even so, I'm not against creating a v2 module. I don't think it's a lot of work and it's certainly the safest approach for both the chaincode APIs since it avoids anyone accidentally updating module dependencies and pulling in breaking changes.

tobigiwa commented 5 months ago

I was waiting for other comments, buh I'll just go ahead, you are very correct with this, it is the only way forward. Also, as you've highlighted, the changes are quite minimal.

What the way forward now? I'll like to do a little part in this.

denyeart commented 3 months ago

The last few comments have actually made me more concerned about a move to apiv2.

Since the apiv2 announcement at https://go.dev/blog/protobuf-apiv2 mentions that existing consumers can stay on apiv1 indefinitely, I haven't seen much motivation to move up with breaking changes that may impact fabric users.

bestbeforetoday commented 3 months ago

At the wire level the change is 100% compatible. It makes no difference to the client or server whether the other is using apiv1 or apiv2. The incompatibilities come when either:

  1. Code is using the protobuf APIs (not just the fabric-protos bindings), which are largely similar but do have some key differences.
  2. The namespace conflict that occurs in the Go protobuf implementation when multiple bindings for the exactly the same protobuf messages are mixed in the same application.

The first is contained to the application code so is resolved by making any necessary changes to (Fabric) code.

The second impacts users where their application code depends on both fabric-protos and on Fabric (core, chaincode or client API) code, which in turn depends on a specific fabric-protos version. To make certain the protobuf API version used doesn't catch out the application developer, I took the approach of a major version change for fabric-chaincode-go. For users who are making use of fabric-protos in their application code, they can chose which version implementation they want to use and migrate when they are ready.

The major version change for fabric-chaincode-go is also required to fix the problematic pass-by-value of protobuf messages in the public API.

bestbeforetoday commented 3 months ago

For reference, Go chaincodes are updated to use fabric-protos-go-apiv2:

pfi79 commented 1 month ago

What motivated me?

I was unable to update the fabric-chaincode-go/v2 package in fabric. I really didn't like it.

My plan.

I decided to try an update. And then thought about how to break that upgrade into smaller more manageable pieces.

  1. update the parts of the code that won't work in the new versions of protobuf, protos-apiv2 and fabric-chaincode-go/v2. (https://github.com/hyperledger/fabric/pull/4962 https://github.com/hyperledger/fabric/pull/4963 and https://github.com/hyperledger/fabric/pull/4964)
  2. update the old protobuf library to the new one (No new fabric-chaincode-go/v2 added yet and github.com/hyperledger/fabric-protos-go-apiv2)
  3. The hardest and biggest one. Replacing fabric-protos-go with fabric-protos-go-apiv2. Here you will have to replace 4 libraries that depend on protos-go at the same time. These are github.com/hyperledger/fabric-chaincode-go/v2 , github.com/hyperledger/fabric-protos-go-apiv2 , github.com/IBM/idemix and github.com/hyperledger/fabric-config. Hence here, preferably there should only be package renames and imports. And a minimum of code changes.

My versions of github.com/IBM/idemix and github.com/hyperledger/fabric-config are ready. But I don't offer them for changes yet, I want to build the whole thing and make sure it works.

As soon as the version with the new protobuf is ready I will post it here.

pfi79 commented 1 month ago

https://github.com/hyperledger/fabric/pull/4970

I present to you my version of the changes to the fabric. I'm not sure it's finished. But I suggest you consider it and continue the discussion.

bestbeforetoday commented 1 month ago

@pfi79 from my perspective, this is a great piece of work. I really like the way you tackled some of the sub-/pre-work in separate PRs, like avoiding pass-by-value for protobuf messages. All versions of the v1 protobuf APIs from the last few years are actually implemented on top of the v2 protobuf API, so it is possible to adopt some v2-specific functions in advance of a full switch by converting v1 proto.Message to v2 proto.Message structs.

One of the tricky aspects is that the protobuf change impacts not only the core Fabric codebase but also dependencies such as fabric-config and idemix. For cases where the public API of those libraries cannot remain unchanged, perhaps the right approach is to deliver v2 implementations of those dependencies (similar to fabric-chaincode-go/v2) in advance of the critical change in the Fabric codebase that would switch to those v2 dependencies. I think you've essentially proved out potential v2 implementations with the versions you've delivered in your forks. This is exactly what I did for fabric-chaincode-go and fabric-contract-api-go to prove I could deliver a fully working chaincode implementation based on the v2 protobuf API.

For Idemix, I wonder if a core implementation could avoid Fabric-specific content (protobufs) entirely, with a fabric-idemix layer that presents that core Idemix capability in a form that is more consumable by Fabric if it doesn't make sense for that code to live in core Fabric. I say this without looking at the Idemix code in any detail to know if it makes real sense; just an idea to consider.

pfi79 commented 1 month ago

@pfi79 from my perspective, this is a great piece of work. I really like the way you tackled some of the sub-/pre-work in separate PRs, like avoiding pass-by-value for protobuf messages. All versions of the v1 protobuf APIs from the last few years are actually implemented on top of the v2 protobuf API, so it is possible to adopt some v2-specific functions in advance of a full switch by converting v1 proto.Message to v2 proto.Message structs.

One of the tricky aspects is that the protobuf change impacts not only the core Fabric codebase but also dependencies such as fabric-config and idemix. For cases where the public API of those libraries cannot remain unchanged, perhaps the right approach is to deliver v2 implementations of those dependencies (similar to fabric-chaincode-go/v2) in advance of the critical change in the Fabric codebase that would switch to those v2 dependencies. I think you've essentially proved out potential v2 implementations with the versions you've delivered in your forks. This is exactly what I did for fabric-chaincode-go and fabric-contract-api-go to prove I could deliver a fully working chaincode implementation based on the v2 protobuf API.

For Idemix, I wonder if a core implementation could avoid Fabric-specific content (protobufs) entirely, with a fabric-idemix layer that presents that core Idemix capability in a form that is more consumable by Fabric if it doesn't make sense for that code to live in core Fabric. I say this without looking at the Idemix code in any detail to know if it makes real sense; just an idea to consider.

Thank you so much for your comment.

I am ready to redo my pr in fabric-config and idemix similar to fabric-chaincode-go/v2

pfi79 commented 4 weeks ago

I suggest that you choose one of the code changes for fabric-config:

https://github.com/hyperledger/fabric-config/pull/66 or https://github.com/hyperledger/fabric-config/pull/67

pfi79 commented 4 weeks ago

I suggest that you choose one of the code changes for idemix:

https://github.com/IBM/idemix/pull/47 or https://github.com/IBM/idemix/pull/48

can someone run pipelines there (github actions)?

pfi79 commented 3 weeks ago

@denyeart at your request, I'm specifying what needs to be done:

What is the complete set of PRs that will be needed across all repositories? Are they all open at this time?

  1. modify fabric-config
  2. modify idemix
  3. modify fabric

All pr's are open, I look forward to discussing them and am willing to make changes as soon as possible, as requested by the community. There are no other repositories for changes

What tests will be required to mitigate the risks associated with the upgrade?

I don't have anything to suggest here yet. Can the community suggest what should be added?

pfi79 commented 3 weeks ago

Colleagues, I realize that pr in idemix we are waiting for @ale-linux .

But, can any maintainer consider pr in fabric-config?

denyeart commented 3 weeks ago

I've merged the fabric-config PR.

In terms of testing... I've built Fabric components from the PR https://github.com/hyperledger/fabric/pull/4970 and tested them in a mixed environment with Fabric v2.5.9 nodes. Everything seems to work in a mixed environment. Nice job @pfi79 !

pfi79 commented 3 weeks ago

I've merged the fabric-config PR.

In terms of testing... I've built Fabric components from the PR #4970 and tested them in a mixed environment with Fabric v2.5.9 nodes. Everything seems to work in a mixed environment. Nice job @pfi79 !

That's weird. It shouldn't work in v2.5.9. When the program starts, all protobuf files are registered in the global registry of the program. And github.com/hyperledger/fabric-protos-go and github.com/hyperledger/fabric-protos-go-apiv2 should fight over a single namespace.

You can test this by creating a simple application that plugs in both libraries. And run it. It should crash with panic. https://go.dev/play/p/PL4AGl4U6mU

https://protobuf.dev/reference/go/faq/#namespace-conflict

pfi79 commented 2 weeks ago

it seems to me that this discussion can be closed

bestbeforetoday commented 2 weeks ago

Great work getting these changes done @pfi79 ⭐

denyeart commented 2 weeks ago

I've merged the fabric-config PR. In terms of testing... I've built Fabric components from the PR #4970 and tested them in a mixed environment with Fabric v2.5.9 nodes. Everything seems to work in a mixed environment. Nice job @pfi79 !

That's weird. It shouldn't work in v2.5.9. When the program starts, all protobuf files are registered in the global registry of the program. And github.com/hyperledger/fabric-protos-go and github.com/hyperledger/fabric-protos-go-apiv2 should fight over a single namespace.

You can test this by creating a simple application that plugs in both libraries. And run it. It should crash with panic. https://go.dev/play/p/PL4AGl4U6mU

https://protobuf.dev/reference/go/faq/#namespace-conflict

What I meant is that I tested a mixed network comprised of v2.5.9 nodes (using old protobuf) and v3.0.0 nodes (using new protobuf) and everything works. This test was important because we would expect users to do a rolling upgrade of nodes where there would be a mix of old nodes and new nodes during the upgrade process.