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

💬 fanquake commented on pull request "depends: build zeromq with CMake":
(https://github.com/bitcoin/bitcoin/pull/29723#issuecomment-2077233577)
> IPC build issue should be fixed in https://github.com/zeromq/libzmq/pull/4672

This was resolved using a different change. Have pulled in that patch, rebased and updated the PR description.

💬 laanwj commented on issue "Add IPv6 pinhole support using UPnP / NAT-PMP":
(https://github.com/bitcoin/bitcoin/issues/17012#issuecomment-2077233941)
FWIW the version of UPnP in depends (2.2.2) does have the Pinhole commands:
```
upnpcommands.h: int * inboundPinholeAllowed);
upnpcommands.h:UPNP_GetOutboundPinholeTimeout(const char * controlURL, const char * servicetype,
upnpcommands.h:UPNP_AddPinhole(const char * controlURL, const char * servicetype,
upnpcommands.h:UPNP_UpdatePinhole(const char * controlURL, const char * servicetype,
upnpcommands.h:UPNP_DeletePinhole(const char * controlURL, const char * servicet
...

💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-2077236613)
Unfortunately that didn't help, so the mystery remains.

💬 setavenger commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-2077253080)
Just curious. Does the index use [this](https://github.com/bitcoin/bitcoin/blob/0eb1459efa91c6a3bc2964007057cef2f7280a57/src/primitives/transaction.h#L46) to compare outpoints? Does it do (txid, vout) or (txid||vout)?

💬 instagibbs commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1579527839)
this is from `test_orphan_consensus_failure` causing the exact same parent to be added to `m_recent_rejects_reconsiderable`

💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#discussion_r1579531159)
@josibake can I get a helper function along the lines of:

```cpp
bool MaybeSilentPayment(CTransactionRef tx);
```

💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-2077270747)
@setavenger paging @josibake.