Skip to content

Fix QBFT identifier length check#539

Merged
GalRogozinski merged 6 commits into
mainfrom
fix-qbft-id-len-check-nofork
Jun 12, 2025
Merged

Fix QBFT identifier length check#539
GalRogozinski merged 6 commits into
mainfrom
fix-qbft-id-len-check-nofork

Conversation

@MatheusFranco99
Copy link
Copy Markdown
Contributor

@MatheusFranco99 MatheusFranco99 commented May 6, 2025

Overview

This PR adds a check for the QBFT Identifier field to ensure it has length 56, and adjusts the tests accordingly.

This is a non-fork version of the better refactoring: #536

Closes #533

alan-ssvlabs
alan-ssvlabs previously approved these changes May 7, 2025
Copy link
Copy Markdown
Contributor

@alan-ssvlabs alan-ssvlabs left a comment

Choose a reason for hiding this comment

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

LGTM
Depends on #537 right?

@MatheusFranco99
Copy link
Copy Markdown
Contributor Author

Depends on #537 right?

I believe not, as this shouldn't affect the round validation

GalRogozinski
GalRogozinski previously approved these changes May 12, 2025
Copy link
Copy Markdown
Contributor

@GalRogozinski GalRogozinski left a comment

Choose a reason for hiding this comment

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

Good job!

y0sher
y0sher previously approved these changes May 27, 2025
Copy link
Copy Markdown
Contributor

@y0sher y0sher left a comment

Choose a reason for hiding this comment

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

lgtm

@MatusKysel MatusKysel requested a review from Copilot May 27, 2025 08:09

This comment was marked as outdated.

MatusKysel
MatusKysel previously approved these changes May 27, 2025
AgeManning
AgeManning previously approved these changes May 29, 2025
Copy link
Copy Markdown

@AgeManning AgeManning left a comment

Choose a reason for hiding this comment

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

I have approved

@MatheusFranco99 MatheusFranco99 force-pushed the fix-qbft-id-len-check-nofork branch from ba397e5 to 83b0dcb Compare June 11, 2025 10:02
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes the QBFT identifier length check by ensuring that the "Identifier" (and similarly, the "ID") fields in test JSON files are updated to the expected 56‐character value rather than the old, shorter encoded string. Key changes include updating all relevant test files to replace "AQIDBA==" with a new 56‐character literal and adjusting associated test expectations.

Reviewed Changes

Copilot reviewed 417 out of 417 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
qbft/controller decide wrong msg type.json Updated identifier and ID values to the new 56‐character format
qbft/controller decide unknown signer.json Updated identifier and ID values to the new 56‐character format
qbft/controller decide past instance.json Updated identifier and ID and adjusted nested message fields accordingly
qbft/controller decide no quorum.json Updated identifier and ID values to the new 56‐character format
qbft/controller decide late decided.json Updated identifier and ID values as well as nested data fields
qbft/controller decide late decided smaller quorum.json Updated identifier and ID values accordingly
qbft/controller decide late decided bigger quorum.json Updated identifier and ID values accordingly
qbft/controller decide invalid value (should pass).json Updated identifier and ID values accordingly
qbft/controller decide invalid msg.json Updated identifier and ID values accordingly
qbft/controller decide invalid full data.json Updated identifier and ID values accordingly
qbft/controller decide has quorum.json Updated identifier and ID values accordingly
qbft/controller decide future instance.json Updated identifier and ID values accordingly
qbft/controller decide duplicate signer.json Updated identifier and ID values accordingly
qbft/controller decide duplicate msg.json Updated identifier and ID values accordingly
qbft/controller decide current instance.json Updated identifier and ID values accordingly
Comments suppressed due to low confidence (1)

qbft/spectest/generate/state_comparison/tests_ControllerSpecTest/qbft controller decide wrong msg type.json:3

  • Verify that the new identifier string exactly meets the QBFT specification requirements (i.e. the correct length and encoding) to avoid potential mismatches in production.
"Identifier": "AQIDBAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=",

@GalRogozinski GalRogozinski merged commit 4faf15c into main Jun 12, 2025
2 checks passed
@GalRogozinski GalRogozinski deleted the fix-qbft-id-len-check-nofork branch June 12, 2025 08:10
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.

Invalid QBFT identifiers in tests

7 participants