💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2288892100)
personally speaking I'm unsure what is gained by moving the fuzz tests to their own PR since they depend on the refactors. I don't think we want to merge the other commits unless the fuzz harnesses are attached to it. I'm continuing to review either way.

👍 ryanofsky approved a pull request: "log: expand BCLog::LogFlags (categories) to 64 bits"
(https://github.com/bitcoin/bitcoin/pull/26619#pullrequestreview-2238322169)
Code review ACK b31a0cd0378184b2b9eb8f4bd3120cbd32c62005, just dropping mask_bit constant since last review. I still think
#26697 is a better alternative to this PR because it is more type-safe and gets rid of bitwise flags and operations outside the bitset class, but it could also be a followup to this PR.

📝 hebasto opened a pull request: "test: Fix test log file name"
(https://github.com/bitcoin/bitcoin/pull/30654)
https://github.com/bitcoin/bitcoin/pull/19385 dropped `.cpp` infix (see https://github.com/bitcoin/bitcoin/pull/19385#issuecomment-1078960406 and https://github.com/bitcoin/bitcoin/pull/19385#issuecomment-1078970958). However, `src/test/README.md` still refers to the `foo_tests.cpp.log` pattern.

This PR restores the pre-PR19385 behaviour, as appending is easier to implement than replacing when porting this functionality to CMake.

🚀 hebasto merged a pull request: "Update translation source file for v28.0 string freeze"
(https://github.com/bitcoin-core/gui/pull/833)

💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2289016249)
> I don't think we want to merge the other commits unless the fuzz harnesses are attached to it. I'm continuing to review either way.

Ok sounds good, I'll leave as is. Just hoping to make reviewer easier.

💬 ryanofsky commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717082261)
re: https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716226929

The comment seems not true and not helpful.

- If the concern is _not_ stack space used inside the function, and this warning is only being added because the return type of the function is a variable-sized std::array, then I'm not sure why every function that returns a variable-sized std::array shouldn't have the same warning. This actually seems less deserving of a warning than other functions returning a std::array o
...

💬 glozow commented on pull request "Move maximum timewarp attack threshold back to 600s from 7200s":
(https://github.com/bitcoin/bitcoin/pull/30647#issuecomment-2289038179)
Makes sense to me, sanity checked you can sync with this. Is https://github.com/TheBlueMatt/bips/blob/7f9670b643b7c943a0cc6d2197d3eabe661050c2/bip-XXXX.mediawiki still the right place to see great consensus cleanup spec?

📝 glozow opened a pull request: "doc: mention bip94 support"
(https://github.com/bitcoin/bitcoin/pull/30655)
Followup to #29775, noticed while looking at #30604 and #30647. See [release process](https://github.com/bitcoin/bitcoin/blob/master/doc/release-process.md#before-every-major-and-minor-release).

💬 maflcko commented on pull request "doc: mention bip94 support":
(https://github.com/bitcoin/bitcoin/pull/30655#issuecomment-2289070915)
ACK 99eeb51bf65fa00ee863f32d70f478bb9db0e351

💬 paplorinc commented on pull request "coins: Add move operations to Coin and CCoinsCacheEntry":
(https://github.com/bitcoin/bitcoin/pull/30643#issuecomment-2289075127)
> configure clang-tidy to produce this warning in our own CI instead of Sonar

The ["coverage report"]( https://github.com/bitcoin/bitcoin/pull/30643#issuecomment-2284919165) already has the Sonarcloud section - I have opened this issue because of the #28280 corecheck [CI Sonar warnings](https://corecheck.dev/bitcoin/bitcoin/pulls/28280). Or do you mean we should be able to reproduce this locally as well via `clang-tidy`?

> if I want a class that is guaranteed to have a move constructor, I
...

📝 paplorinc opened a pull request: "coins: Simplify moves to ternary in `coins.cpp`"
(https://github.com/bitcoin/bitcoin/pull/30656)
Split out of https://github.com/bitcoin/bitcoin/pull/30643

See related discussions:
* https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1676800505
* https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1676801434
* https://github.com/bitcoin/bitcoin/pull/17487#discussion_r402037193

And reproducer that demonstrates the behavior of all 3 cases:
* https://gist.github.com/paplorinc/66f13938d867d82893d7ab7a2ebc5717

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1717123190)
Addressed in https://github.com/bitcoin/bitcoin/pull/30656

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1717123576)
Addressed in https://github.com/bitcoin/bitcoin/pull/30656

💬 maflcko commented on pull request "coins: Add move operations to Coin and CCoinsCacheEntry":
(https://github.com/bitcoin/bitcoin/pull/30643#issuecomment-2289087961)
> > configure clang-tidy to produce this warning in our own CI instead of Sonar
>
> The ["coverage report"](https://github.com/bitcoin/bitcoin/pull/30643#issuecomment-2284919165) already has the Sonarcloud section - I have opened this issue because of the #28280 corecheck [CI Sonar warnings](https://corecheck.dev/bitcoin/bitcoin/pulls/28280). Or do you mean we should be able to reproduce this locally as well via `clang-tidy`?
>
> > if I want a class that is guaranteed to have a move constr
...

💬 paplorinc commented on pull request "coins: allow write to disk without cache drop":
(https://github.com/bitcoin/bitcoin/pull/17487#discussion_r1717126291)
FYI: I've revived the if-vs-ternary for ::move in https://github.com/bitcoin/bitcoin/pull/30656

💬 sipa commented on pull request "coins: Add move operations to Coin and CCoinsCacheEntry":
(https://github.com/bitcoin/bitcoin/pull/30643#issuecomment-2289092299)
@maflcko `etd:is_nothrow_move_constructible_v` will also return true if a nothrow copy constructor is available (because a copy constructor is eligible in any context where a move constructor is called), so that isn't enough to verify that an efficient version exists.

💬 maflcko commented on pull request "coins: Simplify std::move to ternary in `coins.cpp`":
(https://github.com/bitcoin/bitcoin/pull/30656#discussion_r1717133218)
Can you explain what problem switching an `if` to ternary is solving? Linking to 3 discussion threads and one external gist, which may be deleted at any time seems not helpful to reviewers.

💬 fanquake commented on pull request "test: Fix test log file name":
(https://github.com/bitcoin/bitcoin/pull/30654#issuecomment-2289104629)
> However, src/test/README.md still refer
> This PR restores the pre-PR19385 behaviour,

Not sure this PR should be called a "fix"? As it seems the fix would be to just update the docs to match the current (intended) behaviour.

This instead seems to be a reversion to prior behaviour to facilitate something CMake related. I'm curious why this has only become an issue now, given we've already got the tests in CMake? Or is this something that was missing and is being ported now?

💬 maflcko commented on pull request "test: Fix test log file name":
(https://github.com/bitcoin/bitcoin/pull/30654#issuecomment-2289116723)
Not sure about changing the test-only behavior here. Seems fine to just adjust the readme in the cmake pull, as the line has to be touched anyway. No need to be exact in porting this edge case?

📝 theStack opened a pull request: "test: add functional test for XORed block/undo files (`-blockxor` option)"
(https://github.com/bitcoin/bitcoin/pull/30657)
This PR adds a dedicated functional test for XORed block data/undo file support (bitcoind option `-blocksxor`, see PR #28052). In order to verify that the XOR pattern has been applied, the {blk,rev}*.dat files are rewritten un-XORed manually by the test while the node is shut down; the node is then started again with `-blocksxor=0`, and both the data and undo files are verified via the `verifychain` RPC (with checklevel=2). Note that starting bitcoind with `-blocksxor=0` fails if a xor key is pr
...