Add height voting for chits #2102

StephenButtolph · 2023-09-27T04:58:38Z

Why this should be merged

Reduces the consensus dependency on vote bubbling by explicitly voting for the next block to accept.

How this works

Introduces a new field to PullQuery and PushQuery messages RequestedHeight and correspondingly adds PreferredIDByHeight to Chits messages.

When a node issues a query - it will request the block at height +1 to its last accepted block. When a virtuous node responds, this means that the requesting node can always fall back to apply the vote on top of its last accepted block. This is important because this converts bubbling of votes from a liveness requirement to a performance optimization. This significantly simplifies the requirements around transitive voting.

How this was tested

CI

Co-authored-by: Dan Laine <daniel.laine@avalabs.org> Signed-off-by: Stephen Buttolph <stephen@avalabs.org>

StephenButtolph · 2023-10-03T20:16:12Z

snow/engine/snowman/transitive.go

This file is the most important file to have 👀 on

dboehm-avalabs · 2023-10-04T16:57:39Z

snow/engine/snowman/transitive.go

+				zap.Stringer("lastAcceptedID", lastAcceptedID),
+				zap.Error(err),
+			)
+			acceptedAtHeight = lastAcceptedID


What are the implications of responding with an ID that is not actually the accepted ID at that height? My understanding is that this occurs when the current node does not have any accepted id at height because it has not sync'd to that height yet. Would it make more sense to return some sort of empty ID or does the response effectively not matter in this case?

And similar question for below where it seems like you can return an ID for a height after the requested height.

height because it has not sync'd to that height yet.

this may also occur if the height is pruned or it was never fetched (bc state synced)

Generally people should only be requesting heights "close" to tip here. This case specifically handles the case where it is only safe to return Accepted state. It would also be safe to simply drop the requests here. But we do this to avoid causing the peer to hit a timeout.

It's safe for us to return any Accepted blocks here - but the most "useful" blocks to send are the lastAcceptedID + acceptedIDAtRequestedHeight. If we don't have access to the acceptedID at the requested height, we can return any accepted ID - which we just default to the last accepted ID.

We could probably reduce our bandwidth a bit here by changing the proto format to have some logic for "inheriting" fields. In this code path we return 3 32-byte hashes but we could get away with sending 1-2 32-bytes hashes.

This feels like a follow-up though haha

Below - it is safe to return any preferred value.

gyuho

just nits on docs, otherwise, lgtm

message/outbound_msg_builder.go

snow/networking/sender/sender.go

gyuho · 2023-10-04T16:53:55Z

snow/engine/snowman/transitive.go

+	}
+
+	// Will record chits once [preferredID] and [preferredIDAtHeight] have been
+	// issued into consensus


So, even if we failed here https://github.com/ava-labs/avalanchego/pull/2102/files#diff-1b154e2d9b5b0909a66199417df6ff79ee9a18e83075b39ef2acf3634558342eR724-R736 (non-populated preferred id at height set to preferred id), this is safe? can we doc regarding v1.11 upgrade?

The invariants for a correct node is that preferredID transitively references both preferredIDAtHeight and acceptedID. So it would be safe to default preferredIDAtHeight to either preferredID or acceptedID in the handler

snow/engine/snowman/transitive.go

patrick-ogrady · 2023-10-04T19:30:12Z

message/inbound_msg_builder.go

+			ChainId:             chainID[:],
+			RequestId:           requestID,
+			PreferredId:         preferredID[:],
+			PreferredIdAtHeight: preferredIDAtHeight[:],


This also allows us to apply votes on a failed chit, right? Or am I too optimistic?

We use the AcceptedID for failed chits. I don't think it would be safe to re-use the PreferredIDAtHeight in the same way unless we included an additional boolean (to say if PreferredIDAtHeight was accepted).

patrick-ogrady · 2023-10-04T19:31:42Z

proto/p2p/p2p.proto

