💬 ryanofsky commented on pull request "multiprocess: Add -ipcbind option to bitcoin-node":
(https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1746474704)
re: https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1733490705

> I think it would be good to sanity-check that invalid addresses make `bind` and `connect` throw:

Thanks added suggested tests.

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1746523131)
> By counting the excess we could give more fine-grained errors, e.g:

I don't think this is useful, because the error message will say that there are "too few/many format specifiers". However, the inverse might be true: "Too many/few args".

So I'll leave this as-is for now. I'd say the important thing is the error, not the exact wording of the error message.

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1746523781)
> If you still insist that this is better, please resolve the comment.

Yes, for now I'll resolve this. Thanks for the review!

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1746525983)
Compilers can only walk so far in a consteval/constexpr context, so that it doesn't matter if there are 31 bits to count, or 64.

I'll leave this as-is for now.

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#issuecomment-2333261303)
> throw "Invalid format specifier!";


In tinyformat there is no such thing, so I'll leave this as-is for now.

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1746533300)
Thanks, switched to lambdas! (Kept the string_view for now)

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#issuecomment-2333269373)
Replied to all review comments and force pushed a small style-only change in the test.

👍 TheCharlatan approved a pull request: "multiprocess: Add -ipcbind option to bitcoin-node"
(https://github.com/bitcoin/bitcoin/pull/30509#pullrequestreview-2285020731)
Re-ACK e225f7cbc3e6842f8e7f1c482c2aacd810e99c1b

💬 TheCharlatan commented on pull request "multiprocess: Add -ipcbind option to bitcoin-node":
(https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1746579005)
Huh, no idea what I was thinking here, but `rpc_help.py` is obviously wrong. Maybe I copy-pasted a wrong value? I think where it could go is the `system.cpp` fuzz tests.

💬 Sjors commented on pull request "signet: fixing mining for OP_TRUE challenge":
(https://github.com/bitcoin/bitcoin/pull/29032#discussion_r1746620425)
Oops, should be `OP_TRUE`

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