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

💬 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_r1816776293)
Seems a bit excessive for a test, but ended up changing it to e.g. `const size_t key_size{rng.randrange(static_cast<size_t>(10))};`. Will push a bit later.