🤔 ryanofsky reviewed a pull request: "multiprocess: Add -ipcbind option to bitcoin-node"
(https://github.com/bitcoin/bitcoin/pull/30509#pullrequestreview-2284579707)
Rebased af24810eeed397c2fe5f61ef9769e1b7ee0ecdf9 -> e225f7cbc3e6842f8e7f1c482c2aacd810e99c1b ([`pr/ipc-bind.5`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-bind.5) -> [`pr/ipc-bind.6`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-bind.6), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/ipc-bind.5-rebase..pr/ipc-bind.6)) implementing suggestions and updating to use cmake

re: https://github.com/bitcoin/bitcoin/pull/30509#pullrequestreview-2263113816

> It would be ni
...

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

> I think this should be added to the `rpc_help.py` tests.

Is there mistake in this suggestion? This shouldn't be related to RPC

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

> Nit: There is a problem with the indentation here.

Thanks! Fixed

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