Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

replace simple_qa_power with extensible flags #1395

Merged
merged 2 commits into from
Sep 2, 2023

Conversation

LesnyRumcajs
Copy link
Contributor

See this discussion for more details. In short, we want to have an easily extensible flags field for the SectorOnChainInfo instead of a field per flag, which is wasteful.

@ZenGround0 Hopefully, I introduced your description correctly. Is the u8 correct in this context?

Closes #1275

Copy link
Contributor

@ZenGround0 ZenGround0 left a comment

Choose a reason for hiding this comment

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

Thank you this looks great

In order for this change to work correctly we will also need to migrate our existing sector infos from using a bool in this field to using the flag type. Luckily there is already a sector migration in the works for this upgrade. Would you mind doing that work too before I approve this? I want to make sure it doesn't fall through the cracks.

The work that is needed is

  1. Create the same sort of int flag type in golang (Look into go enum type construction)
  2. Add in the previous value to the new sector info format here: https://github.com/filecoin-project/go-state-types/blob/master/builtin/v12/migration/miner.go#L390, old values of false should set a zero bitfield, old values of true should set the SIMPLE_QA_POWER value of the bitfield using the new go enum type

Please reach out on slack if you'd like some help onboarding to go-state-types code. Since Im going to be unavailable in a week or so @snissn is also a good person to reach out to as he is currently working on a more complex piece of the sector info migration.

actors/miner/src/types.rs Outdated Show resolved Hide resolved
Copy link
Member

@anorth anorth left a comment

Choose a reason for hiding this comment

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

This introduces the first direct dependency on bitflags, so I think it's worth pausing to consider if it's worthwhile. Every dependency is code that could cause strife someday and isn't under our control. Our dependency tree is already a mess, and we transitively depend on bitflags a few times already. But for something so simple, maybe we could avoid taking this incremental risk?

actors/miner/src/types.rs Outdated Show resolved Hide resolved
@Stebalien
Copy link
Member

Note: The SDK & FVM currently use bitflags. I agree our dependency tree is a mess, but bitflags is probably not a huge concern.

@ZenGround0
Copy link
Contributor

I agree our dependency tree is a mess

Are the particular problems documented somewhere? I've been blissfully unaware of this.

@anorth
Copy link
Member

anorth commented Aug 31, 2023

Are the particular problems documented somewhere? I've been blissfully unaware of this.

Not that I'm aware, outside the 4600-line Cargo.lock. Some actual investigation would be needed to prune out the things required only for testing, etc.

@LesnyRumcajs
Copy link
Contributor Author

So, what is the consensus regarding bitflags? Should we keep it or have a more raw solution?

@ZenGround0
Copy link
Contributor

I think we're 2-1 on keeping bitflags vs dropping. From my POV the only thing blocking this from merge is getting the corresponding migration code.

@LesnyRumcajs
Copy link
Contributor Author

LesnyRumcajs commented Sep 1, 2023

I think we're 2-1 on keeping bitflags vs dropping. From my POV the only thing blocking this from merge is getting the corresponding migration code.

Done! I'll be on PTO from tomorrow for two weeks so do not hesitate to edit this PR or nag someone from Forest to drive this to completion if need be.

@ZenGround0 ZenGround0 added this pull request to the merge queue Sep 2, 2023
Merged via the queue into master with commit 8076305 Sep 2, 2023
@ZenGround0 ZenGround0 deleted the sector-info-qap-to-flags branch September 2, 2023 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

SectorInfo.simple_qap => SectorInfo.flags
4 participants