💬 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**".
(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
(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
...
(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
(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
...
(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
(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
(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.
(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.
(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.
(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.
(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.
💬 Emzy commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2197644218)
Tested ACK aba5047
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2197644218)
Tested ACK aba5047
💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1659322903)
In that case stick to it and I'll add it in a separate PR once this is merged!
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1659322903)
In that case stick to it and I'll add it in a separate PR once this is merged!
🤔 tdb3 reviewed a pull request: "refactor: add coinbase constraints to BlockAssembler::Options"
(https://github.com/bitcoin/bitcoin/pull/30356#pullrequestreview-2148873843)
Approach ACK
This adds flexibility for SV2, having less magic numbers is better. I support adding a configuration parameter for this (would prevent others from needing to fork).
Left a couple small nits for now, but plan to test/exercise in more detail.
(https://github.com/bitcoin/bitcoin/pull/30356#pullrequestreview-2148873843)
Approach ACK
This adds flexibility for SV2, having less magic numbers is better. I support adding a configuration parameter for this (would prevent others from needing to fork).
Left a couple small nits for now, but plan to test/exercise in more detail.
💬 tdb3 commented on pull request "refactor: add coinbase constraints to BlockAssembler::Options":
(https://github.com/bitcoin/bitcoin/pull/30356#discussion_r1659263337)
nit: Is it comprehensive using the term "pool" here, or would it be better to keep things generic (and perhaps add an example)? This is reserving space in the template for the coinbase and the entity requesting the template could be a pool or a solo miner (albeit less commonly), right? Maybe something like `maximum additional serialized bytes the block requestor can add in coinbase transaction outputs (for example, to enable pool payouts)`.
nit: weight and bytes seem to be used interchang
...
(https://github.com/bitcoin/bitcoin/pull/30356#discussion_r1659263337)
nit: Is it comprehensive using the term "pool" here, or would it be better to keep things generic (and perhaps add an example)? This is reserving space in the template for the coinbase and the entity requesting the template could be a pool or a solo miner (albeit less commonly), right? Maybe something like `maximum additional serialized bytes the block requestor can add in coinbase transaction outputs (for example, to enable pool payouts)`.
nit: weight and bytes seem to be used interchang
...
💬 ajtowns commented on pull request "RFC: Instanced logs":
(https://github.com/bitcoin/bitcoin/pull/30338#issuecomment-2197683031)
> Re [#30338 (comment)](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 :)
Have a look at https://github.com/ajtowns/bitcoin/pull/9 and see what you think; obviously needs a l
...
(https://github.com/bitcoin/bitcoin/pull/30338#issuecomment-2197683031)
> Re [#30338 (comment)](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 :)
Have a look at https://github.com/ajtowns/bitcoin/pull/9 and see what you think; obviously needs a l
...
💬 Mariuszok12 commented on pull request "wallet: use LogTrace for walletdb log messages at trace level":
(https://github.com/bitcoin/bitcoin/pull/30355#discussion_r1659360805)
3H9yBBp3fmaZTf1iyg6Vjv1WTAENeva5Qh
(https://github.com/bitcoin/bitcoin/pull/30355#discussion_r1659360805)
3H9yBBp3fmaZTf1iyg6Vjv1WTAENeva5Qh
🤔 Mariuszok12 reviewed a pull request: "wallet: use LogTrace for walletdb log messages at trace level"
(https://github.com/bitcoin/bitcoin/pull/30355#pullrequestreview-2149041661)
3H9yBBp3fmaZTf1iyg6Vjv1WTAENeva5Qh
(https://github.com/bitcoin/bitcoin/pull/30355#pullrequestreview-2149041661)
3H9yBBp3fmaZTf1iyg6Vjv1WTAENeva5Qh
💬 Mariuszok12 commented on pull request "policy: Add `OP_TRUE <0x4e73>` as a standard output script":
(https://github.com/bitcoin/bitcoin/pull/30352#discussion_r1659361958)
3H9yBBp3fmaZTf1iyg6Vjv1WTAENeva5Qh
(https://github.com/bitcoin/bitcoin/pull/30352#discussion_r1659361958)
3H9yBBp3fmaZTf1iyg6Vjv1WTAENeva5Qh
💬 Mariuszok12 commented on pull request "policy: Add `OP_TRUE <0x4e73>` as a standard output script":
(https://github.com/bitcoin/bitcoin/pull/30352#discussion_r1659362071)
3H9yBBp3fmaZTf1iyg6Vjv1WTAENeva5Qh
(https://github.com/bitcoin/bitcoin/pull/30352#discussion_r1659362071)
3H9yBBp3fmaZTf1iyg6Vjv1WTAENeva5Qh