This repository has been archived by the owner on Nov 15, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Collective: Benchmark with greater MaxProposals
#12454
Merged
paritytech-processbot
merged 19 commits into
paritytech:master
from
Szegoo:collective-bench
Nov 14, 2022
Merged
Changes from all commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
a8bc2cc
Collective: Benchmark with greated
Szegoo 8ce5fa2
fix
Szegoo ddefb59
remove bs
Szegoo d3e2579
id_to_remark_data
Szegoo 0830040
fix
Szegoo 08fcd43
remove hardcoded
Szegoo a887818
clean up
Szegoo 375f999
simplify
Szegoo 93586a2
questionable renaming
Szegoo 167efdb
better variable name
Szegoo 80e9937
better solution
Szegoo bb65d9a
no need for large length
Szegoo 61deeac
better solution
Szegoo 79416e6
Update frame/collective/src/benchmarking.rs
Szegoo f57994a
fix
Szegoo 1f5208d
test
Szegoo 1769703
remove test
Szegoo bd89f9b
Merge branch 'paritytech:master' into collective-bench
Szegoo 6e2de72
Merge branch 'paritytech:master' into collective-bench
Szegoo File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Why these changes?
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.
The goal of this PR was to be able to set the
MaxProposal
to a value higher than255
. In the benchmarks, we use theremark
function to represent a proposal. Because we cannot have two identical proposals what we did until now is make the remark data be a variable number repeated a number of times(b
times). This comes with a limit since the remark data is aVec<u8>
so it cannot accept a number greater than255
. To fix this we use little-endian to encode these greater numbers. So to represent a number greater than255
we need a vector that is longer than 1. That is why we updated the range ofb
so that it starts from 2. @bkchrI hope the explanation makes sense :)
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.
Then it should be changed to
4
, becauseMaxMembers
is anu32
which would lead to duplicate proposals when using more than 65k max members. You should also leave some comment on why we start at 4.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.
#12454 (comment)
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 okay 🙈
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.
I'm not happy with this, but okay :P