-
Notifications
You must be signed in to change notification settings - Fork 1
New Target for BeaconVote Decode Encode #41
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
base: unstable
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Adds a new differential fuzzing target for BeaconVote SSZ encoding/decoding to compare Go and Rust implementations for potential network consensus issues.
- Implements
diff_fuzz_beacon_vote_decode_encode
target with Rust fuzzing harness and Go FFI integration - Adds comprehensive corpus generation with valid BeaconVotes, malformed cases, and SSZ structure edge cases
- Integrates the new target into the fuzzing infrastructure with proper categorization as high-priority
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
run_fuzzer.sh | Adds beacon vote target to high-priority differential fuzzing list |
fuzz/fuzz_targets/differential/diff_fuzz_beacon_vote_decode_encode.rs | Implements Rust side of differential fuzzing for BeaconVote encode/decode |
fuzz/Cargo.toml | Registers the new beacon vote fuzzing target binary |
diff_fuzzing/src/lib.rs | Adds Go FFI wrapper for beacon vote decode/encode operations |
diff_fuzzing/sfuzz.go | Implements Go-side BeaconVote SSZ decode/encode with FFI export |
corpus_generator/src/main.rs | Registers beacon vote generator in the main corpus generator registry |
corpus_generator/src/generators/mod.rs | Exports BeaconVote generator module |
corpus_generator/src/generators/beacon_vote.rs | Implements comprehensive corpus generation for BeaconVote fuzzing scenarios |
Comments suppressed due to low confidence (1)
diff_fuzzing/sfuzz.go:415
- [nitpick] This appears to be only indentation/formatting changes to existing code. These formatting changes are not directly related to the BeaconVote feature and should be separated into a different commit.
message, err := specqbft.NewProcessingMessage(msg)
if err != nil {
panic(err.Error())
}
var log = zap.NewNop()
if fInst.debug {
log, _ = zap.NewDevelopment()
}
_, _, _, err = fInst.instance.ProcessMsg(context.TODO(), log, message)
if err != nil {
println(err.Error())
// Do not return an error, as this error might be appropriate for input message.
// Just compare the outputted messages in the harness.
}
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
diff_fuzzing/sfuzz.go
Outdated
|
||
ssvtypes "github.com/ssvlabs/ssv/protocol/v2/types" | ||
"go.uber.org/zap" | ||
|
||
"github.com/herumi/bls-eth-go-binary/bls" | ||
specqbft "github.com/ssvlabs/ssv-spec/qbft" | ||
spectypes "github.com/ssvlabs/ssv-spec/types" | ||
"github.com/ssvlabs/ssv/utils/threshold" | ||
qbft "github.com/ssvlabs/ssv/protocol/v2/qbft" | ||
instance "github.com/ssvlabs/ssv/protocol/v2/qbft/instance" | ||
|
||
"github.com/ssvlabs/ssv/utils/threshold" | ||
|
||
// Import the actual validation package |
Copilot
AI
Sep 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The import reordering appears unnecessary and makes the diff harder to read. Consider keeping imports in their original order unless there's a specific reason for the change.
Copilot uses AI. Check for mistakes.
diff_fuzzing/sfuzz.go
Outdated
doReceive(fInst, fInst.messageBuffer.messages[msgIdx]) | ||
msgIdx++ |
Copilot
AI
Sep 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] This formatting change from msgIdx++;
to msgIdx++
is unrelated to the BeaconVote functionality and should be separated into a different commit.
Copilot uses AI. Check for mistakes.
diff_fuzzing/sfuzz.go
Outdated
|
||
// Check if output buffer is large enough | ||
out := unsafe.Slice((*byte)(out_ptr), out_size) | ||
if len(result) > int(out_size) { |
Copilot
AI
Sep 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comparison should be against out_size
directly since it's already an int
parameter, not cast it from something else. The cast int(out_size)
suggests out_size
might be a different type, but the function signature shows it as int
.
if len(result) > int(out_size) { | |
if len(result) > out_size { |
Copilot uses AI. Check for mistakes.
return Ok(ValidationResult::Boundary); | ||
} | ||
|
||
if data.len() < 32 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it be != 32?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think (but I'm not sure), according to BeaconVote
, an SSZ BeaconVote
contains multiple 32-byte roots (block_root, source.root, target.root) plus epochs, so the minimum valid SSZ size is well above 32 bytes. The check data.len() < 32
quickly rejects obviously too-short inputs. Using != 32
would wrongly reject all valid encodings, since real BeaconVote
encodings are typically much larger than exactly 32 bytes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know what the correct size is, but does it change? Apparently, it's like this (needs confirmation):
SSV BeaconVote structure:
pub struct BeaconVote {
pub block_root: Hash256, // 32 bytes
pub source: Checkpoint, // 40 bytes
pub target: Checkpoint, // 40 bytes
}
Breakdown:
- block_root (Hash256): 32 bytes
- source (Checkpoint): 40 bytes
- epoch (uint64): 8 bytes
- root (Hash256): 32 bytes
- target (Checkpoint): 40 bytes
- epoch (uint64): 8 bytes
- root (Hash256): 32 bytes
Total: 32 + 40 + 40 = 112 bytes
So every size different from this should be invalid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. BeaconVote
is a fixed-size SSZ container, so any size other than exactly 112 bytes is invalid. The current < 32
check is wrong, it should be != 112
since SSZ guarantees deterministic encoding for fixed structures with no size variation possible.
} | ||
|
||
fn validate_corpus_entry(&self, data: &[u8]) -> Result<ValidationResult, Box<dyn std::error::Error>> { | ||
let critical_sizes: Vec<usize> = crate::common::SizeBoundaryUtils::get_critical_size_boundaries() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not related to this PR, but not sure get_critical_size_boundaries
is a descriptive name
if critical_sizes.contains(&data.len()) { | ||
return Ok(ValidationResult::Boundary); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we return this here and stop validation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My idea was, when the input length matches a critical boundary size, we don’t mark it as valid or invalid right away. Instead, we label it as ValidationResult::Boundary
and keep it for testing edge cases about size. By returning at this point, we stop normal validation. This way, the input isn’t thrown away or used up, but saved in the corpus as a special boundary case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by critical? Why (1_000, "small_message"),
would be relevant for a BeaconVote
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By critical, I mean sizes that are interesting for testing, not normal BeaconVotes
. We keep very small, very large, or exact boundary sizes to check if the decoder crashes or has off-by-one bugs. That’s why we return Boundary
and stop normal validation. I followed the style of previous generators, but we can change it if you have a different idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked this one
fn validate_corpus_entry(&self, data: &[u8]) -> Result<ValidationResult, Box<dyn std::error::Error>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I'm not sure we can generalize boundaries for different types. Wouldn't they be like type_size
, type_size - 1
, type_size + 1
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, the ssv_message
generator does not use this boundary logic. I followed what I saw in some other generators, but maybe it is not the best fit here.
} | ||
|
||
impl BeaconVoteGenerator { | ||
fn generate_valid_beacon_votes(&self, output_dir: &Path, rng: &mut StdRng) -> Result<usize, Box<dyn std::error::Error>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What usize
means here? What kind of error could this function return if it's supposed to generate valid beacon votes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The usize in the return type is the count of files generated (how many valid beacon votes were written to the corpus). I just follow the previous generator implementations because all of them return the same output (
fn generate_validator_key_boundaries(&self, output_dir: &Path) -> Result<usize, Box<dyn std::error::Error>> { |
This PR adds a new fuzzing target for
BeaconVote
encode/decode.The goal is to increase code coverage for SSV clients by testing how
BeaconVote
data is serialized and deserialized in Rust and Go.The fuzzer must explore many
BeaconVote
scenarios to check that encoding and decoding are consistent across implementations.The inputs are valid
BeaconVotes
, malformed cases, and SSZ structure edge cases.Corpus generator for these inputs, plus the registration of the new target in the fuzzing pipeline.