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

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

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1659222412)
I was trying to explain why using 10 iterations instead of 0 made sense. I have instead just changed it to 0 iterations.

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1659223824)
What prohibits us from accessing the previous ones in constant time during iteration (I haven't had the energy to delve into it properly yet)?

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1659225286)
Can you find a way to separate the side-effects? I find this very confusing.

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1659225694)
Got it, thanks

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1659228321)
It means "what the **queue size** needs to be before we can process an element from the **front**".

💬 paplorinc commented on pull request "scripted-diff: Log parameter interaction not thrice":
(https://github.com/bitcoin/bitcoin/pull/30358#issuecomment-2197514809)
ACK fa1bc7c88bb720f02955dbe4e2a174b5a52f3af8

💬 TheCharlatan commented on pull request "RFC: Instanced logs":
(https://github.com/bitcoin/bitcoin/pull/30338#issuecomment-2197523046)
Re https://github.com/bitcoin/bitcoin/pull/30338#issuecomment-2197436060

Thanks for going through this. Having something like `LaunchSettings` seems like a good thing.

> Fixing the formatting of buffered logs is a bit of work, but not very complicated. It doesn't seem particularly related to the kernel, though. Would you review a PR to do that?

yes :)

> I'm not sure what you mean? There's DisconnectTestLogger(); StartLogging(); ?

Besides the incoherent naming between the two, it a
...

👍 TheCharlatan approved a pull request: "scripted-diff: Log parameter interaction not thrice"
(https://github.com/bitcoin/bitcoin/pull/30358#pullrequestreview-2148843354)
Nice, ACK fa1bc7c88bb720f02955dbe4e2a174b5a52f3af8

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

Thanks I appreciate the clarification, and I agree with most of those points, except I genuinely do think having one convenience macro for each log level is nice. And LogPrintLevel exists so it is even possible to ignore the convenience macros if you want to do that.

Just as an observation, I get the sense that I am not the only one who likes the convenience macros, and that when other people read your comments comp
...

💬 fjahr commented on pull request "Correct Error Code in OP_IF/OP_NOTIF Empty Stack Check":
(https://github.com/bitcoin/bitcoin/pull/30359#issuecomment-2197545407)
> This change is accompanied by unit tests in the src/test/ directory that specifically verify the corrected behavior for OP_IF and OP_NOTIF with empty stacks.

Those seem to be missing