💬 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.

💬 aureleoules commented on pull request "serialization: c++20 endian/byteswap/clz modernization":
(https://github.com/bitcoin/bitcoin/pull/29263#issuecomment-1908490513)
@fanquake thanks for reaching out, I fixed the issue.

💬 theuni commented on pull request "serialization: c++20 endian/byteswap/clz modernization":
(https://github.com/bitcoin/bitcoin/pull/29263#issuecomment-1908499136)
@aureleoules Thanks! These numbers really don't make much sense to me and don't match what I've seen locally, but I'll work on trying to reproduce.

💬 ryanofsky commented on issue "assumeutxo: nTx and nChainTx violations in CheckBlockIndex":
(https://github.com/bitcoin/bitcoin/issues/29261#issuecomment-1908513362)
> So during the period where we receive blocks for the background chain in some, possibly random, order, there will be a mix of blocks with updated `nTx` and fake `nTx` ones, so that the condition could fail everywhere in the range of assumed-valid blocks.

Thanks for figuring this out and trying this. Trying to put this all together it seems like these are the cases:

1. Case where IsValid() is true. Then nChainTx must be prev nChainTx + nTx.
2. Case where nTx is 0. Then nChainTx must be 0
...

💬 josibake commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1465282827)
The goal is not to support every possible script. Rather, we want to limit it to a sane subset to avoid technical debt and keep the implementation simple. In fact, the only reason for including `P2PKH` at all is due to the fact it is still widely used and makes up a significant percentage of the UTXO set, otherwise we would have only allowed Segwit script templates as this gives us non-malleability guarantees.

💬 vasild commented on pull request "util: check for errors after close and read in AutoFile":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r1465295573)
This assert actually failed in CI (wow!):

https://cirrus-ci.com/task/5586893237125120

```
Run autofile with args ['/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz', '-runs=1', PosixPath('/ci_container_base/ci/scratch/qa-assets/fuzz_seed_corpus/autofile')]INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 3765617163
INFO: Loaded 1 modules (547059 inline 8-bit counters): 547059 [0x55cd54dd29b8, 0x55cd54e582ab),
INFO: Loaded 1 PC tabl
...

👍 ryanofsky approved a pull request: "wallet: clarify replaced_by_txid and replaces_txid in help output"
(https://github.com/bitcoin/bitcoin/pull/29302#pullrequestreview-1841980191)
Code review ACK ff54314d4abed3bf9a78daf785a01c63af15c69d. Seems like a helpful clarification

💬 helpau commented on issue "Huge disk fragmentation":
(https://github.com/bitcoin/bitcoin/issues/29296#issuecomment-1908614398)
In my test environment fragmentation is only caused by Bitcoin Core, and affects Bitcoin Core. I think to eliminate fragmentation, all you need to do is increase buffers(e.g. to 2MB for .ldb files), limit simultaneous writes for different files, or preallocate disc space. I don't see your answer as a solution, as it boils down to:
1)buying an SSD(which imposes limitations, and doesn't completely solve the problem)
2)using defragmentation(requires time, user involvement and restarting Bitcoin
...

💬 achow101 commented on pull request "init: settings, do not load auto-generated warning msg":
(https://github.com/bitcoin/bitcoin/pull/29301#issuecomment-1908640373)
ACK 987a1b51eeb72c7fcb085470817a4fe85fcc3c7c

💬 achow101 commented on pull request "wallet: clarify replaced_by_txid and replaces_txid in help output":
(https://github.com/bitcoin/bitcoin/pull/29302#issuecomment-1908641427)
ACK ff54314d4abed3bf9a78daf785a01c63af15c69d

🚀 achow101 merged a pull request: "wallet: clarify replaced_by_txid and replaces_txid in help output"
(https://github.com/bitcoin/bitcoin/pull/29302)