💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1824243351)
I've checked once more. It fails without patching: https://cirrus-ci.com/task/4951518795792384.

💬 hebasto commented on pull request "msvc: Update vcpkg manifest":
(https://github.com/bitcoin/bitcoin/pull/31186#discussion_r1824250163)
This `"$comment"` field actually comments the following line:https://github.com/bitcoin/bitcoin/blob/fad685ce54d39a333a6c75641b5d519cb0345959/vcpkg.json#L3-L4

💬 hodlinator commented on pull request "msvc: Update vcpkg manifest":
(https://github.com/bitcoin/bitcoin/pull/31186#discussion_r1824271501)
Saw that, but it mentions it by name so I thought maybe keeping the lexicographical order the first commit tries to introduce would have precedent.

💬 hebasto commented on pull request "msvc: Update vcpkg manifest":
(https://github.com/bitcoin/bitcoin/pull/31186#discussion_r1824280283)
I've dropped mentioning of lexicographical order from the commit message. The goal of the reordering is to minimize diffs for invocations like `vcpkg add port libqrencode`.

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1824285323)
Nit

```
const size_t removed = m_local_set.erase(wtxid) + m_delayed_local_set.erase(wtxid);
// Data must be in one of the sets at most
Assume(removed <= 1);
return removed == 1;
```

💬 l0rinc commented on pull request "benchmark: Improve SipHash_32b accuracy to avoid potential optimization issues":
(https://github.com/bitcoin/bitcoin/pull/30349#discussion_r1824303409)
Thanks for the hints, I've replaced the increment of the bytes with a xor, this should keep the values in check. The overflow of `i` should be well-defined in this case, please let me know if you think any other change is needed since I couldn't reproduce the sanitizer warnings.
I have checked to make sure this still helps with the benchmarking of https://github.com/bitcoin/bitcoin/pull/30442.
Done in https://github.com/bitcoin/bitcoin/compare/c88b1bde7500ace70a694f36a82521f92221936c..100cded5
...

💬 sipa commented on issue "bench: `linearizeoptimallyexample11` benchmark now running 4x slow than previously":
(https://github.com/bitcoin/bitcoin/issues/31178#issuecomment-2449639956)
@maflcko Worth trying, but I have been using `Assume` in many places assuming it'll be optimized out entirely in production code. If we leave this `g_fuzzing` in place, we will need to re-evaluate whether all those `Assume`s are still worth having if they have a runtime impact.

💬 l0rinc commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1824305883)
Is there anything else for me to do here?

💬 maflcko commented on pull request "benchmark: Improve SipHash_32b accuracy to avoid potential optimization issues":
(https://github.com/bitcoin/bitcoin/pull/30349#discussion_r1824308828)
> The overflow of `i` should be well-defined in this case

Unsigned integer overflow is well-defined (and relied upon) either way. This is just about documenting where one is expected and where one is unexpected.

💬 l0rinc commented on pull request "benchmark: Improve SipHash_32b accuracy to avoid potential optimization issues":
(https://github.com/bitcoin/bitcoin/pull/30349#discussion_r1824312152)
So are you ok with the current version?

💬 maflcko commented on issue "bench: `linearizeoptimallyexample11` benchmark now running 4x slow than previously":
(https://github.com/bitcoin/bitcoin/issues/31178#issuecomment-2449665430)
Then I think the only solution is to make it a build-time option again:

```cpp
constexpr bool g_fuzzing{
#ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
true
#else
false
#endif
};

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