feat: record stats for majority vs. committee size ratio

[bajtos]

I'd like to get visibility into the size of the majorities - is it a tight majority or a super majority?

Links:

• Find majorities in Spark measurements roadmap#59

[juliangruber]

Suggested change

	// Ignore committees that are too small or with no majority
	if (majoritySize > 0 && measurements > 0) {
	// Ignore committees that are too small or with no majority
	if (measurements > 0 && majoritySize > 0) {

I think in this order the condition matches the comment

[juliangruber]

Also, size zero != "too small", right? You could argue that a committee with one measurement is also too small. Is it instead that we want to ignore just empty committees?

[bajtos]

I see how this is confusing, I had to re-read the code to understand what's going on.

majoritySize > 0 means that 1) the committee is large enough to give us confidence in the majority being honest 2) a majority was found in the measurements.

measurements > 0 is a safety check to prevent division by zero.

Now that I think about it, when majoritySize > 0 then measurements > 0 must be true as well, because "majority" size is always smaller or equal thant "committee size" (number of measurements).

I propose reworking the code as follows:

if (majoritySize > 0) {
  assert(measurements > 0, "majority cannot be larger than the committee size")
  // calculation
}

How can I improve the comment and/or the condition to make the intention more clear?

I can change majoritySize to number | undefined so that committees evaluated to COMMITTEE_TOO_SMALL and MAJORITY_NOT_FOUND will get majoritySize === undefined.

WDYT?

[juliangruber]

Sounds good! What about

if (majoritySize > 0) {
  assert(measurements > 0, "majority cannot be empty")
}

To me it takes two extra steps to realize why the assertion message is correct, even though we don't compare the values directly.

[bajtos]

Sounds good! What about

if (majoritySize > 0) {
  assert(measurements > 0, "majority cannot be empty")
}

That's not what we are asserting.

measurements = majoritySize + sumOf(minoritySizes)

[bajtos]

It is the same. I find the assertion message you proposed misleading.

We are not asserting that the majority cannot be empty - we already know that inside the if (majoritySize > 0) branch.

We are asserting that the invariants hold, i.e. majoritySize <= measurements.

Maybe this will be easier to reason about if we rename measurement to committeeSize. Then your proposal says this:

assert(committeeSize > 0, "majority cannot be empty")

Do you see now why I find that misleading?

[juliangruber]

I see! To me we don't need to rename the variables, only change the assertion message.

With this one I got confused:

if (majoritySize > 0) {
  assert(measurements > 0, "majority cannot be larger than the committee size")
  // calculation
}

We're not actually testing whether one is bigger than the other, so it relies on another invariant.

What about we make this as simple as possible:

if (majoritySize > 0) {
  assert(measurements > 0, "if there is a majority, there have to be measurements")
  // calculation
}

[bajtos]

@juliangruber would you like me to apply the following change as well?

I can change majoritySize to number | undefined so that committees evaluated to COMMITTEE_TOO_SMALL and MAJORITY_NOT_FOUND will get majoritySize === undefined.

[juliangruber]

Sgtm, but I don't have a strong opinion on it

[bajtos]

See d5c8883

[bajtos]

I added three new changes:

(1)

if (majoritySize > 0) {
  assert(measurements > 0, "if there is a majority, there have to be measurements")
  // calculation
}

(2)
Renamed majoritySizes to majorityToCommitteeRatios

(3)
Renamed the InfluxDB field prefix from majority_sizes_percents_ to majority_ratios_percents_

@juliangruber LGTY now?

[juliangruber]

Cool! Just #347 (comment) from my side

[bajtos]

@juliangruber PTAL again

[bajtos]