💬 dergoegge commented on pull request "Add support for "partial" fuzzers that indicate usefulness":
(https://github.com/bitcoin/bitcoin/pull/27552#discussion_r1182489285)
Just noting that this was only recently added to libFuzzer: https://reviews.llvm.org/D128749?id=441094

I think that is fine, running with older versions of libFuzzer makes little sense anyway.

🤔 dergoegge reviewed a pull request: "Add support for "partial" fuzzers that indicate usefulness"
(https://github.com/bitcoin/bitcoin/pull/27552#pullrequestreview-1409007192)
Concept ACK

💬 dergoegge commented on pull request "Add support for "partial" fuzzers that indicate usefulness":
(https://github.com/bitcoin/bitcoin/pull/27552#discussion_r1182486207)
Maybe worth noting that this will only work for libFuzzer? (or i guess any engine that uses the libFuzzer harness and respects the -1 return value)

💬 sipa commented on pull request "Add support for "partial" fuzzers that indicate usefulness":
(https://github.com/bitcoin/bitcoin/pull/27552#issuecomment-1531411293)
@MarcoFalke Fair question. I think the primary advantage is that it should help with the speed of fuzzing, by avoiding spending time on less interesting things. It is however somewhat delicate as you point out - if you mark too many things as "uninteresting", I can imagine it actually becomes harder to find a mutation path from one interesting test case to another.

🚀 fanquake merged a pull request: "test: added coverage to rpc_scantxoutset.py"
(https://github.com/bitcoin/bitcoin/pull/27453)

💬 sipa commented on pull request "Add support for "partial" fuzzers that indicate usefulness":
(https://github.com/bitcoin/bitcoin/pull/27552#discussion_r1182499692)
I see it a bit more abstract: the macro is for *writing* a test that has such a return value. Whether the fuzz infrastructure uses it in an independent question (and if there are ones using `LLVMFuzzerTestOneInput` that don't support the return value -1 at all, we should make sure it also isn't returned, even if `FUZZ_PARTIAL_TARGET` is used).

💬 MarcoFalke commented on pull request "Add support for "partial" fuzzers that indicate usefulness":
(https://github.com/bitcoin/bitcoin/pull/27552#issuecomment-1531421409)
Yeah, it may help or hurt, depending on your goal and the fuzz target. My recommendation would be to make this off by default, and add an option to enable it at run time. This certainly can't hurt and may help for the use cases that want to enable it.

💬 MarcoFalke commented on pull request "Add support for "partial" fuzzers that indicate usefulness":
(https://github.com/bitcoin/bitcoin/pull/27552#discussion_r1182509668)
Suggestion, if you want to go down the route to make this a runtime option:

```cpp
static const reject_unwated_inputs{std::getenv("REJECT_UNWANTED_FUZZ_INPUTS")};
```

(or similar)

💬 sipa commented on pull request "Add support for "partial" fuzzers that indicate usefulness":
(https://github.com/bitcoin/bitcoin/pull/27552#issuecomment-1531436567)
@MarcoFalke Perhaps, but I don't worry too much if it's used conservatively. The "having to go through uninteresting cases to get to interesting ones" is a concern with or without this functionality, because after all, the uninteresting cases are already unlikely to trigger much (useful) coverage, and the coverage that they do trigger is likely unrelated to what is interesting. The actual solution libfuzzer has for this concern is attempting multiple (up to 5, IIRC) mutations in one step.

Of
...

💬 TheCharlatan commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#issuecomment-1531437760)
Thank you for the discussions,

Updated ed3159e0eb105d9f46659bd8f8b2db27d94841de -> 9c197181803e08e1cce4460190cc06fc2e093a80 ([removeBlockstorageArgs_15](https://github.com/TheCharlatan/bitcoin/tree/removeBlockstorageArgs_15) -> [removeBlockstorageArgs_16](https://github.com/TheCharlatan/bitcoin/tree/removeBlockstorageArgs_16), [compare](https://github.com/TheCharlatan/bitcoin/compare/removeBlockstorageArgs_15..removeBlockstorageArgs_16))
* Reverted change made for passing a `NodeContext` to
...

🚀 fanquake merged a pull request: "ci: use LLVM/clang-16 in native_asan job"
(https://github.com/bitcoin/bitcoin/pull/27360)

👋 brunoerg's pull request is ready for review: "test: merge banning test from p2p_disconnect_ban to rpc_setban"
(https://github.com/bitcoin/bitcoin/pull/26863)

💬 liuyangc3 commented on issue "Build broken when enabling fuzzing on Apple M1 hw using homebrew llvm.":
(https://github.com/bitcoin/bitcoin/issues/27550#issuecomment-1531462324)
Yeah thanks for the advice, I will try more before ask help here. maybe is not the bitcoin code issue. will get back later thanks again

💬 fanquake commented on issue "Build broken when enabling fuzzing on Apple M1 hw using homebrew llvm.":
(https://github.com/bitcoin/bitcoin/issues/27550#issuecomment-1531463486)
Ok. We'll close this for now. Comment back when you've had a chance to test, and we can reopen if need be.

✅ fanquake closed an issue: "Build broken when enabling fuzzing on Apple M1 hw using homebrew llvm."
(https://github.com/bitcoin/bitcoin/issues/27550)

💬 fanquake commented on pull request "net: use interruptible async getaddrinfo wrapper from libevent for DNS":
(https://github.com/bitcoin/bitcoin/pull/27505#issuecomment-1531483899)
In general, I'm ~0 on leaning into further libevent usage, especially for something like this. The upstream code is currently not necessarily very well maintained or tested. There is also at least one open issue which reports `evdns_getaddrinfo` just "crashing occasionally": https://github.com/libevent/libevent/issues/1130. Although it's not entirely clear if this is user error.

💬 brunoerg commented on pull request "fuzz: addrman, add coverage for `network` field in `Select()`":
(https://github.com/bitcoin/bitcoin/pull/27549#discussion_r1182566968)
Well, since it already imports `test/fuzz/util/net.h` there is no reason, gonna change it!

💬 laanwj commented on issue "Improve porting documentation for legacy-only wallet RPCs":
(https://github.com/bitcoin/bitcoin/issues/25363#issuecomment-1531539449)
Yes, lgtm, thanks!

✅ laanwj closed an issue: "Improve porting documentation for legacy-only wallet RPCs"
(https://github.com/bitcoin/bitcoin/issues/25363)

💬 fjahr commented on pull request "Add ASM optimizations for MuHash3072":
(https://github.com/bitcoin/bitcoin/pull/19181#issuecomment-1531553456)
I have now tested SafeGCD vs SafeGCD+ASM (see https://github.com/fjahr/bitcoin/tree/pr21590-safegcd-asm) and the gains from including the ASM code are still substantial.

GCC 13.1 - SafeGCD only:

```
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 9,168.73 | 109,066.35 | 2.1% | 1.06 | `MuHash`
| 7,571.75 | 132,069.88 | 0.2% |
...