💬 glozow commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1604604969)
Agree there's room for improvement clarity-wise.

Perhaps a good approach is to have each flag control 1 part of the validation logic (even if 1 flag implies another flag)
- whether you call `LimitMempoolSize()` within `Finalize()`
- whether all conflicts should be rejected immediately
- whether `CheckFeeRate` should be skipped in `PreChecks`
- whether sibling eviction can be considered
- whether package RBF can be considered

The `ATMPArgs` constructors should decide the flags based on
...

🤔 glozow reviewed a pull request: "test: expand LimitOrphan and EraseForPeer coverage"
(https://github.com/bitcoin/bitcoin/pull/30082#pullrequestreview-2058508184)
lgtm

💬 glozow commented on pull request "test: expand LimitOrphan and EraseForPeer coverage":
(https://github.com/bitcoin/bitcoin/pull/30082#discussion_r1601941249)
Note you've only jumped 1 second here

💬 glozow commented on pull request "test: expand LimitOrphan and EraseForPeer coverage":
(https://github.com/bitcoin/bitcoin/pull/30082#discussion_r1601942742)
Would be nice if the constants were moved to txorphanage.h so we don't need the magic number here. Could be useful in other tests as well.

👍 hebasto approved a pull request: "kernel: Streamline util library"
(https://github.com/bitcoin/bitcoin/pull/29015#pullrequestreview-2062749982)
re-ACK 1c26d10b23bb7a7d0204b4a8b44a50fad89f2a2a.

💬 hebasto commented on pull request "[PoC] qt, depends: Add wayland support without build-time nor fixed run-time deps":
(https://github.com/bitcoin/bitcoin/pull/29959#issuecomment-2117111606)
@laanwj
> Closing for now. It exists as a PoC, if anyone is truly interested in wayland support in bitcoin core's binary releases, and interested in helping test and review, let me know.

I'm sorry for not paying attention to this great PR. I just don't have time to focus on it at the moment. It is about to be changed soon :)

💬 laanwj commented on issue "stringop-overflow warning with GCC 14":
(https://github.com/bitcoin/bitcoin/issues/30114#issuecomment-2117118213)
Looks like it derives that `.begin()` is the same as `.end()` which would bring the size to `-1`. This would require `CPubKey::GetLen` to return 0, which is possible if the first byte is invalid. Maybe it'd make sense to make it return `1` in that case because there's always the first byte?

💬 vasild commented on pull request "net: Replace libnatpmp with built-in PCP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1604635967)
Here is a standalone program to get the default gateway using a netlink socket:

<details>
<summary>netlink_get_default_route.cc</summary>

```cc
#include <arpa/inet.h>
#include <assert.h>
#include <errno.h>
#include <net/if.h>
#include <netinet/in.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/socket.h>
#include <sys/types.h>
#include <unistd.h>

#ifdef __linux__
#include <linux/rtnetlink.h>
#elif defined(__FreeBSD__)
#include <netlink/netlink.h
...

💬 laanwj commented on pull request "[PoC] qt, depends: Add wayland support without build-time nor fixed run-time deps":
(https://github.com/bitcoin/bitcoin/pull/29959#issuecomment-2117145929)
Thank you. To be clear that wasn't aimed at you. i know you're busy enough. Just at the wider FOSS community, who are always full of talk about the next big thing but when push comes to shove... In any case we can pick this up at some point. #29923 is basically complete so this would be the next step in the particular progression.

💬 stickies-v commented on pull request "rpc: avoid copying into UniValue":
(https://github.com/bitcoin/bitcoin/pull/30115#issuecomment-2117153581)
Concept ACK

> Indeed, I believe c-i should be doing that check.

It does: https://github.com/bitcoin/bitcoin/blob/2f53f2273da020d7fabd7c65a1bc7e69a31249b2/src/.clang-tidy#L6

💬 wiz commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2117166802)
testnet4 faucet up at https://mempool.space/testnet4/faucet
feel free to donate coins to `tb1q0dzcgv7scppjxsnwlzpkt02vlmc5rtr40wyjgr`

💬 vasild commented on pull request "net: Replace libnatpmp with built-in PCP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1604672913)
I realized that in the IPv4 case this would send the trailing 16-4=12 bytes from `request.dst_data[]`. This seems harmless, but better send exactly what's needed:

```suggestion
if (sock.Send(&request, request.hdr.nlmsg_len, 0) != request.hdr.nlmsg_len) {
LogPrintLevel(BCLog::NET, BCLog::Level::Error, "send() to netlink socket: %s\n", SysErrorString(errno));
```

Also, I am not sure if we should worry about partial writes with netlink sockets, maybe the send can be interrupted
...

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1604736981)
It's a datagram (packet socket), so *partial* writes and reads cannot happen. Every packet is interpreted as a new one. Using `SendComplete` would be a bug.

Truncated and corrupted packets could happen in the case of UDP, but as NETLINK is a communication interface with the kernel, that would be rare. No retries are needed. Would still want to detect it and error out gracefully, though, for robustness.

💬 glozow commented on pull request "policy: restrict all TRUC (v3) transactions to 25KvB":
(https://github.com/bitcoin/bitcoin/pull/29873#issuecomment-2117319556)
I've changed the limit to 10KvB. This means the maximum v3 package vsize is 11KvB.

The `test_nondefault_package_limits` test hit a snag (CPFP carve out grants a free +10kvB descendant size limit even if the tx isn't the 2nd child [1]), so I added a commit to explicitly disable CPFP carve out for v3. This change should be fine since CPFP carveout's intended purpose clearly doesn't apply in the v3 world. TLDR since carveout grants +10KvB to the descendant limit, there wasn't a way for the mempo
...

💬 glozow commented on pull request "policy: restrict all TRUC (v3) transactions to 10KvB":
(https://github.com/bitcoin/bitcoin/pull/29873#issuecomment-2117516628)
I've changed the limit to 10KvB. This means the maximum v3 package vsize is 11KvB.

The `test_nondefault_package_limits` descendant limit test hit a snag (CPFP carve out grants a free +10kvB which makes it impossible to make a descendant limit stricter than 11KvB), so I added a commit to explicitly disable CPFP carve out for v3. This change should be fine since CPFP carveout's intended purpose clearly doesn't apply in the v3 world. Alternatively we can delete this test, but it felt weird that
...

👍 ryanofsky approved a pull request: "rpc: Remove index-based Arg accessor"
(https://github.com/bitcoin/bitcoin/pull/29997#pullrequestreview-2063336413)
Code review ACK fa3169b0732d7eb4b9166e7ecc6b7cfb669a9b54. One changes since last review are some documentation improvements

💬 willcl-ark commented on pull request "rpc: avoid copying into UniValue":
(https://github.com/bitcoin/bitcoin/pull/30115#issuecomment-2117545232)
Concept ACK.

The code changes all look correct to me too, however

> @willcl-ark, however, noticed that memory usage may increase in some cases. Logically this makes no sense to me. The only plausible explanation imo is that because the moves are faster, more ops/second occur in some cases.

I still have not been able to explain why i see these type of results, but I think it's most likely user error somewhere along the line.

Here is one example of a puzzling result I have, in this tes
...

👋 BrandonOdiwuor's pull request is ready for review: "Feature: Use different datadirs for different signets"
(https://github.com/bitcoin/bitcoin/pull/29838)

💬 BrandonOdiwuor commented on pull request "Feature: Use different datadirs for different signets":
(https://github.com/bitcoin/bitcoin/pull/29838#discussion_r1604968619)
I have updated the PR to use the `first 8 bytes of the hash160` and replaced the `-` with `_` as suggested

💬 BrandonOdiwuor commented on pull request "Feature: Use different datadirs for different signets":
(https://github.com/bitcoin/bitcoin/pull/29838#issuecomment-2117559488)
@Sjors and @RandyMcMillan I love the idea of the `signet_XXXX` config section and would like to work on it if there is a lot of interest in a follow-up PR