💬 kevkevinpal commented on pull request "doc: Update mentions of generating bitcoin.conf":
(https://github.com/bitcoin/bitcoin/pull/30154#issuecomment-2127000695)
lgtm ACK [42f788d](https://github.com/bitcoin/bitcoin/pull/30154/commits/42f788d071095b62783c53a75f06831855e86bbd)

The `share/examples/bitcoin.conf` files links to `contrib/devtools/README.md` so instead of send the users to a file to link to another file it makes sense just to use that link directly here

💬 laanwj commented on pull request "doc: note that you can assume C++20.":
(https://github.com/bitcoin/bitcoin/pull/30136#discussion_r1611627399)
Sure, i'm fine with wording it in a way that doesn't imply people understand all nuances, say "The code can use the C++20 standard.", but due to the context it should be a statement about the code, not a compiler requirement.

💬 edilmedeiros commented on pull request "doc: Update mentions of generating bitcoin.conf":
(https://github.com/bitcoin/bitcoin/pull/30154#discussion_r1611636584)
Yes.

I don't think adding a check just for a marginal document worth the maintenance burden.

💬 instagibbs commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1611638058)
touched up the commit message

💬 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.
```