💬 ajtowns commented on pull request "versionbits refactoring":
(https://github.com/bitcoin/bitcoin/pull/29039#issuecomment-1848232374)
At the end of this sequence of patches the `VersionBitsCache` object provides a standalone interface to all the "versionbits" features needed by bitcoin core, which means that if the BIP9/speedy trial implementation is changed in future, it should be possible to do that in one place, without needing to change validation/RPC code as well.

This also simplifies and modernises some of the code, much of which has been proposed previously (eg, removing the params arguments from AbstractThresholdCon
...

💬 helpau commented on pull request "sync: improve CCoinsViewCache ReallocateCache":
(https://github.com/bitcoin/bitcoin/pull/28945#issuecomment-1848265773)
Hi, is this benchmark for HDD or SSD? Did you tried to run benchmark with huge dbcache and high height?

💬 martinus commented on pull request "test: doc: follow-up #28368":
(https://github.com/bitcoin/bitcoin/pull/29013#discussion_r1421360505)
@maflcko I just looked through the code a bit to find other usages of `fuzzed_data_provider` that depend on the evaluation order, and there are quite a few of them. E.g.

* https://github.com/bitcoin/bitcoin/blob/master/src/test/fuzz/addrman.cpp#L272
* https://github.com/bitcoin/bitcoin/blob/master/src/test/fuzz/addrman.cpp#L286-L290
* https://github.com/bitcoin/bitcoin/blob/master/src/test/fuzz/banman.cpp#L79
* https://github.com/bitcoin/bitcoin/blob/master/src/test/fuzz/bloom_filter.cpp#L
...

💬 maflcko commented on issue "Software wont launch":
(https://github.com/bitcoin/bitcoin/issues/29033#issuecomment-1848332041)
> Four hours have passed, software is stuck on "verifying blocks, press Q to close" initial screen.

The software is verifying the blocks that were downloaded previously. This may take a long time, depending on your system. You can look at the tail of the debug log to see the current exact status.

> I killed the process as it was only at 20% of "verifying", so I noticed it won't open for the entire day.

Killing means it'll likely have discarded the progress. This shouldn't cause any issu
...

💬 martinus commented on pull request "sync: improve CCoinsViewCache ReallocateCache":
(https://github.com/bitcoin/bitcoin/pull/28945#issuecomment-1848332095)
I only measured it on an SSD, and only with relatively low dbcache size so flushing is triggered more often which I think would give more accurate results. But more benchmarks are definitely appreciated :)

💬 maflcko commented on issue "Software wont launch":
(https://github.com/bitcoin/bitcoin/issues/29033#issuecomment-1848332099)
You can find the debug.log in your [data dir](https://github.com/bitcoin/bitcoin/blob/master/doc/files.md#data-directory-location).

Please be aware that the debug log might contain personally identifying information.

💬 maflcko commented on issue "test: Intermittent issue in rpc_net.py":
(https://github.com/bitcoin/bitcoin/issues/29030#issuecomment-1848332405)
Yes, I haven't looked at the code, but I presumed it was the same issue/fix.

💬 maflcko commented on pull request "test: fix `addnode` functional test failure on OpenBSD":
(https://github.com/bitcoin/bitcoin/pull/29035#issuecomment-1848360906)
lgtm. Is the code in 26.x and does it need backport?

💬 theStack commented on pull request "test: fix `addnode` functional test failure on OpenBSD":
(https://github.com/bitcoin/bitcoin/pull/29035#issuecomment-1848394286)
> lgtm. Is the code in 26.x and does it need backport?

No backport needed, the PR containing the code (#28155, commit 94e8882d820969ddc83f24f4cbe1515a886da4ea) was merged to master after 26.x branched off.

📝 maflcko opened a pull request: " refactor: Remove pre-C++20 code, fs::path cleanup"
(https://github.com/bitcoin/bitcoin/pull/29040)
This:

* Removes dead code.
* Avoids unused copies in some places.
* Adds copies in other places for safety.

💬 maflcko commented on pull request "Avoid returning references to mutex guarded members":
(https://github.com/bitcoin/bitcoin/pull/28774#issuecomment-1848396710)
> > No objection to changing the code, but if this is worth it to change, it should be done for all places in the whole codebase, not just some places.
>
> These are all reported (by clang 18). I see `ArgsManager::GetDataDir()` has the same issue. Maybe it was not detected because of the `?:` operator. I have extended the first commit to cover that too.

Thanks, I've taken the commit and put it in https://github.com/bitcoin/bitcoin/pull/29040, because it fits into the other fs::path changes
...

📝 theStack opened a pull request: "test: fix intermittent error in rpc_net.py (#29030)"
(https://github.com/bitcoin/bitcoin/pull/29041)
Asserting for the debug log message "Added connection peer=" is insufficient for ensuring that this new connection will show up in a following getpeerinfo() call, as the debug message is written in the CNode ctor, which means it hasn't necessarily been added to CConnman.m_nodes at this point.

Solve this by using the recently introduced `wait_for_new_peer` helper (see #29006, commit 00e0658e77f66103ebdeb29def99dc9f937c049d), which is more robust.

Fixes #29030.

💬 maflcko commented on pull request "test: fix intermittent error in rpc_net.py (#29030)":
(https://github.com/bitcoin/bitcoin/pull/29041#issuecomment-1848397786)
lgtm ACK ea00f982d21aab51001d422225f00626a74db298

💬 hebasto commented on pull request "Add missing byteswap functions for MSVC":
(https://github.com/bitcoin/bitcoin/pull/29036#issuecomment-1848407150)
> Converted this to draft because I haven't actually tested the compile for MSVC (relying on c-i :( )

It compiles OK.

> and because we'll need solid benchmarks before making any decisions.

What subset of benchmarks we might consider representative enough?

For now, I'm observing very tiny (a couple of %) improvements in some of them.

Also we should remember that MSVC build uses no optimisation (see e94ae810a55da16fce6040714677b9d50781bebd).

💬 josibake commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#issuecomment-1848409559)
ACK https://github.com/bitcoin/bitcoin/commit/0295b44c257fe23f1ad344aff11372d67864f0db

Thanks for taking the suggestions! Looks like the CI failure is a transient failure, I ran the same test locally and it passed. Probably just needs to be re-run.

📝 maflcko opened a pull request: "doc: Clarify C++20 comments"
(https://github.com/bitcoin/bitcoin/pull/29042)
Turns out "class template argument deduction for aggregates" is one of the few things implemented only in recent compilers, see https://en.cppreference.com/w/cpp/compiler_support/20

So clarify the comments.

💬 hebasto commented on pull request "refactor: Remove pre-C++20 code, fs::path cleanup":
(https://github.com/bitcoin/bitcoin/pull/29040#issuecomment-1848422903)
Concept ACK.

> This silences the following (clang 18):
>
> ```
> common/args.cpp:288:31: error: returning variable 'm_cached_blocks_path' by reference requires holding mutex 'cs_args' [-Werror,-Wthread-safety-reference-return]
> 288 | if (!path.empty()) return path;
> | ^
> ```

Does it happen on the master branch?

📝 martinus opened a pull request: "fuzz/util: make FuzzedDataProvider & FastRandomContext usage deterministic"
(https://github.com/bitcoin/bitcoin/pull/29043)
There exist many usages of `fuzzed_data_provider` where it is evaluated directly in the function call.
Unfortunately, the order of evaluation of function arguments is unspecified, and a simple example shows that it can differ e.g. between clang++ and g++: https://godbolt.org/z/jooMezWWY

When the evaluation order is not consistent, the same fuzzing/random input will produce different output, which is bad for coverage/reproducibility. This PR fixes all these cases I have found where unspecifie
...

💬 martinus commented on pull request "test: doc: follow-up #28368":
(https://github.com/bitcoin/bitcoin/pull/29013#discussion_r1421427984)
I created #29043

💬 murchandamus commented on pull request "Add multiplication operator to CFeeRate":
(https://github.com/bitcoin/bitcoin/pull/29037#discussion_r1421429261)
Sure, I updated it to use the same formula in both lines, so that only the parameters appear in either order