@@ -303,6 +303,7 @@ message PushQuery {
  uint64 deadline = 3;
  bytes container = 4;
  EngineType engine_type = 5;
+  uint64 requested_height = 6;


Should we populate requestedHeight last in the messages above then?

The order doesn't actually matter... I felt like the code flowed more naturally with the order I used... The only reason the proto uses this order is because it is required for compatibility

snow/engine/snowman/transitive.go

patrick-ogrady · 2023-10-04T19:34:16Z

snow/engine/snowman/transitive.go

@@ -293,7 +307,7 @@ func (t *Transitive) Chits(ctx context.Context, nodeID ids.NodeID, requestID uin
 func (t *Transitive) QueryFailed(ctx context.Context, nodeID ids.NodeID, requestID uint32) error {
 	lastAccepted, ok := t.acceptedFrontiers.LastAccepted(nodeID)
 	if ok {
-		return t.Chits(ctx, nodeID, requestID, lastAccepted, lastAccepted)
+		return t.Chits(ctx, nodeID, requestID, lastAccepted, lastAccepted, lastAccepted)


snow/engine/snowman/transitive.go

patrick-ogrady · 2023-10-04T19:37:41Z

snow/engine/snowman/transitive.go

 	} else {
-		t.Sender.SendChits(ctx, nodeID, requestID, t.Consensus.Preference(), lastAccepted)
+		var ok bool
+		preferenceAtHeight, ok = t.Consensus.PreferenceAtHeight(requestedHeight)


should the above check be <= last accepted? last accepted is no longer considered processing, right?

The last accepted block is still tracked by the consensus instance (and is faster to access than asking the VM). This is documented on the PreferenceAtHeight method

patrick-ogrady · 2023-10-04T19:39:34Z

snow/networking/handler/handler.go

+				zap.String("field", "PreferredIDAtHeight"),
+				zap.Error(err),
+			)
+			// TODO: Require this field to be populated correctly after v1.11.x


Can we add an issue for tracking this?

Added to #1965

patrick-ogrady · 2023-10-04T19:45:30Z

snow/engine/snowman/transitive.go

+			)
+			// If the requested height is higher than our preferred tip, we
+			// don't prefer anything at the requested height yet.
+			preferenceAtHeight = preference


should we be responding with something empty here?

will this require us to go try and fetch this height? based on the logic, as far as I understand it, we shouldn't necessarily go fetch nextHeightToAccept as it may not exist yet.

In this case - the peer is going to drop whatever we give them, as they are further ahead than we are (anything we vote for here is already accepted by the peer). So all we really care about is
a) Respond so that they don't hit a timeout
a) Don't give a byzantine response (aka, something we don't prefer)

No - we will not attempt to fetch anything based on this logic

patrick-ogrady · 2023-10-04T21:36:33Z

snow/engine/snowman/transitive.go

