Implement traversal based early termination #3337

yacovm · 2024-08-27T16:09:03Z

This commit makes the early termination logic of the snowflake poll consider also transitive voting. A block b0 is transitively voted by all votes for block b1 if b0 is an ancestor of b1.

This transitive termination logic also works for shard common block prefixes. If block b0 is a direct ancestor of b1 and b2 and the latter have a shared prefix, then the logic in this commit may not early terminate the vote if a future vote might improve the confidence of the shared prefix, as it correspons to a snowflake instance.

Note: I will rename the file name early_term_no_traversal.go after this is merged. I didn't rename them in the same commit so that it will be easy to distinguish between old code and new code.

Why this should be merged

Considering transitive votes in early termination logic improves consensus performance by reducing the time to block finalization due to earlier termination than when not taking into account transitive voting.

How this works

When the early termination logic is asked whether to terminate or not, it creates a graph out of the votes, where each ID of a block corresponds to a vertex. Parent vertices point to their descendants and descendants to their parents.
When considering how many votes for a block ID exist, the direct vote as well as the votes for the descendants are now taken int account.

Additionally, in order to adhere to the same logic that snowman employs for the bit decomposition of blocks, the termination logic also takes into account votes for shared bit prefixes of block IDs. For each vertex, its direct descendants are observed, and a graph of shared prefixes is built. Every inner vertex in the graph that corresponds to a unary node that represents a shared prefix is also taken account in transitive voting.

How this was tested

Unit tests for added code are included.

I ran a modified build on mainnet for 24 hours alongside a regular build and observed that transitive voting reduces occurrence of consensus rounds lasting more than 1200 milliseconds from 14% to 1.5% on mainnet.

The following graphs show the latency of consensus rounds as a function of time on mainnet.
Without transitive voting:

With transitive voting:

marun

How confident are you that the limited mainnet testing performed thus far represents the expected behavior when the feature is deployed to a majority of nodes?
The description only mentions the advantages of this change. Any downsides or tradeoffs? Presumably there is more computational cost, but hopefully that would be a worthy tradeoff.

snow/consensus/snowman/poll/early_term_no_traversal.go

snow/consensus/snowman/poll/graph.go

snow/consensus/snowman/poll/early_term_no_traversal.go

snow/consensus/snowman/poll/graph_test.go

snow/consensus/snowman/poll/early_term_no_traversal.go

snow/consensus/snowman/poll/prefix.go

yacovm · 2024-09-27T14:35:25Z

How confident are you that the limited mainnet testing performed thus far represents the expected behavior when the feature is deployed to a majority of nodes?

The unit tests I added represent much more extreme cases than what happens on mainnet. On mainnet, the block depth this entire code operates on, is either 1 or 2 blocks deep. So in that aspect I'm confident.

However before we deploy to mainnet, we will deploy to testnet, and the testnet behavior is not that different than mainnet, from what I've seen.

The description only mentions the advantages of this change. Any downsides or tradeoffs? Presumably there is more computational cost, but hopefully that would be a worthy tradeoff.

The rest of the snowman code already performs transitive voting. I'm only completing the last piece that is needed. Without this code, we terminate the poll longer than necessary and needlessly slow down consensus. So even if there are downsides, it makes no sense to withhold this, as it was always the intent to implement this.

snow/consensus/snowman/poll/graph.go

snow/consensus/snowman/poll/prefix.go

chains/manager.go

aaronbuchwald · 2024-10-22T20:35:36Z

snow/context.go

+type BlockTraversal interface {
+	GetParent(id ids.ID) ids.ID
+}


I'm not sure this fits here, but also not sure if there's a better place to put it since we need to supply an implementation from the snowman engine to the snowman consensus instance

aaronbuchwald · 2024-10-22T20:39:05Z

snow/engine/snowman/config.go

