📝 dergoegge converted_to_draft a pull request: "fuzz: Use FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION for pow checks"
(https://github.com/bitcoin/bitcoin/pull/29305)
Alternative to the mocking of `CheckProofOfWork` in #28043 for avoiding fuzzers to be blocked on proof-of-work checks.

More on `FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION`: https://llvm.org/docs/LibFuzzer.html#fuzzer-friendly-build-mode

💬 dergoegge commented on pull request "fuzz: Use FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION for pow checks":
(https://github.com/bitcoin/bitcoin/pull/29305#issuecomment-1908321342)
Using clang with `-fsanitize=fuzzer` does actually not define this by default.

💬 instagibbs commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#issuecomment-1908321622)
@dergoegge fuzzer created a circular reference which caused incongruities between descendant counts and actually walking the descendants in mempool. Please run again?

💬 fanquake commented on pull request "depends: Do not override `CFLAGS` when building SQLite with `DEBUG=1`":
(https://github.com/bitcoin/bitcoin/pull/29287#issuecomment-1908327313)
> Changes in https://github.com/bitcoin/bitcoin/pull/29282 might not be strictly required now. However, I consider them an improvement.

I think it'd be easiest to make (any) additional changes together here, so we don't have to go over the flags twice. Can you combine any relevant changes and close the other PR.

> Could this be the root cause for https://github.com/bitcoin/bitcoin/issues/27222?

I think so. Rebased #27495 on top.

Related, I guess `-g` will no-longer be used, when comp
...

💬 fanquake commented on pull request "depends: patch libool out of libnatpmp/miniupnpc":
(https://github.com/bitcoin/bitcoin/pull/29298#issuecomment-1908331771)
Concept ACK - maybe upstream will take these patches as well? I guess we can try and send them, given we'll have to send CMake fixes in either case.

Rebased #21778 on top of this rather than #29232.

🚀 fanquake merged a pull request: "fuzz: Exit and log stderr for parse_test_list errors"
(https://github.com/bitcoin/bitcoin/pull/29304)

🤔 murchandamus reviewed a pull request: "wallet: Keep track of the wallet's own transaction outputs in memory"
(https://github.com/bitcoin/bitcoin/pull/27286#pullrequestreview-1841667091)
ReACK https://github.com/bitcoin/bitcoin/commit/39f0ab3e126704b30f7ed2b86690f60b631d2ec4 compared to my prior review, there are only the changes mentioned in response to @aureleoules.

💬 fanquake commented on pull request "serialization: c++20 endian/byteswap/clz modernization":
(https://github.com/bitcoin/bitcoin/pull/29263#issuecomment-1908366535)
Concept ACK - I think moving to using the builtins is ok. With the eventual plan to migrate to `std::byteswap`.

@aureleoules everytime I visit the [coverage/benchmarks for this PR](https://corecheck.dev/bitcoin/bitcoin/pulls/29263), I get a 500 error. Can you take a look?

📝 ismaelsadeeq opened a pull request: "doc: clarify that `BroadcastTransaction` comment"
(https://github.com/bitcoin/bitcoin/pull/29308)
`BroadcastTransaction` is also called by `submitpackage` RPC.

The comment needs to be updated, all transactions that are accepted into the mempool post package processing are broadcasted to peers individually here
https://github.com/bitcoin/bitcoin/blob/ea4ddd8652d9dd1e7698e2a6f84c606cf24a2e3e/src/rpc/mempool.cpp#L926

👍 vasild approved a pull request: "doc: clarify `BroadcastTransaction` comment"
(https://github.com/bitcoin/bitcoin/pull/29308#pullrequestreview-1841734543)
ACK cca20d94b787d0881a61509445f4827f913e051d

I was looking at the same comment and was thinking it needs an update. Thanks!

💬 theuni commented on pull request "build: always set `-g -O2` in `CORE_CXXFLAGS`":
(https://github.com/bitcoin/bitcoin/pull/29205#issuecomment-1908411460)
> > While always overriding Autotools' defaults, are there any particular reasons not using -O3 instead of -O2?
>
> AFAIK `-O3` isn't safe.

I remember these discussions almost 10 years ago when compilers (notably gcc) put aggressive and (iirc?) non-standards-compliant optimizations behind `-O3`. I'm guessing that's probably not the case anymore these days, but I do agree somewhat with @luke-jr that we'd need to investigate the safety of this before blindly changing.

Either way, it's out
...

💬 TheCharlatan commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1465145033)
Good catch!
I think the real problem here is though that the scheduler is now created too early, so I'll revert moving its initialization out of `AppInitMain` and will instead leave it there and initialize the signals right after it. Then we can have all these components be initialized in `ChainTestingSetup`.

💬 TheCharlatan commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#issuecomment-1908419204)
Thank you for the continued feedback @ryanofsky!

Updated 22048c19e236e4b683f1c8192883545d5c68f793 -> bdd985af62ff9e94490ecc701d6063eaab172add ([noGlobalSignals_9](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_9) -> [noGlobalSignals_10](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_10), [compare](https://github.com/TheCharlatan/bitcoin/compare/noGlobalSignals_9..noGlobalSignals_10))

* Addressed @ryanofsky's [comment](https://github.com/bitcoin/bitcoin/pull/2896
...

✅ dergoegge closed a pull request: "fuzz: Use FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION for pow checks"
(https://github.com/bitcoin/bitcoin/pull/29305)

💬 maflcko commented on pull request "util: check for errors after close and read in AutoFile":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r1465152180)
not sure. `detail_fread` is an implementation detail and thin wrapper, with the goal to simply return the return value of `std::fread`. It should be left for the caller to decide about the error.

In any case, if you want to do this, you'd have to apply it to both branches of the `if`. The error message shouldn't differ just because a file is xor'd or not.

Finally, it would be good to explain whether this is a bugfix or a refactor/belt-and-suspenders. I don't think an error/eof could have o
...

💬 maflcko commented on pull request "build: Enable -Wunreachable-code":
(https://github.com/bitcoin/bitcoin/pull/28999#discussion_r1465156898)
> It was written this way so changes to code could be checked at compile time without developers needing rebuild with --without-bdb and --with-sqlite=yes options. So this seems like a case where the compiler warning is unhelpful, and it would be nice if there was a way to disable it.

I think it can be disabled via the verbose `pragma clang diagnostic ignored`, but that is ugly and verbose. A better workaround could be to use `if constexpr(...USE_BDB...) { return MakeBDB();} else { return erro
...

💬 dergoegge commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#issuecomment-1908435057)
> Please run again?

Running now.

Some coverage reports from the previous runs (one per harness): http://bitcoind-fuzz.dergoegge.de/instagibbs/bitcoin/2024-01-diagram-checks/coverage/

💬 willcl-ark commented on pull request "rpc: add 'getnetmsgstats' RPC":
(https://github.com/bitcoin/bitcoin/pull/28926#issuecomment-1908444454)
> > I have applied the suggestions below in [`vasild/getnetmsgstats`](https://github.com/vasild/bitcoin/commits/getnetmsgstats) plus some other simplifications. Feel free to pick them.
>
> Should I PR the above branch?

Hey Vasil, I think this makes most sense, especially seeing as you've co-authored/written most of the changes here, and your improvements on the new branch!

I've been testing your branch out today and am a fan of the simplicity vs this changeset. I did kind of prefer the
...

👍 theuni approved a pull request: "build: always set `-g -O2` in `CORE_CXXFLAGS`"
(https://github.com/bitcoin/bitcoin/pull/29205#pullrequestreview-1841788516)
ACK 00c1e2aa4496b5f038ae5199dbd16d8313766533

Quickly tested with macos + depends, which I consider to be one of our wonkiest builds wrt flags. Looks correct to me with or without overridden CXXFLAGS.

I also think having CORE_CXXFLAGS come first generally makes more sense so that it's always override-able.

💬 ryanofsky commented on pull request "build: Enable -Wunreachable-code":
(https://github.com/bitcoin/bitcoin/pull/28999#discussion_r1465181728)
> if a piece of code compiles does not mean it has no bugs

I've noticed that sometimes. And it would be unreasonable to expect our CI to run every line of code, but I think it's reasonable to expect our CI to at least _compile_ every line of code and not leave blind spots like this. So I really like your constexpr idea, and can try it out in #25722 after I finish rebasing it. Thanks for suggesting that.