🚀 fanquake merged a pull request: "deploy: remove some tools when cross-compiling for macOS"
(https://github.com/bitcoin/bitcoin/pull/29890)

🚀 fanquake merged a pull request: "doc: Bash is needed in gen_id and is not installed on FreeBSD by default"
(https://github.com/bitcoin/bitcoin/pull/29953)

💬 BrandonOdiwuor commented on pull request "test: Add test case for spending bare multisig":
(https://github.com/bitcoin/bitcoin/pull/29120#discussion_r1579465882)
Thanks for this clarification @willcl-ark

🤔 sr-gi reviewed a pull request: "p2p: opportunistically accept 1-parent-1-child packages"
(https://github.com/bitcoin/bitcoin/pull/28970#pullrequestreview-2015631578)
Third pass, reviewing the changes of the first two, plus going all the way to 30c9e6bc4e9f8b8b606e55435dc3f743cb2dd670

I think the code looks good overall, but I have some questions regarding some corner cases that I'm not completely sure about (see inline).

Finally, something I've noticed: Given a `1p1c` whose parent we've seen already and we just received the child, the parent will be tried on its own twice before being considered as a package. This is because we will try it first when r
...

💬 sr-gi commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1576401327)
Reviewing the changes in this file I realized something seems to be odd with the way the test was initially designed:

`duplicate_input` is defined as a flag that, if set, will allow duplicate inputs to be used when building the transaction. However, the way this is approached is weird: if `duplicate_input` is not set, upon picking prevout as input, we will kick it off `outpoints` to make sure we don’t pick it up again as another input. Later on, all selected inputs are added back to outpoints
...

💬 sr-gi commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1575257763)
In b9caa4cfcbd1e176ab3ecd61973ab6721570aecb

I'm not too familiar with fuzzing, but wouldn't it be better if this be set unconditionally? This will trigger on the first iteration of the loop, and then based on a coin flip AFAICT, which means that if the variable is not overwritten, we will call `orphanage.AddChildrenToWorkSet(*ptx_potential_parent);` with the same transaction multiple times. This is harmless, given the internals just add data to a set, so multiple calls won't change it, but it
...

💬 sr-gi commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1578402851)
In 30c9e6bc4e9f8b8b606e55435dc3f743cb2dd670

Is the placeholder fee needed? `create_self_transfer_multi` already has a default fee, and the value shouldn't matter, should it?

💬 sr-gi commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1576744547)
In bb3bf0a39c036fbd94b99ed7002db7d9bdf0a1d6

I think you can potentially get rid of this variable by having a reference to a `Package` instead, which is set to the `maybe_cpfp_package` when the package cannot be found in `m_recent_rejects_reconsiderable`.

That way you won't need to construct the package again in the current `if (tx_orphan)` scope.

💬 sr-gi commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1576672946)
In bb3bf0a39c036fbd94b99ed7002db7d9bdf0a1d6

nit: ~most~ newest

💬 sr-gi commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1578441000)
In 30c9e6bc4e9f8b8b606e55435dc3f743cb2dd670

nit: This could be done more idiomatically, but feel free to disregard

```suggestion
self.peers = [self.nodes[i].add_p2p_connection(P2PInterface()) for i in range(self.num_nodes)]
```

💬 sr-gi commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1576613747)
In bb3bf0a39c036fbd94b99ed7002db7d9bdf0a1d6

nit: skipping any combinations **that** have already been tried (?)

💬 sr-gi commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1578426417)
In 30c9e6bc4e9f8b8b606e55435dc3f743cb2dd670

nit: There's no need to reserve here, this could be defined as `txns_to_send = [[]]` and subsequent calls could call `append` instead

💬 sr-gi commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1578404294)
In 30c9e6bc4e9f8b8b606e55435dc3f743cb2dd670

`confirmed_only` is only used if no utxos are provided, but this is not the case, so it shouldn't be needed.

💬 BrandonOdiwuor commented on pull request "test: Add test case for spending bare multisig":
(https://github.com/bitcoin/bitcoin/pull/29120#discussion_r1579468078)
response at https://github.com/bitcoin/bitcoin/pull/29120#discussion_r1579299212

💬 instagibbs commented on pull request "test: Add test case for spending bare multisig":
(https://github.com/bitcoin/bitcoin/pull/29120#discussion_r1579470057)
@willcl-ark yeah my comment was nonsense, deleted

👍 hebasto approved a pull request: "depends: pass verbose through to cmake based makefiles"
(https://github.com/bitcoin/bitcoin/pull/29960#pullrequestreview-2022502664)
ACK 7c69baf227252511455bc06e315f6a3c7fc5a398. Tested using the folowing command:
```
make -C depends native_capnp_built MULTIPROCESS=1 V=1
```

💬 bstin commented on pull request "contrib: rpcauth.py - Add new option (-json) to output text in json format":
(https://github.com/bitcoin/bitcoin/pull/29433#issuecomment-2077220700)
Thanks for your help @maflcko, I squashed and hope that it looks ok now.

💬 andrewtoth commented on pull request "RPC: Return `permitbaremultisig` and `maxdatacarriersize` in `getmempoolinfo`":
(https://github.com/bitcoin/bitcoin/pull/29954#discussion_r1579494260)
Could have a test with the `True` case?

💬 andrewtoth commented on pull request "RPC: Return `permitbaremultisig` and `maxdatacarriersize` in `getmempoolinfo`":
(https://github.com/bitcoin/bitcoin/pull/29954#discussion_r1579492975)
To avoid any confusion

```suggestion
{RPCResult::Type::BOOL, "permitbaremultisig", "True if the mempool accepts transactions with bare multisig outputs"},
```

💬 laanwj commented on issue "Add IPv6 pinhole support using UPnP / NAT-PMP":
(https://github.com/bitcoin/bitcoin/issues/17012#issuecomment-2077224493)
Indeed, UPnP has an explicit command `AddPinhole` (https://upnp.org/specs/gw/UPnP-gw-WANIPv6FirewallControl-v1-Service.pdf) that's seperate from `Add(Any)PortMapping`.

The PCP (succssor to NAT-PMP) RFC (https://datatracker.ietf.org/doc/html/rfc6887) doesn't say anything about pinholes (although they do mention "Simple firewall control"). Though i would guess the MAP opcode allows that, and more, it just leaves it up to the router how the external to internal mapping should be.

It's kind of
...