💬 laanwj commented on pull request "depends: Switch from multilib to platform-specific toolchains":
(https://github.com/bitcoin/bitcoin/pull/32162#discussion_r2020508512)
i've always been confused about the `-pc-` in the architecture tuple-is it directly related to use (or non-use) of multilib?

💬 laanwj commented on pull request "depends: Switch from multilib to platform-specific toolchains":
(https://github.com/bitcoin/bitcoin/pull/32162#issuecomment-2765334131)
Concept ACK, multilib is more or less specific to x86, it's better if multi-platform is handled in a more agnostic and consistent way.

💬 hebasto commented on pull request "depends: Switch from multilib to platform-specific toolchains":
(https://github.com/bitcoin/bitcoin/pull/32162#discussion_r2020523649)
I referred to the following sources:
- https://en.wikipedia.org/wiki/32-bit_computing
- https://en.wikipedia.org/wiki/64-bit_computing

Happy to revert if requested.

💬 hebasto commented on pull request "depends: Switch from multilib to platform-specific toolchains":
(https://github.com/bitcoin/bitcoin/pull/32162#discussion_r2020536135)
It's hard to say.

Referring to `x86_64-pc-linux-gnu` is still necessary when building depends natively on `x86_64`, because:
```
$ uname -m
x86_64
$ ./depends/config.sub $(./depends/config.guess)
x86_64-pc-linux-gnu
```

However, dropping the `-pc-` infix [helped](https://github.com/google/oss-fuzz/pull/13187) to with Clang's paths in the OSS-Fuzz environment.

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