💬 jonatack commented on issue "ci: failure in `wallet_assumeutxo.py --descriptors`":
(https://github.com/bitcoin/bitcoin/issues/29234#issuecomment-1887823265)
Thanks for reporting. Ran into it as well.

https://github.com/bitcoin/bitcoin/actions/runs/7492061657/job/20394747721?pr=29230#step:27:7866

💬 jamesob commented on pull request "log: Nuke error(...)":
(https://github.com/bitcoin/bitcoin/pull/29236#issuecomment-1887882541)
Big Concept ACK. `error()` confuses me basically every time I run into it.

👍 stickies-v approved a pull request: "Update to new logging API"
(https://github.com/bitcoin/bitcoin/pull/29231#pullrequestreview-1816554251)
ACK 2632d038754d975e76926b7c068309c7aadc82f5

Since we don't have significant merge conflicts I would this support this PR having a non-scripted diff commit to first fix incorrect levels (instead of having to fix and then re-fix) but am okay with this approach too.

Suggested fixes in https://github.com/bitcoin/bitcoin/compare/master...stickies-v:bitcoin:2024-01/fix-log-level-29231 - happy to open a separate pull for it too?

💬 ryanofsky commented on pull request "logging: Simplify API for level based logging":
(https://github.com/bitcoin/bitcoin/pull/28318#discussion_r1449355955)
> which is trivial to do with `grep`?

This doesn't seem trivial if some log messages have categories and some don't. Like there is no way anymore to only see a list of all the messages only from wallet, or all the messages only from net_processing. I don't think this is terrible, but it does seem weird, and I wonder if any log systems work like this where they go out of their way to define categories, but then choose not to include categories in the most important messages.

💬 maflcko commented on pull request "ci: Rename tasks (previous releases, macOS cross)":
(https://github.com/bitcoin/bitcoin/pull/29218#issuecomment-1887924197)
CI failure can be ignored

💬 GregTonoski commented on issue "[Bug]: Blockspace price shouldn't be higher for a simple transaction (price discrimination against simple txs)":
(https://github.com/bitcoin/bitcoin/issues/29146#issuecomment-1887925923)
> 2024-01-11T14:31:31 <@sipa> sigh, there is economic demand for block space, whether you like it or not - the buyers and sellers of that space both want the transaction to occur, i don't see what anyone thinks they can do about that

Of course, there is economic demand for data storage. All the more so if it is permanent, distributed, replicated, immutable and branded by Bitcoin.

At the same time, there is economic demand for money (satoshi) transfers.

The block space is limited and
...

💬 ajtowns commented on pull request "logging: Simplify API for level based logging":
(https://github.com/bitcoin/bitcoin/pull/28318#discussion_r1449401248)
> This doesn't seem trivial if some log messages have categories and some don't.

If you want all log messages to have categories for easier grepping (or for importing into a database), there's the `-loglevelalways=1` option introduced in this PR.

> Like there is no way anymore to only see a list of all the messages only from wallet,

If you want to focus on messages from some section of the code, then that's what the `-logsourcelocations=1` option is for.

Your wording suggests that
...

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