Add a protocol version to consensus messages by HagarMeir · Pull Request #757 · hyperledger/SmartBFT

HagarMeir · 2026-11-05T13:02:08Z

Written by Claude (Anthropic's AI assistant) on behalf of Hagar Meir.

Fixes #756.

Message gets a version field.
Controller.SendConsensus and Controller.BroadcastConsensus send every consensus message with types.ProtocolVersion, which is 1. Every component sends through the controller, so this covers all of them. A message may be shared, for example when it is resent, so it isn't changed in place: it gets a new envelope that shares the content, built once for a broadcast.
Controller.ProcessMessages ignores messages outside types.MinSupportedProtocolVersion (0) to types.ProtocolVersion, with a warning, once per sender and version, that nodes of different protocol versions must not run in the same cluster.
A message without the field has version 0 and is still accepted, so nodes with and without this change keep working together.
No signature covers the whole Message, so adding the field doesn't affect any signature.

Tests:

TestSupportedProtocolVersion checks the accepted range.
TestControllerMessageVersion checks that sent and broadcast messages carry the version without changing the message passed in or copying its content, that versions in the range are processed and others are not, and that the warning is logged once per sender and version.
TestMessagesCarryProtocolVersion runs a 4-node cluster through decisions, heartbeats and a view change, and checks that every consensus message on the wire carries the version, whichever component sends it.
TestMixedProtocolVersions simulates nodes of an earlier release, which send no version, and a node of a later version, whose messages are ignored.

🤖 Generated with Claude Code

Consensus messages carried no protocol version, so a node could not tell whether a message came from a node that speaks the same protocol. Add a version field to Message. The controller sends every consensus message with ProtocolVersion, which is 1, and ignores messages of a later version with a warning. A message without the field has version 0 and is still accepted, so nodes with and without this change keep working together. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Hagar Meir <hagar.meir@ibm.com>

tock-ibm · 2026-11-05T13:34:57Z

+	"github.com/stretchr/testify/require"
+)
+
+// TestMessagesCarryProtocolVersion makes sure that every consensus message a node sends carries its protocol version,


How do you test that a node running this commit (with version) operates correctly with a node running with an earlier commit?
That is, backwards compatibility?
It is pretty straightforward to reason that it should, but is it tested?

