🤔 Sjors reviewed a pull request: "Enhanced error messages for invalid network prefix during address parsing."
(https://github.com/bitcoin/bitcoin/pull/27260#pullrequestreview-3446684328)
Thanks for splitting base58 and bech32 stuff into separate commits. I think that 04d5d4acab4259505654a506c838fecb29fdb88d _Base58 decoding logic + bech32 decoding network awareness_ could itself be split, see inline comment from @l0rinc, since it's still contains a lot of bech32 changes despite the commit name.

My main other suggestion is to drop the first commit (and don't add the network name to error messages).

See inline comment for how to fix the test.

💬 Sjors commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r2513222433)
In f8513e38b369889df0b402f06d67fcf656495d53 _[Functional] Introduce muti network testing_: the extra `(m)` breaks the test

💬 Sjors commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r2513235396)
In f8513e38b369889df0b402f06d67fcf656495d53 _[Functional] Introduce muti network testing_:

The elegant solution imo is to move these test vectors into a JSON file, see e.g. `rpc_getblockstats.py`.

But I think the current approach is fine. You could also duplicate the body of `test_validateaddress_on_network` into `test_validateaddress_mainnet` and `test_validateaddress_signet` rather than copying the constants. Each for loop could then be preceded with `self.log.debug("Valid mainnet addre
...

💬 Sjors commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r2513250539)
Nit for c974f892c7b6a61630c555f08ec201d911d5fd62 _Bech32 decoding logic_: unrelated whitespace change and random looking `asdas` in the commit message.

💬 Sjors commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r2513216866)
In f8513e38b369889df0b402f06d67fcf656495d53 _[Functional] Introduce muti network testing_:

These were disabled in #27727, IIUC in favour the official test vectors. Seems fine to delete them. If they're actually interesting, then they can be restored and added to the BIP.

(nit: typo in commit message)

💬 Sjors commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r2513196144)
In 7b71e6e056f5567152097d5c9a1a8e2f59c1b661 _Include Base58 encoded prefixes in chainparams_: this commit only adds base58 prefixes, silent payments will use bech32m.

💬 Sjors commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r2513186197)
In 055dbaed69a2577b967fae3d9b7646ef3b01a254 _Added chaintype user-facing string display_:

This is used as follows in later commits:

> `Invalid Base58 %s address.`

In the original code it would not specify the network:

> `Invalid or unsupported Base58-encoded address.`

I don't think it's very useful to add the network name to address. Certainly not for mainnet, because most users have no idea (and no need to know) about test networks.

My suggestion would be drop this helper enti
...

💬 Sjors commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r2513273103)
In 04d5d4acab4259505654a506c838fecb29fdb88d _Base58 decoding logic + bech32 decoding network awareness_: splitting bech32 and base58 handling in an earlier commit might also help to focus the current commit more on base58. I still a lot of lines touching bech32 code.

💬 Sjors commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r2513247821)
If this works out of the box it's worth taking, but otherwise I would keep the current approach. Address validation is simple enough that I'm not worried about the node being in a confused state.

💬 laanwj commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2513309951)
i remember that in the past we had tons of issues with thread-local variables. They've been a pain in the ass on some platforms, and decided to not use them except for one case. Not sure if this is still the case, but if not, this needs to be updated:
https://github.com/bitcoin/bitcoin/blob/master/src/util/threadnames.cpp#L39

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3515590666)
I restored to the previous commit id `29ef3c62de`:
`29ef3c62de...d419fbc42b`: remove the functional test
`d419fbc42b..29ef3c62de`: restore the functional test
the fuzz test wasn't there in the first place.

I think it needs some more polishing and is not ready for a non-draft PR.

💬 janb84 commented on pull request "guix: build for Linux HOSTS with `-static-libgcc`":
(https://github.com/bitcoin/bitcoin/pull/33181#issuecomment-3515604722)
### My Guix Build Output

**Host architecture:** `aarch64`
**Commit:** `f06c6e189831`

```shell
05fc5cae41b19bbfdec5942e334e7b7d78ee0406ba69c221381b0b07b0a9e886 guix-build-f06c6e189831/output/aarch64-linux-gnu/SHA256SUMS.part
961613408c4d0b3b169488b43edc838135be0b712740b4015457f4f2808e103b guix-build-f06c6e189831/output/aarch64-linux-gnu/bitcoin-f06c6e189831-aarch64-linux-gnu-debug.tar.gz
906437c316d50e1f60fdfe2098fe72f917be109a68a2103e915183ed4fe01947 guix-build-f06c6e189831/outpu
...

🤔 janb84 reviewed a pull request: "guix: build for Linux HOSTS with `-static-libgcc`"
(https://github.com/bitcoin/bitcoin/pull/33181#pullrequestreview-3446920253)
Concept ACK f06c6e18983139dd63873b3537d2f87b8c6ec752

The build uses a lot more space 49,4 GIB ( 51 GiB (freespace before run - 1.6 GiB freespace after run) (will post proof later, have to run)

Good followup would be to adjust the guix build gate accordantly.
(will do some extra research on this to rule out something has changed on my guix vm)

💬 laanwj commented on pull request "Changing the rpcbind argument being ignored to a pop up warning, inst…":
(https://github.com/bitcoin/bitcoin/pull/33813#discussion_r2513364532)
Do we want to change this one to a warning as well? It even say "WARNING".

💬 maflcko commented on pull request "ci: Enable experimental kernel stuff in ASan task":
(https://github.com/bitcoin/bitcoin/pull/33845#issuecomment-3515685725)
I was wondering if the std::span nullptr check was part of the C++26 library hardening, but it seems not (https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2025/p3471r4.html#span-constructors), so I guess if we want it, we'd still have to write it ourselves.

💬 maflcko commented on pull request "rpc: add "ischange: true" to decoded tx outputs in wallet gettransaction response":
(https://github.com/bitcoin/bitcoin/pull/32517#discussion_r2513459402)
https://github.com/bitcoin/bitcoin/actions/runs/19238750700/job/54998849583?pr=33842#step:12:208:

```
File "D:\a\bitcoin\bitcoin/test/functional/wallet_basic.py", line 550, in run_test

assert vout["ischange"]

~~~~^^^^^^^^^^^^

KeyError: 'ischange'

💬 fanquake commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2513507443)
There's also our `thread_local` clang-tidy plugin: https://github.com/bitcoin/bitcoin/tree/master/contrib/devtools/bitcoin-tidy.

⚠️ fanquake opened an issue: "ci: failure in `wallet_basic.py` KeyError: 'ischange'"
(https://github.com/bitcoin/bitcoin/issues/33848)
https://github.com/bitcoin/bitcoin/actions/runs/19238750700/job/54998849583?pr=33842#step:12:208:
```bash
File "D:\a\bitcoin\bitcoin/test/functional/wallet_basic.py", line 550, in run_test

assert vout["ischange"]

~~~~^^^^^^^^^^^^

KeyError: 'ischange'
```

💬 fanquake commented on issue "ci: failure in `wallet_basic.py` KeyError: 'ischange'":
(https://github.com/bitcoin/bitcoin/issues/33848#issuecomment-3515909671)
cc @pinheadmz @achow101 #32517

💬 big14way commented on issue "Inconsistent CJDNS address handling in Local addresses and AddLocal logs":
(https://github.com/bitcoin/bitcoin/issues/33471#issuecomment-3515930062)
@willcl-ark thank you for clarifyingi will look through the CONTRIBUTING.md frist
i appreciate it