update libs #4970

pfi79 · 2024-08-29T17:48:32Z

#3650

Libraries have been updated:

github.com/hyperledger/fabric-chaincode-go/v2 v2.0.0
github.com/hyperledger/fabric-protos-go-apiv2 v0.3.3
github.com/hyperledger/fabric-config v0.3.0
github.com/IBM/idemix v0.0.2-0.20240913182345-72941a5f41cd

From the features:

proto.Marshal(nil) now does not return an error
from proto to string, protobuf library inserts floating spaces into text and it is dangerous to compare with constant strings.
it is almost impossible to break a change into small parts

There are floating bugs, especially in unit tests, which appear periodically

Signed-off-by: Fedor Partanskiy <fredprtnsk@gmail.com>

C0rWin

@pfi79 thanks, this is a long standing technical debt we had it the project. I am fine with the change in general, not sure it's possible to scope it to iduce less changed files.

need to carefully decided the order of how we would like to make a merge, cause it affects other repos.

regardless, I think additional couple of eyes needed to conclude on the review due to it's size (@denyeart @yacovm)

C0rWin · 2024-09-05T21:26:05Z

common/ledger/blkstorage/block_serialization.go

+	buf = addHeaderBytes(block.Header, buf)
+	info.txOffsets, buf = addDataBytesAndConstructTxIndexInfo(block.Data, buf)
+	buf = addMetadataBytes(block.Metadata, buf)
+	return buf, info, nil


if function not going to return and error, I guess you can get rid of it in function signature also.

C0rWin · 2024-09-05T21:30:51Z

common/ledger/blkstorage/block_stream.go

