🤔 ryanofsky reviewed a pull request: "doc: Add multiprocess design doc"
(https://github.com/bitcoin/bitcoin/pull/28978#pullrequestreview-1816915195)
re: https://github.com/bitcoin/bitcoin/pull/28978#pullrequestreview-1800375604

> left a couple of improvements but agreed that iterating in future PRs is better.

Thanks for the close reading and suggestions. I implemented them for now in the combined PR #10102 commit https://github.com/bitcoin/bitcoin/pull/10102/commits/ee4b9138c837f6dc6b8f063b0df27573736d6578 and will split it out into another PR at some point.

💬 ryanofsky commented on pull request "doc: Add multiprocess design doc":
(https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1449474349)
re: https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1439476415

> nit

Thanks, fixed in #10102

💬 ryanofsky commented on pull request "doc: Add multiprocess design doc":
(https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1449474251)
re: https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1439470004

>

Thanks, fixed in #10102

💬 ryanofsky commented on pull request "doc: Add multiprocess design doc":
(https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1449474151)
re: https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1439454381

> dead link

Thanks, fixed in #10102

💬 ryanofsky commented on pull request "doc: Add multiprocess design doc":
(https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1449474426)
re: https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1439417050

> nit: Perhaps useful to parameterize the host platform to highlight it's configurable? E.g.

Thanks I split it into a separate variable and clarified how it could be set in #10102.

👍 theStack approved a pull request: "log mempool loading progress"
(https://github.com/bitcoin/bitcoin/pull/29227#pullrequestreview-1817064507)
ACK 6c3ca0cac38f3b9e307cd0522ba9e3b7a2bd05a5

Reviewed the code and tested locally (mempool with ~26k txs), LGTM. Probably less important, but I think it would be nice to have more verbose logging also for dumping the mempool. I've heard sentences like "I wonder why Bitcoin Core takes so long to shutdown sometimes?" repeatedly from users.

🤔 theStack reviewed a pull request: "Make v2transport default for addnode RPC when enabled"
(https://github.com/bitcoin/bitcoin/pull/29239#pullrequestreview-1817078452)
Concept ACK

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