💬 Sjors commented on pull request "[PoC, nomerge] IPv6 PCP pinhole test":
(https://github.com/bitcoin/bitcoin/pull/30005#issuecomment-2085177069)
Tried with a router running OPNsense:

```
2024-04-30T12:16:13Z [net:debug] pcp6: gateway: ...
2024-04-30T12:16:13Z [net:warning] pcp6: Opening pinhole for addr: ...
2024-04-30T12:16:14Z [net:debug] pcp6: Timeout
2024-04-30T12:16:14Z [net:debug] pcp6: Retrying (1)
2024-04-30T12:16:15Z [net:debug] pcp6: Timeout
2024-04-30T12:16:15Z [net:debug] pcp6: Retrying (2)
2024-04-30T12:16:16Z [net:debug] pcp6: Timeout
2024-04-30T12:16:16Z [net:debug] pcp6: Giving up after 3 tries
2024-04-30T12:1
...

💬 brunoerg commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1584709863)
In 6b4693ee859d6bddf559a0b32749b1dafe015ef4 "net: implement opening PRIVATE_BROADCAST connections": Just a question: why using `compare_exchange_weak` instead of `fetch_sub`?

💬 laanwj commented on pull request "[PoC, nomerge] IPv6 PCP pinhole test":
(https://github.com/bitcoin/bitcoin/pull/30005#issuecomment-2085190180)
It may be some router setting to enable it, the more secure routers have these kind of protocols disabled by default. Or maybe it doesn't support it. There isn't any reply, not even 2 NOT_AUTHORIZED.

💬 laanwj commented on pull request "logging: Update to new logging API":
(https://github.com/bitcoin/bitcoin/pull/29231#issuecomment-2085194038)
@ajtowns Sorry, i didn't mean to derail this, i shouldn't have commented.

💬 laanwj commented on pull request "rpc: Remove index-based Arg accessor":
(https://github.com/bitcoin/bitcoin/pull/29997#issuecomment-2085258103)
Concept ACK

💬 ryanofsky commented on pull request "Improve new LogDebug/Trace/Info/Warning/Error Macros":
(https://github.com/bitcoin/bitcoin/pull/29256#issuecomment-2085290323)
> > I think most of the criticism of this PR from AJ is saying that the changes here are unnecessary, not really that they are harmful,
>
> I said "As I've already said, I think `m_log.SetCategory(NET); m_log.Debug("connected to foo")` is much worse than `LogDebug(NET, "connected to foo")`." Does that really not come across as me saying they're harmful? I really don't think I'm being **that** unclear. Anyway, I'll bow out, I'm already sorry for touching this.

I think it's possible you migh
...

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