💬 maflcko commented on pull request "Lint: support running individual rust linters and improve subtree exclusion":
(https://github.com/bitcoin/bitcoin/pull/29965#issuecomment-2078730201)
The Win64 GHA test failure looks unrelated. If you want, you can split up 54b2115bb848e1aad8a86aae858de63a851235ce into its own pull request, as it is an independent improvement. But up to you.

💬 maflcko commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1580556442)
https://github.com/bitcoin/bitcoin/pull/29967/files

💬 laanwj commented on pull request "test: Add two more urlDecode tests":
(https://github.com/bitcoin/bitcoin/pull/29967#issuecomment-2078798420)
Code review ACK https://github.com/bitcoin/bitcoin/pull/29967/commits/fa55972a758865a6bd0114afe72e51877896d495

💬 fanquake commented on pull request "ZMQ: Support UNIX domain sockets":
(https://github.com/bitcoin/bitcoin/pull/27679#discussion_r1580606777)
FYI https://github.com/bitcoin/bitcoin/runs/24257306970:
```bash
init.cpp: In function ‘bool AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)’:
init.cpp:1319:20: error: unused variable ‘unix’ [-Werror=unused-variable]
1319 | const bool unix{port_option.second};
| ^~~~
```

👍 stickies-v approved a pull request: "test: Add two more urlDecode tests"
(https://github.com/bitcoin/bitcoin/pull/29967#pullrequestreview-2024343300)
ACK fa55972a758865a6bd0114afe72e51877896d495

💬 fjahr commented on pull request "test: Add two more urlDecode tests":
(https://github.com/bitcoin/bitcoin/pull/29967#issuecomment-2078824038)
ACK fa55972a758865a6bd0114afe72e51877896d495

💬 fanquake commented on pull request "refactor: remove remaining unused code from cpp-subprocess":
(https://github.com/bitcoin/bitcoin/pull/29961#issuecomment-2078841172)
> Note that there are some API functions of the Popen class that we don't use, e.g. wait(), pid(), poll(), kill(), but they sound IMHO common enough to be useful in the future, so not sure how deep we should go there.

Deleting as much as possible seems fine. If code turns out to be needed in future (although I wouldn't think we are going to expand this interface), it can be retrieved from the git history.

💬 maflcko commented on pull request "depends: Do not consider `CC` environment variable for detecting system":
(https://github.com/bitcoin/bitcoin/pull/29963#issuecomment-2078866286)
> The log shows no cross-compiling, but it is cross-compiling to `i686-pc-linux-gnu`.

Can you link to the exact lines in the log that show "no cross-compiling".

📝 maflcko opened a pull request: "refactor: Avoid unused-variable warning in init.cpp"
(https://github.com/bitcoin/bitcoin/pull/29968)
Fixes https://github.com/bitcoin/bitcoin/pull/27679#discussion_r1580606777

💬 maflcko commented on pull request "ZMQ: Support UNIX domain sockets":
(https://github.com/bitcoin/bitcoin/pull/27679#discussion_r1580659104)
Fixed in #29968

💬 laanwj commented on pull request "build: add `-Wundef`":
(https://github.com/bitcoin/bitcoin/pull/29876#discussion_r1580663336)
i don't know, i don't think what i imagined is actually doable: make sure the entire build system->program interface consists of 0/1 defines (mandatory) not error-prone `#define`/`#undef`. There's just too many. So resolving this, retracting my comment.

👍 laanwj approved a pull request: "build: add `-Wundef`"
(https://github.com/bitcoin/bitcoin/pull/29876#pullrequestreview-2024427057)
ACK cc09c165eb0bbb4b41a9132734bee55969e9d9a8

👍 Sjors approved a pull request: "test: Add two more urlDecode tests"
(https://github.com/bitcoin/bitcoin/pull/29967#pullrequestreview-2024435378)
utACK fa55972a758865a6bd0114afe72e51877896d495

💬 Sjors commented on pull request "test: Add two more urlDecode tests":
(https://github.com/bitcoin/bitcoin/pull/29967#discussion_r1580675570)
I had to scratch my head on this one...

It iterates from left to right, the first thing it finds is `%`. It then tries if the next two characters are hex, which they are not, so it puts the literal `%` in the result. Then it moves to the next `%` which is followed by valid hex `ff`. Since `0xff` is [invalid unicode](https://stackoverflow.com/questions/5657467/is-byte-0xff-valid-in-a-utf-8-encoded-string), we have to represent it with the [escape sequence](https://en.cppreference.com/w/cpp/lan
...

🚀 fanquake merged a pull request: "test: Add two more urlDecode tests"
(https://github.com/bitcoin/bitcoin/pull/29967)

💬 Sjors commented on pull request "refactor: remove remaining unused code from cpp-subprocess":
(https://github.com/bitcoin/bitcoin/pull/29961#issuecomment-2078920046)
As long as #29868 works on top of this, it's fine by me. I'll do some testing.

💬 laanwj commented on pull request "guix: remove bzip2 from deps":
(https://github.com/bitcoin/bitcoin/pull/29895#issuecomment-2078921911)
> The current set of the changes look fine. However, to actually review all 10 changed packages, it would be nice to have a script that prints whether the tarball has any differences from the upstream source control it claims to come from.

i expect the `gz` and `bz2` are the same `tar` data just compressed with a different compressor. Going to check, though.

This, along with seeing if the binary output is the same (apart from version markers) before and after this PR.

💬 Sjors commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#discussion_r1580697073)
#29961 removes this, that's the only conflict I found.

💬 maflcko commented on pull request "refactor: Avoid unused-variable warning in init.cpp":
(https://github.com/bitcoin/bitcoin/pull/29968#issuecomment-2078938388)
@fanquake Can you cherry-pick this into your pull, to confirm that it also works for you? (I only tested it locally)

💬 fanquake commented on pull request "refactor: Avoid unused-variable warning in init.cpp":
(https://github.com/bitcoin/bitcoin/pull/29968#issuecomment-2078941827)
Pulled it in

👍 hebasto approved a pull request: "refactor: remove remaining unused code from cpp-subprocess"
(https://github.com/bitcoin/bitcoin/pull/29961#pullrequestreview-2024532007)
ACK 908c51fe4afeba0af500c6275027b1afa1b3bd19. It is compatible with https://github.com/bitcoin/bitcoin/pull/29961.

I'd suggest to remove the mentioning of the `check_output` function from all comments. That function was not even pulled from the upstream.