From a64e6378782f7757772d257cfbc8053fe6ed8485 Mon Sep 17 00:00:00 2001 From: "Masih H. Derkani" Date: Thu, 21 Nov 2024 15:20:13 +0000 Subject: [PATCH] Consistently use cert pointer in verification The finality certificate APIs at times switch between pointer and values. This introduces the need for extra gymnastics when validating certs, where a slice of cert pointers need to be converted to values to use the cert verification APIs. Instead, consistently accept pointers to certs. --- certexchange/polling/poller.go | 2 +- certs/certs.go | 4 ++-- certs/certs_test.go | 24 ++++++++++++------------ host.go | 2 +- 4 files changed, 16 insertions(+), 16 deletions(-) diff --git a/certexchange/polling/poller.go b/certexchange/polling/poller.go index f0551863..503ce89c 100644 --- a/certexchange/polling/poller.go +++ b/certexchange/polling/poller.go @@ -135,7 +135,7 @@ func (p *Poller) Poll(ctx context.Context, peer peer.ID) (*PollResult, error) { // TODO: consider batching verification, it's slightly faster. next, _, pt, err := certs.ValidateFinalityCertificates( p.SignatureVerifier, p.NetworkName, p.PowerTable, p.NextInstance, nil, - *cert, + cert, ) if err != nil { res.Status = PollIllegal diff --git a/certs/certs.go b/certs/certs.go index 40c3a7c2..4d91fa32 100644 --- a/certs/certs.go +++ b/certs/certs.go @@ -89,7 +89,7 @@ func NewFinalityCertificate(powerDelta PowerTableDiff, justification *gpbft.Just // finalized, the instance of the first invalid finality certificate, and the power table that // should be used to validate that finality certificate, along with the error encountered. func ValidateFinalityCertificates(verifier gpbft.Verifier, network gpbft.NetworkName, prevPowerTable gpbft.PowerEntries, nextInstance uint64, base *gpbft.TipSet, - certs ...FinalityCertificate) (_nextInstance uint64, chain gpbft.ECChain, newPowerTable gpbft.PowerEntries, err error) { + certs ...*FinalityCertificate) (_nextInstance uint64, chain gpbft.ECChain, newPowerTable gpbft.PowerEntries, err error) { for _, cert := range certs { if cert.GPBFTInstance != nextInstance { return nextInstance, chain, prevPowerTable, fmt.Errorf("expected instance %d, found instance %d", nextInstance, cert.GPBFTInstance) @@ -144,7 +144,7 @@ func ValidateFinalityCertificates(verifier gpbft.Verifier, network gpbft.Network // Verify the signature of the given finality certificate. This doesn't validate the power delta, or // any other parts of the certificate, just that the _value_ has been signed by a majority of the // power. -func verifyFinalityCertificateSignature(verifier gpbft.Verifier, powerTable gpbft.PowerEntries, nn gpbft.NetworkName, cert FinalityCertificate) error { +func verifyFinalityCertificateSignature(verifier gpbft.Verifier, powerTable gpbft.PowerEntries, nn gpbft.NetworkName, cert *FinalityCertificate) error { scaled, totalScaled, err := powerTable.Scaled() if err != nil { return fmt.Errorf("failed to scale power table: %w", err) diff --git a/certs/certs_test.go b/certs/certs_test.go index e9cbabb4..61e2d19b 100644 --- a/certs/certs_test.go +++ b/certs/certs_test.go @@ -214,7 +214,7 @@ func TestFinalityCertificates(t *testing.T) { tsg := sim.NewTipSetGenerator(rng.Uint64()) base := gpbft.TipSet{Epoch: 0, Key: tsg.Sample(), PowerTable: tableCid} - certificates := make([]certs.FinalityCertificate, 10) + certificates := make([]*certs.FinalityCertificate, 10) powerTables := make([]gpbft.PowerEntries, 10) var removedPowerEntries []gpbft.PowerEntry for i := range certificates { @@ -223,7 +223,7 @@ func TestFinalityCertificates(t *testing.T) { justification := makeJustification(t, rng, tsg, backend, base, uint64(i), powerTables[i], powerTable) cert, err := certs.NewFinalityCertificate(certs.MakePowerTableDiff(powerTables[i], powerTable), justification) require.NoError(t, err) - certificates[i] = *cert + certificates[i] = cert base = *justification.Vote.Value.Head() } @@ -291,7 +291,7 @@ func TestBadFinalityCertificates(t *testing.T) { // Unexpected instance number { - nextInstance, chain, newPowerTable, err := certs.ValidateFinalityCertificates(backend, networkName, powerTable, 0, nil, *certificate) + nextInstance, chain, newPowerTable, err := certs.ValidateFinalityCertificates(backend, networkName, powerTable, 0, nil, certificate) require.ErrorContains(t, err, "expected instance 0, found instance 1") require.EqualValues(t, 0, nextInstance) require.Equal(t, powerTable, newPowerTable) @@ -299,7 +299,7 @@ func TestBadFinalityCertificates(t *testing.T) { } // Wrong base. { - nextInstance, chain, newPowerTable, err := certs.ValidateFinalityCertificates(backend, networkName, powerTable, 1, certificate.ECChain.Head(), *certificate) + nextInstance, chain, newPowerTable, err := certs.ValidateFinalityCertificates(backend, networkName, powerTable, 1, certificate.ECChain.Head(), certificate) require.ErrorContains(t, err, "base tipset does not match finalized chain") require.EqualValues(t, 1, nextInstance) require.Equal(t, powerTable, newPowerTable) @@ -309,7 +309,7 @@ func TestBadFinalityCertificates(t *testing.T) { // Discard most of the power table. Given the initial power distribution, we can guarantee // that we require more than 10 participants. { - nextInstance, chain, newPowerTable, err := certs.ValidateFinalityCertificates(backend, networkName, powerTable[:10], 1, nil, *certificate) + nextInstance, chain, newPowerTable, err := certs.ValidateFinalityCertificates(backend, networkName, powerTable[:10], 1, nil, certificate) require.ErrorContains(t, err, "but we only have 10 entries in the power table") require.EqualValues(t, 1, nextInstance) require.Equal(t, powerTable[:10], newPowerTable) @@ -322,7 +322,7 @@ func TestBadFinalityCertificates(t *testing.T) { require.NoError(t, err) powerTableCpy := slices.Clone(powerTable) powerTableCpy[firstSigner].PubKey = powerTableCpy[(int(firstSigner)+1)%len(powerTableCpy)].PubKey - nextInstance, chain, _, err := certs.ValidateFinalityCertificates(backend, networkName, powerTableCpy, 1, nil, *certificate) + nextInstance, chain, _, err := certs.ValidateFinalityCertificates(backend, networkName, powerTableCpy, 1, nil, certificate) require.ErrorContains(t, err, "invalid signature on finality certificate") require.EqualValues(t, 1, nextInstance) require.Empty(t, chain) @@ -335,7 +335,7 @@ func TestBadFinalityCertificates(t *testing.T) { powerTableCpy := slices.Clone(powerTable) // increase so we definitely have enough power powerTableCpy[firstSigner].Power = big.Add(powerTableCpy[firstSigner].Power, gpbft.NewStoragePower(1)) - nextInstance, chain, _, err := certs.ValidateFinalityCertificates(backend, networkName, powerTableCpy, 1, nil, *certificate) + nextInstance, chain, _, err := certs.ValidateFinalityCertificates(backend, networkName, powerTableCpy, 1, nil, certificate) require.ErrorContains(t, err, "incorrect power diff") require.EqualValues(t, 1, nextInstance) require.Empty(t, chain) @@ -352,7 +352,7 @@ func TestBadFinalityCertificates(t *testing.T) { require.NoError(t, err) powerTableCpy[firstSigner].Power = gpbft.NewStoragePower(0xffff) - nextInstance, chain, _, err := certs.ValidateFinalityCertificates(backend, networkName, powerTableCpy, 1, nil, *certificate) + nextInstance, chain, _, err := certs.ValidateFinalityCertificates(backend, networkName, powerTableCpy, 1, nil, certificate) require.ErrorContains(t, err, "no effective power after scaling") require.EqualValues(t, 1, nextInstance) require.Empty(t, chain) @@ -373,7 +373,7 @@ func TestBadFinalityCertificates(t *testing.T) { return nil })) - nextInstance, chain, _, err := certs.ValidateFinalityCertificates(backend, networkName, powerTableCpy, 1, nil, *certificate) + nextInstance, chain, _, err := certs.ValidateFinalityCertificates(backend, networkName, powerTableCpy, 1, nil, certificate) require.ErrorContains(t, err, fmt.Sprintf("has insufficient power: %d < 2/3 %d", activePower, totalPower)) require.EqualValues(t, 1, nextInstance) require.Empty(t, chain) @@ -383,7 +383,7 @@ func TestBadFinalityCertificates(t *testing.T) { { certCpy := *certificate certCpy.ECChain = nil - nextInstance, chain, newPowerTable, err := certs.ValidateFinalityCertificates(backend, networkName, powerTable, 1, nil, certCpy) + nextInstance, chain, newPowerTable, err := certs.ValidateFinalityCertificates(backend, networkName, powerTable, 1, nil, &certCpy) require.ErrorContains(t, err, "empty finality certificate") require.EqualValues(t, 1, nextInstance) require.Equal(t, powerTable, newPowerTable) @@ -395,7 +395,7 @@ func TestBadFinalityCertificates(t *testing.T) { certCpy := *certificate certCpy.ECChain = slices.Clone(certCpy.ECChain) slices.Reverse(certCpy.ECChain) - nextInstance, chain, newPowerTable, err := certs.ValidateFinalityCertificates(backend, networkName, powerTable, 1, nil, certCpy) + nextInstance, chain, newPowerTable, err := certs.ValidateFinalityCertificates(backend, networkName, powerTable, 1, nil, &certCpy) require.ErrorContains(t, err, "chain must have increasing epochs") require.EqualValues(t, 1, nextInstance) require.Equal(t, powerTable, newPowerTable) @@ -411,7 +411,7 @@ func TestBadFinalityCertificates(t *testing.T) { certCpy.PowerTableDelta[0].PowerDelta = gpbft.NewStoragePower(0) certCpy.PowerTableDelta[0].SigningKey = nil - nextInstance, chain, newPowerTable, err := certs.ValidateFinalityCertificates(backend, networkName, powerTable, 1, nil, certCpy) + nextInstance, chain, newPowerTable, err := certs.ValidateFinalityCertificates(backend, networkName, powerTable, 1, nil, &certCpy) require.ErrorContains(t, err, "failed to apply power table delta") require.EqualValues(t, 1, nextInstance) require.Equal(t, powerTable, newPowerTable) diff --git a/host.go b/host.go index 6226fdf9..91d19c27 100644 --- a/host.go +++ b/host.go @@ -909,7 +909,7 @@ func (h *gpbftHost) saveDecision(decision *gpbft.Justification) (*certs.Finality if err != nil { return nil, fmt.Errorf("forming certificate out of decision: %w", err) } - _, _, _, err = certs.ValidateFinalityCertificates(h, h.NetworkName(), current.PowerTable.Entries, decision.Vote.Instance, nil, *cert) + _, _, _, err = certs.ValidateFinalityCertificates(h, h.NetworkName(), current.PowerTable.Entries, decision.Vote.Instance, nil, cert) if err != nil { return nil, fmt.Errorf("certificate is invalid: %w", err) }