💬 jonatack commented on pull request "tinyformat: enforce compile-time checks for format string literals":
(https://github.com/bitcoin/bitcoin/pull/31149#discussion_r1816714440)
Thanks for the example. As the primary author of this code, I think the run-time error would be more clear, though a compile-time error would catch it one CLI command earlier. I haven't found this kind of error to be an issue for me over the past half decade on this code.

Also, this code path is manually tested by myself, reviewers and users. Perhaps for that reason, and because it's non-critical code, a maintainer explicitly asked that no test coverage be written for it.

So either way is
...

💬 jonatack commented on pull request "tinyformat: enforce compile-time checks for format string literals":
(https://github.com/bitcoin/bitcoin/pull/31149#discussion_r1816718706)
> I'm just going with our clang-format settings as defined in src/.clang-format

It's not a hard requirement to retouch existing code with that tool, and I'd prefer avoiding the churn.

🚀 fanquake merged a pull request: "optimization: Preallocate addresses in GetAddr based on nNodes"
(https://github.com/bitcoin/bitcoin/pull/29608)

💬 instagibbs commented on pull request "Package validation: accept packages of size 1":
(https://github.com/bitcoin/bitcoin/pull/31096#issuecomment-2437829923)
@stickies-v `IsWellFormedPackage` coverage already looks good to me, added our unit test for single tx package processing, thanks

🤔 marcofleon reviewed a pull request: "fuzz: fuzz connman with non-empty addrman + ASMap"
(https://github.com/bitcoin/bitcoin/pull/29536#pullrequestreview-2395370037)
Code review ACK 552cae243a1bf26bfec03eccd1458f3bf33e01dc. Changes match the PR description.

💬 fanquake commented on pull request "optimization: Preallocate addresses in GetAddr based on nNodes":
(https://github.com/bitcoin/bitcoin/pull/29608#issuecomment-2437847307)
The `clang-tidy` check I was thinking of was https://clang.llvm.org/extra/clang-tidy/checks/performance/inefficient-vector-operation.html.

💬 real-or-random commented on pull request "cleanse: switch to SecureZeroMemory for Windows cross-compile":
(https://github.com/bitcoin/bitcoin/pull/26950#issuecomment-2437850544)
> @real-or-random @laanwj's comment is mostly correct. iirc at the time this was one of a few MSVC specific defines we had left in the codebase. Since then others have been added, but mostly to just workaround, or ignore bugs in MSVC.

Thanks, makes sense.

>
> > (except perhaps performance if the implementation of SecureZeroMemory on mingw-w64 is worse than what the compiler can come up with for the memset).
>
> I would think the code would not be worse performance wise, as it's implem
...

💬 brunoerg commented on pull request "[tests] New fuzz target wallet_rpc":
(https://github.com/bitcoin/bitcoin/pull/30570#issuecomment-2437854933)
I think we will have here the same problems that we usually have for other wallet targets. The size of keypool, wallet encryption, etc... may cause timeout issues.

💬 stickies-v commented on pull request "tinyformat: enforce compile-time checks for format string literals":
(https://github.com/bitcoin/bitcoin/pull/31149#discussion_r1816740327)
> As the primary author of this code, I think the run-time error would be more clear

I'm sorry, what? For what reason would we not use the logic we already have to catch (some/most) of these format errors at compile-time, and defer them to run-time instead, relying on manual review to make sure each code-path is ran, potentially crashing user software? That seems like the opposite of security engineering best practices to me.

> a maintainer explicitly asked that no test coverage be written
...

👍 TheCharlatan approved a pull request: "tinyformat: enforce compile-time checks for format string literals"
(https://github.com/bitcoin/bitcoin/pull/31149#pullrequestreview-2395385698)
Nice, ACK 23560b468c474bbc6a3abd6c0778da62766ac8f7

🤔 hodlinator reviewed a pull request: "optimization: pack util::Xor into 64/32 bit chunks instead of doing it byte-by-byte"
(https://github.com/bitcoin/bitcoin/pull/31144#pullrequestreview-2395174160)
Concept ACK a3dc138798e2f2c7aa1c9b37633c16c1b523a251

Operating on CPU words rather than individual bytes. :+1:

Not entirely clear to me from https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1814741753 whether the optimizer is able to use SIMD. Guess picking through the binary of a GUIX-build would give a definitive answer.

The verbosity of `std::memcpy` hurts readability but alignment issues are real.

💬 hodlinator commented on pull request "optimization: pack util::Xor into 64/32 bit chunks instead of doing it byte-by-byte":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1816662396)
Might as well use `std::span` for new code.

💬 hodlinator commented on pull request "optimization: pack util::Xor into 64/32 bit chunks instead of doing it byte-by-byte":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1816612914)
Seems like prior author just rounded off to some *not too unreasonable* tab-indentation (efd2474d17098c754367b844ec646ebececc7c74). Function isn't touched in this PR so should probably resist touching here and below.

💬 hodlinator commented on pull request "optimization: pack util::Xor into 64/32 bit chunks instead of doing it byte-by-byte":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1816619457)
Experimented with changed to brace-initialization which uncovered some slight narrowing/widening occurring. (Thought I had an angle for making the code more robust in a more material way but that attempt failed).
```suggestion
auto expected_xor{[](Span<std::byte> write, Span<const std::byte> key, size_t key_offset) {
if (key.size()) {
for (auto& b : write) {
b ^= key[key_offset++ % key.size()];
}
}
}};

FastRandomConte
...

💬 rkrux commented on pull request "functional test: Additional package evaluation coverage":
(https://github.com/bitcoin/bitcoin/pull/31152#discussion_r1816744003)
Thanks for adding this comment, helpful!

💬 rkrux commented on pull request "functional test: Additional package evaluation coverage":
(https://github.com/bitcoin/bitcoin/pull/31152#discussion_r1816746874)
IIUC shouldn't `usage` be used here instead of `bytes` because the first argument of assert uses memory usage?
https://github.com/bitcoin/bitcoin/blob/master/src/rpc/mempool.cpp#L699-L700

💬 TheCharlatan commented on pull request "tinyformat: enforce compile-time checks for format string literals":
(https://github.com/bitcoin/bitcoin/pull/31149#discussion_r1816753074)
> I'd prefer avoiding the churn.

We usually call things churn that change things multiple times within the same PR. Indenting the lines below a changed line that belong to the same logical unit is a good thing in my eyes. If we would not do that anywhere, the code would look weird and probably make `clang-format` unusable after a while.

🤔 stickies-v reviewed a pull request: "util: Treat Assume as Assert when evaluating at compile-time"
(https://github.com/bitcoin/bitcoin/pull/31150#pullrequestreview-2395423920)
Post-merge ACK fa69a5f4b76a4e2a02db6c32d9c3311ce5fe29bd - nice!

💬 stickies-v commented on pull request "tinyformat: enforce compile-time checks for format string literals":
(https://github.com/bitcoin/bitcoin/pull/31149#discussion_r1816767397)
> The rest of this PR seems fine.

We can't have the rest of this PR without updating `bitcoin-cli.cpp`, unless someone implements the complete format string validation logic, for which as per my earlier comment, see https://github.com/bitcoin/bitcoin/pull/30546.

💬 l0rinc commented on pull request "optimization: pack util::Xor into 64/32 bit chunks instead of doing it byte-by-byte":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1816775973)
Seems it was a [deliberate formatting](https://github.com/bitcoin/bitcoin/commit/efd2474d17098c754367b844ec646ebececc7c74#diff-73552341d85aec344497b47f1c2aa53a7e01a415030e40f56b1c454aef5f209dR221-R222), so I'll revert.
Will push after I have the block serialization benchmark working.