Network upgrade support #3781

Stebalien · 2020-09-12T03:20:18Z

This patch starts adding support for network upgrades.

It adds an actors abstraction layer for loading abstract (cross-version) actors.
It starts switching over to a shared deadline type.
It adds an abstraction for ADTs (hamt/amt).
It removes the callback-based API in the StateManager (difficult to abstract across actor versions).
It does not actually add support for actors v2. We can do that in a followup patch but that should be relatively easy.

This patch is heavily WIP and does not compile. Feel free to push changes directly to this branch.

Notes:

~~State tree access now needs a network version, because the HAMT type will change.~~ The state-tree now includes a version.
I haven't figured out a nice way to abstract over changes to the message types. However, many of them will be type aliased to actors v0 in actors v2 so we can likely continue using the v0 versions (or use the v2 versions everywhere). I've been renaming imports to v0* to make it clear that we're importing types from a specific actors version.

TODO:

~~Consider merging incremental improvements? We'd have to get this compiling again first but we could merge in the new abstractions, and slowly switch over.~~
Finish migrating to the new abstractions.
Remove all actor state types from the public API. See miner.State.Info() for the planned approach here.
Fix the tests. This is likely going to be a massive pain.
Use a specific state-tree version, instead of the actors version.
Figure out how to handle chain queries across version upgrades (specifically, hamt version changes).
Figure out VM upgrade process.
Figure out block format upgrades. I.e., message AMT in blocks.

This patch starts adding support for network upgrades. * It adds an actors abstraction layer for loading abstract (cross-version) actors. * It starts switching over to a shared deadline type. * It adds an abstraction for ADTs (hamt/amt). * It removes the callback-based API in the StateManager (difficult to abstract across actor versions). * It _does not_ actually add support for actors v2. We can do that in a followup patch but that should be relatively easy. This patch is heavily WIP and does not compile. Feel free to push changes directly to this branch. Notes: * State tree access now needs a network version, because the HAMT type will change. * I haven't figured out a nice way to abstract over changes to the _message_ types. However, many of them will be type aliased to actors v0 in actors v2 so we can likely continue using the v0 versions (or use the v2 versions everywhere). I've been renaming imports to `v0*` to make it clear that we're importing types from a _specific_ actors version. TODO: * Consider merging incremental improvements? We'd have to get this compiling again first but we could merge in the new abstractions, and slowly switch over. * Finish migrating to the new abstractions. * Remove all actor state types from the public API. See `miner.State.Info()` for the planned approach here. * Fix the tests. This is likely going to be a massive pain.

api/api_full.go

api/types.go

Stebalien · 2020-09-12T03:22:43Z

chain/actors/builtin/builtin.go

