💬 laanwj commented on pull request "torcontrol: Define tor reply code as const to improve our maintainability":
(https://github.com/bitcoin/bitcoin/pull/32166#discussion_r2020460428)
nit: Please use the same comment syntax as the other comments

💬 eval-exec commented on pull request "torcontrol: Define tor reply code as const to improve our maintainability":
(https://github.com/bitcoin/bitcoin/pull/32166#discussion_r2020461693)
Thank you, fixed.

💬 maflcko commented on pull request "rpc: Support v3 raw transactions creation":
(https://github.com/bitcoin/bitcoin/pull/31936#issuecomment-2765256363)
Are you still working on this?

💬 saikiran57 commented on pull request "wallet: removed duplicate call to GetDescriptorScriptPubKeyMan":
(https://github.com/bitcoin/bitcoin/pull/32023#issuecomment-2765272443)
Hi @maflcko could you please ACK if your okay with response.

💬 laanwj commented on pull request "torcontrol: Define tor reply code as const to improve our maintainability":
(https://github.com/bitcoin/bitcoin/pull/32166#discussion_r2020484368)
It's strange that the only torproject.org link for the C Tor manual is a 2019 backup of the site, but haven't found anything better either.
The only up-to-date canonical source is the manual page, which isn't particularly readable on the web https://gitlab.torproject.org/tpo/core/tor/-/blob/main/doc/man/tor.1.txt
(nothing to do here really, just noting)

👍 laanwj approved a pull request: "torcontrol: Define tor reply code as const to improve our maintainability"
(https://github.com/bitcoin/bitcoin/pull/32166#pullrequestreview-2728361165)
Code review ACK f31ce35966bb84608938b0ba2272b415bcd42618

💬 eval-exec commented on pull request "torcontrol: Define tor reply code as const to improve our maintainability":
(https://github.com/bitcoin/bitcoin/pull/32166#discussion_r2020488844)
Yes the web url link is old, but the `IsolateSOCKSAuth` explaination content is same with https://gitlab.torproject.org/tpo/core/tor/-/blob/main/doc/man/tor.1.txt

💬 laanwj commented on pull request "torcontrol: Define tor reply code as const to improve our maintainability":
(https://github.com/bitcoin/bitcoin/pull/32166#discussion_r2020497489)
Oh yes the site is correct, i'm just a bit worried about it going offline at some point, so was trying to find a more up-to-date official link. But it doesn't exist.

💬 Bue-von-hon commented on pull request "rpc: Support v3 raw transactions creation":
(https://github.com/bitcoin/bitcoin/pull/31936#issuecomment-2765314756)
> Are you still working on this?

sure!

💬 laanwj commented on pull request "depends: Switch from multilib to platform-specific toolchains":
(https://github.com/bitcoin/bitcoin/pull/32162#discussion_r2020506583)
No strong opinion on XX-bit versus XX bit, but mind that i made the opposite change (for consistently) recently in a0c9595810c7d8bb17d8b5bea8d916db194b5239

💬 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