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

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1659220493)
I'd rather not. The point of `ExhaustiveCandidateFinder` is really testing the correctness of `SimpleCandidateFinder` (whose correctness may not be obvious to reviewers), so that `SimpleCandidateFinder` on its turn can be used to test `SearchCandidateFinder`. I added comments to explain that.

Both of these are done within the same fuzz test, which is perhaps confusing? I could split up the tests (an exhaustive-vs-simple test, and a simple-vs-search test).

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1659220875)
Indeed. I don't think 1 iteration matters.

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

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