feat: migration: sector deal ids index cached #210

snissn · 2023-08-29T22:07:26Z

Add migration cache to newMinerMigrator constructor.
Implement sector index caching in migrateSectorsWithCache.
Optimize sector migration by leveraging diffs in cached AMT.
Manage sector-to-deal mappings using a HAMT structure.

- Introduced `SectorDealIDs` struct to store DealIDs associated with a sector in the market module. - Implemented CBOR marshaling and unmarshaling for `SectorDealIDs`. - Updated related references and dependencies in the market package.

- Enhanced `State` struct in the market module to include `SectorDeals`, a mapping of sector IDs to deal IDs grouped by storage provider. - Updated CBOR serialization logic in `cbor_gen.go`: - Adjusted `lengthBufState` to reflect new field. - Implemented CBOR marshaling and unmarshaling for `SectorDeals`. - This addition facilitates more efficient queries for sector-specific deal data.

- Extended sector migration functions in `miner.go` to support `minerAddr`. - Introduced `sectorToDealIdHamt` to map sector numbers to their associated deal IDs. - Added console print to log `sectorToDealIdHamtCid` and `minerAddr` for subsequent construction of `sector_deal_ids_index`. - This is a preliminary commit, and the logging will be removed after constructing the required index.

The HamtCid struct was introduced in the market module, along with the necessary methods to marshal and unmarshal it using CBOR. Since this information is auto-generated, additional detailed comments have been omitted. - Added HamtCid type definition in market_state.go - Implemented MarshalCBOR and UnmarshalCBOR methods for HamtCid in cbor_gen.go - Updated gen.go to include the HamtCid type in the main function

- Add `sectorDeals` field in `minerMigrator` for mapping of sector IDs to deal IDs. - Modify sector migration methods to be methods of the `minerMigrator` struct. - Introduce a mechanism to map sector numbers to their associated deal IDs and add to the state tree. - Correct manifest actor matching for `market11Cid`. - Initiate a second migration for the market actor post miner migration. "

- Removed `builtin/v12/migration/market.go` as the migration logic is now centralized in `top.go`. - Enhanced error messages in the migration process for better debugging. - Added safety checks before updating sector deal IDs in the market actor.

…d market object to use this new state

- Remove v11 market state import. - Transition from GetActorV4/SetActorV4 to GetActorV5/SetActorV5 methods. - Use v12 market state type for migration. - Add address field when updating the market actor in the state tree.

This reverts commit ca993fe.

…arket actor

- Add migration cache to newMinerMigrator constructor. - Implement sector index caching in migrateSectorsWithCache. - Optimize sector migration by leveraging diffs in cached AMT. - Manage sector-to-deal mappings using a HAMT structure.

builtin/v12/migration/miner.go

- Rename `outDealHamt` to `outDealHamtCid` for clarity. - Streamline the function `addSectorToDealIDHamtToSectorDeals` to be called once in `migrateSectorsWithCache`. - Remove redundant calls to `addSectorToDealIDHamtToSectorDeals` in `migrateSectorsFromScratch` and `migrateSectorsWithDiff`. - Pass `cid.Cid` directly to `addSectorToDealIDHamtToSectorDeals` to improve type handling.

ZenGround0

Unfortunately I missed a large and important piece of this migration when we first went over the specification of what we want.

This migration creates an index over all deal ids referenced by sector infos. However half of all sector infos are dead and their deal ids are therefore also dead / about to be dead. We want to only add deal ids that are active in the market actor to the index, otherwise they will never be cleaned up.

I think the easiest way to do this is create a set of active deal ids by reading the v11 market actor state before the miner migrations, passing this into the miner migrator and then only adding deal ids that are in this set.

builtin/v12/migration/top.go

migration/util.go

builtin/v12/migration/top.go

builtin/v12/migration/miner.go

arajasek · 2023-09-11T17:02:53Z

builtin/v12/migration/miner.go

