FABGW-25 Build ChaincodeInterest in TX simulator #2767

andrew-coleman · 2021-07-20T11:07:09Z

During transaction simulation, capture extra information to build a ChaincodeInterest structure specific to the chaincode invocation as follows:

Which private data collections are read from.
The SBE policies associated with any states that a written to.

Following simulations, the ChaincodeInterest structure is built and returned in the ProposalResponse message.

Signed-off-by: andrew-coleman andrew_coleman@uk.ibm.com

core/ledger/kvledger/txmgmt/txmgr/query_executor.go

manish-sethi · 2021-07-20T13:59:20Z

core/ledger/kvledger/txmgmt/txmgr/tx_simulator.go

@@ -21,13 +23,14 @@ type txSimulator struct {
 	pvtdataQueriesPerformed   bool
 	simulationResultsComputed bool
 	paginatedQueriesPerformed bool
+	keySignaturePolicies      map[string][]byte


Cannot this be simply a map[string]struct{}. Where key of the map entry will be the policy itself.

Otherwise, this will be a map[string]map[string][]byte to store <ns,key> as an entry. Moreover, you would need to add this details in the functions for pvt data as well - i.e., SetPrivateData and SetPrivateDataMetadata. So, holistically, this would need to store <ns,coll,key> as an entry.

manish-sethi · 2021-07-20T14:02:43Z

core/ledger/kvledger/txmgmt/txmgr/tx_simulator.go

+	simResults.PrivateReads = s.privateReads.Clone()
+	for _, policy := range s.keySignaturePolicies {
+		simResults.KeySignaturePolicies = append(simResults.KeySignaturePolicies, policy)
+	}


Is this cloning of both required?

Yes, I've put a comment in the code explaining. There is an existing unit test that catches this.

manish-sethi · 2021-07-20T14:26:20Z

core/ledger/ledger_interface.go

+}
+
+// Read returns whether a private data collection has been read during TX simulation.
+func (pr PrivateReads) Read(ns, coll string) bool {


Exists may be a more meaningful name.

manish-sethi · 2021-07-20T14:27:18Z

core/ledger/ledger_interface.go

@@ -470,10 +470,42 @@ func (filter PvtNsCollFilter) Has(ns string, coll string) bool {
 	return collFilter[coll]
 }

+// PrivateReads captures which private data collections are read during TX simulation.
+type PrivateReads map[string]map[string]bool


Not a big deal but better to use canonical type map[string]map[string]struct{} for a set.

manish-sethi · 2021-07-20T14:32:32Z

core/endorser/endorser.go

+				ccInterest.Chaincodes = append(ccInterest.Chaincodes, &pb.ChaincodeCall{
+					Name:            prws.GetNamespace(),
+					CollectionNames: []string{cprws.GetCollectionName()},
+					NoPrivateReads:  !simResult.PrivateReads.Read(prws.Namespace, cprws.CollectionName),


Just a clarification question - so multiple entries for a single chaincode are expected - one for each collection? I figured that though there a slice for specifying multiple collection names but only single flag for NoPrivateReads - so is this a workaround to overcome that?

Yes exactly. I've added a comment in the code to this effect

manish-sethi · 2021-07-20T14:45:07Z

core/endorser/endorser.go

+			if nsrws.GetNamespace() == "_lifecycle" {
+				continue


At other places we avoided this hardcoding via this function. see if you could use this here easily. Otherwise, can introduce a function in that package that returns these names - which would cover the name for legacy lifecycle name as well (i.e., lscc).

manish-sethi · 2021-07-20T14:48:09Z

core/endorser/endorser.go

+		for _, policyBytes := range simResult.KeySignaturePolicies {
+			policy, err := protoutil.UnmarshalSignaturePolicy(policyBytes)
+			if err != nil {
+				return nil, err
+			}
+			ccInterest.Chaincodes[0].KeyPolicies = append(ccInterest.Chaincodes[0].KeyPolicies, policy)
+		}


Perhaps it won't make any difference functionally but just to confirm that you deliberately mention all the SBE in the first entry for the chaincode?

Yes, in reality it makes no difference where they are

manish-sethi

A few more comments

manish-sethi · 2021-07-20T17:23:55Z

core/ledger/kvledger/txmgmt/txmgr/tx_simulator.go

+	if versionedValue, err = s.txmgr.db.GetPrivateDataHash(ns, coll, key); err != nil {
+		return err
+	}
+	_, metadata, _ = decomposeVersionedValue(versionedValue)


Suggested change

if versionedValue, err = s.txmgr.db.GetPrivateDataHash(ns, coll, key); err != nil {

return err

}

_, metadata, _ = decomposeVersionedValue(versionedValue)

s.txmgr.db.GetPrivateDataMetadataByHash(ns, coll, util.ComputeStringHash(key))

like GetStateMetadata in the case of public key, GetPrivateDataMetadataByHash applies an optimization that internally skips looking up for the keys for a chaincode that never used SBE

manish-sethi · 2021-07-20T20:06:53Z

core/ledger/kvledger/txmgmt/txmgr/tx_simulator.go

+	return s.policyFromMetadata(metadata)
+}
+
+func (s *txSimulator) policyFromMetadata(metadata []byte) error {


nit: The current function name gives a feel as if this function just does the transformation and returns the policy. As this function actually makes a side-effect on state, can we call it addToKeySignaturePolicies (or something of this effect)

manish-sethi · 2021-07-20T20:12:41Z

core/ledger/kvledger/txmgmt/txmgr/tx_simulator.go

+}
+
+// If this private collection key has a SBE policy, add that policy to the set
+func (s *txSimulator) checkPrivateKeySignaturePolicy(ns string, coll string, key string) error {


Missed invoking this function from functions SetPrivateData and SetPrivateDataMetadata? Likely you missed covering this in test as well.

manish-sethi · 2021-07-20T21:03:23Z

core/endorser/endorser.go

+		for _, prws := range simResult.PvtSimulationResults.GetNsPvtRwset() {
+			for _, cprws := range prws.GetCollectionPvtRwset() {
+				// Since each collection in a chaincode could have different values of the NoPrivateReads flag, create a new Chaincode entry for each.
+				ccInterest.Chaincodes = append(ccInterest.Chaincodes, &pb.ChaincodeCall{
+					Name:            prws.GetNamespace(),
+					CollectionNames: []string{cprws.GetCollectionName()},
+					NoPrivateReads:  !simResult.PrivateReads.Exists(prws.Namespace, cprws.CollectionName),
+				})
+				chaincodes[prws.GetNamespace()] = struct{}{}
+			}
+		}
+	}


I missed noticing in this block earlier

Even in the private read-write, we should skip the _lifecycle namespace (can skip IsSysCC(nsrws.GetNamespace())) -- As the client would specifically target the orgs for these transactions.

This logic assumes that if a collection is read from, it is written to as well. So, the collections that are only present in the PrivateReads and not in the pvt-write-set will get a miss.

Good catch. This logic was left over from my prototype which was building a full private read set, so this assumption would have held. I've now rewritten this entirely in a (hopefully) clearer way. And added an extra test case for this situation.

During transaction simulation, capture extra information to build a ChaincodeInterest structure specific to the chaincode invocation as follows: - Which private data collections are read from. - The SBE policies associated with any states that a written to. Following simulations, the ChaincodeInterest structure is built and returned in the ProposalResponse message. Signed-off-by: andrew-coleman <andrew_coleman@uk.ibm.com>

manish-sethi

LGTM. Just left a pretty minor comment. I am fine merging this as is and you can fix the comments in a separate PR.

manish-sethi · 2021-07-21T14:45:15Z

core/endorser/endorser.go

+
+	// 3. Add/merge collections from the PrivateReads struct
+	for ns, entry := range simResult.PrivateReads {
+		if cc := chaincodes[ns]; cc == nil {


Skip systems chaincode in this block as well

Add a test that asserts that the system chaincodes are indeed skipped

This commit addresses the closing comments in PR hyperledger#2767: - Skip systems chaincode in this block as well - Add a test that asserts that the system chaincodes are indeed skipped Signed-off-by: andrew-coleman <andrew_coleman@uk.ibm.com>

This commit addresses the closing comments in PR #2767: - Skip systems chaincode in this block as well - Add a test that asserts that the system chaincodes are indeed skipped Signed-off-by: andrew-coleman <andrew_coleman@uk.ibm.com>

andrew-coleman requested a review from a team as a code owner July 20, 2021 11:07

andrew-coleman marked this pull request as draft July 20, 2021 11:07

andrew-coleman marked this pull request as ready for review July 20, 2021 12:19

manish-sethi reviewed Jul 20, 2021

View reviewed changes

andrew-coleman force-pushed the build_interest branch 2 times, most recently from ec33bc8 to 7dceb7e Compare July 20, 2021 17:06

manish-sethi reviewed Jul 20, 2021

View reviewed changes

andrew-coleman force-pushed the build_interest branch 2 times, most recently from 2789e5c to 1f417db Compare July 21, 2021 11:07

andrew-coleman marked this pull request as draft July 21, 2021 11:22

andrew-coleman force-pushed the build_interest branch from 1f417db to b381832 Compare July 21, 2021 11:27

andrew-coleman force-pushed the build_interest branch from b381832 to 4d34579 Compare July 21, 2021 11:28

andrew-coleman marked this pull request as ready for review July 21, 2021 12:31

manish-sethi approved these changes Jul 21, 2021

View reviewed changes

manish-sethi merged commit 26ec54a into hyperledger:main Jul 21, 2021

andrew-coleman deleted the build_interest branch July 21, 2021 14:52

andrew-coleman mentioned this pull request Jul 22, 2021

FABGW-25 Test for system chaincode #2771

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FABGW-25 Build ChaincodeInterest in TX simulator #2767

FABGW-25 Build ChaincodeInterest in TX simulator #2767

andrew-coleman commented Jul 20, 2021

manish-sethi Jul 20, 2021

manish-sethi Jul 20, 2021

andrew-coleman Jul 20, 2021

manish-sethi Jul 20, 2021

manish-sethi Jul 20, 2021

manish-sethi Jul 20, 2021

andrew-coleman Jul 20, 2021

manish-sethi Jul 20, 2021

manish-sethi Jul 20, 2021

andrew-coleman Jul 20, 2021

manish-sethi left a comment

manish-sethi Jul 20, 2021

manish-sethi Jul 20, 2021

manish-sethi Jul 20, 2021

manish-sethi Jul 20, 2021

andrew-coleman Jul 21, 2021

manish-sethi left a comment •

edited

Loading

manish-sethi Jul 21, 2021

FABGW-25 Build ChaincodeInterest in TX simulator #2767

FABGW-25 Build ChaincodeInterest in TX simulator #2767

Conversation

andrew-coleman commented Jul 20, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manish-sethi 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

manish-sethi left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manish-sethi left a comment •

edited

Loading