@@ -16,7 +16,7 @@ import (
 // Config wraps all the parameters needed for a snowman engine
 type Config struct {
 	common.AllGetsServer
-
+	BlockTraversal      snow.BlockTraversal


Should we set this field in TestBootstrapPartiallyAccepted as well to make sure the new field is populated everywhere it's used?

I'm not sure I follow. Since I don't set this field, and the test passes, why set it if it's apparently not tested?

aaronbuchwald · 2024-10-22T20:41:47Z

snow/consensus/snowman/topological.go

+		if block.blk == nil {
+			return ids.Empty
+		}
+		return block.blk.Parent()


Can we skip block.blk == nil? We should have the invariant that block.blk is never nil since we only assign to it in one place and always with a value that we've already called Parent() on

Could we switch from using ids.Empty to signify that we don't have the block to returning an extra boolean?

Can we skip block.blk == nil? We should have the invariant that block.blk is never nil since we only assign to it in one place and always with a value that we've already called Parent() on

We have the invariant, however, what if we won't have it? I think it's better to be safe than sorry and just keep this check for safety to avoid crashing the node.

Could we switch from using ids.Empty to signify that we don't have the block to returning an extra boolean?

Yes I switched.

aaronbuchwald · 2024-10-23T01:43:57Z

snow/consensus/snowman/poll/prefix.go

+	return descendant
+}
+
+// prefixGroup represents a bunch of IDs (stored in the members field),


Should the type definition be at the top of the file rather than here? Reading through this code I see a new type in the first function and jump to here to see what it is.

Sure, moved it to the top of the file.

aaronbuchwald · 2024-10-23T01:58:10Z

snow/consensus/snowman/poll/prefix.go

+// bifurcationsWithCommonPrefix invokes f() on this and descendant prefixGroups
+// which represent common prefixes and not an ID in its entirety.


how does this comment match with the actual condition that's enforced?

longestSharedPrefixes returns a tree where each vertex is either the head, has a bifurcation, or is a leaf.

len(prefixGroup.prefix) > 0 filters out the leaves.
if prefixGroup.isBifurcation() filters out the head.

aaronbuchwald · 2024-10-23T02:02:11Z

snow/consensus/snowman/poll/prefix.go

+	return originPG
+}
+
+func determineDescendant(pg *prefixGroup) *prefixGroup {


Would it be cleaner to remove this function and handle this within a switch statement that handles the existing case pg0 != nil && pg1 != nil and then handles the case that either one of them is nil to determine the descendant?

It will be cleaner code wise, but I want to emphasize the different semantics here - the case where both are not nil is a bifurcation, and the complement of that is that we extend the current prefix as you correctly pointed out. These two cases are not the same and therefore to make it clearer in the code, I did not put them in a single switch statement.

If you feel strongly about it, I can collapse them in a switch statement, but I thought it would be more clear to the reader this way.

snow/consensus/snowman/poll/prefix.go

aaronbuchwald · 2024-10-23T02:58:55Z

snow/consensus/snowman/poll/early_term_no_traversal_test.go

+type parentGetter func(id ids.ID) ids.ID
+
+func (p parentGetter) GetParent(id ids.ID) ids.ID {
+	return p(id)
+}


Ah I had not seen a function turned directly into a single function interface this way before, nice

aaronbuchwald · 2024-10-23T12:31:29Z

snow/consensus/snowman/poll/early_term_no_traversal_test.go

+func returnSelfID(id ids.ID) ids.ID {
+	return id
+}


with the current handling of ids.Empty would it make more sense to returns ids.Empty as a default value rather than returning self as a parent, which could result in a loop?

This commit makes the early termination logic of the snowflake poll consider also transitive voting. A block `b0` is transitively voted by all votes for block `b1` if `b0` is an ancestor of `b1`. This transitive termination logic also works for shard common block prefixes. If block `b0` is a direct ancestor of `b1` and `b2` and the latter have a shared prefix, then the logic in this commit may not early terminate the vote if a future vote might improve the confidence of the shared prefix, as it correspons to a snowflake instance. Signed-off-by: Yacov Manevich <[email protected]>

Signed-off-by: Yacov Manevich <[email protected]>

yacovm requested a review from StephenButtolph as a code owner August 27, 2024 16:09

yacovm self-assigned this Aug 27, 2024

yacovm force-pushed the earlyterm_yes_traversal branch 3 times, most recently from ccfaac5 to 7b6f256 Compare August 27, 2024 16:37

yacovm added the consensus This involves consensus label Aug 27, 2024

yacovm force-pushed the earlyterm_yes_traversal branch from ff5a4d6 to 68e69aa Compare September 19, 2024 19:12

michaelkaplan13 requested review from aaronbuchwald, marun, joshua-kim and tsachiherman and removed request for marun September 24, 2024 16:13

marun reviewed Sep 27, 2024

View reviewed changes

yacovm force-pushed the earlyterm_yes_traversal branch from 70fb2dc to 6ff8140 Compare September 27, 2024 16:18

marun reviewed Sep 27, 2024

View reviewed changes

snow/consensus/snowman/poll/graph.go Outdated Show resolved Hide resolved

marun reviewed Sep 27, 2024

View reviewed changes

snow/consensus/snowman/poll/prefix.go Outdated Show resolved Hide resolved

marun reviewed Sep 27, 2024

View reviewed changes

snow/consensus/snowman/poll/prefix.go Outdated Show resolved Hide resolved

marun approved these changes Sep 28, 2024

View reviewed changes

aaronbuchwald reviewed Oct 23, 2024

View reviewed changes

yacovm force-pushed the earlyterm_yes_traversal branch 6 times, most recently from b94bb26 to ea2813c Compare October 23, 2024 22:59

yacovm added 3 commits November 15, 2024 16:41

Address code review comments

8276ebe

Signed-off-by: Yacov Manevich <[email protected]>

Address code review comments II

6596f15

Signed-off-by: Yacov Manevich <[email protected]>

yacovm added 4 commits November 15, 2024 16:41

Address code review comments III

b327508

Signed-off-by: Yacov Manevich <[email protected]>

Address code review comments IV

1435a73

Signed-off-by: Yacov Manevich <[email protected]>

document TestSharedPrefixes test fields

4dc167e

Signed-off-by: Yacov Manevich <[email protected]>

Simplify test, and use local max votes instead of global

99deea8

Signed-off-by: Yacov Manevich <[email protected]>

yacovm force-pushed the earlyterm_yes_traversal branch from 87784e6 to 99deea8 Compare November 15, 2024 19:12

Simplify test

be96b36

Signed-off-by: Yacov Manevich <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement traversal based early termination #3337

Implement traversal based early termination #3337

yacovm commented Aug 27, 2024

marun left a comment

yacovm commented Sep 27, 2024

aaronbuchwald Oct 22, 2024

aaronbuchwald Oct 22, 2024

yacovm Oct 23, 2024

aaronbuchwald Oct 22, 2024

aaronbuchwald Oct 22, 2024

yacovm Oct 23, 2024

aaronbuchwald Oct 23, 2024

yacovm Oct 23, 2024

aaronbuchwald Oct 23, 2024

yacovm Oct 23, 2024

aaronbuchwald Oct 23, 2024

yacovm Oct 23, 2024

aaronbuchwald Oct 23, 2024

aaronbuchwald Oct 23, 2024 •

edited

Loading

yacovm Oct 23, 2024

		// bifurcationsWithCommonPrefix invokes f() on this and descendant prefixGroups
		// which represent common prefixes and not an ID in its entirety.

Implement traversal based early termination #3337

Are you sure you want to change the base?

Implement traversal based early termination #3337

Conversation

yacovm commented Aug 27, 2024

Why this should be merged

How this works

How this was tested

marun left a comment

Choose a reason for hiding this comment

yacovm commented Sep 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronbuchwald Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronbuchwald Oct 23, 2024 •

edited

Loading