Test with latest actors version #6998

Stebalien · 2021-08-06T21:45:44Z

We were already trying to do this, but had some issues:

We were forced to run all upgrades early, in-sequence.
1. This actually has some issues as can be seen in Deadlines can get messed up around the start of a new network specs-actors#1453.
2. We had to carefully wait for upgrades to finish before moving on.
3. This slows down the tests.
We weren't consistently testing with the latest version.
1. Lots of tests would just run the default upgrade schedule, and would sometimes fail if the upgrade did something unexpected.
2. We weren't always testing the latest code.

This patch:

Sets the genesis version, where applicable.
Defaults to an empty upgrade schedule with genesis at the latest network version.
Makes some of the tests more reliable (e.g., wait a full proving period instead of the remainder of the proving period).
Fixes genesis at actors v2.

~~TODO figure out why it's failing:~~

I'm seeing errors about missing t_aux files in tests that try to use the actual prover. I have no idea how these tests worked before.
When I remove the check for t_aux files (do we need them with fauxseals?), post still doesn't work. I'm seeing an error about trying to prove deadline 0 when we're already in deadline 1, which is suspiciously like Deadlines can get messed up around the start of a new network specs-actors#1453.

Answer: in case 2, my machine was just too slow. CI and my new machine are faster.

Stebalien · 2021-08-09T22:29:41Z

Rebasing on #7011. The tests now pass locally when doing this. I believe this was a timing issue (I'm now using a faster computer).

However, this has me a bit concerned (for the tests, not the network). Now that we're starting from a later actors version, we may have to prove pre-seal sectors immediately where before the proving epoch would start later. In theory, we could start in the middle of of an active proving window. I'm pretty sure that's what was happening, actually...

Stebalien · 2021-08-09T23:24:56Z

Given that the tests pass reliably in CI, I think we're probably fine.

Stebalien · 2021-08-10T06:38:06Z

Hm. So I still see this issue if I enable TestDealWithMarketAndMinerNode (at least locally). Now, this test was disabled for a reason, but this is still interesting.

Properly handle genesis in actors tests. Fast-forward upgrading to actors v13 doesn't work because there needs to be at least a day between v0 genesis and v13 (due to differences in miner cron).

Stebalien · 2021-08-11T21:17:17Z

itests/ccupgrade_test.go

-		5000, // after
+		-1,  // before
+		162, // while sealing
+		560, // after upgrade deal


This was happening a bit too early and causing a test failure.

Stebalien · 2021-08-11T21:17:50Z

itests/ccupgrade_test.go

-		-1,   // before
-		162,  // while sealing
-		530,  // after upgrade deal
-		5000, // after


There wasn't really a point to this. It was just testing with the previous actors version.

ZenGround0

LGTM

ZenGround0 · 2021-08-11T21:07:26Z

chain/gen/genesis/miners.go

@@ -584,14 +587,23 @@ func currentEpochBlockReward(ctx context.Context, vm *vm.VM, maddr address.Addre
 	}

 	// TODO: This hack should move to reward actor wrapper


Does this TODO still represent our current plans? If so I can look into scoping this work on the actors side. However it seems like our idea of this has changed and this is no longer considered a hack. If so let's remove the TODO.

Ideally this would move into the reward actor wrapper. That is, the shim in chain/actors/builtin/reward. We have some abstractions there around message sending.

ZenGround0 · 2021-08-11T21:11:19Z

itests/kit/ensemble.go

@@ -208,11 +216,15 @@ func (n *Ensemble) Miner(miner *TestMiner, full *TestFullNode, opts ...NodeOpt)
 			genm    *genesis.Miner
 		)

+		// Default 2KiB sector for the network version


nit: seems like this should be an ensemble option so the default can be overridden, though maybe this should be left to whoever first wants to use non-2KiB sectors in an itest

Yeah, I'm going to leave it to them.

ZenGround0 · 2021-08-11T21:20:14Z

