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

Removed without_storage_info from messages and schema pallets #1702

Merged
merged 16 commits into from
Oct 23, 2023

Conversation

aramikm
Copy link
Collaborator

@aramikm aramikm commented Oct 6, 2023

Goal

The goal of this PR is to remove without_storage_info which is necessary to be able to calculate max encoded length for PoV.

Closes
#1696

Changes

  • Since we can not submit a MaxEncodedLen PoV for messages due to oversized PoV we would need to use measured until we reworked the messages pallet.
  • Reduced the MessagesMaxPayloadSizeBytes from 50K to 3K to reduce the chance of Oversized PoV even for measured mode until the rework of messages pallet.
  • Reduced the MessagesMaxPerBlock from 7000 to 200 to reduce the chance of Oversized PoV even for measured mode until the rework of messages pallet.

Checklist

  • Chain spec updated
  • Weights updated

Notes

I checked all the submitted messages on rococo and main-net and there are no messages that violate these restrctions
Here is list of all messages on rococo and there are none on main-net. The biggest one has less than 700 bytes

messages.txt

Merge order

This most probably will get merged after node upgrade PR unless something changes

@codecov
Copy link

codecov bot commented Oct 6, 2023

Codecov Report

Merging #1702 (c1ed2b6) into main (831337e) will increase coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1702      +/-   ##
==========================================
+ Coverage   87.88%   87.94%   +0.06%     
==========================================
  Files          50       50              
  Lines        4241     4238       -3     
==========================================
  Hits         3727     3727              
+ Misses        514      511       -3     
Files Coverage Δ
pallets/messages/src/lib.rs 90.00% <100.00%> (+0.59%) ⬆️
pallets/messages/src/types.rs 75.67% <100.00%> (+1.99%) ⬆️
pallets/schemas/src/lib.rs 86.13% <ø> (+0.62%) ⬆️
pallets/schemas/src/types.rs 100.00% <ø> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@github-actions github-actions bot added the metadata-changed Metadata has changed since the latest full release label Oct 6, 2023
@github-actions github-actions bot removed the metadata-changed Metadata has changed since the latest full release label Oct 7, 2023
@github-actions github-actions bot added the metadata-changed Metadata has changed since the latest full release label Oct 17, 2023
@github-actions github-actions bot added metadata-changed Metadata has changed since the latest full release and removed metadata-changed Metadata has changed since the latest full release labels Oct 17, 2023
@github-actions github-actions bot removed the metadata-changed Metadata has changed since the latest full release label Oct 18, 2023
@github-actions
Copy link

Running benchmarks and calculating weights. DO NOT MERGE! A new commit will be added upon completion...

@github-actions
Copy link

Finished running benchmarks. Updated weights have been committed to this PR branch in commit 9b2f552.

demisx pushed a commit that referenced this pull request Oct 18, 2023
@github-actions github-actions bot added the metadata-changed Metadata has changed since the latest full release label Oct 18, 2023
@aramikm aramikm marked this pull request as ready for review October 18, 2023 22:11
@aramikm aramikm requested a review from wilwade as a code owner October 18, 2023 22:11
Copy link
Collaborator

@saraswatpuneet saraswatpuneet left a comment

Choose a reason for hiding this comment

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

Based on description and code review ✅

For off ipfs type messages we would not exceed pov, so I think it's safe to go with it for time being

Copy link
Collaborator

@shannonwells shannonwells left a comment

Choose a reason for hiding this comment

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

I downloaded the branch and ran make test and make start as well.

The two comments I have are non-blocking but that python script does deserve some explanation/comments at the top if you want to add it. It looks pretty generic looking, so I can't tell what it's meant to be used for.

Edit: also meant to note that I went through a couple of the parity PRs for comparison.

runtime/common/src/constants.rs Outdated Show resolved Hide resolved
scripts/python/get_messages.py Outdated Show resolved Hide resolved
@aramikm aramikm force-pushed the remove_without_storage_info branch from 9b2f552 to 450d889 Compare October 20, 2023 23:29
@github-actions github-actions bot removed the metadata-changed Metadata has changed since the latest full release label Oct 20, 2023
@enddynayn
Copy link
Collaborator

👍 looks great!

@github-actions github-actions bot added metadata-changed Metadata has changed since the latest full release metadata-version-not-incremented Metadata has changed since the latest full release, but the version has not been incremented labels Oct 21, 2023
@github-actions
Copy link

Running benchmarks and calculating weights. DO NOT MERGE! A new commit will be added upon completion...

@github-actions github-actions bot removed metadata-changed Metadata has changed since the latest full release metadata-version-not-incremented Metadata has changed since the latest full release, but the version has not been incremented labels Oct 23, 2023
Copy link
Collaborator

@enddynayn enddynayn left a comment

Choose a reason for hiding this comment

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

stupendous! 💯

@github-actions
Copy link

Running benchmarks and calculating weights. DO NOT MERGE! A new commit will be added upon completion...

@github-actions
Copy link

Finished running benchmarks. Updated weights have been committed to this PR branch in commit d70798e.

demisx pushed a commit that referenced this pull request Oct 23, 2023
@github-actions github-actions bot added metadata-changed Metadata has changed since the latest full release metadata-version-not-incremented Metadata has changed since the latest full release, but the version has not been incremented labels Oct 23, 2023
@aramikm aramikm force-pushed the remove_without_storage_info branch from d70798e to c1ed2b6 Compare October 23, 2023 19:57
@github-actions github-actions bot removed metadata-changed Metadata has changed since the latest full release metadata-version-not-incremented Metadata has changed since the latest full release, but the version has not been incremented labels Oct 23, 2023
@aramikm aramikm self-assigned this Oct 23, 2023
@github-actions github-actions bot added the metadata-changed Metadata has changed since the latest full release label Oct 23, 2023
@aramikm aramikm merged commit 4736b69 into main Oct 23, 2023
30 checks passed
@aramikm aramikm deleted the remove_without_storage_info branch October 23, 2023 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
metadata-changed Metadata has changed since the latest full release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants