💬 ajtowns commented on pull request "RFC: Instanced logs":
(https://github.com/bitcoin/bitcoin/pull/30338#issuecomment-2197436060)
> Re [#30338 (comment)](https://github.com/bitcoin/bitcoin/pull/30338#issuecomment-2194940918)
>
> > I don't understand where this is coming from. Libraries can export globals, eg stdin, so I don't see why you're claiming it's necessary for the kernel when it's not even necessary for libc.
>
> Being forced to eliminate all globals doesn't seem to be the claim here?

The opposite of instanced parameters is globals; if instanced parameters are necessary, then globals aren't possible. The c
...

💬 mzumsande commented on issue "Slow sync ":
(https://github.com/bitcoin/bitcoin/issues/30360#issuecomment-2197436677)
Looks like you were close to the tip, and a peer didn't send you the requested block `00000000000000000002a85bda12e6debef8023cc98e9133f78511bafe0952ac`. This looks similar to #29281 and would be fixed by the open PR #29664.

💬 ajtowns commented on pull request "logging: Use LogFatal instead LogError for fatal errors":
(https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1659168284)
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 ...")`.

💬 ajtowns commented on pull request "Improve new LogDebug/Trace/Info/Warning/Error Macros":
(https://github.com/bitcoin/bitcoin/pull/29256#issuecomment-2197457587)
> AJ, thanks for the response. I want to let your comment stand, and not reply in detail right away to wait for input on this PR from other potential reviewers.

That comment had already stood for eight weeks, and included an example patchset of changes that could improve things with regards to your complaints about wallet logging, while discussion continued on this. Instead we've had no progress and no discussion, which seems to me to be pretty counter productive. I've opened up what I assume
...

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1659178102)
With `m_next` we have constant time insertion at the head, which would be fine if we only added flagged entries and cleared them as we traverse them.

However, we also need to delete entries in constant time if they are both `FRESH` and `DIRTY` and get spent. For this we need a doubly linked list.

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