Skip to content

[Feature] Do not sync when peer consensus version differs#3936

Merged
vicsn merged 15 commits intostagingfrom
feat/sync-consensus-version
Oct 24, 2025
Merged

[Feature] Do not sync when peer consensus version differs#3936
vicsn merged 15 commits intostagingfrom
feat/sync-consensus-version

Conversation

@kaimast
Copy link
Copy Markdown
Contributor

@kaimast kaimast commented Oct 14, 2025

This PR fixes #3822. It adds a latest_consensus_version to a block response to prevent nodes that did not upgrade from block advancement.

If the peers use different versions of snarkOS, it will either fail to deserialize the block response, or abort when it detects that the version do not match.

Note: This requires upgrading the message version, as the format for BlockResponse changed.

@kaimast kaimast force-pushed the feat/sync-consensus-version branch from d081cc9 to 453f986 Compare October 14, 2025 20:25
@kaimast kaimast marked this pull request as ready for review October 14, 2025 22:02
@kaimast
Copy link
Copy Markdown
Contributor Author

kaimast commented Oct 14, 2025

Strangely, I had to increase the machine size for another CI job. Some recent change caused a lot of our builds to take longer or use up more memory.

@kaimast kaimast requested review from ljedrz and vicsn and removed request for vicsn October 15, 2025 04:24
Copy link
Copy Markdown
Collaborator

@vicsn vicsn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Core logic LGTM! Just a note on backwards compatibility

Comment thread node/bft/events/src/block_response.rs Outdated
Comment thread node/router/messages/src/block_response.rs Outdated
Comment thread node/bft/src/helpers/channels.rs Outdated
Copy link
Copy Markdown
Collaborator

@ljedrz ljedrz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same point as @vicsn made; we should be able to make it backwards compatible, and only make the new field a hard requirement after a while.

This commit also deduplicates block request/response code between router and gateway, and improves testing.
@kaimast
Copy link
Copy Markdown
Contributor Author

kaimast commented Oct 16, 2025

Thanks for the feedback! I changed the PR to now interpret the start_height of the block request as a version number. For old messages, it is never zero, as nodes do not request the genesis block from other nodes.
For newer messages, it is set to zero for now. In the future, we can use the first byte(s) to encode an actual version.

To make this change simpler, snarkos_node_router_messages now re-export the BlockResponse and BlockRequest types from snarkos_node_bft_events, instead of re-implementing them.

Finally, I needed to make changes to the prop tests for these two message types because they didn't actually generate valid block requests or responses. For example, it picked two random numbers as start and end height, so that the ranges could be invalid. The number of blocks in the response also was not guaranteed to be equal to the size of the request range.

Comment thread node/bft/events/src/block_response.rs Outdated
Comment thread node/router/messages/src/block_request.rs
Comment thread node/router/messages/src/block_request.rs
ljedrz
ljedrz previously approved these changes Oct 16, 2025
Copy link
Copy Markdown
Collaborator

@ljedrz ljedrz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some nits, but nothing blocking; LGTM 👌.

Copy link
Copy Markdown
Collaborator

@vicsn vicsn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx! We're almsot there!

Comment thread node/bft/events/src/block_response.rs Outdated
Comment thread node/router/src/inbound.rs Outdated
Comment thread node/router/messages/src/block_request.rs
Comment thread node/src/client/router.rs Outdated
Comment thread node/src/validator/router.rs Outdated
Comment thread node/sync/src/block_sync.rs Outdated
Comment thread node/bft/events/src/block_response.rs Outdated
@kaimast
Copy link
Copy Markdown
Contributor Author

kaimast commented Oct 24, 2025

I tested this finally, and you will get an output like the following starting at consensus V12 for nodes that have not upgraded:

2025-10-24T00:19:22.875901Z  WARN snarkvm_utilities::errors: Failed to insert block response — The peer did not send a consensus version
2025-10-24T00:19:22.875950Z  WARN snarkos_node::client::router: Failed to process inbound message from '127.0.0.1:4130' - Peer '127.0.0.1:4130' sent an invalid block response

Before merging/approving: take a look at the most recent change in ddb841b, which ensures messages are always rejected if the local consensus version would be greater than 12.
I believe this is the behavior we want but please double check.

@kaimast kaimast requested a review from vicsn October 24, 2025 00:27
@vicsn
Copy link
Copy Markdown
Collaborator

vicsn commented Oct 24, 2025

@kaimast thank you, this is the behaviour we want:

  • before V12, old and new nodes can sync from each other
  • after V12 old and new nodes can't sync from each other anymore due to the serialization mismatch

Please confirm

@kaimast
Copy link
Copy Markdown
Contributor Author

kaimast commented Oct 24, 2025

@kaimast thank you, this is the behaviour we want:

* before V12, old and new nodes can sync from each other

* after V12 old and new nodes can't sync from each other anymore due to the serialization mismatch

Please confirm

Yes. That's the behavior.

vicsn
vicsn previously approved these changes Oct 24, 2025
Comment thread node/bft/events/src/block_response.rs Outdated
@vicsn vicsn merged commit 0ff8f1b into staging Oct 24, 2025
2 of 5 checks passed
@vicsn vicsn deleted the feat/sync-consensus-version branch October 24, 2025 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Prevent network from advancing on fork

4 participants