-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
feat: limit PoSted partitions to 3 #11327
Conversation
chain/actors/policy/policy.go
Outdated
@@ -685,6 +691,66 @@ func GetAddressedSectorsMax(nwVer network.Version) (int, error) { | |||
} | |||
} | |||
|
|||
func getPoStedPartitionsMax(nwVer network.Version) (int, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, this isn't really necessary. It's "correct", but we could also just limit to miner12.PoStedPartitionsMax
for all actor versions 0-12 without actually breaking anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I'm not sure how much I like that -- it is wrong to do so. But I'm also very uninterested in "supporting" earlier network versions, so I'm not opposed to making that change.
Your proposed change also has the nice advantage that SPs will start respecting the v12 limit when they upgrade to this release, which reduces the risk of inflight PoSts getting funky over the upgrade.
So...yeah, let's do it.
We should add a note about this limitation to https://github.com/filecoin-project/lotus/blob/master/node/config/types.go#L293-L304 |
So I ran this branch in a local devnet w/needed dependecies:
And can confirm that after the nv-upgrade I see WdPoSt-msg being sliced into message with 2 partitions Dedaline table:
PoSt cycle begins:
WdPoSt msg submission:
Inspecting one of the msgs:
|
Thanks for the callout! |
650eee3
to
612e5f3
Compare
612e5f3
to
9630456
Compare
Will re-run the testing for sanity to see that we are now splitting messages before nv21. But looks good to me once CI is happy |
9630456
to
74a4577
Compare
@@ -298,7 +298,7 @@ func TestWDPostDoPostPartLimitConfig(t *testing.T) { | |||
//stm: @CHAIN_SYNCER_COLLECT_CHAIN_001, @CHAIN_SYNCER_COLLECT_HEADERS_001, @CHAIN_SYNCER_VALIDATE_TIPSET_001 | |||
//stm: @CHAIN_SYNCER_NEW_PEER_HEAD_001, @CHAIN_SYNCER_VALIDATE_MESSAGE_META_001, @CHAIN_SYNCER_STOP_001 | |||
ctx := context.Background() | |||
expectedMsgCount := 364 | |||
expectedMsgCount := 8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@magik6k Please review changes to magic numbers here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those seem to make sense
74a4577
to
2a644e2
Compare
Can confirm that WdPoSt-messages are now getting split into partitions of 3 before nv21: Dedaline table:
PoSt cycle begins:
WdPoSt msg submission:
Inspecting one of the msgs:
|
Integrates filecoin-project/builtin-actors#1436