-
Notifications
You must be signed in to change notification settings - Fork 13
fuzz: Extend block and transaction round trip tests #125
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
Conversation
abc93c5 to
ba9941d
Compare
|
Nice! Would splitting |
| let tx_count = block.transaction_count(); | ||
| assert_eq!(tx_count, block.transactions().count()); | ||
|
|
||
| for (i, tx) in block.transactions().enumerate().take(1000) { | ||
| if let Ok(indexed_tx) = block.transaction(i) { | ||
| assert_eq!(tx.txid(), indexed_tx.txid()); | ||
| } | ||
|
|
||
| assert_eq!(tx.inputs().count(), tx.input_count()); | ||
| assert_eq!(tx.outputs().count(), tx.output_count()); | ||
|
|
||
| for (j, input) in tx.inputs().enumerate().take(10) { | ||
| if let Ok(indexed_input) = tx.input(j) { | ||
| let op1 = input.outpoint(); | ||
| let op2 = indexed_input.outpoint(); | ||
| assert_eq!(op1.txid(), op2.txid()); | ||
| assert_eq!(op1.index(), op2.index()); | ||
| assert_eq!(op1.is_null(), op2.is_null()); | ||
| } | ||
| } | ||
|
|
||
| for (j, output) in tx.outputs().enumerate().take(10) { | ||
| if let Ok(indexed_output) = tx.output(j) { | ||
| assert_eq!(output.value(), indexed_output.value()); | ||
| assert_eq!( | ||
| output.script_pubkey().to_bytes(), | ||
| indexed_output.script_pubkey().to_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 think it would be nicer to extend the existing roundtrip targets with these additional tests. Both just consume a byte string and it would be good to have both of them consume a robust corpus of both hand-rolled block data and fuzzer inputs.
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.
Makes sense!
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.
Thanks, this makes more sense. I've added the checks to the round trip tests and then simplified the tx extraction test to be a quick verification at the end of the block test. The more I looked at it, I think the previous version was a bit overkill.
Let me know what you guys think / if anything else can be added.
Add checks for iterator vs indexed access on inputs, outputs, and transactions.
ba9941d to
b7539ea
Compare
sedited
left a comment
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.
Thanks!
lgtm
Adds a new
kernel_primitivesfuzz target that tests the Rust wrapper around Bitcoin Core's block and transaction data structures.Here's the coverage report.