Skip to content

Conversation

@shinghim
Copy link
Contributor

No description provided.

@apoelstra
Copy link
Member

Heads up that I no longer run honggfuzz fuzz tests in my local CI because honggfuzz has been broken so long. Nice that they have a new version a couple days ago, but it's too much of a PITA for me to restore deleted code to bring it back just to test it.

So thanks for updating the dep (and alerting me to the new version) but at some point we need to just cut over to cargo-fuzz.

@apoelstra
Copy link
Member

In dad80b5:

You add the arbitrary_witness test to the list here but you don't add the test itself til the next commit. Can you fix the commits so that the honggfuzz update is separate from everything else?

@shinghim
Copy link
Contributor Author

Fixed the commit. I'll also start working on moving over to cargo-fuzz

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 3f20f06; successfully ran local tests

Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

ACK 3f20f06

Comment on lines +16 to +17
let deserialized: Result<Witness, _> = deserialize(serialized.as_slice());
assert!(deserialized.is_ok(), "Deserialization error: {:?}", deserialized.err().unwrap());
Copy link
Member

Choose a reason for hiding this comment

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

Just a question, not requesting any change; what is the purpose of these two lines instead of just calling unwrap() on line 16?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No real purpose, I just had it like that so i could quickly find Deserialization error: in the output in a previous test and just kept using it. I can change to just use .unwrap() on the deserialize output here and in the other places where it's used in a follow up so it's less verbose

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually i'll just remove this entirely since it unwraps and checks equality in the next line

Copy link
Member

Choose a reason for hiding this comment

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

Oh, yeah, unwrap would probably be better here. I'm just so used to fuzztests ending in assertions.

@apoelstra apoelstra merged commit 3abd567 into rust-bitcoin:master Sep 29, 2025
25 checks passed
apoelstra added a commit that referenced this pull request Sep 30, 2025
3f20d54 Remove unnecessary check (Shing Him Ng)
8e7f2d2 Move serialization round trip check before object mutation (Shing Him Ng)

Pull request description:

  I thought I tested all of the targets I updated before I published them, but this one slipped through the cracks. Updated to move the serialization before object mutation [like before](https://github.com/rust-bitcoin/rust-bitcoin/blob/66c4ea20ff30cfa3e20f3073bd3beca3117177fd/fuzz/fuzz_targets/bitcoin/deserialize_transaction.rs#L10-L11), and I ran `cycle.sh` to make sure that there weren't any glaringly obvious bugs like the one that I pushed. Also remove some unnecessary `assert`s mentioned in #5029


ACKs for top commit:
  apoelstra:
    ACK 3f20d54; successfully ran local tests; but how come our CI was fine with these?


Tree-SHA512: cdf79a0bb73a6b3d236bd567ce46c56e7eca94947c93ce7ef63146dc1256c520ec17b3d9462570753d64930381a383bf48112cd409ca0f287a30ae6da90e9cf5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants