💬 brunoerg commented on pull request "fuzz: More accurate coverage reports":
(https://github.com/bitcoin/bitcoin/pull/30156#issuecomment-2127045849)
Concept ACK

💬 jadijadi commented on pull request "Showing Local Addresses in Node Window":
(https://github.com/bitcoin-core/gui/pull/626#discussion_r1611661580)
Agree with this. Will change because "LocalAddress" can be confusing in BTC context.

👍 willcl-ark approved a pull request: "rpc: avoid copying into UniValue"
(https://github.com/bitcoin/bitcoin/pull/30115#pullrequestreview-2073920635)
ACK d7707d9843b03f20d2a8c5a45d7b3db58e169e6f

Whilst I still measure _slightly_ increased resource usage vs master, even after adding some strategic calls to `shrink_to_fit()` on the `Univalue.values` member (which likely didn't work for the reasons @maflcko described above), I am now convinced this is not something to worry about in practice.

I've done more measurements over the last few days, and even when testing codepaths which create very large numbers of `UniValue`s, I only ever see a
...

💬 vasild commented on pull request "util: check for errors after close and read in AutoFile":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r1611668338)
It is best if all users of `AutoFile` explicitly call the `fclose()` method and check that it succeeds (option 3. from PR description). This is not feasible because there are many users of `AutoFile`. However, some of those users already do call the `fclose()` method explicitly. For those users it is easy and I added checks whether `fclose()` succeeds. `DumpMempool()` is such a user and the change is:

```diff
throw std::runtime_error("FileCommit failed");
- file.fclose()
...

💬 furszy commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1611669887)
> I think you'll have to tell maintainers if you want to address this nit, or leave it for a follow-up, otherwise they won't know whether it is fine to merge this from your side or not.

ok, yes. I was thinking on the second approach, but let's move forward. This will let me un-draft #27837. Happy to review any follow-up doing this.
Thanks Marko for the ping and this discussion.

💬 kevkevinpal commented on pull request "fuzz: More accurate coverage reports":
(https://github.com/bitcoin/bitcoin/pull/30156#issuecomment-2127147044)
Concept ACK [52506a0](https://github.com/bitcoin/bitcoin/pull/30156/commits/52506a0bb28a0c052deef9f484dc94e83de4f0f7)

Made a clean build of this change using
```
sudo make clean && ./autogen.sh && CC=clang CXX=clang++ ./configure --enable-fuzz --with-sanitizers=address,fuzzer,undefined && sudo make -j"$(($(nproc)+1))" install
```

## Running fuzz tests twice

(master: f15778536ad421f9805f0c005f0eb7b84dda1b4e)
```
FUZZ=process_message src/test/fuzz/fuzz -runs=1 qa-assets/fuzz_seed_co
...

💬 instagibbs commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1611733528)
ok, makes sense why I only see it used in `cs_main` contexts, since it's a global mutex. thanks

💬 sdaftuar commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#issuecomment-2127160378)
ACK 63cfa4ce88cac26abcacf3b243916e722ab17d75

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1611597515)
Could make this slightly more readable with comments and two nullptr:

```cpp
if (sysctl(/*name=*/mib, /*namelen=*/sizeof(mib) / sizeof(int), /*oldp=*/nullptr, /*oldlenp=*/&l, /*newp=*/nullptr, /*newlen=*/0) < 0) {
```

🤔 Sjors reviewed a pull request: "net: Replace libnatpmp with built-in PCP+NATPMP implementation"
(https://github.com/bitcoin/bitcoin/pull/30043#pullrequestreview-2073809550)
Finished macOS review of QueryDefaultGatewayImpl.

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1611637097)
I guess you can't just do `for (const struct rt_msghdr* rt : buf)`

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1611600021)
A useful hint to the reader what's going on here, and where it's documented:

```cpp
// The size of the available data can be determined by calling sysctl() with
// the NULL argument for oldp. See sysctl(3).
```

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1611597943)
```cpp
if (sysctl(/*name=*/mib, /*namelen=*/sizeof(mib) / sizeof(int), /*oldp=*/buf.data(), /*oldlenp=*/&l, /*newp=*/nullptr, /*newlen=*/0) < 0) {
```

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1611661588)
Do we want to check `rt->rtm_errno` first?

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1611731634)
This seems quite brittle and I don't fully understand it. I did test that it seems necessary, e.g. doing `sa++;` causes it to not map IPv6 ports. If I look at `struct sockaddr_storage` I'm seeing `_SS_ALIGNSIZE (sizeof(__int64_t)`, but using `uint64_t` instead of `uint32_t` in your `ROUNDUP` also doesn't work.

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1611729605)
```cpp
// Skip sockaddr entries for bit flags we're not interested in,
// move cursor.
```

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1611691990)
```cpp
// We only read from this address if a rtm_addrs bit flag is set.
```

💬 glozow commented on pull request "policy: bump TX_MAX_STANDARD_VERSION to 3":
(https://github.com/bitcoin/bitcoin/pull/29496#issuecomment-2127184039)
Added constant for https://github.com/bitcoin/bitcoin/pull/29873#discussion_r1610126595
Also added a test that wallet does not signal v3 as it uses the default `CURRENT_VERSION` (this is correct as it wouldn't be safe to make transactions v3 before any of the network has adopted this change). I believe the `nVersion` can be changed via `CCoinControl::m_version`, though I'm not sure if that is accessible so I didn't write a test for it.

Rebased on top of #29873 + master (conflict with #29086)
...

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1611755766)
i'm not sure either, i see it in some other libraries, but no alignment is mentioned in the manual page for `route` at all. Leaving out `ROUNDUP` works fine btw. Might just delete it.

🚀 achow101 merged a pull request: "test: improve robustness of connect_nodes()"
(https://github.com/bitcoin/bitcoin/pull/30118)