💬 stickies-v commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1449435185)
What do you think about adding one line to these docs in each file to reference the other file? Might be useful to e.g. keep tests in sync if problems are found, or when people wonder why this is (at first sight) only tested for legacy or descriptor wallets.

```suggestion
# inputs of the transaction to detect it, so the parent must be processed before the child.
# This behaviour is tested for descriptor wallets in wallet_rescan_unconfirmed.py.
```

💬 stickies-v commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1449439228)
Makes sense, thank you! I failed to appreciate that we're calling the wallet `gettransaction` RPC, and not the not `getrawtransaction` RPC.

💬 stickies-v commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1449427996)
probably should be removed?

👍 stickies-v approved a pull request: "doc: refer to "Node relay options" in policy/README"
(https://github.com/bitcoin/bitcoin/pull/29235#pullrequestreview-1816833251)
ACK cecf2dfeb7f04fc3f6022cd0f1df44cb5150e149

💬 achow101 commented on pull request "sqlite: Disallow writing from multiple `SQLiteBatch`s":
(https://github.com/bitcoin/bitcoin/pull/29112#issuecomment-1888041460)
> Another possible deadlock can arise when the txn abortion fails during the destruction of the batch object. Essentially, if the txn abortion fails and the object is destructed, the semaphore remains locked indefinitely, leading to a deadlock in any subsequent write operation.
>
> Made a test replicating the behavior; cherry-pick [5d2b79a](https://github.com/bitcoin/bitcoin/commit/5d2b79a65dcb10abdd349471c125595d677909ab) and [fbd59f8](https://github.com/bitcoin/bitcoin/commit/fbd59f89d3f5ed
...

💬 furszy commented on pull request "init: handle empty settings file gracefully":
(https://github.com/bitcoin/bitcoin/pull/29144#discussion_r1449469346)
yeah. Leftover from a previous implementation. I was checking the complete error message, including the settings file path (which was making the Windows CI job angry).

🤔 ishaanam reviewed a pull request: "wallet: Add CoinGrinder coin selection algorithm"
(https://github.com/bitcoin/bitcoin/pull/27877#pullrequestreview-1816719303)
Concept ACK

I'm still reviewing this PR, but I have left some initial review comments.

💬 ishaanam commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1449406274)
In e38c150f6dff886c470ba993c1725112c1ee5093 "coinselection: Add CoinGrinder algorithm ":

Shouldn't the comment be updated in the previous commit?

💬 ishaanam commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1449421239)
In e38c150f6dff886c470ba993c1725112c1ee5093 "coinselection: Add CoinGrinder algorithm":

The comment describing `m_min_change_target` in `CoinSelectionParams` should be updated, because currently it describes this variable as being used in Knapsack solver.

💬 ishaanam commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1449443744)
In e38c150f6dff886c470ba993c1725112c1ee5093 "coinselection: Add CoinGrinder algorithm":

Why do the current and selection amounts matter if they would both result in change? Shouldn't the best selection only be updated if the current selection's waste is less than the best selection's waste?

💬 ryanofsky commented on pull request "serialization: Support for multiple parameters":
(https://github.com/bitcoin/bitcoin/pull/28929#issuecomment-1888120269)
Rebased 97338eec9e35401f42c376062ea161a22143d628 -> ffc1e9fd8cf53cfcf4d7926ca2963958eacf73d7 ([`pr/iparams.2`](https://github.com/ryanofsky/bitcoin/commits/pr/iparams.2) -> [`pr/iparams.3`](https://github.com/ryanofsky/bitcoin/commits/pr/iparams.3), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/iparams.2-rebase..pr/iparams.3)) due to minor conflict with #28451

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

> Perhaps useful to elaborate on what the `mpgen` wrapping adds to the Cap'n Proto output?

Great suggestion, done in #10102. The main difference is `mpgen` clients subclasses have a high-level blocking interface, while capnp client subclasses require you to do asynchronous and pass arguments and return values in capnp message format.

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