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

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