💬 fanquake commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-2125173212)
> Another mismatch, this time on riscv64 -_-

I've just rebuilt on 2 different machines (one aarch64 one x86_64), and they match your output here (removing my comment above):
```bash
00becde2dd12878e3b9f50f27899a6a8b752343dade7c71781632715c3001473 guix-build-e8c25e8a35e3/output/aarch64-linux-gnu/SHA256SUMS.part
a685b9cee54014e74639be1e8db2d55b7c008fdb3b31c1c708c364a49b56759a guix-build-e8c25e8a35e3/output/aarch64-linux-gnu/bitcoin-e8c25e8a35e3-aarch64-linux-gnu-debug.tar.gz
d612281584098
...

💬 achow101 commented on pull request "depends: Fetch miniupnpc sources from an alternative website":
(https://github.com/bitcoin/bitcoin/pull/30151#issuecomment-2125175024)
ACK 21b8a14d37c19ce292d5529597e0d52338db48a9

The main page of tuxfamily.org has 2 posts stating that they've had weeks of downtime in the past 6 months, so I think it makes sense to switch to a different mirror as they seem to be having issues. It's unfortunate that miniupnp.free.fr is http only, but we check the hash so I think it's fine.

💬 jadijadi commented on pull request "Showing Local Addresses in Node Window":
(https://github.com/bitcoin-core/gui/pull/626#discussion_r1610274712)
This is more beautiful and understandable. Replaced the `it->first` & `it->second` with `addr` & `info`:

```
for (const auto& [addr, info] : hosts) {
local_addresses += QString::fromStdString(addr.ToStringAddr());
if (!addr.IsPrivacyNet()) local_addresses += ":" + QString::number(info.nPort);
local_addresses += ", ";
}
local_addresses.chop(2); // remove last ", "
```

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1610286827)
If so i would prefer not supporting it at all for older FreeBSD. It's so rare compared to the other operating systems already, and most of the userbase will be setting their own firewall. But we indeed need to check that it doesn't cause a compilation error.

👍 0xB10C approved a pull request: "net: add ASMap info in `getrawaddrman` RPC"
(https://github.com/bitcoin/bitcoin/pull/30062#pullrequestreview-2071759324)
ACK 1e54d61c4698debf3329d1960e06078ccbf8063c

I've played around with the `getrawaddrman` RPC with and without an asmap file loaded. Also loaded a json dump into addrman-observer. Works as expected.

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2125210478)
Okay - added a NAT-PMP fallback and removed all user visible option changes, except for documentation.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2125213533)
`e85cc59bad...057c79365c`: in order to decide whether to private broadcast to IPv4/IPv6 peers, instead of (before this force push) requiring that `-onion=` is explicitly set, do this (after this force push): keep track if we ever managed to connect to an `.onion` address via the `NET_ONION` proxy, if yes assume that this proxy is a Tor proxy and connecting to IPv4/IPv6 addresses through it will be done via the Tor network and via a Tor exit node.

This is better because it is stronger guarante
...

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1610296362)
> > Is there no better way of checking whether `-proxy` is an actual Tor proxy or just a regular SOCKS5 proxy?

> maybe we can keep track of whether we have have successfully managed to connect to at least one .onion address

Done in `e85cc59bad...057c79365c`, see https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2125213533, also dropped the change in `tor.md` because that is not needed anymore.

Looks better now, thank you!

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1610299087)
> If so i would prefer not supporting it at all for older FreeBSD.

Seems fine to me.

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1610302369)
Concept ACK on keep just using `-natpmp` for both PCP and NATPMP.

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1610306635)
Whoops, fixed

💬 glozow commented on pull request "policy: restrict all TRUC (v3) transactions to 10kvB":
(https://github.com/bitcoin/bitcoin/pull/29873#discussion_r1610308435)
Ah that's a good idea. I can add it to #29496?

💬 instagibbs commented on pull request "policy: restrict all TRUC (v3) transactions to 10kvB":
(https://github.com/bitcoin/bitcoin/pull/29873#discussion_r1610309923)
sgtm

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1610320133)
Added a version check for at least 13.2.

🤔 theStack reviewed a pull request: "rpc: introduce getversion RPC"
(https://github.com/bitcoin/bitcoin/pull/30112#pullrequestreview-2071898266)
Concept ACK

💬 theStack commented on pull request "rpc: introduce getversion RPC":
(https://github.com/bitcoin/bitcoin/pull/30112#discussion_r1610377253)
I think the cleanest approach to access the version info from a functional test is via `test/config.ini`, which is generated by the build system based on substitutions in `test/config.ini.in`. The values are available as (nested) dictionary via `self.config`. Simple example:

```diff
diff --git a/test/config.ini.in b/test/config.ini.in
index 291599da45..49bf203cf8 100644
--- a/test/config.ini.in
+++ b/test/config.ini.in
@@ -8,6 +8,9 @@
[environment]
PACKAGE_NAME=@PACKAGE_NAME@
PACKA
...

💬 BenWestgate commented on pull request "contrib: Fixup verify-binaries OS platform parsing":
(https://github.com/bitcoin/bitcoin/pull/30147#issuecomment-2125375660)
> There are also similar changes in #28418 but I didn't check this PR yet to see if they are the same.

This PR includes a **test.py** for the new functionality. My diff is smaller: Only 3 adds to `verify.py`
Either could close my linked issue.

I like my parser better and #28418 's error messages. I will borrow his strings and squash this.

Also my bad for almost duplicating someone else's PR. I did search for "verify.py" but didn't find any issues. I'll give #28418 a review.

💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#discussion_r1610397733)
After my fix I found another inconsistency, at block [709,640](https://mempool.space/block/0000000000000000000a3a458944554e24d1ef8d3ef8707067b01dcaf71526cd?page=39) and dust limit 992 your index finds 3 while Bitcoin Core finds 2.

I manually looked up the eligible transactions:
1. https://mempool.space/tx/350f06dff749f910b0ad8d918496850c676c22854e24a046304e73bdfe399a02
2. https://mempool.space/tx/0fdda311049e8a1450ef08bab4076c0e00f8e8334c861ed441f24980b8ac6a90
3. https://mempool.space/tx/7
...

💬 achow101 commented on pull request "rpc: avoid copying into UniValue":
(https://github.com/bitcoin/bitcoin/pull/30115#issuecomment-2125431251)
ACK d7707d9843b03f20d2a8c5a45d7b3db58e169e6f

Checked that only `std::move`s were added, and letting the compiler warn for use after move, which there does not appear to be any.

💬 BenWestgate commented on pull request "contrib: verify-binaries accept full arch-platform specifier":
(https://github.com/bitcoin/bitcoin/pull/28418#issuecomment-2125438436)
Concept ACK

Have not tested this branch but based on reading the diff it Closes #30145.

Approach:
The script doesn't need to know what part of the string is `arch`, `os` or `platform` to match the file.
Considering this, a much simpler parser produces the same functionality:
https://github.com/bitcoin/bitcoin/blob/9f6ccfced950cc4e334163af911ffc620bd5e216/contrib/verify-binaries/verify.py#L102-L111
Is there a reason to capture these in separate variables that I am missing?

Comparing
...