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.

Sign up for GitHub

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

Jump to bottom

feat: adjust witness size limits #11511

Merged
merged 2 commits into from
Jun 7, 2024
Merged

feat: adjust witness size limits #11511

merged 2 commits into from
Jun 7, 2024

Conversation

pugachAG
Copy link
Contributor

@pugachAG pugachAG commented Jun 6, 2024

This PR includes the following:

  • Set size limit for compressed witness to 32MiB. This is at least twice as big as the witness we expected to handle, so it give us quite some safety margin.
  • Set size limit for the compressed witness to 64MiB. Same as above, this values give us pretty big margin on top of witness sizes we expect.
  • Reduce partial witness cache size to 40 entries to limit memory usage at ~1.3GiB. 40 entries should be more than enough to handle all kinds of crazy chain forks 🙃

@pugachAG pugachAG force-pushed the adjust-partial-witness-limits branch from 8075dda to 2d23a4e Compare June 6, 2024 20:16
@pugachAG pugachAG marked this pull request as ready for review June 6, 2024 20:16
@pugachAG pugachAG requested a review from a team as a code owner June 6, 2024 20:16
shreyan-gupta
shreyan-gupta approved these changes Jun 6, 2024
View reviewed changes
Copy link
Contributor

@shreyan-gupta shreyan-gupta left a comment

Choose a reason for hiding this comment

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

Verifying, we have both MAX_COMPRESSED_STATE_WITNESS_SIZE and MAX_RAW_STATE_WITNESS_SIZE to take into account decoding size bomb thingi issue?

@pugachAG
Copy link
Contributor Author

pugachAG commented Jun 6, 2024

@shreyan-gupta only MAX_RAW_STATE_WITNESS_SIZE is used for that (in fact that is the only place where that is currently used). MAX_COMPRESSED_STATE_WITNESS_SIZE limit is applied before that when verifying partial witness.
So yeah, effectively both limits are currently applied.

@codecov Codecov
Copy link

codecov bot commented Jun 6, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.41%. Comparing base (cceb7b5) to head (f91fc2c).
Report is 14 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master   #11511      +/-   ##
==========================================
+ Coverage   71.35%   71.41%   +0.05%     
==========================================
  Files         787      788       +1     
  Lines      159385   159881     +496     
  Branches   159385   159881     +496     
==========================================
+ Hits       113734   114178     +444     
  Misses      40734    40734              
- Partials     4917     4969      +52
Flag Coverage Δ
backward-compatibility 0.23% <0.00%> (-0.01%) ⬇️
db-migration 0.23% <0.00%> (-0.01%) ⬇️
genesis-check 1.36% <0.00%> (-0.01%) ⬇️
integration-tests 37.69% <100.00%> (+0.15%) ⬆️
linux 68.83% <100.00%> (+0.07%) ⬆️
linux-nightly 70.93% <100.00%> (+0.04%) ⬆️
macos 52.45% <100.00%> (+1.66%) ⬆️
pytests 1.57% <0.00%> (-0.01%) ⬇️
sanity-checks 1.37% <0.00%> (-0.01%) ⬇️
unittests 66.10% <100.00%> (+0.03%) ⬆️
upgradability 0.28% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tayfunelmas
tayfunelmas reviewed Jun 6, 2024
View reviewed changes
core/primitives/src/stateless_validation.rs
pub const MAX_COMPRESSED_STATE_WITNESS_SIZE: ByteSize = ByteSize::mib(32);

/// Represents max allowed size of the raw (not compressed) state witness,
/// corresponds to the size of borsh-serialized ChunkStateWitness.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit. I think MAX_UNCOMPRESSED_STATE_WITNESS_SIZE could be clearer. or maybe MAX_STATE_WITNESS_SIZE? "raw" is ambiguous.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agree, done in f91fc2c

tayfunelmas
tayfunelmas approved these changes Jun 6, 2024
View reviewed changes
Copy link
Contributor

@tayfunelmas tayfunelmas left a comment

Choose a reason for hiding this comment

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

Can we even reduce it further like 16M and 32M respectively? maybe experiment with it later, do not want to block this one as it also clarifies the constant names etc.

jancionear
jancionear approved these changes Jun 7, 2024
View reviewed changes
Copy link
Contributor

@jancionear jancionear left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, with all the limits we don't expect the witness to be larger than 16MB, so 32MB should give us a safe margin.

@jancionear
Copy link
Contributor

Can we even reduce it further like 16M and 32M respectively? maybe experiment with it later, do not want to block this one as it also clarifies the constant names etc.

I would leave it at 32/64, it's safer this way. There might be some weird case where the witness gets above 16MB and with 16MB limit the network would stall. 32MB is safer, and doesn't really hurt.

@pugachAG
Copy link
Contributor Author

pugachAG commented Jun 7, 2024

@tayfunelmas Yes, I think we could reduce it further. For that we would need to carefully consider all current size limits in place (storage proof, transactions, current WIP with receipts). Currently selected values still have a lot of margin to be on the safe side, but I want to merge it ASAP to replace the current limit of 512MiB with something more reasonable.

@pugachAG pugachAG enabled auto-merge June 7, 2024 12:37
@pugachAG pugachAG added this pull request to the merge queue Jun 7, 2024
Merged via the queue into master with commit f7887fe Jun 7, 2024
29 checks passed
@pugachAG pugachAG deleted the adjust-partial-witness-limits branch June 7, 2024 13:06
@jancionear jancionear mentioned this pull request Jun 27, 2024
Merged
github-merge-queue bot pushed a commit that referenced this pull request Jun 28, 2024
@jancionear
Increase MAX_COMPRESSED_STATE_WITNESS_SIZE to 48 MiB (#11683)
80f08d7 
The `MAX_COMPRESSED_STATE_WITNESS_SIZE` has been set to 32 MiB in
#11511. Back then it was a
reasonable choice, but since then we increased some of the runtime
parameters, which increased the worst case witness size by ~17MiB.
(#11582,
#11629). I'd feel safer with a
larger limit on the compressed witness size.
jancionear added a commit that referenced this pull request Jun 28, 2024
@jancionear
Increase MAX_COMPRESSED_STATE_WITNESS_SIZE to 48 MiB (#11683)
32397b9 
The `MAX_COMPRESSED_STATE_WITNESS_SIZE` has been set to 32 MiB in
#11511. Back then it was a
reasonable choice, but since then we increased some of the runtime
parameters, which increased the worst case witness size by ~17MiB.
(#11582,
#11629). I'd feel safer with a
larger limit on the compressed witness size.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jancionear jancionear jancionear approved these changes

@shreyan-gupta shreyan-gupta shreyan-gupta approved these changes

@tayfunelmas tayfunelmas tayfunelmas approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pugachAG @jancionear @tayfunelmas @shreyan-gupta