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

👍 hodlinator approved a pull request: "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests"
(https://github.com/bitcoin/bitcoin/pull/30746#pullrequestreview-2407594484)
ACK 6fd185c035c1cc4dd961cf14a2087e97fb069440

range-diff shows all notable changes since my [Concept ACK](https://github.com/bitcoin/bitcoin/pull/30746#pullrequestreview-2397249971) are in response to my suggestions.

Ran each new fuzz target with:
```
₿ FUZZ=<target name> build_fuzz/src/test/fuzz/fuzz -max_total_time=30 -jobs=8
```
Also ran:
```
₿ ctest -j<X> --test-dir build --output-on-failure -R base
```

💬 hebasto commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2449539386)
Rebased due to the conflict with the merged bitcoin/bitcoin#31015.

🤔 maflcko reviewed a pull request: "bench: Remove various extraneous benchmarks"
(https://github.com/bitcoin/bitcoin/pull/31153#pullrequestreview-2407602211)
No strong opinion on the removals. I think the doc update is valuable and could be extended. Simply declaring the removed benchmarks as potentially not valuable, when looked at too narrowly, could be less controversial and still achieve the essence of the goal of this pull request.

💬 maflcko commented on pull request "bench: Remove various extraneous benchmarks":
(https://github.com/bitcoin/bitcoin/pull/31153#discussion_r1824235336)
Maybe add a note to say that performance optimizations of micro-benchmarks are not valuable, if they do not show a visible end-user end-to-end performance improvement? Also could mention that the contribution of benchmarks that do not cover an end-user end-to-end scenario may not be valuable?


Even if they do, I think some improvements may still be rejected, if the code bloat or review/maintenance is too much to justify the improvement.

💬 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