👍 theStack approved a pull request: "refactor: Use our own implementation of urlDecode"
(https://github.com/bitcoin/bitcoin/pull/29904#pullrequestreview-2021985089)
Code-review ACK 992c714451676cee33d3dff49f36329423270c1c

💬 m3dwards commented on pull request "depends: build libnatpmp with CMake":
(https://github.com/bitcoin/bitcoin/pull/29708#issuecomment-2076797188)
My Guix builds:

```shell
a134241bc9ff9823ce078ea10dbac544ff63536ee3f66fb557414ca191b1d62d guix-build-3c1ae3ee33d4/output/aarch64-linux-gnu/SHA256SUMS.part
f5a46ab7a5faae9ce50b2d084cf65d14372b354b9f8369178f1348b70a3b675b guix-build-3c1ae3ee33d4/output/aarch64-linux-gnu/bitcoin-3c1ae3ee33d4-aarch64-linux-gnu-debug.tar.gz
e2353ed4f0738026fe16cd90b503ef2f02f31e0b68ba42ea80924997225ae374 guix-build-3c1ae3ee33d4/output/aarch64-linux-gnu/bitcoin-3c1ae3ee33d4-aarch64-linux-gnu.tar.gz
780608996
...

💬 Sjors commented on pull request "doc: add LLVM instruction for macOS < 13":
(https://github.com/bitcoin/bitcoin/pull/29934#issuecomment-2076802405)
I was able to build `bitcoind` (didn't try QT) with `llvm@17`, but _not_ with the current `llvm` 18.1.4. See log: https://gist.github.com/Sjors/194eb5d67e2e1132df7ff86f103ba16b

It's probably fine to drop `@17` anyway, and maybe suggest trying lower versions if building fails. But it would be nice if there's a simple fix for this.

💬 vasild commented on issue "Error: Cannot resolve -bind address: 'bitcoind:8334=onion'":
(https://github.com/bitcoin/bitcoin/issues/26484#issuecomment-2076815988)
Yes, line `1841` is for when there is no `=` inside the argument, like in `-bind=1.2.3.4:8333`. Line `1853` is for when there is `=`, like `-bind=1.2.3.4:8333=onion`.

👍 maflcko approved a pull request: "refactor: Use our own implementation of urlDecode"
(https://github.com/bitcoin/bitcoin/pull/29904#pullrequestreview-2022081812)
Did some differential fuzzing, as this is an ideal fit for it.

ACK 992c714451676cee33d3dff49f36329423270c1c 👈

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4
...

💬 maflcko commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1579216072)
Some more tests:

`%%ffg`
`%fg`

🤔 paplorinc reviewed a pull request: "refactor: Use our own implementation of urlDecode"
(https://github.com/bitcoin/bitcoin/pull/29904#pullrequestreview-2022089197)
I think we should document at least that we thought of the `%00` corner case, e.g. via a test case or a change.

I have verified that the behavior is otherwise the same before and after via:
```
std::string GenerateRandomString() {
std::random_device rd;
std::mt19937 gen(rd());
std::uniform_int_distribution<> length_dis(1, 10);
std::uniform_int_distribution<> char_dis(1, 255);

size_t length = length_dis(gen);
std::string res;
res.reserve(length);

f
...

💬 paplorinc commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1579220761)
There's still one sneaky difference that I've found: `%00`.
Previously this prematurely terminated the string, now it's kept:
```C++
BOOST_CHECK_EQUAL(urlDecode("a%00b"), std::string("a\u0000b", 3));
```
this failed previously, passes now.

💬 maflcko commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1579230097)
> `%00`

Have you seen 992c714451676cee33d3dff49f36329423270c1c ?

💬 paplorinc commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1579232922)
Yes https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1572090699

💬 paplorinc commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1579235693)
Those cases were added since, I see, thanks.

💬 maflcko commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1579235954)
This change is intentional, is what I am trying to say.

💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-2076895408)
@setavenger I built your index and then ran the following comparison script:

```sh
#!/bin/bash
set -e
TIP=`bitcoin-cli getblockcount`
for height in $(seq 709632 $TIP); do
if [ $((height % 10000)) -eq 0 ]; then
echo "$height..."
fi
HASH=`bitcoin-cli getblockhash $height`
diff <(curl -s localhost:8000/tweak-index/$height | jq 'sort') <(bitcoin-cli getsilentpaymentblockdata $HASH | jq '.tweaks | sort') || (echo $height; exit 1)
done
```

The need for `sort` is a bit anno
...

💬 setavenger commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-2076903047)
> The need for `sort` is a bit annoying, but not a big deal. Unfortunately it does find a difference at height 716,120.
Is that a difference so far or the only one found? I'll investigate that block. Thanks for raising this to my attention.

💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-2076930535)
@setavenger it's the first one - will run the script again once it's clarified.

💬 setavenger commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-2076943704)
I believe the transaction in question is this one [here](https://mempool.space/tx/e37ecd93f347f39e2fa28f592ed6586316c86cdd06bafef36da08c53c2b013c9).

💬 willcl-ark commented on pull request "test: Add test case for spending bare multisig":
(https://github.com/bitcoin/bitcoin/pull/29120#discussion_r1579299212)
This is due to the `OP_CHECKMULTISG` off-by-one bug. See for example: https://bitcoin.stackexchange.com/questions/116960/why-checkmultisig-bug-cant-be-solved

@instagibbs I think your suggestion here is incorrect? We do need to scriptsig otherwise the tx is simply invalid.

💬 laanwj commented on issue "[Linux] Add wayland support":
(https://github.com/bitcoin/bitcoin/issues/19950#issuecomment-2076944941)
See #29959 for a new wayland PoC.

💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-2076970523)
I'll rebase on this https://github.com/bitcoin/bitcoin/pull/28241#discussion_r1575031937 just to make sure and will re-generate the Bitcoin Core index and then try again.

💬 laanwj commented on issue "Require thread_local":
(https://github.com/bitcoin/bitcoin/issues/29952#issuecomment-2076987731)
Concept ACK (if indeed all platforms we care about can do this now, including mingw)