-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
feat: roundtrip fuzz harness for PooledTransactions
#5125
feat: roundtrip fuzz harness for PooledTransactions
#5125
Conversation
Codecov Report
see 5 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more.
|
what's the bug? |
recmo/uint#335 is one surfaced by this fuzz test Another is related to payload length checks, PR soon |
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.
Just one comment about preserving existing behavior from decode_pooled_transactions_data
in at least one test
#[test] | ||
fn decode_pooled_transactions_data() { | ||
let network_data_path = | ||
PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("testdata/pooled_transactions_with_blob"); | ||
let data = fs::read_to_string(network_data_path).expect("Unable to read file"); | ||
let hex_data = hex::decode(data.trim()).unwrap(); | ||
roundtrip_pooled_transactions(hex_data); | ||
} |
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.
hmm, we should probably keep the test as-is, since previously it would panic on unsuccessful decoding. so we want to keep:
let txs = PooledTransactions::decode(&mut &hex_data[..]).unwrap();
Maybe we could have another test that iterates over our existing testdata/
files and runs the roundtrip test. Then we could both cover unsuccessful initial decoding, and failed roundtrips.
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 added an assertion that the existing tests decodes successfully in 9eb594c
d855f4e
to
9eb594c
Compare
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.
lgtm, pending @Rjected
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.
LGTM!
Add a roundtrip fuzz harness for
PooledTransactions
by converting an existing unit test into a fuzz test. This harness identified a new bug (cc @Rjected). Run the harness withcargo test-fuzz -p reth-eth-wire --run-until-crash
and reproduce the crash withcargo test-fuzz -p reth-eth-wire --replay crashes
.Partially addresses #4962