📝 maflcko opened a pull request: "log: Nuke error(...)"
(https://github.com/bitcoin/bitcoin/pull/29236)
`error(...)` has many issues:

* It is often used in the context of `return error(...)`, implying that it has a "fancy" type, creating confusing with `util::Result/Error`
* `-logsourcelocations` does not work with it, because it will pretend the error happened inside of `logging.h`
* The log line contains `ERROR: `, as opposed to `[error]`, like for other errors logged with `LogError`.

Fix all issues by removing it.

💬 fanquake commented on pull request "libconsensus: adapt API header to be compliant to ANSI C":
(https://github.com/bitcoin/bitcoin/pull/28661#issuecomment-1887738422)
Could draft for now, dependant on the outcome of #29189?

💬 mzumsande commented on pull request "log mempool loading progress":
(https://github.com/bitcoin/bitcoin/pull/29227#issuecomment-1887747143)
Concept ACK. I remember being surprised in the past that >5 minutes after starting up the node, my mempool was still loading.

📝 alfonsoromanz opened a pull request: "[depends] Allow PATH with spaces in directory names."
(https://github.com/bitcoin/bitcoin/pull/29237)
The goal of this PR is to help close https://github.com/bitcoin/bitcoin/pull/28733. I reverted the change on `depends/config.guess` based on the feedback provided in the previous PR. I've also incorporated the test mentioned by @maflcko

💬 alfonsoromanz commented on pull request "depends: Allow PATH with spaces in directory names.":
(https://github.com/bitcoin/bitcoin/pull/28733#issuecomment-1887761563)
Hi @maflcko! I opened this PR for this issue: https://github.com/bitcoin/bitcoin/pull/29237

✅ fanquake closed a pull request: "depends: Allow PATH with spaces in directory names."
(https://github.com/bitcoin/bitcoin/pull/28733)

💬 maflcko commented on pull request "[depends] Allow PATH with spaces in directory names.":
(https://github.com/bitcoin/bitcoin/pull/29237#issuecomment-1887780628)
lgtm ACK 4756114e505cff8848fb6344ef9a48d8822066c1

Thank you!

💬 maflcko commented on pull request "Update to new logging API":
(https://github.com/bitcoin/bitcoin/pull/29231#discussion_r1449283143)
Can remove the useless `__func__` when changing the log output anyway?

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