Use new (non-deprecated) protocol buffer bindings #3650
Comments
|
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. |
|
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. |
|
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? |
|
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 |
I can help on fabric-chaincode-go v1 change dev. |
|
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. |
|
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. |
|
@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. |
|
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 |
|
@davidkhala this would require a large community vote... maybe maintainers like @denyeart @bestbeforetoday @ryjones can shed light to that better. |
|
@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. |
|
Sorry it took this long to respond, I'll get on it including a solution I have in mind. Pardon me again. |
|
@tobigiwa Any update? |
|
@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: // 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 |
|
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. |
|
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. |
|
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. |
|
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:
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. |
|
For reference, Go chaincodes are updated to use fabric-protos-go-apiv2:
|
|
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.
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. |
|
I present to you my version of the changes to the fabric. |
|
@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 |
|
I suggest that you choose one of the code changes for fabric-config: hyperledger/fabric-config#66 |
|
I suggest that you choose one of the code changes for idemix: IBM/idemix#47 can someone run pipelines there (github actions)? |
|
@denyeart at your request, I'm specifying what needs to be done:
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.
I don't have anything to suggest here yet. Can the community suggest what should be added? |
|
Colleagues, I realize that pr in idemix we are waiting for @ale-linux . But, can any maintainer consider pr in fabric-config? |
That's weird. It shouldn't work in v2.5.9. You can test this by creating a simple application that plugs in both libraries. And run it. It should crash with panic. |
|
it seems to me that this discussion can be closed |
|
Great work getting these changes done @pfi79 ⭐ |
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. |
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.
The text was updated successfully, but these errors were encountered: