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

💬 sdaftuar commented on pull request "policy: restrict all TRUC (v3) transactions to 10kvB":
(https://github.com/bitcoin/bitcoin/pull/29873#issuecomment-2125445040)
ACK 154b2b2296edccb5ed24e829798dacb6195edc11

💬 maflcko commented on pull request "validation: Make ReplayBlocks interruptible":
(https://github.com/bitcoin/bitcoin/pull/30155#discussion_r1610472962)
```suggestion
LogInfo("Flushing intermediate state of replay\n");
```

nit: For new code it would be better to use the non-deprecated non-confusing alias `LogInfo` over `LogPrintf`.

💬 sdaftuar commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1610484226)
Do you think it might make sense to explicitly group these variables into a single struct that makes it clear we have one grouping of per-subpackage state? All these variables that are at the same scope level as other things that don't get reset seems maybe a bit messy...

💬 sdaftuar commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#issuecomment-2125532662)
utACK a84b57462cd3dfcd059783696aacd796ef1793d4

Left one nit/question for you, but looks good to me regardless.