VoterSet and Bitfield Refactoring#93
Conversation
|
Looks good at a skim - going to give it a more in-depth review soon. Thanks! |
| { | ||
| /// Create a new `VoteGraph` with base node as given. | ||
| pub fn new(base_hash: H, base_number: N) -> Self { | ||
| pub fn new(base_hash: H, base_number: N, base_node: V) -> Self { |
There was a problem hiding this comment.
do we have any use-case where we are initializing this to non-default? The Default ~ Zero property is important for a lot of the GHOST-stuff and this may discourage that.
There was a problem hiding this comment.
This was primarily motivated by the fact that making use of a type parameter constraint (i.e. trait bound) in a method whereby the type parameter appears nowhere in the method's arguments necessarily leaves the parameter uninstantiated in expressions of the form
let graph = VoteGraph::new(...);and thus makes type inference harder. Previously V could then be instantiated by the type system through observing subsequent occurrences of graph.insert(...). Now the parameter of insert is some W such that V: for<'a> AddAssign<&'a W>, so V is still undetermined. It can of course be solved by giving an explicit type ascription to expressions such as the above (let graph: ... = ), it just seemed to me that passing the base node in the constructor is preferable as it is more explicit and avoids this type ambiguity.
Generally the VoteGraph has no clue what V::default() produces, i.e. there was and is no Default ~ Zero property. However, it is certainly true that this allows constructing a VoteGraph with a base node that is different from what the default instance returns and the latter is used inside the VoteGraph to construct intermediate new nodes on demand. If that is undesirable, I can certainly go with the extra type annotations.
|
I've been fuzz-testing the old implementation for the last 5 days on the |
Codecov Report
@@ Coverage Diff @@
## master #93 +/- ##
=========================================
Coverage ? 90.12%
=========================================
Files ? 12
Lines ? 3787
Branches ? 0
=========================================
Hits ? 3413
Misses ? 374
Partials ? 0
Continue to review full report at Codecov.
|
I'm fine with waiting until the fuzz-testing is in master and the |
|
If there is still interest in this PR, I can at some point rebase it over the current master. Let me know. |
|
Yep, there's still interest! Please do rebase it. |
|
Could you re-target the PR again against master as well? |
Of course. |
Bitfield:
* Simplify definition, as Bitfield::Blank and LiveBitfield(vec![])
are equivalent.
* Dynamically-sized with lazy allocation makes for a more ergonomic
API and keeps bitfields smaller. If there are few voters, most votes
are likely on the same nodes / blocks but in that case bitfields
are almost never resized anyway. As there are more and more voters,
votes are spread across more and more nodes / blocks and thus not
all nodes need a fully-sized bitfield.
* Avoid allocations when merging bitfields. Previously merging would
always allocate a new bitfield, now the other bitfield is merged
into `self` and only allocates if `self` needs to be resized.
* Avoid allocations when casting votes and computing weights.
Previously both casting of new votes and computing weights
involved creating and merging bitfields. Now incorporating a
vote into the vote graph is a `set_bit` operation while
computing the vote weight on a node is based a bitfield-merging
iterator that does not allocate.
* Better tests.
VoterSet:
* Make it impossible to construct inconsistent sets as well as
empty sets. Both were previously possible.
* Better tests.
Related changes:
* Introduce a `weights` module for encapsulating vote-weight
arithmetic behind newtypes.
* Introduce a `round::context` module for encapsulating the
context of a round in which vote weights are computed,
consisting of the voter set and recorded equivocations.
Due to switching to a BTreeMap.
#93 introduced some backwards-incompatible changes to the API and we've already release v0.11.0.
Codecov Report❌ Patch coverage is
Additional details and impacted files
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
4 participants
This is a proposal for a refactoring (or rather rewrite) of the
voter_setandbitfieldmodules alongside smaller related changes in theroundandvote_graph, targeting thev0.11.0branch (i.e. based on #81).Below is a rough outline of the main changes.
Bitfield
Bitfield::BlankandLiveBitfield(vec![])are equivalent.
API and keeps bitfields smaller with larger voter sets. The reasoning
here is the following: If there are few voters, most votes
are likely on the same or very few nodes / blocks but in that case bitfields
are almost never resized anyway. As there are more and more voters,
votes are likely spread across more and more nodes / blocks and thus not
all nodes need a full-sized bitfield (whose size grows with the voter set).
always allocate a new bitfield, now the other bitfield is merged
into
selfand only allocates ifselfneeds to be resized.Previously both recording of new votes and computing weights
involved creating and merging bitfields. Now incorporating a
vote into an existing vote-node is a
set_bitoperation whilecomputing the vote weight on a node is based on a bitfield-merging
iterator that does not allocate.
VoterSet
empty sets. Both were previously possible.
Related changes
weightsmodule for encapsulating vote-weightarithmetic behind newtypes.
round::contextmodule for encapsulating thecontext of a round in which vote weights are computed,
consisting of the voter set and recorded equivocations.
This combines some functionality from the previous
bitfieldmodule and some from the parent
roundmodule.