fix(network): fill same-MSP endorser slots with distinct identities - #2139
fix(network): fill same-MSP endorser slots with distinct identities#2139HayimShaul wants to merge 1 commit into
Conversation
📊 Token Validation BenchmarkComparison of this PR against the base branch. 🟢 improvement · 🔴 regression · ➖ within ±1.0% noise.
|
81ec699 to
f5e34e1
Compare
📊 Token Validation BenchmarkComparison of this PR against the base branch. 🟢 improvement · 🔴 regression · ➖ within ±1.0% noise.
|
f5e34e1 to
47812f0
Compare
AkramBitar
left a comment
AkramBitar
left a comment
There was a problem hiding this comment.
Three notes on the fix itself.
| if _, ok := seen[string(id)]; ok { | ||
| // configured may legitimately list the same endorser twice; it still provides a | ||
| // single endorsement, so bucket it once. | ||
| continue | ||
| } | ||
| seen[string(id)] = struct{}{} |
There was a problem hiding this comment.
Dedup is byte-exact, so two byte-distinct identities carrying the same signing key count as two endorsers and can fill both same-MSP slots — one physical signer, the collapse this PR closes.
The sibling threshold_rule path treats exactly that as an anomaly (fabricx/endorsement/nspolicy.go:209-212: two distinct identities with the same key "would mean two different endorsers share a private key"), so the two paths now disagree on the same question. Narrow in practice, but the godoc promises "never the same endorser twice", which byte equality does not quite deliver.
Either reuse ecdsaPublicKeyOf to close it, or add one godoc line saying distinctness is by identity bytes.
| } | ||
| } | ||
|
|
||
| return nil, errors.Errorf("no configured endorser covers any of the [%d] policy-satisfying MSP set(s) with a distinct endorser per required signer", len(candidates)) |
There was a problem hiding this comment.
Both failure modes produce this one string, and neither is diagnosable: with AND(Org1, Org1) and one configured Org1 endorser, the operator learns only that [1] set failed — not which MSP was short, nor by how much. This PR’s docs make "fewer distinct endorsers than required signers" its own error class, so the error should be able to name it.
selectDistinctForMSPSet returns only bool. Returning the failing mspID plus required-vs-available would give: "MSP [Org1MSP] requires 2 distinct signers but only 1 configured endorser belongs to it".
| // A greedy per-slot pick is complete here: every slot requiring a given MSP ID draws from | ||
| // the same pool, so the only way this fails is that some MSP ID appears in requiredMSPIDs | ||
| // more times than that MSP has distinct configured endorsers - genuinely unsatisfiable | ||
| // whatever the order of the picks. No backtracking is needed. |
There was a problem hiding this comment.
Confirming this argument rather than leaving the next reviewer to re-derive it: byMSP partitions the identities, so slots for MSP m draw only from pool[m] and the pools are disjoint. Feasibility is therefore just count(m) <= |pool[m]| per MSP, which greedy always attains — no ordering does better, so skipping backtracking is correct, not merely convenient.
Same for the test: it asserts both endorsers reach both slots, not just distinctness. The obvious "take the first unused" fix would pass the latter and fail the former.
47812f0 to
42c2192
Compare
SelectEndorsersForMSPSets sampled one configured endorser per required MSP ID of a candidate set without excluding identities already picked for an earlier slot of that same set. A namespace endorsement policy requiring two signers from one MSP - e.g. AND(Org1MSP.member, Org1MSP.member), whose principal set inquire.SatisfiedBy() reports as ["Org1MSP", "Org1MSP"] - could therefore be satisfied by asking a single endorser to sign twice, collapsing the intended 2-of-N-within-org guarantee onto a single point of trust. Per-slot sampling now excludes identities already selected for the same candidate set, so every returned identity is distinct and the result length always matches the chosen candidate set. A set requiring more distinct signers from an MSP than that MSP has configured endorsers is no longer satisfiable and is skipped, leaving the caller to try the remaining candidate sets before failing - correctness over availability, as elsewhere in this selector. Duplicate entries in the configured endorser list now collapse to a single candidate: such a duplicate denotes one endorser providing one endorsement, so counting it twice both skewed the uniform random pick and overstated how many distinct endorsers an MSP actually offers. Signed-off-by: Hayim.Shaul@ibm.com <hayimsha@fhe03.vpc.cloud9.ibm.com>
42c2192 to
f1007d6
Compare
2 participants
Fixes #2048
SelectEndorsersForMSPSetssampled one configured endorser per required MSP ID of a candidate set without excluding identities already picked for an earlier slot of the same set. A namespace endorsement policy requiring two signers from one MSP — e.g.AND(Org1MSP.member, Org1MSP.member), whose principal setinquire.SatisfiedBy()reports as["Org1MSP", "Org1MSP"]— could therefore be satisfied by one endorser signing twice, collapsing the intended 2-of-N-within-org guarantee onto a single point of trust.Changes:
docs/services/network-fabricx.md,docs/services/network-fabric.md,docs/configuration.md) to state that same-MSP signer slots are filled with distinct endorsers, and that an MSP with fewer distinct endorsers than required signers is a hard error.make checksandmake lintpass; the affected packages pass under-race.