+			// Because we only return accepted state here, it's fairly likely
+			// that the requested height is higher than the last accepted block.
+			// That means that this code path is actually quite common.
+			t.Ctx.Log.Debug("failed fetching accepted block",


Should we added the accepted block miss metric here too? or do we consider this a different scenario?

I felt like this was different. This case is generally expected to be triggered... whereas the other case isn't. We could add another metric for this case specifically (and the other cases). But I'm not sure how much we really care about them.

gyuho

lgtm

StephenButtolph and others added 6 commits September 25, 2023 21:51

Move vote bubble before poll termination

9d70593

merged

53fffe1

Update snow/engine/snowman/voter.go

d9227fc

Co-authored-by: Dan Laine <daniel.laine@avalabs.org> Signed-off-by: Stephen Buttolph <stephen@avalabs.org>

Merge branch 'early-bubble-votes' into bubble-vote-messaging

400255c

Add height voting for chits

cc6905a

Merge branch 'dev' into early-bubble-votes

80fef0a

StephenButtolph added consensus This involves consensus networking This involves networking bubble votes labels Sep 27, 2023

StephenButtolph added this to the v1.10.12 milestone Sep 27, 2023

Merge branch 'early-bubble-votes' into bubble-vote-messaging

95cf59f

Base automatically changed from early-bubble-votes to dev September 28, 2023 21:56

StephenButtolph mentioned this pull request Oct 3, 2023

Standardize consensus variable names in tests #2129

Merged

StephenButtolph added 7 commits October 3, 2023 14:45

merged

c5cdc20

merged

a9fc1ff

nit

54e0e11

logging additions

178882b

nit

f12b555

add comment

fbb3817

add log

36b3879

StephenButtolph commented Oct 3, 2023

View reviewed changes

Merge branch 'dev' into bubble-vote-messaging

155eec9

StephenButtolph marked this pull request as ready for review October 3, 2023 20:16

StephenButtolph requested review from danlaine, gyuho and hexfusion as code owners October 3, 2023 20:16

StephenButtolph self-assigned this Oct 3, 2023

dboehm-avalabs reviewed Oct 4, 2023

View reviewed changes

gyuho reviewed Oct 4, 2023

View reviewed changes

patrick-ogrady reviewed Oct 4, 2023

View reviewed changes

snow/engine/snowman/transitive.go Outdated Show resolved Hide resolved

patrick-ogrady reviewed Oct 4, 2023

View reviewed changes

snow/engine/snowman/transitive.go Show resolved Hide resolved

patrick-ogrady reviewed Oct 4, 2023

View reviewed changes

StephenButtolph mentioned this pull request Oct 4, 2023

v1.11.x Cleanups #1965

Closed

11 tasks

StephenButtolph added 3 commits October 4, 2023 16:41

Address PR review comments

5f1005e

Add invariant comment

157a4e8

Merge branch 'dev' into bubble-vote-messaging

061d231

patrick-ogrady reviewed Oct 4, 2023

View reviewed changes

StephenButtolph added 2 commits October 4, 2023 19:17

merged

755e374

nit

bdf293c

StephenButtolph requested review from joshua-kim and aaronbuchwald October 5, 2023 02:19

gyuho approved these changes Oct 5, 2023

View reviewed changes

joshua-kim approved these changes Oct 5, 2023

View reviewed changes

StephenButtolph added 2 commits October 5, 2023 14:13

add metric

83fd55b

Merge branch 'dev' into bubble-vote-messaging

a97e487

StephenButtolph enabled auto-merge October 5, 2023 19:32

StephenButtolph added this pull request to the merge queue Oct 5, 2023

Merged via the queue into dev with commit 5f9bb08 Oct 5, 2023
16 checks passed

StephenButtolph deleted the bubble-vote-messaging branch October 5, 2023 20:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add height voting for chits #2102

Add height voting for chits #2102

StephenButtolph commented Sep 27, 2023 •

edited

StephenButtolph Oct 3, 2023

dboehm-avalabs Oct 4, 2023

dboehm-avalabs Oct 4, 2023

patrick-ogrady Oct 4, 2023

StephenButtolph Oct 4, 2023 •

edited

StephenButtolph Oct 4, 2023 •

edited

StephenButtolph Oct 4, 2023

gyuho left a comment

gyuho Oct 4, 2023

StephenButtolph Oct 4, 2023

patrick-ogrady Oct 4, 2023

StephenButtolph Oct 4, 2023

patrick-ogrady Oct 4, 2023

StephenButtolph Oct 4, 2023

patrick-ogrady Oct 4, 2023

patrick-ogrady Oct 4, 2023

StephenButtolph Oct 4, 2023 •

edited

patrick-ogrady Oct 4, 2023

StephenButtolph Oct 4, 2023

patrick-ogrady Oct 4, 2023

StephenButtolph Oct 4, 2023

patrick-ogrady Oct 4, 2023

StephenButtolph Oct 4, 2023

gyuho left a comment

Add height voting for chits #2102

Add height voting for chits #2102

Conversation

StephenButtolph commented Sep 27, 2023 • edited

Why this should be merged

How this works

How this was tested

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

StephenButtolph Oct 4, 2023 • edited

Choose a reason for hiding this comment

StephenButtolph Oct 4, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gyuho left a 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

StephenButtolph Oct 4, 2023 • edited

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

gyuho left a comment

Choose a reason for hiding this comment

StephenButtolph commented Sep 27, 2023 •

edited

StephenButtolph Oct 4, 2023 •

edited

StephenButtolph Oct 4, 2023 •

edited

StephenButtolph Oct 4, 2023 •

edited