Skip to content

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Apr 5, 2021

Previously it only merged the psbt with itself, now it tries to merge another.

@DrahtBot DrahtBot added the Tests label Apr 5, 2021
@maflcko
Copy link
Member Author

maflcko commented Apr 5, 2021

Note this breaks the existing inputs' format, but can trivially be fixed:

for f in ./psbt/*; do echo "$f" && sed -i 's|\\|\\\\|g' "$f"; done

@maflcko
Copy link
Member Author

maflcko commented Apr 7, 2021

@practicalswift Thomas J, mind taking a look here? This only changes the fuzz target you added, so shouldn't be too hard to review.

@practicalswift
Copy link
Contributor

@MarcoFalke Sure! Thanks for the ping!

Tested ACK faaf395

FWIW I believe the code paths that this PR adds coverage for is a subset of the code paths covered by the RPC fuzzer (see #21169). That is just an observation and not an argument against this PR.

I think we should strive for covering our functions both 1.) from the "micro level" as in this case, and 2.) from the "macro level" as in the case of say the process_message, process_messages and rpc fuzzers.

Why?

  1. The macro level harnesses are more constrained and the fuzzer will thus focus its finite resources on realistic code paths which may be problematic as the code is written today ("reachable" bugs).
  2. The micro level harnesses give the fuzzer permission to explore also code paths which are not exercised by current callers but may be exercised in the future by future callers (currently "unreachable" latent bugs).

In other words: Strong Concept ACK for reaching into the PSBT code from both the micro level (this PR) and from the macro level (RPC fuzzer, see #21169) :)

@maflcko maflcko merged commit f0fa324 into bitcoin:master Apr 9, 2021
@maflcko maflcko deleted the 2104-fuzzPsbt branch April 9, 2021 16:56
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 9, 2021
faaf395 fuzz: Extend psbt fuzz target a bit (MarcoFalke)

Pull request description:

  Previously it only merged the psbt with itself, now it tries to merge another.

ACKs for top commit:
  practicalswift:
    Tested ACK faaf395

Tree-SHA512: e1b1d31a47d35e1767285bc2fda176c79cb0550d6d383fe467104272e61e1c83f6cbc0c7d6bbc0c3027729eec13ae1f289f8950117ee91e0fb3703e66d5e6918
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants