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

💬 hebasto commented on pull request "refactor: remove remaining unused code from cpp-subprocess":
(https://github.com/bitcoin/bitcoin/pull/29961#issuecomment-2078982716)
> > 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.

I agree.

💬 hebasto commented on pull request "depends: Do not consider `CC` environment variable for detecting system":
(https://github.com/bitcoin/bitcoin/pull/29963#issuecomment-2079001326)
> > 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".

https://github.com/hebasto/bitcoin/actions/runs/8822538616/job/24220973382#step:14:245:
```
Cross compiling ....................... FALSE
```

Please note that that was PR branch from the CMake migration project. That branch detects cross-compiling basing on `host` and `build` values when building with depends.

💬 hebasto commented on pull request "ZMQ: Support UNIX domain sockets":
(https://github.com/bitcoin/bitcoin/pull/27679#discussion_r1580765514)
No warnings for me locally using
```
x86_64-w64-mingw32-g++-posix (GCC) 13-posix
```

💬 fanquake commented on pull request "ZMQ: Support UNIX domain sockets":
(https://github.com/bitcoin/bitcoin/pull/27679#discussion_r1580767936)
> No warnings for me locally using

Did you turn on the relevant warning flag

💬 hebasto commented on pull request "ZMQ: Support UNIX domain sockets":
(https://github.com/bitcoin/bitcoin/pull/27679#discussion_r1580774357)
> > No warnings for me locally using
>
> Did you turn on the relevant warning flag

My bad! :facepalm:

💬 laanwj commented on pull request "guix: remove bzip2 from deps":
(https://github.com/bitcoin/bitcoin/pull/29895#issuecomment-2079034463)
Always some surprises...
```python
#!/usr/bin/env python3
from os import path
import os
from subprocess import run
import bz2
import gzip

GIT='git'
TMPDIR='tmp_checkout'
BRANCH='remove_xz_guix'
BASE_COMMIT='c05c214f2e9cfd6070a3c7680bfa09358fd9d97a'

def compare_streams(f, g, bsize=1024*1024):
while True:
b1 = f.read(bsize)
b2 = g.read(bsize)
if b1 != b2:
return False
if len(b1) == 0 and len(b2) == 0: # Both at EOF

...

💬 Sjors commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#issuecomment-2079035706)
Tested the Guix build on Windows 11.

💬 fanquake commented on pull request "depends: Do not consider `CC` environment variable for detecting system":
(https://github.com/bitcoin/bitcoin/pull/29963#issuecomment-2079037412)
> Surely this isn't the only place env vars like this would cause issues? Feels like a cat-and-mouse game.

Yea, as-is this doesn't seem like a great fix, and may just break other things?

A better diff might be something like:
```diff
diff --git a/depends/Makefile b/depends/Makefile
index 005d9696fb..091511758d 100644
--- a/depends/Makefile
+++ b/depends/Makefile
@@ -51,7 +51,7 @@ FALLBACK_DOWNLOAD_PATH ?= https://bitcoincore.org/depends-sources
C_STANDARD ?= c11
CXX_STANDARD ?= c
...

👍 hebasto approved a pull request: "refactor: Avoid unused-variable warning in init.cpp"
(https://github.com/bitcoin/bitcoin/pull/29968#pullrequestreview-2024633825)
ACK fa5ba429a71eb81848560b4a547d1717efd33593, tested on Ubuntu 24.04 with `-Wall`.

💬 Sjors commented on pull request "guix: remove bzip2 from deps":
(https://github.com/bitcoin/bitcoin/pull/29895#issuecomment-2079052784)
@laanwj did you mean `'-b', BRANCH` instead of `'-b', branch`?

I also get a difference on qrencode when running this on macOS 13.6.6 with Python 3.12.1.

💬 laanwj commented on pull request "guix: remove bzip2 from deps":
(https://github.com/bitcoin/bitcoin/pull/29895#issuecomment-2079056842)
Contents are the same, there is a slight difference in the metadata:
```
--- qrencode-4.1.1.tar_gz.hex 2024-04-26 11:58:29.335356741 +0200
+++ qrencode-4.1.1.tar_bz2.hex 2024-04-26 11:58:16.387499575 +0200
@@ -4,7 +4,7 @@
00000060 00 00 00 00 30 30 30 30 37 35 35 00 30 30 30 31 |....0000755.0001|
00000070 37 35 30 00 30 30 30 31 37 35 30 00 30 30 30 30 |750.0001750.0000|
00000080 30 30 30 30 30 30 30 00 31 33 37 33 34 33 30 33 |0000000.13734303|
-00000090 36 36 36 00 30 3
...

💬 hebasto commented on pull request "depends: Do not consider `CC` environment variable for detecting system":
(https://github.com/bitcoin/bitcoin/pull/29963#issuecomment-2079059322)
> A better diff might be something like:
> ...
> which would at least be using the option that is meant to be used for this.

I agree. Implemented.

Thanks!