Fix dissemination failure despite satisfying RequiredPeerCount

[drupadh-dinesh]

Type of change

• Bug fix

Description

Previously, private data dissemination created multiple dissemination plans and assigned acknowledgement requirements (MinAck) to specific peers. As a result, dissemination could fail when an acknowledgement was not received from one of the designated peers, even if acknowledgements from other selected peers satisfied the collection's RequiredPeerCount.

This change updates dissemination planning to:

• Select peers across organisations first, then select additional peers up to MaximumPeerCount.
• Aggregate selected peers into a single dissemination plan.
• Validate acknowledgements against the overall RequiredPeerCount instead of requiring acknowledgements from specific peers.
• Prevent dissemination failures when the required number of acknowledgements is received from the selected peer set, regardless of which individual peers respond.

Additional details

Implementation changes include:

• Replacing multiple per-peer dissemination plans with a single dissemination plan containing all selected peers.
• Aggregating peer eligibility using a PKI ID lookup set.
• Selecting additional peers using a partial Fisher-Yates shuffle to avoid duplicate selection and improve efficiency.
• Enforcing MaximumPeerCount during peer selection across organisations.

[drupadh-dinesh]

Hi @pfi79 ,
Could you please review when you have a chance? Thanks!

[Atishyy27]

I traced this against SendByCriteria (gossip_impl.go L626-666) and the new single-criteria approach looks correct: MinAck acks from any of the selected peers now satisfy RequiredPeerCount, instead of each pre-designated peer having to individually ack — which was exactly the failure mode in the title. The Phase-2 partial Fisher-Yates is correctly bounded by min(len(remainingPeersAcrossOrgs), maximumPeerRemainingCount), and the in-place removal in Phase 1 is safe since ByOrg() allocates fresh slices per call. Nice catch checking the crand.Read error too.

Two observations, neither blocking:

1. Collections configured with MaximumPeerCount == 0 previously produced no plan entries; they now produce one with MaxPeers: 0. It's benign (SendByCriteria no-ops on MaxPeers == 0, L627), but skipping plan construction in that case would avoid a pointless goroutine per collection in disseminate().
2. Worth calling out in the description: when fewer peers are selectable than RequiredPeerCount, MinAck > MaxPeers now fails fast at L650 ("requested to send to at least X peers…"), where the old code silently required fewer acks than configured. I think failing loudly is the right call, but it changes outcomes in degraded-membership scenarios.

[drupadh-dinesh]

I traced this against SendByCriteria (gossip_impl.go L626-666) and the new single-criteria approach looks correct: MinAck acks from any of the selected peers now satisfy RequiredPeerCount, instead of each pre-designated peer having to individually ack — which was exactly the failure mode in the title. The Phase-2 partial Fisher-Yates is correctly bounded by min(len(remainingPeersAcrossOrgs), maximumPeerRemainingCount), and the in-place removal in Phase 1 is safe since ByOrg() allocates fresh slices per call. Nice catch checking the crand.Read error too.

Two observations, neither blocking:

1. Collections configured with MaximumPeerCount == 0 previously produced no plan entries; they now produce one with MaxPeers: 0. It's benign (SendByCriteria no-ops on MaxPeers == 0, L627), but skipping plan construction in that case would avoid a pointless goroutine per collection in disseminate().
2. Worth calling out in the description: when fewer peers are selectable than RequiredPeerCount, MinAck > MaxPeers now fails fast at L650 ("requested to send to at least X peers…"), where the old code silently required fewer acks than configured. I think failing loudly is the right call, but it changes outcomes in degraded-membership scenarios.

@Atishyy27, thanks for the review.

Regarding the second point, this behavior already exists in the current implementation. When the number of eligible peers is less than RequiredPeerCount, dissemination already fails because of the check below:

// With the shift to per peer dissemination in FAB-15389, we must first check
// that there are enough eligible peers to satisfy RequiredPeerCount.
if len(eligiblePeers) < colAP.RequiredPeerCount() {
	return nil, errors.Errorf("required to disseminate to at least %d peers, but know of only %d eligible peers", colAP.RequiredPeerCount(), len(eligiblePeers))
	}