Use the latest computed power table instead of always ranging certs (#882)

Certificate store already keeps track of the latest power table, i.e.,
the power table of the next expected certificate. Use this pre-computed
table whenever the next power table is asked for instead of re-computing
it every time.

This results in a much faster response to `GetPowerTable` for the most
common case: getting the committee as part of monotonic progress of
GPBFT instances.

Fixes #881

Author: masih

certstore/certstore.go

@@ -287,13 +287,31 @@ func (cs *Store) GetPowerTable(ctx context.Context, instance uint64) (gpbft.Powe
 		return nil, fmt.Errorf("cannot return a power table before the first instance: %d", cs.firstInstance)
 	}
 
-	lastPowerTable := cs.firstInstance
-	if latestCert := cs.Latest(); latestCert != nil {
-		lastPowerTable = latestCert.GPBFTInstance + 1
-	}
-	if instance > lastPowerTable {
-		return nil, fmt.Errorf("cannot return future power table for instance %d > %d", instance, lastPowerTable)
-	}
+	// Copy a reference to both the latest cert and latest power table while holding
+	// the lock to guarantee order in case the latest changes while the requested
+	// power table is being retrieved.
+	cs.mu.RLock()
+	latestCert := cs.latestCertificate
+	latestPowerTable := cs.latestPowerTable
+	cs.mu.RUnlock()
+
+	nextCertInstance := cs.firstInstance
+	if latestCert != nil {
+		nextCertInstance = latestCert.GPBFTInstance + 1
+	}
+	if instance > nextCertInstance {
+		return nil, fmt.Errorf("cannot return future power table for instance %d > %d", instance, nextCertInstance)
+	}
+	if instance == nextCertInstance && len(latestPowerTable) != 0 {
+		// Note that the latestPowerTable may be nil/empty, which indicates the certstore
+		// is being opened. Hence, the strict check for non-zero length.
+		return latestPowerTable, nil
+	}
+	// Technically, it's possible to also optimize for the case where the instance is
+	// equal to the latest certificate instance. This is by "subtracting" the latest
+	// cert power table delta from the latest power table. But it's unclear if that
+	// optimization is necessary. Observing the runtime metrics should make it clear
+	// if it is. For now, YAGNI.
 
 	// We store every `powerTableFrequency` power tables. Find the nearest multiple smaller than
 	// the requested instance.

certstore/certstore_test.go

@@ -409,20 +409,31 @@ func TestPowerNoData(t *testing.T) {
 	t.Parallel()
 	ctx := context.Background()
 	ds := ds_sync.MutexWrap(datastore.NewMapDatastore())
-	pt, _ := testPowerTable(20)
+	pt, ptCid := testPowerTable(20)
 
 	cs, err := CreateStore(ctx, ds, 0, pt)
 	require.NoError(t, err)
 	cs.powerTableFrequency = 5
-	// clobber the datastore.
+
+	// Add some certs to force GetPowerTable call to have to range certs.
+	require.NoError(t, cs.Put(ctx, makeCert(0, gpbft.SupplementalData{PowerTable: ptCid})))
+	require.NoError(t, cs.Put(ctx, makeCert(1, gpbft.SupplementalData{PowerTable: ptCid})))
+
+	// Clobber the datastore.
 	cs.ds = ds_sync.MutexWrap(datastore.NewMapDatastore())
 
-	// Should fail to find any power tables.
+	// Should fail to find any power table apart from next. Because, internally it is
+	// cached.
 	_, err = cs.GetPowerTable(ctx, 0)
 	require.ErrorContains(t, err, "failed to load power table")
 	_, err = cs.GetPowerTable(ctx, 1)
-	require.ErrorContains(t, err, "cannot return future power table")
+	require.ErrorContains(t, err, "failed to load power table")
+	_, err = cs.GetPowerTable(ctx, 2)
+	require.NoError(t, err)
 
+	// Should fail to find power tables that are beyond the next expected instance.
+	_, err = cs.GetPowerTable(ctx, 3)
+	require.ErrorContains(t, err, "cannot return future power table")
 }
 
 func TestPowerEmptyPowerTable(t *testing.T) {