💬 jonatack commented on pull request "Improve new LogDebug/Trace/Info/Warning/Error Macros":
(https://github.com/bitcoin/bitcoin/pull/29256#issuecomment-2197461775)
> 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?

All of these, I think. The idea was:

- one `Log(level, cat, msg)` macro with a consistent API replace the current macros
- the logging config options and logging RPC to be soft-aligned into a single user-facing logging API
- the printed logs to be the same format, apart from custom options
- developer documentati
...

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1659182104)
I think this makes the diff easier to follow, since in git it highlights only the `flags` and `GetFlags()` in the line.

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1659186375)
It's mostly convenience. Otherwise to clear flags while iterating we would have to bind `m_next` to another variable first, then call `ClearFlags` on the current pair, and then bind the the other variable holding the old `m_next` to the current pair variable.

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1659190841)
Yes, this code is single threaded so not possible to get past the previous line if `m_prev` is `null`.
It is possible for `m_prev` to point to a pair that has been freed if the linked list head is destroyed, which is why `cacheCoins` must always be destroyed before the linked list head.

💬 ryanofsky commented on pull request "RFC: Instanced logs":
(https://github.com/bitcoin/bitcoin/pull/30338#issuecomment-2197476854)
> Touching every single line of logging code for a "nice to have" doesn't make much sense to me.

AJ I know we are disagreeing about a lot of things, and I do not fault you in any way for this, but your reactions seem very strong relative the actual scope of changes we would like to make.

We should set aside Cory's big scripted diff commit here, which I think is a proof of concept and an illustrative change, not an actual change being proposed to merge. I think the actual changes being cons
...

💬 theuni commented on pull request "RFC: Instanced logs":
(https://github.com/bitcoin/bitcoin/pull/30338#issuecomment-2197483154)
Yes, @ryanofsky. This was a POC (FWIW I am actually very ok with the scripted-diff approach which touches every line, but I didn't expect it to be popular). At this point I'd prefer to throw my weight behind #30342 as it accomplishes what I want without being so invasive.

I won't add to the arguing here as I think you and @TheCharlatan have said what I would've anyway.

💬 jonatack commented on pull request "logging: Use LogFatal instead LogError for fatal errors":
(https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1659205782)
> By overwrought, jonatack means `LogError("CHECKSUM ERROR ...")`, while his preferred non-overwrought api from #25203 looks like `LogPrintLevel(BCLog::NET, BCLog::Level::Error, "CHECKSUM ERROR ...")`.

As you know, I've described what I mean several times, including in my comments in the pull you mention. Here is the most recent one: https://github.com/bitcoin/bitcoin/pull/29256#issuecomment-2197461775

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1659205863)
if there was a separate commit doing only this, it would simplify the other important commits a lot

💬 jonatack commented on pull request "scripted-diff: Use LogInfo/LogDebug over LogPrintf/LogPrint":
(https://github.com/bitcoin/bitcoin/pull/29641#issuecomment-2197492971)
I suggest https://github.com/bitcoin/bitcoin/pull/29256#issuecomment-2197461775 instead.

It may also make sense to finish consensus and work on the simplest possible consistent user-facing API and developer API before doing a mass migration.

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1659216405)
`BitSet<N>::Size()` may differ from `N`.

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1659217667)
Done.

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1659218218)
I've outlawed `MarkDone()` with `select` not a subset of `todo`.

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1659218521)
Added, please have a look if this is what you meant.

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1659218637)
Fixed.

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1659218822)
Fixed.

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1659218927)
Fixed.

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1659219435)
Done, named it `MAX_SIMPLE_ITERATIONS` (and used more readable number 300000).

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2197498881)
> Changing only parts of it can break the tests

I usually try to recreate every line, after I have all the changes by interactively rebasing the current state and adding small refactoring under the current commit.
E.g. these could be separate commits doing a single thing:
* renaming `bool erase` to `bool will_erase`
* changing `it->second.flags` usages to `it->second.GetFlags()` (again, without change in behavior)
* changing `it->second.flags |= ` to use `it->second.AddFlags(CCoinsCacheEn
...

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1659219716)
Done.

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1659220190)
See new unit test.