💬 jamesob commented on pull request "logging: Simplify API for level based logging":
(https://github.com/bitcoin/bitcoin/pull/28318#discussion_r1449405556)
To clarify, I don't think that anyone is debating that this changeset is an improvement over the status quo. The ask (at least from my end) would be to unify the `Log*` interfaces and allow the optional specification of a category. That way, when migrating existing logging statements over, we're not throwing away category information just because something is INFO. And we support users who want to see WARN only most of the time, but care about, say, `net` INFO messages. That doesn't seem like a
...

📝 jamesob opened a pull request: "test: unbreak: exclude windows from wallet_assumeutxo"
(https://github.com/bitcoin/bitcoin/pull/29238)
Basic fix for the CI failure introduced in #28838 until something better can be devised.

Apparently Windows doesn't allow two processes to read the same wallet file. Note that this failure is unique to the tests and not something an end user would see (unless they were running two bitcoind instances...).

📝 sipa opened a pull request: "Make v2transaction default for addnode RPC when enabled"
(https://github.com/bitcoin/bitcoin/pull/29239)
Since #29058, several types of manually configured connections will attempt v2 connections when `-v2transport` is enabled, except for the `addnode` RPC, as that one has an explicit argument to enable or disable.

Make the default for that RPC match the `-v2transport` setting so the behavior matches that of other manual connections from a user perspective.

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

> I think it's much easier to delete legacy without touching the descriptor one if they're in different files.

Thanks, you're right. There's less overlap than I thought there would be.

💬 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