-
Notifications
You must be signed in to change notification settings - Fork 8.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FABGW-25 Endorse using generated ChaincodeInterest #2773
Conversation
internal/pkg/gateway/registry.go
Outdated
if err != nil { | ||
return nil, err | ||
logger.Errorf("PeersForEndorsement failed with error %s for channel %s and ChaincodeInterest %s", err, channel, proto.MarshalTextString(interest)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logger.Errorf("PeersForEndorsement failed with error %s for channel %s and ChaincodeInterest %s", err, channel, proto.MarshalTextString(interest)) | |
logger.Errorw("PeersForEndorsement failed.", "error", err, "channel", channel, "ChaincodeInterest", proto.MarshalTextString(interest)) |
For quite some time, we have been following this logging API for dumping logs in a better machine readable way.
internal/pkg/gateway/registry.go
Outdated
@@ -127,19 +127,29 @@ func (reg *registry) endorsers(channel string, chaincode string) ([]*endorser, e | |||
for _, peer := range receivers { | |||
endorsers = append(endorsers, peer.endorser) | |||
} | |||
|
|||
// if this plan doesn't contain the local org, abandon it in favour of one that does, since we already have a local endorsement |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// if this plan doesn't contain the local org, abandon it in favour of one that does, since we already have a local endorsement | |
// if this plan doesn't contain the `preferOrg` org, abandon it in favour of one that does, since we already have a local endorsement |
The term local org
confused me for a while until I saw the usage of this function.
As far as this function is concerned, preferOrg is supplied and in some case (i.e., when transient data is missing), this could actually be non-local as well.
// Otherwise, just let discovery pick one. | ||
endorsers, err = gs.registry.endorsers(channel, defaultInterest, "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just thinking a loud here - Is it worth a blind try or simply let client choose the gateway of the org that it thinks should succeed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we need to consider a future proposal for allowing the client to choose the 'initial' endorser
if len(interest.GetChaincodes()) == 0 { | ||
interest = defaultInterest | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A clarification question - Will this condition be ever true unless it's a read-only transaction? If no, then, is there any purpose of proceeding from here?
Also, that brings another thought, what if it's read-only transaction for pvtdata? That would record some entries in the interest which would give a wrong signal for proceeding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be true if:
- The Interest structure in the ProposalResponse is nil, which will be the case for legacy peers
- The chaincode invocation doesn't create a RWset. This happens in one of the integration tests where the chaincode only sets an event
internal/pkg/gateway/api.go
Outdated
if hasTransientData && len(endorsers) > 0 { | ||
return nil, status.Error(codes.FailedPrecondition, "non-local organizations are required to endorse; retry specifying endorsing organization(s) to protect transient data") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this caution is need here at this stage. Won't this make this whole approach pretty much unusable with private data as in most of the scenario private data transactions will involve transient data and would require multi-org endorsements.
May be better to set the artificial pvt-reads set for all collections to narrow the scope?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I've implemented your suggestion
Change the Endorse() method logic as described in the design document attached to the Jira. - Process proposal on local org peer. - Using the ChaincodeInterest in the propsal response, invoke discovery to obtain a layout that contains the local org - Obtain the remaining endorsements - Build the transaction to send back to the client. Extra unit tests added to test the new logic. Signed-off-by: andrew-coleman <andrew_coleman@uk.ibm.com>
Change the Endorse() method logic as described in the design document attached to the Jira.
Extra unit tests added to test the new logic.
Signed-off-by: andrew-coleman andrew_coleman@uk.ibm.com