VoterSet and Bitfield Refactoring #93

romanb · 2019-11-21T15:12:42Z

This is a proposal for a refactoring (or rather rewrite) of the voter_set and bitfield modules alongside smaller related changes in the round and vote_graph, targeting the v0.11.0 branch (i.e. based on #81).

Below is a rough outline of the main changes.

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 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).
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 recording votes in the vote graph and computing weights.
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_bit operation while
computing the vote weight on a node is based on 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.
This combines some functionality from the previous bitfield
module and some from the parent round module.

rphmeier · 2019-11-21T17:49:26Z

Looks good at a skim - going to give it a more in-depth review soon. Thanks!

src/round/context.rs

rphmeier · 2019-11-25T12:00:16Z

src/vote_graph.rs

 	N: Copy + Debug + BlockNumberOps,
 {
 	/// 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 {


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.

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.

src/voter_set.rs

rphmeier · 2019-11-25T12:12:55Z

I've been fuzz-testing the old implementation for the last 5 days on the rh-fuzzing branch without exposing any issues. I'm worried that the rewrite opens new opportunities for logic errors, particularly in equivocation counting, GHOST, and completability. I would rather not merge this until it has at least the same degree of testing. I was glad to see the property-based quickcheck testing on individual components.

codecov-io · 2019-11-25T12:32:08Z

Codecov Report

❗ No coverage uploaded for pull request base (master@79bb39b). Click here to learn what that means.
The diff coverage is 93.76%.

@@            Coverage Diff            @@
##             master      #93   +/-   ##
=========================================
  Coverage          ?   90.12%           
=========================================
  Files             ?       12           
  Lines             ?     3787           
  Branches          ?        0           
=========================================
  Hits              ?     3413           
  Misses            ?      374           
  Partials          ?        0

Impacted Files	Coverage Δ
src/fuzz_helpers.rs	`95.83% <100%> (ø)`
src/vote_graph.rs	`98.03% <100%> (ø)`
src/lib.rs	`81.22% <100%> (ø)`
src/voter/mod.rs	`87.28% <100%> (ø)`
src/voter/voting_round.rs	`81.55% <100%> (ø)`
src/weights.rs	`70% <70%> (ø)`
src/bitfield.rs	`89.77% <91.19%> (ø)`
src/voter_set.rs	`97.91% <97.63%> (ø)`
src/round/context.rs	`99.05% <99.05%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 79bb39b...7ded42e. Read the comment docs.

romanb · 2019-11-25T22:13:43Z

I would rather not merge this until it has at least the same degree of testing. [..]

I'm fine with waiting until the fuzz-testing is in master and the v0.11 and this branch subsequently rebased on top.

romanb · 2020-01-16T17:00:42Z

If there is still interest in this PR, I can at some point rebase it over the current master. Let me know.

rphmeier · 2020-01-16T18:12:53Z

Yep, there's still interest! Please do rebase it.

rphmeier · 2020-01-16T18:13:33Z

Could you re-target the PR again against master as well?

romanb · 2020-01-16T19:54:47Z

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.

rphmeier added the A0-pleasereview label Nov 21, 2019

rphmeier reviewed Nov 25, 2019

View reviewed changes

src/round/context.rs Show resolved Hide resolved

rphmeier reviewed Nov 25, 2019

View reviewed changes

src/voter_set.rs Outdated Show resolved Hide resolved

rphmeier reviewed Nov 25, 2019

View reviewed changes

src/voter_set.rs Outdated Show resolved Hide resolved

rphmeier reviewed Nov 25, 2019

View reviewed changes

src/voter_set.rs Outdated Show resolved Hide resolved

rphmeier added A5-grumble and removed A0-pleasereview labels Nov 25, 2019

burdges mentioned this pull request Nov 26, 2019

Align SignerTable with GRANDPA if possible w3f/bls#12

Open

romanb mentioned this pull request Nov 28, 2019

Upgrade to stable futures (#2). #100

Merged

Roman S. Borschel and others added 7 commits January 17, 2020 10:11

Documentation tweak.

b9c3d88

Fix some indentation and small test improvements.

ceb788d

Remove left-over unwrap.

fd098e6

Add missing license headers.

0b594b6

Prefer None over panic in VoterSet construction.

72d053e

Necessary adjustments after rebasing on master.

6bf8c3c

romanb force-pushed the refactor branch from 8be57e4 to 6bf8c3c Compare January 17, 2020 09:24

romanb changed the base branch from v0.11.0 to master January 17, 2020 09:25

romanb added 3 commits January 17, 2020 10:26

Remove unused dependency

f3a7f8a

Remove unused import.

84aadc2

Fix no-std build.

a14ced3

romanb added 2 commits January 17, 2020 10:43

Fix no-std tests.

0958168

Sorting keys in a VoterSet is now redundant.

7ded42e

Due to switching to a BTreeMap.

romanb added A0-pleasereview and removed A5-grumble labels Jan 17, 2020

rphmeier approved these changes Jan 17, 2020

View reviewed changes

rphmeier merged commit 152c495 into paritytech:master Jan 17, 2020

rphmeier added a commit that referenced this pull request Jan 17, 2020

Bump working version to v0.12.0

8bd982f

#93 introduced some backwards-incompatible changes to the API and we've already release v0.11.0.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VoterSet and Bitfield Refactoring #93

VoterSet and Bitfield Refactoring #93

romanb commented Nov 21, 2019 •

edited

Loading

rphmeier commented Nov 21, 2019

rphmeier Nov 25, 2019

romanb Nov 25, 2019 •

edited

Loading

rphmeier commented Nov 25, 2019 •

edited

Loading

codecov-io commented Nov 25, 2019 •

edited

Loading

romanb commented Nov 25, 2019

romanb commented Jan 16, 2020 •

edited

Loading

rphmeier commented Jan 16, 2020

rphmeier commented Jan 16, 2020

romanb commented Jan 16, 2020

VoterSet and Bitfield Refactoring #93

VoterSet and Bitfield Refactoring #93

Conversation

romanb commented Nov 21, 2019 • edited Loading

Bitfield

VoterSet

Related changes

rphmeier commented Nov 21, 2019

rphmeier Nov 25, 2019

Choose a reason for hiding this comment

romanb Nov 25, 2019 • edited Loading

Choose a reason for hiding this comment

rphmeier commented Nov 25, 2019 • edited Loading

codecov-io commented Nov 25, 2019 • edited Loading

Codecov Report

romanb commented Nov 25, 2019

romanb commented Jan 16, 2020 • edited Loading

rphmeier commented Jan 16, 2020

rphmeier commented Jan 16, 2020

romanb commented Jan 16, 2020

romanb commented Nov 21, 2019 •

edited

Loading

romanb Nov 25, 2019 •

edited

Loading

rphmeier commented Nov 25, 2019 •

edited

Loading

codecov-io commented Nov 25, 2019 •

edited

Loading

romanb commented Jan 16, 2020 •

edited

Loading