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

feat: limit PoSted partitions to 3 #1436

Merged
merged 1 commit into from
Oct 11, 2023
Merged

feat: limit PoSted partitions to 3 #1436

merged 1 commit into from
Oct 11, 2023

Conversation

arajasek
Copy link
Contributor

@arajasek arajasek commented Oct 10, 2023

There's concern that certain PoSt operations can be so expensive that they don't fit in a single block at all (currently 10B gas). This is especially concerning if PoSts cannot be disputed.

This PR adds a new dedicated limit for the number of partitions that can be proven in a single PoSt message, and sets it to 2. This limit thus ensures that "bad" proofs can be safely disputed, as they will only ever cover 2 partitions at a time.

I'm proposing adding a new dedicated limit, instead of modifying either ADDRESSED_SECTORS_MAX or ADDRESSED_PARTITIONS_MAX, since those values are used elsewhere. I'd like this change to be very targeted at PoSt messages only.

@arajasek
Copy link
Contributor Author

We'll need to advertise this change loudly so that mining software can adjust to deal with it.

Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

The only possible concern is that this might cause some issues with butterfly/dev networks (i.e., lots of small window posts because partitions are small) but, IMO, this is the right short-term fix.

The right long-term fix is to remove 99% of these limits because they were originally introduced pre-wasm to limit memory/compute.

@codecov-commenter
Copy link

Codecov Report

Merging #1436 (0840c26) into master (b81da94) will decrease coverage by 0.06%.
Report is 1 commits behind head on master.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1436      +/-   ##
==========================================
- Coverage   91.09%   91.03%   -0.06%     
==========================================
  Files         145      145              
  Lines       27862    27867       +5     
==========================================
- Hits        25381    25369      -12     
- Misses       2481     2498      +17     
Files Coverage Δ
actors/miner/src/lib.rs 84.86% <100.00%> (+0.01%) ⬆️
runtime/src/runtime/policy.rs 98.46% <100.00%> (+0.01%) ⬆️

... and 3 files with indirect coverage changes

@anorth
Copy link
Member

anorth commented Oct 10, 2023

The structure of this change seems fine, but 2 seems like a very low limit that I'm not aware of a good justification for.

@zhiqiangxu
Copy link
Contributor

zhiqiangxu commented Oct 11, 2023

Worth mentioning that at venus, the largest partitions in a batch is currently 3.

@arajasek
Copy link
Contributor Author

@anorth @zhiqiangxu Thanks for the feedback! I'm going to increase this to 3.

@arajasek arajasek force-pushed the asr/limit-partitions branch from 0840c26 to da61546 Compare October 11, 2023 13:46
@arajasek arajasek enabled auto-merge October 11, 2023 13:46
@arajasek arajasek changed the title feat: limit PoSted partitions to 2 feat: limit PoSted partitions to 3 Oct 11, 2023
@arajasek arajasek added this pull request to the merge queue Oct 11, 2023
Merged via the queue into master with commit 2e03f77 Oct 11, 2023
@arajasek arajasek deleted the asr/limit-partitions branch October 11, 2023 14:22
@beck-8
Copy link

beck-8 commented Oct 16, 2023

Is it too hasty to directly change 10 to 3 on the consensus side?

@Fatman13
Copy link

Fatman13 commented Oct 20, 2023

Shouldn't this be FIPed?
What would be the required security level for an issue to warrant its bypassing of FIP process?
What is the security level of this issue?
Any analysis on how this would impact current wdPost gas dynamics?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants