Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Qbft testing #128

Merged
merged 77 commits into from
Feb 11, 2025
Merged

Qbft testing #128

merged 77 commits into from
Feb 11, 2025

Conversation

Zacholme7
Copy link
Member

@Zacholme7 Zacholme7 commented Feb 6, 2025

Issue Addressed

#98

Proposed Changes

This PR introduces a framework to provide end to end QBFT Tests. It allows the ability to run multiple instances at once and control the operational status of the nodes along with any byzantine behavior.

There are also a few bugfixes from issues that were discovered during testing.

Notes for clarity

  • The qbft sender/receiver will be replaced in next pr with signing and network functionality
  • cluster_members was changed to a Vec as we need deterministic leader selection for testing and random ordering with the HashSet did not allow for this.

@Zacholme7 Zacholme7 marked this pull request as ready for review February 6, 2025 20:34
Copy link
Member

@dknopik dknopik left a comment

Choose a reason for hiding this comment

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

Great PR! I especially like the support for several kinds of byzantine behaviour. Added some comments on details.

@Zacholme7 Zacholme7 mentioned this pull request Feb 10, 2025
@Zacholme7 Zacholme7 added the ready-for-review This PR is ready to be reviewed label Feb 10, 2025
@dknopik
Copy link
Member

dknopik commented Feb 11, 2025

Thought again about the queue we are passing. I am not sure if Message is a good type for the output queue. I feel like we should send the signed message there, ready to be sent over the network. What do you think?

@Zacholme7
Copy link
Member Author

@dknopik I was actually just planning on scrapping Message in the signing pr as it really doesn't provide any use. The message type is already encoded in the QbftMessage and I dont see a place where the id is useful. Or am I missing some case where we need this?

I think UnsignedSSVMessage is the most appropriate for the output queue. What I have right now in the signing draft is qbft instance outputs the unsigned messages, the unsigned message is passed to a signing task to create a signed message, and then the signed message is serialized and sent to the network.

@dknopik
Copy link
Member

dknopik commented Feb 11, 2025

I was actually just planning on scrapping Message in the signing pr as it really doesn't provide any use. The message type is already encoded in the QbftMessage and I dont see a place where the id is useful. Or am I missing some case where we need this?

Well, Message was more useful back when the qbft crate was more independent from the ssv types/message format, but as we have abandoned that, it is now indeed redundant.

I think UnsignedSSVMessage is the most appropriate for the output queue. What I have right now in the signing draft is qbft instance outputs the unsigned messages, the unsigned message is passed to a signing task to create a signed message, and then the signed message is serialized and sent to the network.

Alright!

@Zacholme7 Zacholme7 requested a review from dknopik February 11, 2025 14:53
Comment on lines 331 to 337
// Queue is full - drop message under constrained bandwidth
warn!("Dropping QBFT message due to full queue: msg={:?}", msg);
}
Err(TrySendError::Closed(_)) => {
// Channel closed - critical failure or shutdown
error!("QBFT message channel closed - initiating shutdown");
// todo!() need some sort of shutdown
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, one more nitpick:

warn!(?msg, "Dropping QBFT message due to full queue");

And for the Err case: a return after the log should suffice here.

Copy link
Member Author

Choose a reason for hiding this comment

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

A bit confused on the Err case. Dont we need some explicit shutdown? Using return here is the same as finishing normally with a implicit (). The instance will still be able to send messages and they will all hit the Closed case. The linter also says the return is unneeded.

Copy link
Member

Choose a reason for hiding this comment

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

aaah, yeah of course, missed that we are inside a closure.

hm. let's leave it as is for now, the instance also quits if the input queue is closed, so it should be fine

Copy link
Member

@dknopik dknopik left a comment

Choose a reason for hiding this comment

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

LVGTM

@dknopik dknopik merged commit 0978e19 into sigp:unstable Feb 11, 2025
10 checks passed
@Zacholme7 Zacholme7 deleted the qbft-testing branch February 11, 2025 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
QBFT ready-for-review This PR is ready to be reviewed testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants