💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1659005029)
this `it->second.GetFlags() & CCoinsCacheEntry::DIRTY` appears quite often, maybe it would make sense to extract it to `it->second.IsDirty()` (and `it->second.IsFresh()`, of course)

💬 ryanofsky commented on issue "RFC: Misused LogError and LogWarning macros":
(https://github.com/bitcoin/bitcoin/issues/30348#issuecomment-2197288729)
re: https://github.com/bitcoin/bitcoin/issues/30348#issue-2378034511

> Current `LogWarning` usages looked appropriate, but were rare, with only 8 usages currently.

This turned out to be incorrect, so I edited this part of the issue description. wilcl-ark found a case where `LogWarning` is currently being misused (https://github.com/bitcoin/bitcoin/pull/30347#issuecomment-2194685490) to warn about an unexpectedly large `-checkblocks` value. I also found more incorrect uses that I missed bef
...

💬 paplorinc commented on pull request "optimization: Reduce cache lookups in CCoinsViewCache::FetchCoin":
(https://github.com/bitcoin/bitcoin/pull/30326#issuecomment-2197292114)
> your time might be better spent reviewing Pull Requests like https://github.com/bitcoin/bitcoin/pull/28280

Thanks for the hint @fanquake, it's a tough PR, I have added my two sats to it: https://github.com/bitcoin/bitcoin/pull/28280#pullrequestreview-2138727793

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1659023545)
I don't fully understand how we need `m_prev` here, but have you investigated whether it's possible to get rid of it and rely on `m_next` only?
Sometimes we still have a reference to both, but I assume that wasn't always the case, right?

💬 ryanofsky commented on pull request "logging: Use LogFatal instead LogError for fatal errors":
(https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1659025992)
re: https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1659004164

> Meta:
>
> It's not clear why people are 👍 on that comment.

I kind of like the mystery of the reactions. But I 👍 AJ's chart because I think it is a good recommendation for how to choose log levels in new code, even if it doesn't really tell you what log levels mean in existing code. I also broadly agree with having few log levels and using few log levels. I think by a large margin most log statements should be u
...

💬 jonatack commented on pull request "Improve new LogDebug/Trace/Info/Warning/Error Macros":
(https://github.com/bitcoin/bitcoin/pull/29256#issuecomment-2197318437)

> All consistent, now accepting the same parameters
> All compatible with log categories, to make it possible to filter or highlight log messages from a particular component
> All compatible with wallet logging, which includes the wallet name in log messages
> Require less verbosity, by not needing category constants to be repeated in log prints from a common source.
> Not tied to one hardcoded global Logger instance


I think I'd be Concept ACK on these, though I've some
...

💬 tdb3 commented on pull request "test: Added coverage to Block not found error using gettxoutsetinfo":
(https://github.com/bitcoin/bitcoin/pull/30340#issuecomment-2197343758)
> thanks @tdb3 and @alfonsoromanz I added a log in [82c454c](https://github.com/bitcoin/bitcoin/pull/30340/commits/82c454c65658439481b50fe71ed097ecb8d70737)
>
> Also @tdb3 I tried doing
>
> ```
> b1hash = index_node.getblockhash(1)
> index_node.invalidateblock(b1hash)
>
> self.log.info("Test obtaining info for a non-existent block hash")
> assert_raises_rpc_error(-5, "Block not found", index_node.gettxoutsetinfo, hash_type="none", hash_or_height=b1hash, use_index=True)
>
> index_n
...

💬 ajtowns commented on pull request "logging: Use LogFatal instead LogError for fatal errors":
(https://github.com/bitcoin/bitcoin/pull/30347#issuecomment-2197354127)
Concept NACK; introducing more different levels just makes it require more effort to use the logging system, and increases the chances they will be used incorrectly.

💬 ryanofsky commented on pull request "Improve new LogDebug/Trace/Info/Warning/Error Macros":
(https://github.com/bitcoin/bitcoin/pull/29256#issuecomment-2197361518)
> We've just over-complicated everything when it could be simple and open. I know a number of other developers think the same but don't speak up much because it seems pointless to do so.

Curious, when you say "over-complicated", are you referring to the implementation of the logging code, or to the usage of the logging API? Or to effects of logging settings?

I don't think the logging API is too complicated. I think #28318 made it more self documenting and simpler.

On the other hand, I w
...

💬 ajtowns commented on pull request "logging: Use LogFatal instead LogError for fatal errors":
(https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1659082202)
> Your [flowchart](https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1657886163) is a good idealization, but it does not describe the realities of current code, which is using logging for ordinary error reporting, not just the most critical errors.

The current code mostly hasn't migrated to the multiple severity levels from the original LogPrintf/LogPrint split. It would be nice to spend time working on that, but we're instead constantly bikeshedding the API instead and treating that
...

💬 ajtowns commented on pull request "logging: Use LogFatal instead LogError for fatal errors":
(https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1659096893)
It's not clear to me that there's any reason to separate the current `LogWarning` / `LogError` levels; those existed prior to #28318 and I documented them in that PR as best I understood them. However, there's not much benefit in a log that says "hey this error is about to cause the node to stop" -- you already get that information by seeing "Shutdown: in progress..." immediately following. So it could make sense to merge Warning/Error into:

* - `LogAlert(fmt, params...)` should be used in p
...

👍 theuni approved a pull request: "kernel: De-globalize validation caches"
(https://github.com/bitcoin/bitcoin/pull/30141#pullrequestreview-2148613939)
utACK 064a401e10f2b2da8f31f682929431d06c671d93 assuming sipa doesn't have a problem with the shared nonce.

🤔 pinheadmz reviewed a pull request: "Broadcast own transactions only via short-lived Tor or I2P connections"
(https://github.com/bitcoin/bitcoin/pull/29415#pullrequestreview-2148337702)
code review ACK 7a7e7d189dfe5efa6ab3c644f55f410412163503

some questions and comments above and below (sorry spread out review over a few days)

I want to run this in warnet again with a lot more TXs and see how we do as well

💬 pinheadmz commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1659087156)
0a6797ffb3eb2d45bce1dc68e830efd89ac2a442

Teleporting the comment from https://github.com/bitcoin/bitcoin/pull/29420#discussion_r1592701864

Why didn't we need to do this on master?

The old log message was `Connected & Listening: 127.0.0.1:15689` not `Connected & Listening: 0:0`

💬 pinheadmz commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1658932157)
a86785dc29faa2dcff301641e2f823b73089dae2

should `1min` be defined as a constant in the header file? This is what we consider stale?

💬 pinheadmz commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1658942976)
5f49e61ffa576aa6c27de30a7db51d17c4f5c516

How could this be condition be different now than when it is checked in init.cpp?

💬 jonatack commented on pull request "logging: Use LogFatal instead LogError for fatal errors":
(https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1659120894)
> The current code mostly hasn't migrated to the multiple severity levels from the original LogPrintf/LogPrint split. It would be nice to spend time working on that, but we're instead constantly bikeshedding the API instead and treating that as a blocker for any other logging changes.

The logging was mostly migrated in the severity level parent PR #25203 before the whole thing was taken off the rails with the overwrought API and rules. (There have also been a number of pulls opened that were
...

👍 hodlinator approved a pull request: "doc: clarify loadwallet path loading for wallets"
(https://github.com/bitcoin/bitcoin/pull/30302#pullrequestreview-2148642582)
ACK 69bf58dc0e25897e9fde435c9823a921590a90dc

Built and tested `bitcoin-cli -regtest loadwallet`.

💬 hodlinator commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1659121673)
Could reduce it down to only one `HelpExampleRpc` if one would want to reduce the number of lines.
```suggestion
+ "\nLoad wallet with files under wallets/foo/ via RPC:\n"
+ HelpExampleRpc("loadwallet", "\"foo\"")
+ "\nLoad wallet with files under wallets/foo/specialWallet/:\n"
+ HelpExampleCli("loadwallet", "\"foo/specialWallet/\"")
+ "\nLoad wallet using absolute path (Unix):\n"

...

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2197401102)
> Fantastic change, would be helpful if you could make it a bit more digestible by separating it into smaller commits (especially splitting high-risk changes from low-risk refactoring), explaining the changes in commit messages.

Thank you @paplorinc! I did try initially to separate the changes more. However, I found the third commit has to have all those changes done in one commit otherwise the test-all-commits CI job will fail. Changing only parts of it can break the tests, and break buildin
...