💬 theStack commented on pull request "Make v2transport default for addnode RPC when enabled":
(https://github.com/bitcoin/bitcoin/pull/29239#discussion_r1449601049)
Isn't this still needed for tests with previous releases (earlier than v26.0), which only accept two arguments for `addnode`?

💬 ariard commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1888377830)
I realized after posting the quick thoughts yesterday that some replacement tactic alters significantly the anti-v3 pinning mitigation at the advantage of an adversary economics, so here one of the most adversarial pinning scenario I can come with.

Assume the attacker strategy is a rule-3 based pinning targeting the double-spend of forwaded HTLC over a target LN routing node. Let's call them Alice and Mallory. Assume the channel is 1_000_000 sats, `max_htlc_value_in_flight`=50% (i.e 500_000 s
...

💬 ariard commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1888389377)
> Is this a pinning issue or a problem with how they negotiated the max htlcs? It's one thing for an attacker to attach
> descendants to the shared transaction and take advantage of the permissive limit. It's another thing to willingly sign a
> huge transaction with somebody and be surprised later if they broadcast it.

See the attack scenario laid out more clearly.

> Is that different from max_accepted_htlcs? Or do you mean a hard limit within the protocol?
> https://github.com/lightnin
...

👍 maflcko approved a pull request: "Make v2transport default for addnode RPC when enabled"
(https://github.com/bitcoin/bitcoin/pull/29239#pullrequestreview-1817540895)
Missing `rpc: ` prefix in title?

💬 maflcko commented on pull request "Make v2transport default for addnode RPC when enabled":
(https://github.com/bitcoin/bitcoin/pull/29239#discussion_r1449951816)
nit: Can be written shorter

```suggestion
bool node_v2transport = node.connman->GetLocalServices() & NODE_P2P_V2;
bool use_v2transport = self.MaybeArg<bool>(2).value_or(node_v2transport);

if (use_v2transport && !node_v2transport) {
```

💬 maflcko commented on pull request "serialization: Support for multiple parameters":
(https://github.com/bitcoin/bitcoin/pull/28929#issuecomment-1888552974)
```
test/serialize_tests.cpp:394:11: error: unused variable 'oi2' [-Werror,-Wunused-variable]
394 | Other oi2;
| ^~~

👍 maflcko approved a pull request: "test: unbreak: exclude windows from wallet_assumeutxo"
(https://github.com/bitcoin/bitcoin/pull/29238#pullrequestreview-1817550673)
lgtm, but wouldn't it be better to just unload the wallet for a short time?

💬 kristapsk commented on pull request "Make v2transport default for addnode RPC when enabled":
(https://github.com/bitcoin/bitcoin/pull/29239#issuecomment-1888559054)
Concept ACK

💬 Sjors commented on pull request "Make (Read/Write)BinaryFile work with char vector":
(https://github.com/bitcoin/bitcoin/pull/29229#issuecomment-1888644663)
I switched to using `std::optional` as the return type. It happily compiles on my end, we'll see what CI thinks...

💬 Sjors commented on pull request "Make (Read/Write)BinaryFile work with char vector":
(https://github.com/bitcoin/bitcoin/pull/29229#issuecomment-1888672189)
Had to `#include <vector>` to please clang13 (should have included it anyway since it's used).

💬 t-bast commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1888673524)
> max_accepted_htlcs is a limit on the number of inbound HTLCs. max_offered_htlc would be some new BOLT-channel policy parameter to limit the number of outbound HTLCs, the idea has floated few times among LN folks to bind max commitment transaction size.

This new `max_offered_htlc` parameter is unnecessary. An HTLC sender unilaterally decides whether they send HTLCs or not, and ensure that they never send more outgoing HTLCs than their `max_accepted_htlcs`. That has always been eclair's behav
...

💬 glozow commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1450161338)
Oops! Removed.

💬 glozow commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1450162485)
Added this comment in the legacy test as I agree it'll be convenient to know what coverage we are/aren't losing when deleting a legacy test.

🚀 fanquake merged a pull request: "ci: Rename tasks (previous releases, macOS cross)"
(https://github.com/bitcoin/bitcoin/pull/29218)

💬 glozow commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1450164269)
Ah yeah, it's a wallet-specific RPC. I've added a check that the "confirmations" value is 1 on wallet 0 a few lines up, prior to the reorg. Hopefully that also helps make it clear why we're testing this way.

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1450168792)
`ShouldFanoutTo` looks fitting for benchmarking, and `partial_sort` had no effect.
I took the former suggestion of course.

🚀 fanquake merged a pull request: "build: Bump clang minimum supported version to 14"
(https://github.com/bitcoin/bitcoin/pull/29208)

👍 stickies-v approved a pull request: "test: wallet rescan with reorged parent + IsFromMe child in mempool"
(https://github.com/bitcoin/bitcoin/pull/29179#pullrequestreview-1817832198)
re-ACK c39e82926a3740642f8f1bb72c6b10b67a6dc724

💬 fanquake commented on pull request "depends: remove dependency on Darwin libtool":
(https://github.com/bitcoin/bitcoin/pull/29232#issuecomment-1888805134)
Guix build (aarch64 & x86_64):
```bash
73062e70b4ec1fa814c0c25f12cbbcfbabfd3a5e82827a3bfeb2f29b30d0b84f guix-build-a28c6bbde0bc/output/aarch64-linux-gnu/SHA256SUMS.part
0965ed91f84129a64b6e0873b5feb182d83cae0dac6d89a51667d5fbe46fc68c guix-build-a28c6bbde0bc/output/aarch64-linux-gnu/bitcoin-a28c6bbde0bc-aarch64-linux-gnu-debug.tar.gz
87ee5bc2103fc10b8e7acbfa776430918538f05fccf024b624f224f73987f745 guix-build-a28c6bbde0bc/output/aarch64-linux-gnu/bitcoin-a28c6bbde0bc-aarch64-linux-gnu.tar.g
...

🤔 dergoegge reviewed a pull request: "p2p: Allow whitelisting outgoing connections"
(https://github.com/bitcoin/bitcoin/pull/27114#pullrequestreview-1817887891)
I don't see what this would be useful for, could you elaborate on the motivation?