@@ -171,11 +213,18 @@ func migrateSectorsWithCache(ctx context.Context, store adt11.Store, cache migra
 		if err != nil {
 			return cid.Undef, xerrors.Errorf("failed to get previous outRoot from cache: %w", err)
 		}
+
+		okSectorIndexOut, prevSectorIndexRoot, err := cache.Read(migration.MinerPrevSectorDealIndexKey(inRoot))


I think you want to look up prevInRoot here, not inRoot. We won't have a cached result for inRoot but we should have one for prevInRoot.

(By looking up inRoot I think you'll wind up doing the migration from scratch every time)

changed here #a008d024f687a6fc24fc0ebac27b4961037343a5

…r inRoot but we should have one for prevInRoot

Co-authored-by: Aayush Rajasekaran <[email protected]>

…com/filecoin-project/go-state-types into mikers/sector_deal_ids_index_cached

- Replaced usage of the `builtin.LoadTree`, `builtin.NewTree`, and methods on `builtin.ActorTree` handling with the adt12 package. - Modified HAMT operations such as loading, creating, and getting the root CID to use the new adt12 methods. - Adjusted method parameters to fit the new HAMT functions from adt12. - Refactored addSectorNumberToDealIdHAMT and removeSectorNumberToDealIdFromHAMT functions to use the new HAMT operations for adding and deleting entries. - Streamlined error messages and handling related to HAMT operations for clarity.

arajasek

More notes.

arajasek · 2023-09-12T17:42:38Z

builtin/v12/market/market_state.go

@@ -55,6 +55,10 @@ type State struct {

 	// Verified registry allocation IDs for deals that are not yet activated.
 	PendingDealAllocationIds cid.Cid // HAMT[DealID]AllocationID
+
+	// New mapping of sector IDs to deal IDS, grouped by storage provider.
+	ProviderSectors cid.Cid // HAMT[Address]HAMT[SectorNumber]SectorDeals


Suggested change

ProviderSectors cid.Cid // HAMT[Address]HAMT[SectorNumber]SectorDeals

ProviderSectors cid.Cid // HAMT[Address]HAMT[SectorNumber]SectorDealIDs

arajasek · 2023-09-12T18:31:49Z

builtin/v12/migration/top.go

+	// Create the new state
+	newMarketState := market12.State{
+		Proposals:                     oldMarketState.Proposals,
+		States:                        oldMarketState.States, // FIXME migrate deal states to include sector number


Yes, but as far as I understand, this work is still outstanding. We need to do the second portion of the migration described here (and to add a sector number to and remove allocation ID from each DealState).

arajasek · 2023-09-12T18:33:13Z

builtin/v12/migration/miner.go

@@ -120,7 +149,7 @@ func (m minerMigrator) MigrateState(ctx context.Context, store cbor.IpldStore, i

 	wrappedStore := adt11.WrapStore(ctx, store)

-	newSectors, err := migrateSectorsWithCache(ctx, wrappedStore, in.Cache, in.Address, inState.Sectors)
+	newSectors, err := m.migrateSectorsWithCache(ctx, wrappedStore, in.Cache, in.Address, inState.Sectors)


@snissn Can you make this change too (ideally as a pre-factor that we land first, but otherwise just as part of this PR)?

arajasek · 2023-09-12T18:33:22Z

builtin/v12/migration/miner.go

@@ -120,7 +149,7 @@ func (m minerMigrator) MigrateState(ctx context.Context, store cbor.IpldStore, i

 	wrappedStore := adt11.WrapStore(ctx, store)

-	newSectors, err := migrateSectorsWithCache(ctx, wrappedStore, in.Cache, in.Address, inState.Sectors)
+	newSectors, err := m.migrateSectorsWithCache(ctx, wrappedStore, in.Cache, in.Address, inState.Sectors)


I'm worried about forgetting about it.

arajasek · 2023-09-12T18:34:17Z

builtin/v12/migration/miner.go

@@ -67,12 +74,34 @@ func newMinerMigrator(ctx context.Context, store cbor.IpldStore, outCode cid.Cid

 	}

+	//load sector index from datastore, or create a new one if not found
+	okSectorIndex, prevSectorIndexRoot, err := cache.Read(migration.MarketSectorIndexKey())


We could consider reading this in top.go, and just passing prevSectorIndexRoot here so that we don't pass the entire cache here. I don't think it matters too much.

arajasek · 2023-09-12T18:34:27Z

builtin/v12/migration/miner.go

+		if err != nil {
+			return nil, xerrors.Errorf("reading cached sectorDeals tree: %w", err)
+		}
+		fmt.Println("Loaded HAMT from cache: ", prevSectorIndexRoot)


arajasek · 2023-09-12T18:36:26Z

builtin/v12/migration/miner.go

+		// New mapping of sector IDs to deal IDS, grouped by storage provider.
+		sectorDeals, err = adt12.MakeEmptyMap(ctxStore, builtin.DefaultHamtBitwidth)
+		if err != nil {
+			return nil, xerrors.Errorf("creating new state tree: %w", err)


bad error message

arajasek · 2023-09-12T18:37:50Z

builtin/v12/migration/miner.go

+	//add to new address sector deal id hamt index
+	err = m.addSectorToDealIDHamtToSectorDeals(sectorToDealIdHamtCid, in.Address)
+	if err != nil {
+		return nil, xerrors.Errorf("failed to add sector to deal id hamt to for minerAddr to HAMT: %w", err)


bad error msg

arajasek · 2023-09-12T18:58:29Z

builtin/v12/migration/miner.go

 	}

 	var sectorInfo miner11.SectorOnChainInfo
 	if err = inArray.ForEach(&sectorInfo, func(k int64) error {
+
+		err = addSectorNumberToDealIdHAMT(sectorToDealIdHamt, sectorInfo, store)


check error here

- Introduced a new `dealToSectorIndex` map in the `minerMigrator` structure to map deal IDs to sector numbers. - Modified methods to update both the HAMT and the newly introduced deal-to-sector index. - Started the foundation for market state migration logic by setting up placeholders and TODOs. - Introduced the `MarketState` type in `miner_types.go` for better type clarity. Note: More work is needed to complete the market state migration, as indicated by the TODOs.

…fs to shrinking temporary lookup table

- Added imports for v12 market and utilities - Removed unused market and miner v12 imports - Migrated `oldMarketState.States` from v11 to v12: - Iterated through each old market state, used `miner.dealToSectorIndex` to update the sector number. - Created a new ADT array for the deal states in v12 and populated it with the updated deal states. - Replaced the old market state's `States` with the new deal states CID in `newMarketState`.

- Updated field name in CBOR serialization and deserialization in `cbor_gen.go`. - Adjusted invariant checks to reference 'DealStates' in `invariants.go`. - Modified type definition in `market_state.go` to reflect the renaming. - Ensured migration logic in `top.go` uses the new 'DealStates' field.

go: downloading google.golang.org/genproto v0.0.0-20230306155012-7f2fa6fef1f4 chain/actors/builtin/market/v12.go:80:18: s.State.States undefined (type "github.com/filecoin-project/go-state-types/builtin/v12/market".State has no field or method States) chain/actors/builtin/market/v12.go:80:51: otherState12.State.States undefined (type "github.com/filecoin-project/go-state-types/builtin/v12/market".State has no field or method States) chain/actors/builtin/market/v12.go:84:52: s.State.States undefined (type "github.com/filecoin-project/go-state-types/builtin/v12/market".State has no field or method States) chain/actors/builtin/market/v12.go:197:53: v12.VerifiedClaim undefined (type "github.com/filecoin-project/go-state-types/builtin/v12/market".DealState has no field or method VerifiedClaim) make: *** [Makefile:89: lotus] Error 1 Revert "Rename 'States' field to 'DealStates' in v12 market" This reverts commit 4e83d01.

- Modified `dealToSectorIndex` from a pointer to a map to a pointer to a `sync.Map`. - Adjusted relevant methods to use `Load` and `Store` from `sync.Map` for concurrency safety. - Updated logic in `MigrateStateTree` to handle loading sector numbers using the `sync.Map`.

arajasek

(forgot to submit review yday)

arajasek · 2023-09-13T18:13:57Z

builtin/v12/migration/miner.go

+	sectorDeals                *adt12.Map
+	OutCodeCID                 cid.Cid
+	marketSectorDealsIndexLock *sync.Mutex
+	dealToSectorIndex          *map[uint64]uint64


Suggested change

dealToSectorIndex *map[uint64]uint64

dealToSectorIndex map[uint64]uint64

should be fine?

arajasek · 2023-09-13T18:24:36Z

builtin/v12/migration/miner.go

+		return xerrors.Errorf("failed to delete sector from sectorToDealIdHamt index: %w", err)
+	}
+
+	//note that this is very slow because the index only goes one direction


I think we should be fine not removing the deal from the map here, so long as we only use this map as a lookup, and not the "list" of deals. That is, if we iterate over some other struct as the list of deals, and only use this for the sectornumber lookup. That way it would be fine to have some extra info here, so long as we aren't missing any deals (which we shouldn't be).

- Implement caching mechanism for previous market deal states. - Enhance deal state migration logic to prioritize cached states. - Add new utility function for previous market states key.

This reverts commit 6856ec7.

…ement

arajasek · 2023-09-21T19:30:14Z

Deferred to nv22.

snissn added 20 commits August 22, 2023 19:13

feat: Add SectorDealIDs with CBOR serialization

cdc4fa4

- Introduced `SectorDealIDs` struct to store DealIDs associated with a sector in the market module. - Implemented CBOR marshaling and unmarshaling for `SectorDealIDs`. - Updated related references and dependencies in the market package.

forgot the file for market migration

f4616ba

Rename variables for clarity in market actor migration code

3d217c0

Save the new market state object to the datastore and set the migrate…

c4ec6a7

…d market object to use this new state

bugfixes: Update migration logic to use v12 market state

2600de7

- Remove v11 market state import. - Transition from GetActorV4/SetActorV4 to GetActorV5/SetActorV5 methods. - Use v12 market state type for migration. - Add address field when updating the market actor in the state tree.

xxx remove migration for testing

ca993fe

Revert "xxx remove migration for testing"

435c9c8

This reverts commit ca993fe.

bufix: place RunMigration after the initial set up work to find the m…

b97547c

…arket actor

save to sector index hamt to cache after doing all the work!

0c8a77a

bugfix: use market11cid for code

c4ef84e

bugfix: use MarketKey for market11cid

232b1c8

add hamt lock

a785f26

use cbor gen for cid

b4865b9

remove duplicate code block

778d677

snissn changed the base branch from mikers/sector_deal_ids_index to master September 1, 2023 23:36

snissn commented Sep 1, 2023

View reviewed changes

builtin/v12/migration/miner.go Outdated Show resolved Hide resolved

snissn added 2 commits September 1, 2023 13:41

pass lock by reference

e7a1505

snissn requested a review from ZenGround0 September 2, 2023 00:59

Merge branch 'master' into mikers/sector_deal_ids_index_cached

26084d2

snissn self-assigned this Sep 2, 2023

snissn marked this pull request as ready for review September 2, 2023 01:00

ZenGround0 reviewed Sep 2, 2023

View reviewed changes

ZenGround0 reviewed Sep 3, 2023

View reviewed changes

arajasek reviewed Sep 11, 2023

View reviewed changes

snissn and others added 10 commits September 11, 2023 12:07

look up prevInRoot here, not inRoot. We won't have a cached result fo…

a008d02

…r inRoot but we should have one for prevInRoot

chain errors

190fe18

Update builtin/v12/migration/top.go

56dc952

Co-authored-by: Aayush Rajasekaran <[email protected]>

Merge branch 'mikers/sector_deal_ids_index_cached' of https://github.…

be33517

…com/filecoin-project/go-state-types into mikers/sector_deal_ids_index_cached

remove old todo

7f7613b

fix error message

a9745f4

add error message

4f851d5

better name for hamt

740b22b

cleanup errors

f791133

arajasek reviewed Sep 12, 2023

View reviewed changes

snissn added 7 commits September 12, 2023 15:53

remove verifreg.AllocationId based on fip-0076 requirements

869619d

remove delete from m.dealToSectorIndex because of algorithmic tradeof…

43bda09

…fs to shrinking temporary lookup table

arajasek reviewed Sep 14, 2023

View reviewed changes

snissn added 7 commits September 14, 2023 15:04

Refactor migration logic to incorporate caching previous deal states

d731666

- Implement caching mechanism for previous market deal states. - Enhance deal state migration logic to prioritize cached states. - Add new utility function for previous market states key.

handle errors

99548ca

add err handler and minor refactor formatting if statements

6856ec7

Revert "add err handler and minor refactor formatting if statements"

143bddb

This reverts commit 6856ec7.

add err handler

f197c10

prints for debugging

6ae71c1

adding more print statements for debugging and cleaning up an if stat…

efc66d1

…ement

arajasek closed this Sep 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: migration: sector deal ids index cached #210

feat: migration: sector deal ids index cached #210

snissn commented Aug 29, 2023

ZenGround0 left a comment

arajasek Sep 11, 2023

arajasek Sep 11, 2023

snissn Sep 11, 2023

arajasek left a comment

arajasek Sep 12, 2023

arajasek Sep 12, 2023

arajasek Sep 12, 2023

arajasek Sep 12, 2023

arajasek Sep 12, 2023

arajasek Sep 12, 2023

arajasek Sep 12, 2023

arajasek Sep 12, 2023

arajasek Sep 12, 2023

arajasek Sep 12, 2023

arajasek left a comment

arajasek Sep 13, 2023

arajasek Sep 13, 2023

arajasek commented Sep 21, 2023

	ProviderSectors cid.Cid // HAMT[Address]HAMT[SectorNumber]SectorDeals
	ProviderSectors cid.Cid // HAMT[Address]HAMT[SectorNumber]SectorDealIDs

	dealToSectorIndex *map[uint64]uint64
	dealToSectorIndex map[uint64]uint64

feat: migration: sector deal ids index cached #210

feat: migration: sector deal ids index cached #210

Conversation

snissn commented Aug 29, 2023

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

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

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

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

arajasek commented Sep 21, 2023