💬 Sjors commented on pull request "[PoC, nomerge] IPv6 PCP pinhole test":
(https://github.com/bitcoin/bitcoin/pull/30005#issuecomment-2085298601)
Unfortunately I'm not able to find any documentation for this.

There is a UPnP plugin, but that's now what you're using (and probably best left uninstalled).

Anyway, people who run OPNsense will now how to manually forward a port, so that's not really the target demographic here.

💬 ajtowns commented on pull request "test: Add test case for spending bare multisig":
(https://github.com/bitcoin/bitcoin/pull/29120#issuecomment-2085314148)
ACK e504b1fa1fa4d014b329dea81bfdf1aa55db238f ; LGTM and just checking the 1-of-3 case seems fine

📝 fanquake converted_to_draft a pull request: "depends: fix Qt to not use `-q` with `ar`"
(https://github.com/bitcoin/bitcoin/pull/30004)
BusyBox `ar` doesn't implement `q`, and GNU `ar` treats `qs` like `r`, so replace the Qt usage of `ar cqs` with `ar rc`, which should work properly everywhere. Played around with making this work a different way, but couldn't seem to make it work properly. Open to other working suggestions (our other `AR` substitution still works correctly, it's just this single qmake bootstrap build invocation that is an issue.

👍 TheCharlatan approved a pull request: "doc: add LLVM instruction for macOS < 13"
(https://github.com/bitcoin/bitcoin/pull/29934#pullrequestreview-2031360698)
ACK 22574046c90c0662f3aa9b1baea074aff54f92a9

💬 hebasto commented on pull request "deploy: remove some tools when cross-compiling for macOS":
(https://github.com/bitcoin/bitcoin/pull/29890#issuecomment-2085348229)
Ported to the CMake-based build system in https://github.com/hebasto/bitcoin/pull/180.

👍 ryanofsky approved a pull request: "rpc: Remove index-based Arg accessor"
(https://github.com/bitcoin/bitcoin/pull/29997#pullrequestreview-2031418230)
Code review ACK fadb3eb57b4d207a678067b89caa45abf1f93702

💬 ryanofsky commented on pull request "rpc: Remove index-based Arg accessor":
(https://github.com/bitcoin/bitcoin/pull/29997#discussion_r1584855621)
In commit "rpc: Remove index-based Arg accessor" (fadb3eb57b4d207a678067b89caa45abf1f93702)

Should s/check_positional/check_named/

💬 maflcko commented on pull request "rpc: Remove index-based Arg accessor":
(https://github.com/bitcoin/bitcoin/pull/29997#discussion_r1584862055)
I think this means the positional `request.params` accessor, so this is better to leave as-is, no?

💬 TheCharlatan commented on pull request "rpc: Optimize serialization and enhance metadata of dumptxoutset output":
(https://github.com/bitcoin/bitcoin/pull/29612#discussion_r1584865007)
I'm surprised we never had to match the magic bytes to a chaintype. Instead of defining new strings here though, could you use `ChainTypeToString(ChainType::MAIN)`?

💬 TheCharlatan commented on pull request "rpc: Optimize serialization and enhance metadata of dumptxoutset output":
(https://github.com/bitcoin/bitcoin/pull/29612#discussion_r1584868473)
I think this is fine where it is, I like keeping these single type utilities small.

💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1584882538)
Ah, just found a mistake. I think this can go in a followup though.
```suggestion
if (rejected_parent_reconsiderable.has_value() && *rejected_parent_reconsiderable != parent_txid.ToUint256()) {
```

👍 TheCharlatan approved a pull request: "Disable util::Result copying and assignment"
(https://github.com/bitcoin/bitcoin/pull/29906#pullrequestreview-2031473523)
ACK 9552619961049d7673f84c40917b0385be70b782

💬 sdaftuar commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1584890852)
Hmm, we're looping over unique parents already, so I think this code should be fine? In which case is there a bug?

💬 instagibbs commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1584892625)
they're unique txids, which scenarios will this fix?

✅ fanquake closed a pull request: "depends: fix Qt to not use `-q` with `ar`"
(https://github.com/bitcoin/bitcoin/pull/30004)

🤔 dergoegge reviewed a pull request: "p2p: opportunistically accept 1-parent-1-child packages"
(https://github.com/bitcoin/bitcoin/pull/28970#pullrequestreview-2031444770)
light Code review ACK e518a8bf8abf3d7b83c9013f56d0dca18ae04d6f

💬 dergoegge commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1584871688)
I think attempting to accept a 1p1c package could also be done when reconsidering transactions from the orphanage in `ProcessOrphanTx`. E.g. you might have a 1p1c package in your orphanage (P -> C) and if P is reconsidered but rejected as `TX_RECONSIDERABLE` you might want to look for C in the orphanage and if found try 1p1c acceptance.

Maybe its not worth adding right now (any scenarios I can think of seem somewhat rare) but perhaps worthwhile later if the data suggests so.

💬 dergoegge commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1584890259)
Why is this sort comparison different from the one in `GetChildrenFromSamePeer`?

These functions are almost identical as well. Perhaps a more generic ```std::vector<std::pair<CTransactionRef, NodeId>> GetChildren(const CTransactionRef&)``` that returns all children and their senders would be less code?

💬 ryanofsky commented on pull request "rpc: Remove index-based Arg accessor":
(https://github.com/bitcoin/bitcoin/pull/29997#discussion_r1584900889)
> I think this means the positional `request.params` accessor, so this is better to leave as-is, no?

I think this was called `check_positional` because it was calling the `Arg(size_t)` function and the the check below was called `check_named` because it was calling the `Arg(string_view)` function. So now that the check below is deleted and this now calling `Arg(string_view)`, it would make more sense to call this `check_named` than `check_positional`.

But the point of this is really to cal
...

💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1584902362)
Ah you're right, nevermind!