💬 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

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

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1659275459)
> What prohibits us from accessing the previous ones in constant time during iteration

Nothing during iteration. But we need to delete entries randomly when a FRESH coin is spent, which is not during any iteration.

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1659299969)
Is this really obvious though that we need to clear flags before destruction? Normally flags wouldn't affect destroying an object, but in this case they also tie the object to a linked list.

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1659304288)
No, but it's in a destructor, calling the `ClearFlags` method - I didn't seen what the comment added beyond what the code already explained well.
It could explain why you chose this style, or something that the code cannot, but this just seemed redundant to me.

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1659308223)
Sorry, can you elaborate on what benefit that would have here, and any possible downsides? I'm not 100% sure what all the differences are between the two methods, so preferred to stick to status quo.