F3#GetManifest can panic

[rvagg]

lotus/chain/lf3/f3.go

Lines 187 to 189 in 9bfd0b4

	func (fff *F3) GetManifest(ctx context.Context) *manifest.Manifest {
	m := fff.inner.Manifest()
	if m.InitialPowerTable.Defined() {

It's possible currently for the inner call, f3.F3#Manifest() to return a nil. Apparently this shouldn't be possible, it should always fall back to the static manifest which is available at compile time: https://github.com/filecoin-project/lotus/pull/12762/files#r1878043815

Unfortunately the current architecture of go-f3's F3 is that it's initialised with a nil manifest, then it has to first Start() and then has to get its first update from the ManifestProvider before it's set. So calling GetManifest() here before that happens can result in a panic, which can happen in itests when F3 is not properly set up.

This should either be guarded here, or fixed in go-f3.

[masih]

Note, manifest can only be nil in dynamic manifest provider: the mechanism that makes passive testing possible. In dynamic manifest provider, there is an option which sets the initial manifest, i.e. the one to use if no other manifest is broadcasted via the manifest server. I see this option is indeed set in Lotus if static manifest is non-nil.

My question is then: under what condition static manifest is set to nil? @rvagg do you have a test that reproduces this in a tagged release?

[rvagg]

func TestManifestNil(t *testing.T) {
	ctx, cancel := context.WithTimeout(context.Background(), time.Minute)
	defer cancel()

	client, _, ens := kit.EnsembleMinimal(t, kit.MockProofs())
	ens.InterconnectAll().BeginMining(2 * time.Millisecond)
	ens.Start()

	client.WaitTillChain(ctx, kit.HeightAtLeast(10))
	manifest, err := client.F3GetManifest(ctx)

	require.NoError(t, err)
	require.NotNil(t, manifest)
}

panic: runtime error: invalid memory address or nil pointer dereference [recovered]
        panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x58 pc=0x21e667c]

goroutine 31 [running]:
testing.tRunner.func1.2({0x45de640, 0x9df7370})
        /snap/go/10751/src/testing/testing.go:1631 +0x24a
testing.tRunner.func1()
        /snap/go/10751/src/testing/testing.go:1634 +0x377
panic({0x45de640?, 0x9df7370?})
        /snap/go/10751/src/runtime/panic.go:770 +0x132
github.com/filecoin-project/lotus/chain/lf3.(*F3).GetManifest(0xc008197470, {0x7af7a60, 0xc00074e150})
        /home/rvagg/go/src/github.com/filecoin-project/lotus/chain/lf3/f3.go:189 +0x3c
github.com/filecoin-project/lotus/node/impl/full.(*F3API).F3GetManifest(...)
        /home/rvagg/go/src/github.com/filecoin-project/lotus/node/impl/full/f3.go:64
command-line-arguments.TestManifestNil(0xc00060d1e0)
        /home/rvagg/go/src/github.com/filecoin-project/lotus/itests/f3_test.go:439 +0x125

[masih]

Thanks.

Looks like if :

• F3 is not enabled, or
• f3 bootstrap epoch is negative

then get manifest will return nil.

This seems to be by design and diverges from my earlier understanding.

Note that in itests, f3 is disabled by default with a dedicated F3Enable function to enable it. Hence the failure of the test above.

In terms of next steps: probably the safest thing to do is to make sure F3GetManifest documentation mentions that it is possible for this API to return a nil manifest under listed conditions, and change calls to this API to defensively check for nil.