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

💬 m3dwards commented on pull request "depends: build libnatpmp with CMake":
(https://github.com/bitcoin/bitcoin/pull/29708#issuecomment-2076991934)
I don't believe any of these are a problem but some differences on flags passed to clang on my Mac were as follows:

When using this PR the following extra flags were seen vs master:

-DENABLE_STRNATPMPERR
-MD
-MT CMakeFiles/natpmp.dir/natpmp.c.o
-MF CMakeFiles/natpmp.dir/natpmp.c.o.d
-I/Users/max/source/bitcoin/depends/work/build/aarch64-apple-darwin23.0.0/libnatpmp/f2433bec24ca3d3f22a8a7840728a3ac177f94ba-2550f8d6ff0

On master these flags were seen which were not present when usi
...

💬 stickies-v commented on pull request "rpc: return warnings as an array instead of just a single one":
(https://github.com/bitcoin/bitcoin/pull/29845#issuecomment-2077026714)
> both curl and bitcoin-cli returned an error:

I suspect you're running with `-rpcdoccheck`, you shouldn't get any errors without it? I'm not sure if `-rpcdoccheck` is meant to be an internal-only testing flag (as per #24695), but it [doesn't seem to be documented as such](https://github.com/bitcoin/bitcoin/blob/2a07c4662d7266158d47f79fa2433ab22e22c907/src/init.cpp#L655), so I guess better to be safe - force pushed to make the docs contingent on the `-rpcdeprecated` too.

Thanks for your di
...

💬 TheCharlatan commented on pull request "kernel: De-globalize fReindex":
(https://github.com/bitcoin/bitcoin/pull/29817#issuecomment-2077029702)
Updated 9b2c9c2fce32fe858d1361e863c72108a384a5c8 -> b0594f6cf157e9bd71b2062cdf0aa65936fd2282 ([noGlobalReindex_1](https://github.com/TheCharlatan/bitcoin/tree/noGlobalReindex_1) -> [noGlobalReindex_2](https://github.com/TheCharlatan/bitcoin/tree/noGlobalReindex_2), [compare](https://github.com/TheCharlatan/bitcoin/compare/noGlobalReindex_1..noGlobalReindex_2))

* Addressed @ryanofsky's [comment](https://github.com/bitcoin/bitcoin/pull/29817#discussion_r1578327024) and [comment](https://github.
...

👍 brunoerg approved a pull request: "Fix typos in description.md and wallet_util.py"
(https://github.com/bitcoin/bitcoin/pull/29938#pullrequestreview-2022321060)
utACK 03e36b3da093e2c23cf51b46f6901cb84ddbf867

💬 maflcko commented on issue "Require thread_local":
(https://github.com/bitcoin/bitcoin/issues/29952#issuecomment-2077068853)
> I couldn't get wine running locally

I hopped into the win64 CI container and ran https://gist.github.com/jamesob/fe9a872051a88b2025b1aa37bfa98605 there. It passed.

But given that the previous test was done on Trusty (14.04?), it will probably be hard to check when it started passing.