💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2121977231)
> Trying this with -pcp=1 on my home internet connection:

Thanks for testing!
The received packet is not a valid PCP response (too short). But interpreting it as one anyway, the version byte is 0x00, result code is 0x01 (UNSUPP_VERSION). So you might have one of those routers that supports NAT-PMP only.

💬 paplorinc commented on pull request "refactor: Model the bech32 charlimit as an Enum":
(https://github.com/bitcoin/bitcoin/pull/30047#issuecomment-2122051145)
re-ACK 7f3f6c6dc80247e6dfb0d406dc53bc8198f029fd

💬 TheCharlatan commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1607890616)
I like this, no need to introduce all this error handling infrastructure for a debug-only option. Applied.

💬 TheCharlatan commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#issuecomment-2122100379)
Updated 79c9c55b27bd113885da2c36ad76e5ed145027b3 -> 4a1df97a016935348e50f384ed52a6671990835f ([noGlobalScriptCache_0](https://github.com/TheCharlatan/bitcoin/tree/noGlobalScriptCache_0) -> [noGlobalScriptCache_1](https://github.com/TheCharlatan/bitcoin/tree/noGlobalScriptCache_1), [compare](https://github.com/TheCharlatan/bitcoin/compare/noGlobalScriptCache_0..noGlobalScriptCache_1))

* Addressed @theuni's [comment](https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1607175446), made `C
...

🚀 fanquake merged a pull request: "wallet: Implement independent BDB parser"
(https://github.com/bitcoin/bitcoin/pull/26606)

💬 fjahr commented on pull request "ci: Add mising -Wno-error=maybe-uninitialized to armhf task":
(https://github.com/bitcoin/bitcoin/pull/30144#issuecomment-2122157154)
utACK fa73431dd4709754c34a4d5ad1c940ff9e628cf3

💬 fjahr commented on pull request "rpc: Avoid getchaintxstats invalid results":
(https://github.com/bitcoin/bitcoin/pull/29720#issuecomment-2122161034)
re-ACK ef692e52b4121ba3d8de91e45b2d33aff946e8d2

Also reviewed https://github.com/bitcoin/bitcoin/pull/30144 to confirm the failure is ok.

💬 fanquake commented on pull request "util: avoid using thread_local variable that has a destructor":
(https://github.com/bitcoin/bitcoin/pull/30095#discussion_r1607953823)
I'm going to do this in #30137.

🚀 fanquake merged a pull request: "util: avoid using thread_local variable that has a destructor"
(https://github.com/bitcoin/bitcoin/pull/30095)

🚀 fanquake merged a pull request: "doc: Update NetBSD Build Guide"
(https://github.com/bitcoin/bitcoin/pull/30143)

💬 maflcko commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-2122188355)
I wonder if it makes sense to collect the fuzz inputs to qa-assets, from testers that still have them. However, the fuzz CI does not compile with BDB, so that'll probably need to be adjusted first.

🤔 virtu reviewed a pull request: "net: add ASMap info in `getrawaddrman` RPC"
(https://github.com/bitcoin/bitcoin/pull/30062#pullrequestreview-2068021191)
Concept ACK

Also did some light testing:
- Some sanity checks on `getrawaddrman` output (everything mapped to zero when asmap is disabled; almost all addresses mapped to non-zero when enabled)
- Functional tests

💬 virtu commented on pull request "net: add ASMap info in `getrawaddrman` RPC":
(https://github.com/bitcoin/bitcoin/pull/30062#discussion_r1607941908)
nit: Looking at my node's `getrawaddrman` output, it felt odd to have `mapped_as` appear after `source`. IMO, the fields become more intuitively understandable when put right after or at least close to the address they refer to.

💬 0xB10C commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#issuecomment-2122215763)
note that https://github.com/bitcoin/bitcoin/pull/29575 removes the misbehavior score (which is exposed via the `net:misbehaving_connection` tracepoint). If/when #29575 is merged, the `misbehaving_connection` tracepoint will change.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1608069282)
`proxy_opt` exists only inside the `if`. What matters after this `if` are the values of `use_proxy` and `proxy`. I think your suggestion is equivalent to the current code. I.e.

```
set use_proxy and proxy (normal)
if (special case)
replace use_proxy and proxy with special values
```
vs
```
if (special case)
set use_proxy and proxy to special values
else
set use_proxy and proxy (normal)
```

Right?

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1608108288)
The transient sessions are not reused. `m_unused_i2p_sessions` description reads:

> If connecting to a host fails, then the created session is put to this pool for reuse.

The code is this:

https://github.com/bitcoin/bitcoin/blob/a786fd2041913d82ca90b561de309421bd24e41b/src/net.cpp#L472-L480

I.e. pop/extract a session from `m_unused_i2p_sessions` and if we cannot connect, then we did not use the session and thus put it back to `m_unused_i2p_sessions`.

Do you think that the descript
...

💬 hebasto commented on pull request "build: Enable `thread_local` for MinGW-w64 builds":
(https://github.com/bitcoin/bitcoin/pull/30099#issuecomment-2122373765)
Rebased on top of the merged #30095.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1608129277)
A few considerations:

* Private broadcast connections are not blocked on the `semOutbound` grant. They come to this code right away.

* Even if there is a high demand for private broadcast connections, we would try to open such ones only half of the time in `CConnman::ThreadOpenConnections()` to avoid such starvation of only opening private broadcast and not others for a long time (see how `MaybePickPrivateBroadcastNetwork()` yields to other connection types if we previously opened a privat
...

💬 laanwj commented on pull request "build: Remove `--enable-threadlocal`":
(https://github.com/bitcoin/bitcoin/pull/30137#issuecomment-2122380476)
Code review ACK 17fe948cce2eb75f0f3f4b0db9d0d90648c7d4af
This removes, to my knowledge, all mentions of thread-local in the build system or comments.
Will test the guix binary on windows.

💬 hebasto commented on pull request "build: Enable `thread_local` for MinGW-w64 builds":
(https://github.com/bitcoin/bitcoin/pull/30099#issuecomment-2122388233)
Closing in favour of https://github.com/bitcoin/bitcoin/pull/30137.