catchup: Fix empty cert if ledger already has a block

catchup/catchpointService.go

@@ -373,8 +373,9 @@
 	var blk *bookkeeping.Block
 	var cert *agreement.Certificate
 	// check to see if the current ledger might have this block. If so, we should try this first instead of downloading anything.
-	if ledgerBlock, err := cs.ledger.Block(blockRound); err == nil {
+	if ledgerBlock, ledgerCert, err0 := cs.ledger.BlockCert(blockRound); err0 == nil {
 		blk = &ledgerBlock
+		cert = &ledgerCert
 	}
 	var protoParams config.ConsensusParams
 	var ok bool
@@ -551,15 +552,17 @@
 		}
 
 		blk = nil
+		cert = nil
 		// check to see if the current ledger might have this block. If so, we should try this first instead of downloading anything.
-		if ledgerBlock, err := cs.ledger.Block(topBlock.Round() - basics.Round(blocksFetched)); err == nil {
+		if ledgerBlock, ledgerCert, err0 := cs.ledger.BlockCert(topBlock.Round() - basics.Round(blocksFetched)); err0 == nil {
 			blk = &ledgerBlock
+			cert = &ledgerCert
 		} else {
-			switch err.(type) {
+			switch err0.(type) {
 			case ledgercore.ErrNoEntry:
 				// this is expected, ignore this one.
 			default:
-				cs.log.Warnf("processStageBlocksDownload encountered the following error when attempting to retrieve the block for round %d : %v", topBlock.Round()-basics.Round(blocksFetched), err)
+				cs.log.Warnf("processStageBlocksDownload encountered the following error when attempting to retrieve the block for round %d : %v", topBlock.Round()-basics.Round(blocksFetched), err0)
 			}
 		}
 

catchup/catchpointService_test.go

@@ -22,34 +22,37 @@ import (
 
 	"github.com/stretchr/testify/require"
 
+	"github.com/algorand/go-algorand/agreement"
 	"github.com/algorand/go-algorand/components/mocks"
 	"github.com/algorand/go-algorand/crypto"
 	"github.com/algorand/go-algorand/data/basics"
 	"github.com/algorand/go-algorand/data/bookkeeping"
 	"github.com/algorand/go-algorand/ledger"
 	"github.com/algorand/go-algorand/ledger/ledgercore"
+	"github.com/algorand/go-algorand/logging"
 	"github.com/algorand/go-algorand/protocol"
 	"github.com/algorand/go-algorand/test/partitiontest"
 )
 
 type catchpointCatchupLedger struct {
 }
 
-func (l *catchpointCatchupLedger) Block(rnd basics.Round) (blk bookkeeping.Block, err error) {
+func (l *catchpointCatchupLedger) BlockCert(rnd basics.Round) (blk bookkeeping.Block, cert agreement.Certificate, err error) {
 	blk = bookkeeping.Block{
 		BlockHeader: bookkeeping.BlockHeader{
 			UpgradeState: bookkeeping.UpgradeState{
 				CurrentProtocol: protocol.ConsensusCurrentVersion,
 			},
 		},
 	}
+	cert = agreement.Certificate{}
 	commitments, err := blk.PaysetCommit()
 	if err != nil {
-		return blk, err
+		return blk, cert, err
 	}
 	blk.TxnCommitments = commitments
 
-	return blk, nil
+	return blk, cert, nil
 }
 
 func (l *catchpointCatchupLedger) GenesisHash() (d crypto.Digest) {
@@ -95,3 +98,64 @@ func TestCatchpointServicePeerRank(t *testing.T) {
 	err := cs.processStageLatestBlockDownload()
 	require.NoError(t, err)
 }
+
+type catchpointAccessorMock struct {
+	mocks.MockCatchpointCatchupAccessor
+	t      *testing.T
+	topBlk bookkeeping.Block
+}
+
+func (m *catchpointAccessorMock) EnsureFirstBlock(ctx context.Context) (blk bookkeeping.Block, err error) {
+	return m.topBlk, nil
+}
+
+func (m *catchpointAccessorMock) StoreBlock(ctx context.Context, blk *bookkeeping.Block, cert *agreement.Certificate) (err error) {
+	require.NotNil(m.t, blk)
+	require.NotNil(m.t, cert)
+	return nil
+}
+
+type catchpointCatchupLedger2 struct {
+	catchpointCatchupLedger
+	blk bookkeeping.Block
+}
+
+func (l *catchpointCatchupLedger2) BlockCert(rnd basics.Round) (blk bookkeeping.Block, cert agreement.Certificate, err error) {
+	return l.blk, agreement.Certificate{}, nil
+}
+
+// TestProcessStageBlocksDownloadNilCert ensures StoreBlock does not receive a nil certificate when ledger has already had a block.
+// It uses two mocks catchpointAccessorMock and catchpointCatchupLedger2 and pre-crafted blocks to make a single iteration of processStageBlocksDownload.
+func TestProcessStageBlocksDownloadNilCert(t *testing.T) {
+	partitiontest.PartitionTest(t)
+
+	var err error
+	blk1 := bookkeeping.Block{
+		BlockHeader: bookkeeping.BlockHeader{
+			Round: 1,
+			UpgradeState: bookkeeping.UpgradeState{
+				CurrentProtocol: protocol.ConsensusCurrentVersion,
+			},
+		},
+	}
+	blk1.TxnCommitments, err = blk1.PaysetCommit()
+	require.NoError(t, err)
+
+	blk2 := blk1
+	blk2.BlockHeader.Round = 2
+	blk2.BlockHeader.Branch = blk1.Hash()
+	blk2.TxnCommitments, err = blk2.PaysetCommit()
+	require.NoError(t, err)
+
+	ctx, cf := context.WithCancel(context.Background())
+	cs := CatchpointCatchupService{
+		ctx:            ctx,
+		cancelCtxFunc:  cf,
+		ledgerAccessor: &catchpointAccessorMock{topBlk: blk2, t: t},
+		ledger:         &catchpointCatchupLedger2{blk: blk1},
+		log:            logging.TestingLog(t),
+	}
+
+	err = cs.processStageBlocksDownload()
+	require.NoError(t, err)
+}

ledger/catchupaccessor.go

@@ -238,7 +238,7 @@ const (
 
 // CatchupAccessorClientLedger represents ledger interface needed for catchpoint accessor clients
 type CatchupAccessorClientLedger interface {
-	Block(rnd basics.Round) (blk bookkeeping.Block, err error)
+	BlockCert(rnd basics.Round) (blk bookkeeping.Block, cert agreement.Certificate, err error)
 	GenesisHash() crypto.Digest
 	BlockHdr(rnd basics.Round) (blk bookkeeping.BlockHeader, err error)
 	Latest() (rnd basics.Round)