From e405753c38fb565d8e5623b5bfe19c2d0b2d48ac Mon Sep 17 00:00:00 2001 From: Rod Vagg Date: Fri, 13 Dec 2024 18:13:22 +1100 Subject: [PATCH 1/5] fix(f3): test API during inactive-F3 modes and make consistent and safe Closes: https://github.com/filecoin-project/lotus/issues/12772 --- api/api_errors.go | 4 +- chain/lf3/f3.go | 11 ++-- itests/f3_test.go | 125 +++++++++++++++++++++++++++++++++++++++++++ node/impl/full/f3.go | 6 +-- 4 files changed, 137 insertions(+), 9 deletions(-) diff --git a/api/api_errors.go b/api/api_errors.go index ffb01289d7b..83cbd32a4b7 100644 --- a/api/api_errors.go +++ b/api/api_errors.go @@ -98,7 +98,7 @@ func (ErrActorNotFound) Error() string { return "actor not found" } type errF3Disabled struct{} -func (errF3Disabled) Error() string { return "f3 is disabled" } +func (errF3Disabled) Error() string { return "F3 is disabled" } type errF3ParticipationTicketInvalid struct{} @@ -124,7 +124,7 @@ func (errF3ParticipationTicketStartBeforeExisting) Error() string { type errF3NotReady struct{} -func (errF3NotReady) Error() string { return "f3 isn't yet ready to participate" } +func (errF3NotReady) Error() string { return "F3 isn't yet ready to participate" } // ErrExecutionReverted is used to return execution reverted with a reason for a revert in the `data` field. type ErrExecutionReverted struct { diff --git a/chain/lf3/f3.go b/chain/lf3/f3.go index db517331040..f9367998fa0 100644 --- a/chain/lf3/f3.go +++ b/chain/lf3/f3.go @@ -184,20 +184,23 @@ func (fff *F3) GetLatestCert(ctx context.Context) (*certs.FinalityCertificate, e return fff.inner.GetLatestCert(ctx) } -func (fff *F3) GetManifest(ctx context.Context) *manifest.Manifest { +func (fff *F3) GetManifest(ctx context.Context) (*manifest.Manifest, error) { m := fff.inner.Manifest() + if m == nil { + return nil, xerrors.Errorf("no known network manifest") + } if m.InitialPowerTable.Defined() { - return m + return m, nil } cert0, err := fff.inner.GetCert(ctx, 0) if err != nil { - return m + return m, xerrors.Errorf("getting certificate for instance 0: %w", err) } var mCopy = *m m = &mCopy m.InitialPowerTable = cert0.ECChain.Base().PowerTable - return m + return m, nil } func (fff *F3) GetPowerTable(ctx context.Context, tsk types.TipSetKey) (gpbft.PowerEntries, error) { diff --git a/itests/f3_test.go b/itests/f3_test.go index 31b2239dabf..cef51dd2e15 100644 --- a/itests/f3_test.go +++ b/itests/f3_test.go @@ -2,6 +2,7 @@ package itests import ( "context" + "reflect" "sync" "testing" "time" @@ -14,10 +15,12 @@ import ( "golang.org/x/sync/errgroup" "github.com/filecoin-project/go-address" + "github.com/filecoin-project/go-f3/certs" "github.com/filecoin-project/go-f3/gpbft" "github.com/filecoin-project/go-f3/manifest" "github.com/filecoin-project/go-state-types/abi" + "github.com/filecoin-project/lotus/api" lotus_api "github.com/filecoin-project/lotus/api" "github.com/filecoin-project/lotus/chain/lf3" "github.com/filecoin-project/lotus/chain/types" @@ -57,6 +60,128 @@ func TestF3_Enabled(t *testing.T) { e.requireAllMinersParticipate() } +// TestF3_Disabled tests the return values and errors of the F3 API when F3 is +// disabled or is not yet running. +func TestF3_InactiveModes(t *testing.T) { + kit.QuietMiningLogs() + + testCases := []struct { + mode string + expectedErrors map[string]any + expectedValues map[string]any + customValidateReturn map[string]func(t *testing.T, ret []reflect.Value) + }{ + { + mode: "disabled", + expectedErrors: map[string]any{ + "F3GetOrRenewParticipationTicket": lotus_api.ErrF3Disabled, + "F3Participate": lotus_api.ErrF3Disabled, + "F3GetCertificate": lotus_api.ErrF3Disabled, + "F3GetLatestCertificate": lotus_api.ErrF3Disabled, + "F3GetManifest": lotus_api.ErrF3Disabled, + "F3GetECPowerTable": lotus_api.ErrF3Disabled, + "F3GetF3PowerTable": lotus_api.ErrF3Disabled, + "F3IsRunning": lotus_api.ErrF3Disabled, + }, + expectedValues: map[string]any{ + "F3GetOrRenewParticipationTicket": (api.F3ParticipationTicket)(nil), + "F3Participate": api.F3ParticipationLease{}, + "F3GetCertificate": (*certs.FinalityCertificate)(nil), + "F3GetLatestCertificate": (*certs.FinalityCertificate)(nil), + "F3GetManifest": (*manifest.Manifest)(nil), + "F3GetECPowerTable": (gpbft.PowerEntries)(nil), + "F3GetF3PowerTable": (gpbft.PowerEntries)(nil), + "F3IsRunning": false, + }, + }, + { + mode: "not running", + expectedErrors: map[string]any{ + "F3GetOrRenewParticipationTicket": api.ErrF3NotReady, + "F3Participate": api.ErrF3NotReady, + "F3GetCertificate": "F3 is not running", + "F3GetLatestCertificate": "F3 is not running", + "F3GetManifest": "no known network manifest", + "F3GetF3PowerTable": "no known network manifest", + }, + expectedValues: map[string]any{ + "F3GetOrRenewParticipationTicket": (api.F3ParticipationTicket)(nil), + "F3Participate": api.F3ParticipationLease{}, + "F3GetCertificate": (*certs.FinalityCertificate)(nil), + "F3GetLatestCertificate": (*certs.FinalityCertificate)(nil), + "F3GetECPowerTable": (gpbft.PowerEntries)(nil), + "F3IsRunning": false, + }, + customValidateReturn: map[string]func(t *testing.T, ret []reflect.Value){ + "F3GetECPowerTable": func(t *testing.T, ret []reflect.Value) { + // special case because it simply returns power table from EC which is not F3 dependent + require.NotNil(t, ret[0].Interface(), "unexpected return value") + }, + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.mode, func(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), time.Minute) + defer cancel() + + opts := []any{kit.MockProofs()} + if tc.mode == "disabled" { + opts = append(opts, kit.F3Enabled(nil)) + } + + client, miner, ens := kit.EnsembleMinimal(t, opts...) + ens.InterconnectAll().BeginMining(2 * time.Millisecond) + ens.Start() + + head := client.WaitTillChain(ctx, kit.HeightAtLeast(10)) + + rctx := reflect.ValueOf(ctx) + rtsk := reflect.ValueOf(head.Key()) + rminer := reflect.ValueOf(miner.ActorAddr) + rticket := reflect.ValueOf([]byte("fish")) + rone := reflect.ValueOf(uint64(1)) + + calls := map[string][]reflect.Value{ + "F3GetOrRenewParticipationTicket": {rctx, rminer, rticket, rone}, + "F3Participate": {rctx, rticket}, + "F3GetCertificate": {rctx, rone}, + "F3GetLatestCertificate": {rctx}, + "F3GetManifest": {rctx}, + "F3GetECPowerTable": {rctx, rtsk}, + "F3GetF3PowerTable": {rctx, rtsk}, + "F3IsRunning": {rctx}, + } + + for fn, args := range calls { + t.Run(fn, func(t *testing.T) { + ret := reflect.ValueOf(client).MethodByName(fn).Call(args) + + if expectedValue, hasExpectedValue := tc.expectedValues[fn]; hasExpectedValue { + require.Equal(t, expectedValue, ret[0].Interface(), "unexpected return value") + } + + if expectedError, hasExpectedError := tc.expectedErrors[fn]; hasExpectedError { + switch err := expectedError.(type) { + case error: + require.ErrorIs(t, ret[1].Interface().(error), err, "unexpected error") + case string: + require.ErrorContains(t, ret[1].Interface().(error), err, "unexpected error") + } + } else { + require.Nil(t, ret[1].Interface(), "unexpected error") + } + + if validate, hasValidate := tc.customValidateReturn[fn]; hasValidate { + validate(t, ret) + } + }) + } + }) + } +} + // TestF3_Rebootstrap tests F3 can be rebootsrapped by changing the manifest // without disrupting miner participation. func TestF3_Rebootstrap(t *testing.T) { diff --git a/node/impl/full/f3.go b/node/impl/full/f3.go index 118099dfcc0..bbaba985b5c 100644 --- a/node/impl/full/f3.go +++ b/node/impl/full/f3.go @@ -25,11 +25,11 @@ type F3API struct { func (f3api *F3API) F3GetOrRenewParticipationTicket(ctx context.Context, miner address.Address, previous api.F3ParticipationTicket, instances uint64) (api.F3ParticipationTicket, error) { if f3api.F3 == nil { log.Infof("F3GetParticipationTicket called for %v, F3 is disabled", miner) - return api.F3ParticipationTicket{}, api.ErrF3Disabled + return nil, api.ErrF3Disabled } minerID, err := address.IDFromAddress(miner) if err != nil { - return api.F3ParticipationTicket{}, xerrors.Errorf("miner address is not of ID type: %v: %w", miner, err) + return nil, xerrors.Errorf("miner address is not of ID type: %v: %w", miner, err) } return f3api.F3.GetOrRenewParticipationTicket(ctx, minerID, previous, instances) } @@ -61,7 +61,7 @@ func (f3api *F3API) F3GetManifest(ctx context.Context) (*manifest.Manifest, erro if f3api.F3 == nil { return nil, api.ErrF3Disabled } - return f3api.F3.GetManifest(ctx), nil + return f3api.F3.GetManifest(ctx) } func (f3api *F3API) F3IsRunning(context.Context) (bool, error) { From 15000433ce4223df6d7200a369b88a51256b8930 Mon Sep 17 00:00:00 2001 From: Rod Vagg Date: Fri, 13 Dec 2024 19:57:54 +1100 Subject: [PATCH 2/5] fixup! fix(f3): test API during inactive-F3 modes and make consistent and safe --- itests/f3_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/itests/f3_test.go b/itests/f3_test.go index cef51dd2e15..49f5f750dba 100644 --- a/itests/f3_test.go +++ b/itests/f3_test.go @@ -109,7 +109,8 @@ func TestF3_InactiveModes(t *testing.T) { "F3Participate": api.F3ParticipationLease{}, "F3GetCertificate": (*certs.FinalityCertificate)(nil), "F3GetLatestCertificate": (*certs.FinalityCertificate)(nil), - "F3GetECPowerTable": (gpbft.PowerEntries)(nil), + "F3GetManifest": (*manifest.Manifest)(nil), + "F3GetF3PowerTable": (gpbft.PowerEntries)(nil), "F3IsRunning": false, }, customValidateReturn: map[string]func(t *testing.T, ret []reflect.Value){ From 6bc379ab0b833358a1dce923a2976d095ef82be3 Mon Sep 17 00:00:00 2001 From: Rod Vagg Date: Fri, 13 Dec 2024 20:20:00 +1100 Subject: [PATCH 3/5] fixup! fix(f3): test API during inactive-F3 modes and make consistent and safe --- chain/lf3/f3.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/chain/lf3/f3.go b/chain/lf3/f3.go index f9367998fa0..479687aac79 100644 --- a/chain/lf3/f3.go +++ b/chain/lf3/f3.go @@ -187,7 +187,7 @@ func (fff *F3) GetLatestCert(ctx context.Context) (*certs.FinalityCertificate, e func (fff *F3) GetManifest(ctx context.Context) (*manifest.Manifest, error) { m := fff.inner.Manifest() if m == nil { - return nil, xerrors.Errorf("no known network manifest") + return nil, xerrors.New("no known network manifest") } if m.InitialPowerTable.Defined() { return m, nil From 5e855d9c23c5a0c19cb0065a514bf09f6a22835c Mon Sep 17 00:00:00 2001 From: Rod Vagg Date: Fri, 13 Dec 2024 21:17:46 +1100 Subject: [PATCH 4/5] fixup! fix(f3): test API during inactive-F3 modes and make consistent and safe --- api/api_errors.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/api/api_errors.go b/api/api_errors.go index 83cbd32a4b7..ffb01289d7b 100644 --- a/api/api_errors.go +++ b/api/api_errors.go @@ -98,7 +98,7 @@ func (ErrActorNotFound) Error() string { return "actor not found" } type errF3Disabled struct{} -func (errF3Disabled) Error() string { return "F3 is disabled" } +func (errF3Disabled) Error() string { return "f3 is disabled" } type errF3ParticipationTicketInvalid struct{} @@ -124,7 +124,7 @@ func (errF3ParticipationTicketStartBeforeExisting) Error() string { type errF3NotReady struct{} -func (errF3NotReady) Error() string { return "F3 isn't yet ready to participate" } +func (errF3NotReady) Error() string { return "f3 isn't yet ready to participate" } // ErrExecutionReverted is used to return execution reverted with a reason for a revert in the `data` field. type ErrExecutionReverted struct { From 3631bf620441f7a7e5c1fc3669731663e0871ccf Mon Sep 17 00:00:00 2001 From: Rod Vagg Date: Fri, 13 Dec 2024 22:35:33 +1100 Subject: [PATCH 5/5] fixup! fix(f3): test API during inactive-F3 modes and make consistent and safe --- chain/lf3/f3.go | 2 +- itests/f3_test.go | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/chain/lf3/f3.go b/chain/lf3/f3.go index 479687aac79..96767ba8a6f 100644 --- a/chain/lf3/f3.go +++ b/chain/lf3/f3.go @@ -194,7 +194,7 @@ func (fff *F3) GetManifest(ctx context.Context) (*manifest.Manifest, error) { } cert0, err := fff.inner.GetCert(ctx, 0) if err != nil { - return m, xerrors.Errorf("getting certificate for instance 0: %w", err) + return m, nil // return manifest without power table } var mCopy = *m diff --git a/itests/f3_test.go b/itests/f3_test.go index 49f5f750dba..eb2ba79a472 100644 --- a/itests/f3_test.go +++ b/itests/f3_test.go @@ -353,7 +353,9 @@ func (e *testEnv) waitTillF3Instance(i uint64, timeout time.Duration) { func (e *testEnv) waitTillManifestChange(newManifest *manifest.Manifest, timeout time.Duration) { e.waitFor(func(n *kit.TestFullNode) bool { m, err := n.F3GetManifest(e.testCtx) - require.NoError(e.t, err) + if err != nil || m == nil { + return false + } return newManifest.Equal(m) }, timeout) }