itests/kit/ensemble_opts.go

 }

 var DefaultEnsembleOpts = ensembleOpts{
 	pastOffset: 10000000 * time.Second, // time sufficiently in the past to trigger catch-up mining.
+	upgradeSchedule: stmgr.UpgradeSchedule{{


nit: the fact that this is so nicely expressed as GenesisNetworkVersion(build.NewestNetworkVersion) makes me wonder if we'd be slightly better off with a DefaultEnsembleOpts of type []EnsembleOpts applied to a zero valued ensembleOpts. The name ensembleOpts could be changed to something different, ensembleCfg?, to make this a little less confusing.

Not very important just riffing.

I agree. I'd much rather have a set of default options (also avoids cases where the default "config" gets modified accidentally). But that'll be a future cleanup.

ZenGround0 · 2021-08-11T21:23:14Z

itests/wdpost_test.go

@@ -63,7 +65,7 @@ func testWindowPostUpgrade(t *testing.T, blocktime time.Duration, nSectors int,
 	require.NoError(t, err)

 	t.Log("Running one proving period")
-	waitUntil := di.PeriodStart + di.WPoStProvingPeriod + 2


Was this what fixed the "proving deadline 0 in deadline 1" issue?

No, that issue appeared to be "my old laptop was too slow to compute 2k sector proofs and seal 2k sectors at the same time".

Here, I just needed to wait a full proving period instead of waiting until our next proving period started. It likely worked before because the sectors happened to align on the right proving periods for this to "just work". But the new code is "more correct".

You can see that issue in the "+20". Termination happens at the end of the deadline always. If we need "slack", something is wrong.

Stebalien requested a review from ZenGround0 August 6, 2021 21:47

Stebalien force-pushed the feat/latest-genesis branch from 713afa3 to 337c65c Compare August 9, 2021 22:27

Stebalien marked this pull request as ready for review August 9, 2021 23:12

Stebalien requested a review from a team as a code owner August 9, 2021 23:12

Stebalien marked this pull request as draft August 9, 2021 23:12

Stebalien mentioned this pull request Aug 9, 2021

fix: don't check for t_aux when proving #7011

Merged

Stebalien added the status/blocked label Aug 9, 2021

This was referenced Aug 10, 2021

Shed: Include network name in genesis-verify #7019

Merged

get other actor controlList #7027

Merged

Stebalien force-pushed the feat/latest-genesis branch from 337c65c to faf893e Compare August 11, 2021 18:29

Stebalien marked this pull request as ready for review August 11, 2021 18:29

Stebalien removed the request for review from ZenGround0 August 11, 2021 18:29

Stebalien removed the status/blocked label Aug 11, 2021

Stebalien added 2 commits August 11, 2021 13:30

fix genesis for actors v2

3235dbf

test: genesis actors version

dcff06b

Properly handle genesis in actors tests. Fast-forward upgrading to actors v13 doesn't work because there needs to be at least a day between v0 genesis and v13 (due to differences in miner cron).

Stebalien force-pushed the feat/latest-genesis branch from faf893e to dcff06b Compare August 11, 2021 20:30

Stebalien requested a review from ZenGround0 August 11, 2021 20:57

Stebalien commented Aug 11, 2021

View reviewed changes

ZenGround0 approved these changes Aug 11, 2021

View reviewed changes

Stebalien merged commit af9a71a into master Aug 11, 2021

Stebalien deleted the feat/latest-genesis branch August 11, 2021 21:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test with latest actors version #6998

Test with latest actors version #6998

Stebalien commented Aug 6, 2021 •

edited

Loading

Stebalien commented Aug 9, 2021

Stebalien commented Aug 9, 2021

Stebalien commented Aug 10, 2021

Stebalien Aug 11, 2021

Stebalien Aug 11, 2021

ZenGround0 left a comment

ZenGround0 Aug 11, 2021

Stebalien Aug 11, 2021

ZenGround0 Aug 11, 2021

Stebalien Aug 11, 2021

ZenGround0 Aug 11, 2021

Stebalien Aug 11, 2021

ZenGround0 Aug 11, 2021

Stebalien Aug 11, 2021

Stebalien Aug 11, 2021

		@@ -584,14 +587,23 @@ func currentEpochBlockReward(ctx context.Context, vm *vm.VM, maddr address.Addre
		}

		// TODO: This hack should move to reward actor wrapper

Test with latest actors version #6998

Test with latest actors version #6998

Conversation

Stebalien commented Aug 6, 2021 • edited Loading

Stebalien commented Aug 9, 2021

Stebalien commented Aug 9, 2021

Stebalien commented Aug 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZenGround0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stebalien commented Aug 6, 2021 •

edited

Loading