Added TestMixedProtocolVersions. One follower, the leader, or half of the nodes send without a version, as an earlier release does. The cluster keeps deciding, including after a view change. (Drafted with Claude's help.)

tock-ibm · 2026-11-05T14:16:44Z

 // ProcessMessages dispatches the incoming message to the required component
 func (c *Controller) ProcessMessages(sender uint64, m *protos.Message) {
 	c.Logger.Debugf("%d got message from %d: %s", c.ID, sender, MsgToString(m))
+	if m.GetVersion() > ProtocolVersion {


The version check has no lower bound, so a newer node still processes an older node's messages. ProcessMessages only rejects version > ProtocolVersion. Suppose a later release bumps ProtocolVersion to 2 because the protocol changed incompatibly. A v2 node still accepts and processes v0/v1 messages, and reads messages of the old protocol under the new rules. #756 says exactly this case should be ignored, but only the older node is protected.

Consider accepting a range instead: add a MinSupportedProtocolVersion (0 today, so nodes without versioning still work) and reject version < min || version > ProtocolVersion. A future bump then only has to raise the minimum.

Done. Added types.MinSupportedProtocolVersion (0 for now), and messages outside [MinSupportedProtocolVersion, ProtocolVersion] are ignored. (Drafted with Claude's help.)

tock-ibm · 2026-11-05T14:16:46Z


+// ProtocolVersion is the version of the consensus protocol this node speaks.
+// Messages without a version come from nodes that predate versioning, and have version 0.
+const ProtocolVersion uint64 = 1


ProtocolVersion is one compile-time constant, used both for what a node sends and for what it accepts. Because of that, a future bump cannot be rolled out one node at a time. Take a 4-node cluster (f=1, quorum 3) moving to a v2 binary. Once two nodes run v2, the two v1 nodes drop every message from them: heartbeats, pre-prepares, prepares, commits and view changes. The v1 nodes see only each other and cannot reach quorum. The v2 nodes cannot reach quorum either, because the v1 nodes never vote on what they dropped. The cluster makes no progress until every node is upgraded, so the only safe upgrade is to stop the whole cluster. The 0 -> 1 step works only because v1 accepts version 0.

If one-node-at-a-time upgrades should keep working, a node could take the version it sends from configuration: it keeps sending the old version until every node runs the new binary, then switches. Alternatively, the accepted range could be made configurable.

Left as is. The version only goes up for a change that isn't compatible with the earlier protocol. Nodes on both sides of such a change shouldn't run in the same cluster, so that upgrade can't be done one node at a time anyway. Compatible changes keep the version and roll out node by node as today. (Drafted with Claude's help.)

tock-ibm · 2026-11-05T14:16:47Z

+	versioned := proto.Clone(m).(*protos.Message)
+	versioned.Version = ProtocolVersion
+	return versioned


proto.Clone deep-copies the whole message on every send, proposal payload included. The leader's pre-prepare carries the whole batch (RequestBatchMaxBytes defaults to 10 MiB), and a new-view message carries every signed view data. Each broadcast now allocates a full extra copy of the batch on the proposing path. Only the envelope needs to be new. The content can be shared, as it was before this change when the original message went to Comm directly. With this change TestController* still pass (checked locally), and it also removes the nil panic mentioned above:

Suggested change

versioned := proto.Clone(m).(*protos.Message)

versioned.Version = ProtocolVersion

return versioned

return &protos.Message{Content: m.GetContent(), Version: ProtocolVersion}

Done. Only the envelope is new, and the content is shared. A broadcast builds the envelope once for all nodes. (Drafted with Claude's help.)

tock-ibm · 2026-11-05T14:16:49Z


+// withProtocolVersion returns the message with this node's protocol version.
+// The message may be shared, for example when it is resent, so it is copied rather than modified in place.
+func withProtocolVersion(m *protos.Message) *protos.Message {


A nil message now panics. m.GetVersion() is nil-safe, but proto.Clone(m) is not, because m is a typed nil pointer inside a non-nil interface. Locally, withProtocolVersion((*protos.Message)(nil)) panics with a nil pointer dereference. Before this change a nil message went to Comm.SendConsensus, which decided what to do with it. Now it crashes whichever goroutine sent it: the view, the view changer or the heartbeat monitor.

I did not find a caller that passes nil today. View.doPhase broadcasts v.lastBroadcastSent, which every PROPOSED/PREPARED path sets. So this is a robustness concern, not a live crash.

Fixed by the new envelope, which uses m.GetContent(). A test now sends nil. (Drafted with Claude's help.)

tock-ibm · 2026-11-05T14:16:51Z

 	c.Logger.Debugf("%d got message from %d: %s", c.ID, sender, MsgToString(m))
+	if m.GetVersion() > ProtocolVersion {
+		// a later version of the protocol is not compatible with this one, so its messages are not processed
+		c.Logger.Warnf("%d got message from %d of protocol version %d, but supports up to version %d, ignoring it; "+


Every dropped message logs a warning, with no rate limit. A node of a later version sends a heartbeat every LeaderHeartbeatTimeout/LeaderHeartbeatCount, plus prepares, commits, view changes and so on. Each one logs this warning, so a mismatched peer floods the log on every receiver for as long as it runs. Consider warning once per sender (or per sender and version) and logging later drops at debug level.

Done. The warning is logged once per sender and version, and later drops at debug. (Drafted with Claude's help.)

tock-ibm · 2026-11-05T14:16:53Z

+// withProtocolVersion returns the message with this node's protocol version.
+// The message may be shared, for example when it is resent, so it is copied rather than modified in place.
+func withProtocolVersion(m *protos.Message) *protos.Message {
+	if m.GetVersion() == ProtocolVersion {


This early return is effectively never taken. Every message that reaches SendConsensus/BroadcastConsensus is built locally with version 0:

the leader's own pre-prepare (View.Propose)

prepares and commits

view-changer messages

heartbeats

state-transfer messages

commits restored from the WAL, which are saved before they are sent

No path resends a message received from another node. The branch only adds a case to reason about. With a shallow envelope (see below), building a new one every time costs almost nothing, so the check can go.

Removed. (Drafted with Claude's help.)

tock-ibm · 2026-11-05T14:16:55Z

 	NewProposer(leader, proposalSequence, viewNum, decisionsInView uint64, quorumSize int) (Proposer, Phase)
 }

+// ProtocolVersion is the version of the consensus protocol this node speaks.


ProtocolVersion lives in internal/bft, so applications cannot refer to it. An application's api.Comm now gets messages with Version set. An application or operator may want to log, expose or check the version a node speaks, for example to confirm that every node is upgraded before a later bump. There is no exported constant for that. Consider defining it in pkg/api or pkg/types and using it from here.

Moved to pkg/types, along with MinSupportedProtocolVersion and SupportedProtocolVersion. (Drafted with Claude's help.)

tock-ibm · 2026-11-05T14:16:56Z

+		})
+	}
+
+	startNodes(nodes, network)


This test runs only a fault-free round, so it does not check what its doc comment claims ("whichever component sends it"). No view change happens, so the ViewChanger's ViewChange, ViewData and NewView messages never reach the wire. Disconnecting the leader after the first delivery would force a view change and cover them.

On the mixed-version question above, the harness already supports it:

MutateSend(target, func(_ uint64, m *smartbftprotos.Message) { m.Version = 0 }) on one node simulates a node that predates versioning. The cluster should keep delivering.

m.Version = bft.ProtocolVersion + 1 simulates a newer node, whose messages the others should ignore.

The test now disconnects the leader after the first decision. It checks that pre-prepare, prepare, commit, heartbeat, view change, view data and new view messages all reach the wire with the version, using a faster heartbeat clock. Mixed versions use MutateSend, as you suggested, including a node of a later version whose messages are ignored. (Drafted with Claude's help.)

tock-ibm · 2026-11-05T14:16:58Z

+	startNodes(nodes, network)
+	nodes[0].Submit(Request{ID: "1", ClientID: "alice"})
+	for i := range numberOfNodes {
+		<-nodes[i].Delivered


Minor: this waits on Delivered without a timeout. If a decision is never delivered, the test blocks until the go test timeout (10 minutes by default) and ends in a goroutine dump instead of a clear failure. Nodes dropping each other's messages, which is what a regression in this feature would cause, is exactly that case. A select with time.After fails right away. Many existing tests use the same pattern.

Done. delivered waits up to a minute and then fails with the node's ID. (Drafted with Claude's help.)

- accept versions from MinSupportedProtocolVersion to ProtocolVersion, both now in pkg/types - send a new envelope that shares the content, instead of a deep copy - warn once per sender and version about ignored messages - test a view change, and nodes of earlier and later versions Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Hagar Meir <hagar.meir@ibm.com>

HagarMeir requested review from pfi79 and tock-ibm November 5, 2026 13:03

tock-ibm reviewed Nov 5, 2026

View reviewed changes

Conversation

HagarMeir commented Nov 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tock-ibm Nov 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

HagarMeir commented Nov 5, 2026 •

edited

Loading

tock-ibm Nov 5, 2026 •

edited

Loading