Fix bug in snapshot request submission

This commit fixes a bug in the snapshot request path.
When the snapshot request is sent with the special value "blockNumber 0",
if a block commit is in progress, the derived value should be one higher
than the last committed block (i.e., equal to the currently processing block).
Instead in the code, it was a typo that made it always set to 1.

Also, add a wait in the discovery integration test for channel config
updates to be committed

Signed-off-by: manish <[email protected]>

core/ledger/kvledger/snapshot_mgmt.go

@@ -7,6 +7,7 @@ SPDX-License-Identifier: Apache-2.0
 package kvledger
 
 import (
+	"fmt"
 	"math"
 	"os"
 	"sync"
@@ -49,6 +50,10 @@ type event struct {
 	blockNumber uint64
 }
 
+func (e *event) String() string {
+	return fmt.Sprintf("{type=%s, blockNumber=%d}", e.typ, e.blockNumber)
+}
+
 type requestResponse struct {
 	err error
 }
@@ -106,7 +111,11 @@ func (l *kvLedger) processSnapshotMgmtEvents(lastCommittedBlockNumber uint64) {
 
 	for {
 		e := <-events
-		logger.Debugw("Event received", "channelID", l.ledgerID, "type", e.typ, "blockNumber", e.blockNumber, "snapshotInProgress=", snapshotInProgress)
+		logger.Debugw("Event received",
+			"channelID", l.ledgerID, "event", e, "snapshotInProgress", snapshotInProgress,
+			"lastCommittedBlockNumber", lastCommittedBlockNumber, "committerStatus", committerStatus,
+		)
+
 		switch e.typ {
 		case commitStart:
 			committerStatus = blocked
@@ -150,7 +159,7 @@ func (l *kvLedger) processSnapshotMgmtEvents(lastCommittedBlockNumber uint64) {
 		case requestAdd:
 			leastAcceptableBlockNum := lastCommittedBlockNumber
 			if committerStatus != idle {
-				leastAcceptableBlockNum = +1
+				leastAcceptableBlockNum++
 			}
 
 			requestedBlockNum := e.blockNumber

core/ledger/kvledger/snapshot_mgmt_test.go

@@ -230,6 +230,38 @@ func TestSnapshotRequests(t *testing.T) {
 	require.Eventually(t, requestsUpdated, time.Minute, 100*time.Millisecond)
 }
 
+func TestSnapshotMgmtConcurrency(t *testing.T) {
+	conf, cleanup := testConfig(t)
+	defer cleanup()
+	provider := testutilNewProvider(conf, t, &mock.DeployedChaincodeInfoProvider{})
+	defer provider.Close()
+
+	ledgerID := "testsnapshotmgmtconcurrency"
+	bg, gb := testutil.NewBlockGenerator(t, ledgerID, false)
+	gbHash := protoutil.BlockHeaderHash(gb.Header)
+	l, err := provider.CreateFromGenesisBlock(gb)
+	require.NoError(t, err)
+	kvledger := l.(*kvLedger)
+	defer kvledger.Close()
+
+	testutilCommitBlocks(t, l, bg, 5, gbHash)
+
+	// Artificially, send event to background goroutine to indicate that commit for block 6 has started
+	// and then submit snapshot request for block 0, while not sending the event for commit done for block 6
+	kvledger.snapshotMgr.events <- &event{typ: commitStart, blockNumber: 6}
+	<-kvledger.snapshotMgr.commitProceed
+
+	require.NoError(t, kvledger.SubmitSnapshotRequest(0))
+	require.Eventually(t,
+		func() bool {
+			r, err := kvledger.snapshotMgr.snapshotRequestBookkeeper.smallestRequest()
+			require.NoError(t, err)
+			return r == 6
+		},
+		10*time.Millisecond, 1*time.Millisecond,
+	)
+}
+
 func TestSnapshotMgrShutdown(t *testing.T) {
 	conf, cleanup := testConfig(t)
 	defer cleanup()

integration/discovery/discovery_test.go

@@ -153,12 +153,13 @@ var _ = Describe("DiscoveryService", func() {
 
 	It("discovers network configuration, endorsers, and peer membership", func() {
 		By("Updating anchor peers")
+		initialHeight := nwo.GetLedgerHeight(network, org1Peer0, "testchannel")
 		network.UpdateChannelAnchors(orderer, "testchannel")
-
+		// wait for anchor peer config updates to be committed
+		nwo.WaitUntilEqualLedgerHeight(network, "testchannel", initialHeight+2, org1Peer0)
 		//
 		// bootstrapping a peer from snapshot
 		//
-
 		By("generating a snapshot at current block number on org1Peer0")
 		blockNum := nwo.GetLedgerHeight(network, org1Peer0, "testchannel") - 1
 		submitSnapshotRequest(network, "testchannel", 0, org1Peer0, "Snapshot request submitted successfully")

release_notes/v2.4.0.md

@@ -13,6 +13,13 @@ These changes keep the fixed-width columns together at the left
 and add a visual break between the logging module name and log message text.
 
 
+Fixes
+------------
+**FAB-18424: peer - Ledger snapshot request submission with special value "blockNumber 0"**
+
+If a ledger snapshot request is submitted with the special value "blockNumber 0", peer is expected to translate the request to last committed block. This patch fixes the issue where, it may happen sometimes that the request is transtlated to block number 1 instead of last committed block. This leads to the situation where no snapshot gets generated, including any future snapshot requests. If you have ever used this special value, we encourage you to check the list of pending snapshots requests. If you notice one or more pending requests that are for the the block numbers lower than the latest committed block, cancel such requests to enable the further snapshot requests to be processed.
+
+
 Dependencies
 ------------
 Fabric v2.4.0 has been tested with the following dependencies: