💬 maflcko commented on pull request "build: work around issue with Boost <= 1.80 and Clang >= 18":
(https://github.com/bitcoin/bitcoin/pull/30821#issuecomment-2333439434)
Can you explain this change a bit better?

The warning will be turned into a hard error in a follow-up release to clang 18.

Testing with clang-20 shows a new warning, according to the release notes https://github.com/llvm/llvm-project/pull/102364/files#diff-ec770381d76c859f5f572db789175fe44410a72608f58ad5dbb14335ba56eb97R138

```
In file included from /b-c/src/rest.cpp:22:
In file included from /b-c/src/rpc/blockchain.h:13:
In file included from /b-c/src/validation.h:28:
/b-c/src/txme
...

💬 maflcko commented on pull request "[28.x] Further backports and rc2":
(https://github.com/bitcoin/bitcoin/pull/30827#discussion_r1746652701)
Not sure if this is the right fix. It may avoid an error when using specific versions of Clang in combination with specific versions of boost. However, it may also result in a new warning with specific versions of Clang: https://github.com/bitcoin/bitcoin/pull/30821#issuecomment-2333439434

💬 vasild commented on pull request "test: add mocked Sock that can read/write custom data and/or CNetMessages":
(https://github.com/bitcoin/bitcoin/pull/30205#discussion_r1746663321)
No, the new `ZeroSock` constructor's body is now in `src/test/util/net.cpp` and it does

```cpp
ZeroSock::ZeroSock() : Sock{g_mocked_sock_fd++} {}
```

Because if all of the `ZeroSock` instances have `m_socket == INVALID_SOCKET`, then they would compare equal by `EqualSharedPtrSock`.

💬 Sjors commented on pull request "signet: fixing mining for OP_TRUE challenge":
(https://github.com/bitcoin/bitcoin/pull/29032#discussion_r1746681102)
I ended up making separate functions in 6f5419e3953781ade75d7986f293ce0fc13e90ce.

💬 Sjors commented on pull request "signet: fixing mining for OP_TRUE challenge":
(https://github.com/bitcoin/bitcoin/pull/29032#issuecomment-2333484176)
Thanks for the review, I took the suggestions.

It could be useful to add version grinding support to OP_TRUE signets.

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1746694411)
it's not the counting, it's to document that this version doesn't have negatives (like the excess version does)

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1746696954)
What's the advantage of the `string_view` version?

💬 vasild commented on pull request "test: add mocked Sock that can read/write custom data and/or CNetMessages":
(https://github.com/bitcoin/bitcoin/pull/30205#issuecomment-2333515870)
> Maybe split [187ba68](https://github.com/bitcoin/bitcoin/commit/187ba684a9d5e63d09b43511b7128cce9edbedf3) into one commit that moves the implementation and another that splits the class.

`e59097a0a5...e995ffa5c3`: do that ^

The cumulative diff before and after this push is identical:

```sh
$ diff -u <(git diff e59097a0a5~2..e59097a0a5) <(git diff e995ffa5c~3..e995ffa5c)
$
```

💬 maflcko commented on pull request "interpreter: use int32_t instead of int type for risczero compile":
(https://github.com/bitcoin/bitcoin/pull/30794#issuecomment-2333518330)
review-only ACK bc52cda1f3c007bdf1ed00aa3011e207c7531017

Changing `int` to `int32_t` in Bitcoin Core should always be fine, because they are assumed to be of the same bit-width and bit-representation. Generally, it is even preferred to use `int32_t`, especially in serialization code, because it documents the width explicitly.

However, I haven't tested this or reviewed that the patch is complete. It would be nice if there was an easy way to highlight all places in the codebase where distinc
...

💬 vasild commented on pull request "test: add mocked Sock that can read/write custom data and/or CNetMessages":
(https://github.com/bitcoin/bitcoin/pull/30205#discussion_r1746705958)
Just stylistic - I though it would be better to have it before the classes declarations. Should I move it at the bottom, where it was?

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1746708893)
> important thing is the error, not the exact wording of the error message

You have separate error for trailing `%`, why not have dedicated errors for when the format specifiers are fewer/more than the args?

💬 mzumsande commented on pull request "Halt processing of unrequested transactions v2":
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2333526650)
> Let’s say you have Peer1 and Peer2 announcing the Transaction ABC, if ABC is concurrently fetch from both Peer1 and Peer2, the `GETDATA` (32-bytes) for ABC would have been wasted as outgoing bandwidth by Local Node. This 32 byte leak can be amplified if batch of transactions are concurrently fetched (bounded by `_INVENTORY_BROADCAST_INTERVAL`).

That has nothing to do with free relay, because it doesn't propagate to the rest of the network (hence the "relay" in the name). It's also not a int
...

👍 l0rinc approved a pull request: "util: Use consteval checked format string in FatalErrorf"
(https://github.com/bitcoin/bitcoin/pull/30546#pullrequestreview-2285402162)
While I would still prefer some of our comments to be considered, I'm also fine with merging the change as is - thanks for making the developer experience more streamlined via these small-but-useful checks!

ACK fa092749094aa483e3ce0243885ce2eb8ed22cbb

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1746712867)
Just being a bit more explicit. A single `s` character may otherwise be missed or removed in the future, leading to a raw pointer compare, which would be wrong.

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1746714182)
> why not

The reason is in my previous reply.

✅ fanquake closed an issue: "cmake: Step "-- Looking for C++ include boost/test/included/unit_test.hpp" takes a long time"
(https://github.com/bitcoin/bitcoin/issues/30787)

🚀 fanquake merged a pull request: "cmake: scope Boost Test check to `vcpkg`"
(https://github.com/bitcoin/bitcoin/pull/30822)

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1746719104)
Ok, in that case I'd prefer `unsigned count{0};`. I may switch to that, if I have to re-touch.

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1746720865)
ok, that's fair, thanks.

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1746725437)
I saw your previous response, I was quoting from it in mine. Anyway, you can resolve this comment.