+// Converts a network version into a specs-actors version.
+func VersionForNetwork(version network.Version) Version {
+	switch version {
+	case network.Version0, network.Version1:


Yes. Network version != actor version. Also, actor version is going to go from 0 (well, 0.9) to 2. That's going to be rather strange..

If it will just be used as a unique key, maybe use a string "v2" instead?

We're now encoding this inside the state tree itself.

@magik6k what version do we actually want in the state tree. We could use:

Network version (seems like overkill).

Actors version (what we're using here).

A specific "state tree" version (for the HAMT). That seems like the best approach, honestly.

Thoughts?

A specific state tree version sounds like what we want here

chain/actors/builtin/init/init.go

Stebalien · 2020-09-12T03:28:59Z

chain/vm/invoker.go

@@ -47,6 +47,7 @@ func NewInvoker() *Invoker {
 	}

 	// add builtInCode using: register(cid, singleton)
+	// NETUPGRADE: register code IDs for v2, etc.


My plan here is to register code IDs for both v1 and v2 actors, at the same time. As far as I know, this should "just work".

Correct, though ideally we'd at least specify minimum / maximum epoch at which a given code CID can be used

cmd/lotus-shed/balances.go

cmd/lotus-storage-miner/proving.go

anorth

Looking good from here

api/types.go

anorth · 2020-09-12T05:29:49Z

chain/actors/builtin/builtin.go

+// Converts a network version into a specs-actors version.
+func VersionForNetwork(version network.Version) Version {
+	switch version {
+	case network.Version0, network.Version1:


If it will just be used as a unique key, maybe use a string "v2" instead?

chain/actors/builtin/market/v0.go

chain/actors/version.go

chain/state/statetree.go

magik6k · 2020-09-14T08:49:15Z

Consider merging incremental improvements? We'd have to get this compiling again first but we could merge in the new abstractions, and slowly switch over

Yes, definitely want non-consensus breaking changes in before, so the PR with the actual consensus upgrade is as small/easy to review as possible

chain/stmgr/utils.go

This is a pretty big footgun.

chain/actors/builtin/market/market.go

arajasek

I think this is good to merge. The biggest thing kicked down the road is probably genesis setup stuff (along with lots of tests explicitly using v0 actors). I think that's fine for now.

Some of the "just use v0 actors it'll be fine" needs to be scrutinised, but we can do a second pass when we actually introduce v1 actors.

magik6k · 2020-09-23T07:58:47Z

chain/stmgr/stmgr.go

@@ -300,7 +290,11 @@ func (sm *StateManager) ApplyBlocks(ctx context.Context, parentEpoch abi.ChainEp
 		return cid.Cid{}, cid.Cid{}, err
 	}

-	rectarr := adt.MakeEmptyArray(sm.cs.Store(ctx))
+	// XXX: Is the height correct? Or should it be epoch-1?


This is matching how the VM GetNtwkVersion works, so most likely correct

(differently something to manually verify when we test the fork)

magik6k · 2020-09-23T08:10:15Z

cmd/lotus-shed/genesis-verify.go

+			case act.IsAccountActor():
+				st, err := account.Load(store, act)
+				if err != nil {
+					// TODO: magik6k: this _used_ to log instead of failing, why?


That's a @whyrusleeping question

magik6k

Looks good, also works, ship it!

Stebalien commented Sep 12, 2020

View reviewed changes

Stebalien requested review from magik6k, anorth and whyrusleeping September 12, 2020 03:34

anorth reviewed Sep 12, 2020

View reviewed changes

magik6k added the P0 P0: Critical Blocker label Sep 14, 2020

magik6k and others added 11 commits September 14, 2020 13:14

Fix some build errors

38f8798

Fix vm build

6809713

More terraforming in chain/

683a581

Merge remote-tracking branch 'origin/master' into refactor/net-upgrade

06ec571

Some post-master merge fixes

fb3f710

rename to simplify merge

2088335

Merge branch 'master' into refactor/net-upgrade

02dcb5e

Progress

cc4d530

add info method to miner state API

7fa6c1e

progress

36f920b

state manager progress

4dabab5

magik6k reviewed Sep 15, 2020

View reviewed changes

chain/stmgr/utils.go Outdated Show resolved Hide resolved

magik6k and others added 10 commits September 15, 2020 15:29

state api impl fixes

4419850

gofmt

9dad38c

more progress

00c6397

fix decode

6bf7976

remove final WithStateTree

4cd92d8

migrate wallet access

53384e8

migrate StateMinserSectorCount

c64f983

Compile fixes

91e9573

start multisig abstraction

4e01fad

sub reward actor

e1ba4eb

arajasek force-pushed the refactor/net-upgrade branch from 2373b19 to cc99ecd Compare September 22, 2020 05:16

arajasek added 2 commits September 22, 2020 01:34

Refinements to stmgr and utils

d56da1b

Resolve some unnecessary actor upgrade TODOs

1dc69e3

arajasek force-pushed the refactor/net-upgrade branch from cc99ecd to 1dc69e3 Compare September 22, 2020 05:35

Stebalien added 7 commits September 22, 2020 10:52

fixup some imports

fc15888

remove ptr indirection

5314ba8

copy actor object when iterating over actors

ab070f2

This is a pretty big footgun.

finish migrating statemanager

bc24fdb

cleanup imports some more

441d7ff

remove old comment

c91774b

use abstract actor type methods

2967c4e

frrist reviewed Sep 22, 2020

View reviewed changes

chain/actors/builtin/market/market.go Show resolved Hide resolved

Stebalien and others added 8 commits September 22, 2020 14:09

add deal state iterator

46fb0e7

Test fix

77f81fc

update oni

7737147

API shouldn't depend on actors directly

ed4bf9b

Reward state interface only needs cbor.Marshaler

e27fc03

Add an error return to all actor state interface methods

476e799

Implement inefficient OnAddressMapChange predicate

819180f

Use actor state addresses

a876a0b

arajasek approved these changes Sep 23, 2020

View reviewed changes

arajasek marked this pull request as ready for review September 23, 2020 06:33

arajasek requested review from Kubuxu and raulk as code owners September 23, 2020 06:33

magik6k changed the title ~~[WIP] Network upgrade support~~ Network upgrade support Sep 23, 2020

magik6k reviewed Sep 23, 2020

View reviewed changes

magik6k approved these changes Sep 23, 2020

View reviewed changes

magik6k merged commit c46e157 into master Sep 23, 2020

magik6k deleted the refactor/net-upgrade branch September 23, 2020 09:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Network upgrade support #3781

Network upgrade support #3781

Stebalien commented Sep 12, 2020 •

edited by arajasek

Loading

Stebalien Sep 12, 2020

anorth Sep 12, 2020

Stebalien Sep 18, 2020

magik6k Sep 21, 2020 •

edited

Loading

Stebalien Sep 12, 2020

magik6k Sep 14, 2020

anorth left a comment

anorth Sep 12, 2020

magik6k commented Sep 14, 2020

arajasek left a comment

magik6k Sep 23, 2020

magik6k Sep 23, 2020

magik6k left a comment

Network upgrade support #3781

Network upgrade support #3781

Conversation

Stebalien commented Sep 12, 2020 • edited by arajasek Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magik6k Sep 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anorth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magik6k commented Sep 14, 2020

arajasek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magik6k left a comment

Choose a reason for hiding this comment

Stebalien commented Sep 12, 2020 •

edited by arajasek

Loading

magik6k Sep 21, 2020 •

edited

Loading