-	length, n := proto.DecodeVarint(lenBytes)
-	if n == 0 {
+	length, n := protowire.ConsumeVarint(lenBytes)
+	if n <= 0 {


what is the use case for negative value of n?

The DecodeVarint method checks for a negative value of n. I just repeated it.

https://github.com/golang/protobuf/blob/master/proto/buffer.go#L41

ConsumeVarint can return -1

C0rWin · 2024-09-05T21:33:18Z

common/ledger/blkstorage/blockfile_mgr.go

-	return buffer.Bytes(), nil
+	buf = protowire.AppendVarint(buf, noBlockFilesMarker)
+
+	return buf, nil


itit will always return the nil error value, IMO you'd better remove it.

yacovm · 2024-09-06T12:12:36Z

@pfi79 thanks, this is a long standing technical debt we had it the project. I am fine with the change in general, not sure it's possible to scope it to iduce less changed files.

need to carefully decided the order of how we would like to make a merge, cause it affects other repos.

regardless, I think additional couple of eyes needed to conclude on the review due to it's size (@denyeart @yacovm)

Given Fabric v3.0 GA should be out very shortly, perhaps it would make sense to merge this afterwards?
I don't think now is a good time be using temporary github forks for idemix and fabric-config given that we aim to cut the GA version very soon.

@denyeart when are you planning to do the GA release?

pfi79 · 2024-09-06T12:19:49Z

@pfi79 thanks, this is a long standing technical debt we had it the project. I am fine with the change in general, not sure it's possible to scope it to iduce less changed files.
need to carefully decided the order of how we would like to make a merge, cause it affects other repos.
regardless, I think additional couple of eyes needed to conclude on the review due to it's size (@denyeart @yacovm)

Given Fabric v3.0 GA should be out very shortly, perhaps it would make sense to merge this afterwards? I don't think now is a good time be using temporary github forks for idemix and fabric-config given that we aim to cut the GA version very soon.

@denyeart when are you planning to do the GA release?

Sorry, but changes to fabric-config and idemix have been proposed a week ago.
If they are accepted, I will replace these versions here immediately.

And I was hoping they would be accepted sooner than this change.

yacovm · 2024-09-06T12:24:30Z

@pfi79 thanks, this is a long standing technical debt we had it the project. I am fine with the change in general, not sure it's possible to scope it to iduce less changed files.
need to carefully decided the order of how we would like to make a merge, cause it affects other repos.
regardless, I think additional couple of eyes needed to conclude on the review due to it's size (@denyeart @yacovm)

Given Fabric v3.0 GA should be out very shortly, perhaps it would make sense to merge this afterwards? I don't think now is a good time be using temporary github forks for idemix and fabric-config given that we aim to cut the GA version very soon.
@denyeart when are you planning to do the GA release?

Sorry, but changes to fabric-config and idemix have been proposed a week ago. If they are accepted, I will replace these versions here immediately.

And I was hoping they would be accepted sooner than this change.

It's great that you have those ready, your maintenance work is appreciated.

Given you have already issued PRs for the module dependencies, it would make sense to merge them first before merging this.

pfi79 · 2024-09-06T12:37:30Z

@pfi79 thanks, this is a long standing technical debt we had it the project. I am fine with the change in general, not sure it's possible to scope it to iduce less changed files.
need to carefully decided the order of how we would like to make a merge, cause it affects other repos.
regardless, I think additional couple of eyes needed to conclude on the review due to it's size (@denyeart @yacovm)

Given Fabric v3.0 GA should be out very shortly, perhaps it would make sense to merge this afterwards? I don't think now is a good time be using temporary github forks for idemix and fabric-config given that we aim to cut the GA version very soon.
@denyeart when are you planning to do the GA release?

Sorry, but changes to fabric-config and idemix have been proposed a week ago. If they are accepted, I will replace these versions here immediately.
And I was hoping they would be accepted sooner than this change.

It's great that you have those ready, your maintenance work is appreciated.

Given you have already issued PRs for the module dependencies, it would make sense to merge them first before merging this.

Can you review and comment on them?

denyeart · 2024-09-06T16:48:39Z

I agree dependent projects should be completed first. @ale-linux is the maintainer for idemix, @ale-linux can you look at these:
IBM/idemix#47
IBM/idemix#48

@pfi79 Thank you for adding some of the plan to #3650. Could you clarify in that issue the entire plan so that it is crystal clear? What is the complete set of PRs that will be needed across all repositories? Are they all open at this time? What tests will be required to mitigate risks associated with the update?

Based on the idemix progress and the overall plan, we can then make a decision about the release.
Ideally a major change like this would go in at a major release boundary. That would imply getting it in prior to cutting v3.0. However, we'll need to assess the overall scope and risk and progress in dependency projects.

denyeart · 2024-09-10T15:55:24Z

go.mod

 	github.com/gorilla/handlers v1.5.1
 	github.com/gorilla/mux v1.8.0
 	github.com/grpc-ecosystem/go-grpc-middleware v1.3.0
 	github.com/hyperledger-labs/SmartBFT v0.0.0-20240616160543-3f61a410b8c1
-	github.com/hyperledger/fabric-chaincode-go v0.0.0-20220713164125-8f0791c989d7
+	github.com/hyperledger/fabric-chaincode-go/v2 v2.0.0-20240802023949-a356b32676fd


A proper v2.0.0 release is now available - https://github.com/hyperledger/fabric-chaincode-go/releases/tag/v2.0.0

denyeart · 2024-09-12T21:19:02Z

I've built Fabric components from this PR and tested them in a mixed environment with Fabric v2.5.9 nodes. Everything seems to work in a mixed environment.

Next step is for @ale-linux to review IBM/idemix#47.

denyeart · 2024-09-13T15:47:08Z

@pfi79 I think you've made updates to the "delete shadow copy proto-struct" PR which diverges from the commit in this PR. I'm agreeable to having all the commits in this PR and closing the other PRs, if you can update everything here. That will make it cleaner for a single PR to test and merge. What do you think?

Signed-off-by: Fedor Partanskiy <fredprtnsk@gmail.com>

pfi79 · 2024-09-13T20:01:53Z

@denyeart @C0rWin @yacovm

All forks cleared, pips passed.
I suggest that you take another close look at the change. Because it doesn't depend on anything else.

denyeart · 2024-09-14T02:53:40Z

@pfi79 Thanks for the persistence and getting this done. I've tested latest commit again in a mixed environment with new orderer and peer nodes alongside v2.5.9 orderers and peer nodes. All looks good.

The changes look good to me. I'd like to get this in and then release v3.0 shortly. I'll give the others one more chance for final review before merging.

C0rWin · 2024-09-15T21:14:41Z

@pfi79 Thanks for the persistence and getting this done. I've tested latest commit again in a mixed environment with new orderer and peer nodes alongside v2.5.9 orderers and peer nodes. All looks good.

The changes look good to me. I'd like to get this in and then release v3.0 shortly. I'll give the others one more chance for final review before merging.

@denyeart looks for me, IMO we can get it in.

denyeart · 2024-09-16T13:47:10Z

@pfi79 I see that you've updated SmartBFT in hyperledger-labs/SmartBFT#597. Do you plan to update Fabric to use the updated SmartBFT? Does it matter either way? We are planning to release Fabric v3.0 shortly...

pfi79 · 2024-09-16T13:57:30Z

@pfi79 I see that you've updated SmartBFT in hyperledger-labs/SmartBFT#597. Do you plan to update Fabric to use the updated SmartBFT? Does it matter either way? We are planning to release Fabric v3.0 shortly...

Yes, and smartbft and gomega, figured out the error. I thought to send them right after this pr. But if you think otherwise, then I can add them to this pr. although this pr is already very big.

denyeart · 2024-09-16T14:30:40Z

@pfi79 I see that you've updated SmartBFT in hyperledger-labs/SmartBFT#597. Do you plan to update Fabric to use the updated SmartBFT? Does it matter either way? We are planning to release Fabric v3.0 shortly...

Yes, and smartbft and gomega, figured out the error. I thought to send them right after this pr. But if you think otherwise, then I can add them to this pr. although this pr is already very big.

Ok thanks, I'll merge this PR and then you can do the follow-on PRs.

pfi79 requested a review from a team as a code owner August 29, 2024 17:48

pfi79 force-pushed the proba-pera branch 2 times, most recently from 6073d10 to 7a3a573 Compare August 29, 2024 19:32

pfi79 mentioned this pull request Aug 29, 2024

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

Closed

pfi79 force-pushed the proba-pera branch from 7a3a573 to 800ddb2 Compare August 30, 2024 07:22

pfi79 added 2 commits August 30, 2024 10:33

delete shadow copy proto-struct

9ae1574

Signed-off-by: Fedor Partanskiy <fredprtnsk@gmail.com>

replacing old functions that are not in the new protobuf package

6933629

Signed-off-by: Fedor Partanskiy <fredprtnsk@gmail.com>

pfi79 force-pushed the proba-pera branch 12 times, most recently from 9b5b5b5 to 951e95a Compare September 2, 2024 09:43

C0rWin reviewed Sep 5, 2024

View reviewed changes

pfi79 force-pushed the proba-pera branch 3 times, most recently from 1c5ece9 to f1db654 Compare September 6, 2024 11:09

pfi79 force-pushed the proba-pera branch 2 times, most recently from 0e918cf to f5cee98 Compare September 7, 2024 09:33

denyeart reviewed Sep 10, 2024

View reviewed changes

pfi79 force-pushed the proba-pera branch 2 times, most recently from 8915e02 to 153c46d Compare September 10, 2024 18:30

pfi79 force-pushed the proba-pera branch 3 times, most recently from fcc744d to 519fc84 Compare September 13, 2024 06:12

pfi79 force-pushed the proba-pera branch 3 times, most recently from f40964a to 3ea871e Compare September 13, 2024 19:17

add new version libs

4ab5da0

Signed-off-by: Fedor Partanskiy <fredprtnsk@gmail.com>

pfi79 force-pushed the proba-pera branch from 3ea871e to 4ab5da0 Compare September 13, 2024 19:17

denyeart approved these changes Sep 16, 2024

View reviewed changes

denyeart merged commit 13792d5 into hyperledger:main Sep 16, 2024
14 of 15 checks passed

pfi79 deleted the proba-pera branch September 16, 2024 14:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update libs #4970

update libs #4970

pfi79 commented Aug 29, 2024 •

edited

Loading

C0rWin left a comment

C0rWin Sep 5, 2024

pfi79 Sep 6, 2024

C0rWin Sep 5, 2024

pfi79 Sep 6, 2024

pfi79 Sep 6, 2024

C0rWin Sep 5, 2024

pfi79 Sep 6, 2024

yacovm commented Sep 6, 2024

pfi79 commented Sep 6, 2024

yacovm commented Sep 6, 2024

pfi79 commented Sep 6, 2024

denyeart commented Sep 6, 2024

denyeart Sep 10, 2024

denyeart commented Sep 12, 2024

denyeart commented Sep 13, 2024

pfi79 commented Sep 13, 2024

denyeart commented Sep 14, 2024

C0rWin commented Sep 15, 2024

denyeart commented Sep 16, 2024

pfi79 commented Sep 16, 2024

denyeart commented Sep 16, 2024

update libs #4970

update libs #4970

Conversation

pfi79 commented Aug 29, 2024 • edited Loading

C0rWin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yacovm commented Sep 6, 2024

pfi79 commented Sep 6, 2024

yacovm commented Sep 6, 2024

pfi79 commented Sep 6, 2024

denyeart commented Sep 6, 2024

Choose a reason for hiding this comment

denyeart commented Sep 12, 2024

denyeart commented Sep 13, 2024

pfi79 commented Sep 13, 2024

denyeart commented Sep 14, 2024

C0rWin commented Sep 15, 2024

denyeart commented Sep 16, 2024

pfi79 commented Sep 16, 2024

denyeart commented Sep 16, 2024

pfi79 commented Aug 29, 2024 •

edited

Loading