💬 laanwj commented on pull request "depends: Switch from multilib to platform-specific toolchains":
(https://github.com/bitcoin/bitcoin/pull/32162#discussion_r2020564681)
No it's fine with me let's just not get into an edit war 😄

🤔 rkrux requested changes to a pull request: "test: create assert_not_equal util"
(https://github.com/bitcoin/bitcoin/pull/29500#pullrequestreview-2728485178)
Concept ACK be71af3cc0b0bcb7d917cc6f2e5fda119f1b1bd6

Requesting changes mainly because an additional comma is printed in case of an assertion error when the error message is not passed, which can be confusing for the reader later.

```
2025-03-31T07:28:44.958000Z TestFramework (INFO): Test fundrawtxn with locked wallet and hardened derivation
2025-03-31T07:28:46.455000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/Users/rkrux/projects/bitcoin/test/
...

💬 rkrux commented on pull request "test: create assert_not_equal util":
(https://github.com/bitcoin/bitcoin/pull/29500#discussion_r2020563537)
I don't mind having an error message being logged and I do agree with the following points:

> It is not a named arg, nor type-safe, so someone writing assert_not_equal("a", "b", "c") or assert_not_equal(1, 2, 3) will be wrong and confusing at best.

> Unique and descriptive error messages make sense when it is the only piece of information given (for example in the context of an init error during startup of the program).

As I see from the usages of `assert_not_equal`, there is only 1 cas
...

💬 laanwj commented on pull request "depends: Switch from multilib to platform-specific toolchains":
(https://github.com/bitcoin/bitcoin/pull/32162#discussion_r2020578974)
Hrm, yea, `config.sub` adds it for x86 archs (but `unknown` for others) so it's likely canonical somehow.
```
$ ./config.sub i386-linux-gnu
i386-pc-linux-gnu
$ ./config.sub x86_64-linux-gnu
x86_64-pc-linux-gnu
$ ./config.sub arm-linux-gnueabihf
arm-unknown-linux-gnueabihf
$ ./config.sub riscv64-linux-gnu
riscv64-unknown-linux-gnu
```
But seems fine to leave it out in the `README.md` because it doesn't include the `-unknown-` infix either.

👍 hodlinator approved a pull request: "build: Switch to Qt 6"
(https://github.com/bitcoin/bitcoin/pull/30997#pullrequestreview-2728523776)
re-ACK c4861570e468d0f52fc7580b75098a82b4ead5f3

Excellent feedback by @fanquake in pointing out patches we could drop and unused tools we built. Thanks @hebasto for not loosing steam!

Changes since [previous review](https://github.com/bitcoin/bitcoin/pull/30997#pullrequestreview-2705248957), 7b51bf1d4c1933677867f7364a2378e41bc23929:

* Reduced complexity of cmake/module/FindQt.cmake compared to Qt5. This was probably what was leading to picking up libraries outside of depends - https://g
...

💬 maflcko commented on pull request "refactor: Avoid copies by using const references or by move-construction":
(https://github.com/bitcoin/bitcoin/pull/31650#issuecomment-2765470297)
Dropped the second commit, changed pull title and description, and rebased. Should be trivial to re-review.

💬 maflcko commented on pull request "refactor: Avoid copies by using const references or by move-construction":
(https://github.com/bitcoin/bitcoin/pull/31650#discussion_r2020590366)
thx, done

💬 maflcko commented on pull request "refactor: Avoid copies by using const references or by move-construction":
(https://github.com/bitcoin/bitcoin/pull/31650#discussion_r2020590512)
thx, done

💬 maflcko commented on pull request "refactor: Avoid copies by using const references or by move-construction":
(https://github.com/bitcoin/bitcoin/pull/31650#discussion_r2020590604)
> I think it's a good change, I like consistency and though a few other similar ones could be added here.

I don't think it is possible to achieve "consistency" here. There is no automated check to catch those in definitions. The goals here are:

* Performance improvements (by the first commit). However, there are no benchmarks, so this is questionable.
* Readability and style related improvements. However, they are not enforced consistently, so this is questionable as well.

So I'll drop
...

💬 maflcko commented on pull request "refactor: Avoid copies by using const references or by move-construction":
(https://github.com/bitcoin/bitcoin/pull/31650#discussion_r2020591301)
(dropped commit for now)

💬 willcl-ark commented on pull request "net, pcp: handle multi-part responses and filter for default route while querying default gateway":
(https://github.com/bitcoin/bitcoin/pull/32159#discussion_r2020591415)
Thanks, that infradead page is very handy!

I made a few changes in 6c694c212f8d898dac9cc1c5637381452c91e79b based on your thoughts (and the protocol page):

- set socket to non-blocking mode to avoid hanging if the kernel doesn't send a response
- use a vector to collect all data from multi-part responses
- exit when `recv()` returns 0 (this should handle single-part messages, AFAICT)

I think relying on receiving no more data from `recv()` to break the receive loop should be as (or per
...

💬 hebasto commented on pull request "depends: Switch from multilib to platform-specific toolchains":
(https://github.com/bitcoin/bitcoin/pull/32162#discussion_r2020593162)
Also please note that `depends/README.md` lists triplets after specifically mentioning "for cross compilation". In such cases: https://github.com/bitcoin/bitcoin/blob/4c1906a500cacab385b09e780b54271b0addaf4b/depends/hosts/default.mk#L1-L3 and toolchain prefixes usually do not include the `-pc-` infix.

💬 Sjors commented on pull request "ci: build multiprocess on most jobs":
(https://github.com/bitcoin/bitcoin/pull/30975#discussion_r2020635559)
Trying this: https://stackoverflow.com/a/21699210

💬 Sjors commented on pull request "ci: build multiprocess on most jobs":
(https://github.com/bitcoin/bitcoin/pull/30975#discussion_r2020647766)
Oh no that makes no sense, this is not yaml.

💬 Sjors commented on pull request "ci: build multiprocess on most jobs":
(https://github.com/bitcoin/bitcoin/pull/30975#discussion_r2020660927)
I added the single space indentation, but otherwise no changes. When testing locally, dropping `\` breaks cmake.

💬 Sjors commented on pull request "ci: build multiprocess on most jobs":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2765616858)
Rebased.

💬 laanwj commented on pull request "net, pcp: handle multi-part responses and filter for default route while querying default gateway":
(https://github.com/bitcoin/bitcoin/pull/32159#discussion_r2020671105)
> I think relying on receiving no more data from recv() to break the receive loop should be as (or perhaps more) robust than checking for the NLM_F_MULTI flag and exiting after first receive if it's not set, but curious what you think here?

Maybe-is it safe to assume that netlink will never block?

We don't want to end up in the same situation as before where we miss data. But due to say, a threading race condition.

i think the safest thing here is to mimic as closely as possible `ip`'s
...

💬 laanwj commented on pull request "net, pcp: handle multi-part responses and filter for default route while querying default gateway":
(https://github.com/bitcoin/bitcoin/pull/32159#discussion_r2020674715)
> use a vector to collect all data from multi-part responses

i'm not sure i see the motivation here. Parsing the packets as seperate units is just as valid, avoids dynamic allocation, and is simpler.

💬 l0rinc commented on pull request "refactor: Avoid copies by using const references or by move-construction":
(https://github.com/bitcoin/bitcoin/pull/31650#issuecomment-2765643722)
crACK fa9cd7bd4f8b4bad4b01bb3709f91d2a651fe47d

<details>
<summary>Diff compared to previous review</summary>

```patch
diff --git a/src/test/argsman_tests.cpp b/src/test/argsman_tests.cpp
--- a/src/test/argsman_tests.cpp (revision 17cb88fefcc390967091a4f63bd04a1b3cefcda1)
+++ b/src/test/argsman_tests.cpp (date 1743412333616)
@@ -52,7 +52,7 @@
struct TestArgsManager : public ArgsManager
{
TestArgsManager() { m_network_only_args.clear(); }
- void ReadConfigString(const std:
...

🤔 hebasto reviewed a pull request: "multiprocess: Add libmultiprocess git subtree"
(https://github.com/bitcoin/bitcoin/pull/31741#pullrequestreview-2728662979)
For now, I've reviewed all commits except the last three that touch the depends build subsystem.

Left a few comments.

💬 hebasto commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r2020675936)
88eabc8c2c0fbb62db5cb6248cc8ab770f3ce78b, nit:
```suggestion
# Copyright (c) 2025-present The Bitcoin Core developers
```