💬 maflcko commented on pull request "benchmark: Improve SipHash_32b accuracy to avoid potential optimization issues":
(https://github.com/bitcoin/bitcoin/pull/30349#discussion_r1824325029)
I just think that it would be nice to not print the warning and to document that the unsigned overflow is expected here.

Steps to reproduce on a fresh Ubuntu 24.04:

```
export DEBIAN_FRONTEND=noninteractive && apt update && apt install curl wget htop git vim ccache -y && git clone https://github.com/bitcoin/bitcoin.git --depth=1 ./b-c && cd b-c && apt install build-essential cmake pkg-config python3-zmq libzmq3-dev libevent-dev libboost-dev libsqlite3-dev libdb++-dev clang llvm -y


...

💬 maflcko commented on pull request "bench: Remove various extraneous benchmarks":
(https://github.com/bitcoin/bitcoin/pull/31153#discussion_r1824337068)
I see your point, but is seems a bit arbitrary to remove this one and not `ExpandDescriptor`, which should also be an end-to-end target, no?

Maybe it would help review if you created a list of all benchmarks, and then added a reason why you kept it, or why you removed it for each one. Otherwise, it may be harder for reviewers to follow and see how the change is self-consistent?

💬 0xB10C commented on issue "Bitcoin Core 28.0 (Flatpak Linux Mint) pointing or saving blockchain to external drives does not work":
(https://github.com/bitcoin/bitcoin/issues/31188#issuecomment-2449694793)
Are you sure your configuration file is read? The file should be named `bitcoin.conf` and not `bitcoin.config`. You can see the `bitcoin.conf` configurations being logged at the start of the `debug.log` during startup?

For example, for me it shows:

```
...
Default data directory /home/user/.bitcoin
Using data directory /home/user/.bitcoin/signet
Config file: /home/user/.bitcoin/bitcoin.conf
Config file arg: [main] prune="550"
Config file arg: [main] rest="1"
Config file arg: [main]
...

📝 dergoegge opened a pull request: "Revert "Introduce `g_fuzzing` global for fuzzing checks""
(https://github.com/bitcoin/bitcoin/pull/31189)
`Assume`'s should not be compiled in for production builds. #31093 broke that property of `Assume`.

Closes #31178

💬 dergoegge commented on pull request "Revert "Introduce `g_fuzzing` global for fuzzing checks"":
(https://github.com/bitcoin/bitcoin/pull/31189#issuecomment-2449696875)
CI will fail and we'll probably need to reconsider #31057 and #31073

💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1824345643)
Please disregard my previous comments.

I've reviewed and tested the current branch once more. Modifications in `contrib/guix/libexec/build.sh` are not required. Setting proper compilers is handled already:
- for the `native_qt` package: https://github.com/bitcoin/bitcoin/blob/af05dd9a12b89224dc7ad229698eeceb3e560ed4/depends/packages/native_qt.mk#L88-L89
- for other CMake-based native packages: https://github.com/bitcoin/bitcoin/blob/af05dd9a12b89224dc7ad229698eeceb3e560ed4/depends/funcs.mk#
...

👍 stickies-v approved a pull request: "[27.x] rc2 or final"
(https://github.com/bitcoin/bitcoin/pull/31154#pullrequestreview-2407773071)
ACK dd1bf8bc4ef75ef43c33bbf755c3e0d2c6c3c5f7

Doc-only change, I'm getting the same manpages results.

💬 maflcko commented on pull request "Revert "Introduce `g_fuzzing` global for fuzzing checks"":
(https://github.com/bitcoin/bitcoin/pull/31189#issuecomment-2449710201)
It would be good to make it `constexpr` instead https://github.com/bitcoin/bitcoin/issues/31178#issuecomment-2449665430 and use that to fix the issues. Happy to do it myself, but I think it should be part of one pull request. Otherwise you'll just switch one issue for another and a follow-up has to be created anyway.

💬 dergoegge commented on pull request "Revert "Introduce `g_fuzzing` global for fuzzing checks"":
(https://github.com/bitcoin/bitcoin/pull/31189#issuecomment-2449717790)
I don't think the approach described in https://github.com/bitcoin/bitcoin/issues/31178#issuecomment-2449665430 would fix the ci issues? If we make it a compile time option we'll see the timeouts in `p2p_headers_presync` for the macos and windows CI either way.

💬 hodlinator commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1824361387)
Nothing more to do as far as this PR goes. Still interested to hear from folks more focused on fuzzing.

💬 maflcko commented on pull request "Revert "Introduce `g_fuzzing` global for fuzzing checks"":
(https://github.com/bitcoin/bitcoin/pull/31189#issuecomment-2449726713)
Yeah, I am saying that the fix should be a complete one, at least when it comes to the `./src` directory. Otherwise it will have to be touched again. I am happy to submit a pull, or review one. But I think there have been enough issue and pull request threads about this topic and it would be good to wrap it up in at most one or two pulls.

✅ dergoegge closed a pull request: "Revert "Introduce `g_fuzzing` global for fuzzing checks""
(https://github.com/bitcoin/bitcoin/pull/31189)

💬 dergoegge commented on pull request "Revert "Introduce `g_fuzzing` global for fuzzing checks"":
(https://github.com/bitcoin/bitcoin/pull/31189#issuecomment-2449729047)
> I am happy to submit a pull

Yes please

👋 fanquake's pull request is ready for review: "[27.x] rc2 or final"
(https://github.com/bitcoin/bitcoin/pull/31154)

💬 hodlinator commented on pull request "optimization: change XOR obfuscation key from `std::vector<std::byte>(8)` to `uint64_t`":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2449751980)
The CI failure in https://github.com/bitcoin/bitcoin/runs/32217592364 might come from a bad rebase reconciliation with master?
```
[12:44:30.723] Duplicate include(s) in src/bench/xor.cpp:
[12:44:30.723] #include <cstddef>
[12:44:30.723] #include <span.h>
[12:44:30.723] #include <streams.h>
[12:44:30.723] #include <random.h>
[12:44:30.723] #include <vector>
[12:44:30.723] #include <bench/bench.h>
[12:44:30.723]
[12:44:30.728] ^---- ⚠️ Failure generated from lint-includes.py
```

📝 glozow opened a pull request: "TxDownloadManager followups"
(https://github.com/bitcoin/bitcoin/pull/31190)
Addresses some remaining followups from #30110:
- https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1818893833
- https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1819638959
- https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1819634235

💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1824397333)
picked up in #31190

💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1824397572)
added in #31190

💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1824398039)
Agree the comment isn't 100% accurate, edited in #31190

💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1824400387)
since txdownloadman is locked with this mutex, and it's the only one with access to these data structures, we have the same locking happening. Since `m_tx_download_mutex` is external, we wouldn't be able to have these annotations I think