Skip to content

Conversation

frrist
Copy link
Member

@frrist frrist commented Jan 26, 2022

addresses #798

Migration

This PR adds a new table: miner_sector_infos_v7 which will track all miner_sector_infos for v7 actors and beyond. The table includes a new column sector_key_cid that will include a CID when the sector is snapped, null otherwise.

@frrist frrist marked this pull request as draft January 26, 2022 02:21
@codecov-commenter
Copy link

codecov-commenter commented Jan 26, 2022

Codecov Report

Merging #823 (ede437b) into master (beea5b7) will decrease coverage by 0.6%.
The diff coverage is 1.8%.

@@           Coverage Diff            @@
##           master    #823     +/-   ##
========================================
- Coverage    31.8%   31.2%   -0.7%     
========================================
  Files          39      39             
  Lines        3785    3869     +84     
========================================
+ Hits         1207    1209      +2     
- Misses       2432    2514     +82     
  Partials      146     146             

@frrist frrist force-pushed the frrist/update-lotusv1.14.0-rc4 branch from f96d167 to f1809e9 Compare February 3, 2022 22:31
);
SELECT set_integer_now_func('miner_sector_infos_v7', 'current_height', replace_if_exists => true);

COMMENT ON TABLE {{ .SchemaName | default "public"}}.miner_sector_infos_v7 IS 'Latest state of sectors by Miner.';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update comment to Latest state of sectors by Miner for actors v7 and above

@frrist frrist force-pushed the frrist/update-lotusv1.14.0-rc4 branch from b651c51 to 9698c31 Compare February 8, 2022 22:53
@frrist frrist changed the title Frrist/update lotusv1.14.0 rc4 Frrist/update lotusv1.14.0 rc Feb 8, 2022
@frrist frrist force-pushed the frrist/update-lotusv1.14.0-rc4 branch from 93cf282 to 3c5ef6a Compare February 11, 2022 19:40
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iand and @placer14 would ya'll mind giving these changes a look.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iand and @placer14 would ya'll mind giving these changes a look.

Copy link
Contributor

@placer14 placer14 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looked at all the things. (Quite a journey!) Some thoughts but mostly seems fine to me. 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Diff noise.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix indent to prevent future diff noise.

Comment on lines +134 to +155
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

documentation++ 💪

(Aside: I'm unsure what value is provided by knowing that the intersections of Faults|Recovers to Terminated are an empty set. I don't believe there's enough information here to derive Terminated sectors with this interface, if that was the intention.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not version.Major == 0? Do we want this behavior for v2?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a bug from a prior change, addressing, thanks

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we no longer need MinerSectorInfoList? How are v1-6 lists represented in miner task results?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This actually isn't used anywhere, deleting the file.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not accurate for mainnet. Here's a sample of the epochs with the largest miner sector info counts:

Sector infos counts by height (sorted)
epoch   count
1308103	62862
1308104	55310
1351277	52732
1351278	48938
957453	44865
957442	44711
1310880	43818
1310869	41598
1308087	39744
1351276	38868
1308085	37501
1308099	36948
1310890	36937
1432490	36692
1308127	35628
1308132	34858
1308106	34773
1310892	34767
1308168	34713
957255	34077
1308113	32569
1310888	32549
1308167	32439
1308147	32433
1308112	32365
1310868	32331
1052506	32193
1135670	31827
1307986	30727
1308101	30246
1308098	30230
1308161	30168
1310894	30118
1308146	30093
1308169	30080
1308141	30060
1308128	30057
1308109	29929
1357687	28750
1308008	28629
1308001	28568
1307995	28451
1308014	28365
806834	28344
1308088	28266
957467	27998

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think a good value here is then?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recalc math using above assumptions.

@frrist frrist marked this pull request as ready for review February 22, 2022 19:57
@frrist frrist force-pushed the frrist/update-lotusv1.14.0-rc4 branch from 02c6971 to ede437b Compare February 22, 2022 20:03
@frrist frrist requested a review from iand February 22, 2022 21:02
@frrist frrist changed the title Frrist/update lotusv1.14.0 rc Update to lotus 1.14.1 & specs-actors 7.0.0 Feb 22, 2022
Copy link
Contributor

@iand iand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some changes needed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a fan of this new name! What was wrong with miner_sector_infos_v7 where v7 indicates the network version that it was introduced?

miner_sector_infos_v7plus seems like painting ourselves into a box. What does it mean if we introduce another change for network version 9?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fair point, will drop the plus

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't look right. This uses the slow insert method if the major schema version is 1 (the current version).You only want to do this if the schema version is < 1 because it allows each individual model to be downgraded to the V0 version in m.Persist.

This is all irrelevant anyway since we deleted the V0 schema

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is all irrelevant anyway since we deleted the V0 schema

In that case could we just remove this all together?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be != but as above this is currently irrelevant since there is only one major version of the schema currently: v1

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming we'll add a major version 2 someday wouldn't this be better as == 0 (what I thought I typed when I pushed this)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be cleaner if you declared the PersistableList first with all the other persistables and then either appended sectorModelV7Plus or sectorModelV6Minus depending on the actor code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed, done.

- add MinerSectorInfoV7Plus model for indexing miner sector info with snapped sectors
@frrist frrist force-pushed the frrist/update-lotusv1.14.0-rc4 branch from ede437b to b4ee9a9 Compare February 23, 2022 18:24
@frrist frrist merged commit 1f079bd into master Feb 23, 2022
@frrist frrist deleted the frrist/update-lotusv1.14.0-rc4 branch February 23, 2022 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants