🚀 fanquake merged a pull request: "ci: gha: Set debug_pull_request_number_str annotation"
(https://github.com/bitcoin/bitcoin/pull/33754)
(https://github.com/bitcoin/bitcoin/pull/33754)
💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2486652576)
Some contention on shared resources is unavoidable. The threads need to synchronize on atomics. There are no locks in this implementation that all other threads need to wait on.
The main thread *may* have to wait here if there is a slow fetch, but it would be reading uncontested memory right up until one worker thread flips this bit. None of the other threads are trying to write to this same bit, so there is no contention between the main thread and any other worker threads.
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2486652576)
Some contention on shared resources is unavoidable. The threads need to synchronize on atomics. There are no locks in this implementation that all other threads need to wait on.
The main thread *may* have to wait here if there is a slow fetch, but it would be reading uncontested memory right up until one worker thread flips this bit. None of the other threads are trying to write to this same bit, so there is no contention between the main thread and any other worker threads.
📝 purpleKarrot opened a pull request: "prevector: simplify implementation of comparison operators and match behavior of `std::vector`"
(https://github.com/bitcoin/bitcoin/pull/33772)
The reduced amount of code reduces maintenance. Providing `prevector::operator<=>` allows classes that are implemented in terms of `prevector` to use the compiler provided `operator<=>`.
However, **there is a breaking change**!
The old implementation claims to be a drop-in replacement for `std::vector`. However, the implementation for `operator<` is different when comparing two vectors of different length, as can be presented with the following code:
```cpp
int main()
{
auto cons
...
(https://github.com/bitcoin/bitcoin/pull/33772)
The reduced amount of code reduces maintenance. Providing `prevector::operator<=>` allows classes that are implemented in terms of `prevector` to use the compiler provided `operator<=>`.
However, **there is a breaking change**!
The old implementation claims to be a drop-in replacement for `std::vector`. However, the implementation for `operator<` is different when comparing two vectors of different length, as can be presented with the following code:
```cpp
int main()
{
auto cons
...
💬 stickies-v commented on pull request "rest: Query predecessor headers using negative count param":
(https://github.com/bitcoin/bitcoin/pull/33752#issuecomment-3480837147)
The reorg potential does make using the interface more cumbersome in general, I'm not sure your approach solves that? You're still limited by what's in the `active_chain`, so you still need logic to deal with what if the index you're requesting a negative count from isn't in the active chain anymore?
(https://github.com/bitcoin/bitcoin/pull/33752#issuecomment-3480837147)
The reorg potential does make using the interface more cumbersome in general, I'm not sure your approach solves that? You're still limited by what's in the `active_chain`, so you still need logic to deal with what if the index you're requesting a negative count from isn't in the active chain anymore?
💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2486669187)
I don't see why aborting early would prevent us from using less precision. It's doubtful there would be a collision, and if there were that block will just be a little slower to connect.
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2486669187)
I don't see why aborting early would prevent us from using less precision. It's doubtful there would be a collision, and if there were that block will just be a little slower to connect.
👍 ryanofsky approved a pull request: "node: add `BlockTemplateCache`"
(https://github.com/bitcoin/bitcoin/pull/33421#pullrequestreview-3411154210)
Code review ACK 084bfbc1ec7f8f64f54d231bb641285622311b59 reviewing first commit in detail, but reviewed fuzz test in second commit pretty lightly. Changes here all seem very straightforward and implmenetation seems simpler than it was initially.
I do still suspect specifying template age in seconds instead of using a more granular measure may be a mistake if this caching is supposed to be useful for mining, but others would know more about this than me. It would seem straightfoward to add anoth
...
(https://github.com/bitcoin/bitcoin/pull/33421#pullrequestreview-3411154210)
Code review ACK 084bfbc1ec7f8f64f54d231bb641285622311b59 reviewing first commit in detail, but reviewed fuzz test in second commit pretty lightly. Changes here all seem very straightforward and implmenetation seems simpler than it was initially.
I do still suspect specifying template age in seconds instead of using a more granular measure may be a mistake if this caching is supposed to be useful for mining, but others would know more about this than me. It would seem straightfoward to add anoth
...
💬 ryanofsky commented on pull request "node: add `BlockTemplateCache`":
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2486593971)
In commit "node: add block template cache to miner" (cd2e843907a6810c8f1acf2b7d80bbf8273a5a00)
I know I suggested `operator==` previously but since this is not checking direct equality maybe it is better to call this method something like `CanReuse` or `ReusableTemplate`. Just an idea. Keeping operator== or using any method name seems ok.
Would also suggest moving detailed comments about specific fields inisde the method implementation. Could just have a general comment like "intentionally
...
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2486593971)
In commit "node: add block template cache to miner" (cd2e843907a6810c8f1acf2b7d80bbf8273a5a00)
I know I suggested `operator==` previously but since this is not checking direct equality maybe it is better to call this method something like `CanReuse` or `ReusableTemplate`. Just an idea. Keeping operator== or using any method name seems ok.
Would also suggest moving detailed comments about specific fields inisde the method implementation. Could just have a general comment like "intentionally
...
💬 ryanofsky commented on pull request "node: add `BlockTemplateCache`":
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2486621134)
In commit "node: add block template cache to miner" (cd2e843907a6810c8f1acf2b7d80bbf8273a5a00)
Probably should use `uint32_t` instead of `const uint32_t&`. Const reference parameter are usually only preferred when parameter types are much bigger.
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2486621134)
In commit "node: add block template cache to miner" (cd2e843907a6810c8f1acf2b7d80bbf8273a5a00)
Probably should use `uint32_t` instead of `const uint32_t&`. Const reference parameter are usually only preferred when parameter types are much bigger.
💬 ryanofsky commented on pull request "node: add `BlockTemplateCache`":
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2486606258)
In commit "node: add block template cache to miner" (cd2e843907a6810c8f1acf2b7d80bbf8273a5a00)
Should mempool be a reference rather if this class can't really function when it is null?
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2486606258)
In commit "node: add block template cache to miner" (cd2e843907a6810c8f1acf2b7d80bbf8273a5a00)
Should mempool be a reference rather if this class can't really function when it is null?
💬 ryanofsky commented on pull request "node: add `BlockTemplateCache`":
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2486576551)
In commit "node: add block template cache to miner" (cd2e843907a6810c8f1acf2b7d80bbf8273a5a00)
Maybe use `auto block_copy = std::make_shared<const CBlock>(m_block_template->block);` here to avoid needing to make a shared_ptr and a second copy of the block below.
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2486576551)
In commit "node: add block template cache to miner" (cd2e843907a6810c8f1acf2b7d80bbf8273a5a00)
Maybe use `auto block_copy = std::make_shared<const CBlock>(m_block_template->block);` here to avoid needing to make a shared_ptr and a second copy of the block below.
💬 ryanofsky commented on pull request "node: add `BlockTemplateCache`":
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2486625191)
In commit "node: add block template cache to miner" (cd2e843907a6810c8f1acf2b7d80bbf8273a5a00)
Maybe s/maximum_/max_/
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2486625191)
In commit "node: add block template cache to miner" (cd2e843907a6810c8f1acf2b7d80bbf8273a5a00)
Maybe s/maximum_/max_/
💬 optout21 commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2486699478)
It doesn't make sense to check the skip for block index 0, that's why 0 is excluded.
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2486699478)
It doesn't make sense to check the skip for block index 0, that's why 0 is excluded.
👍 hebasto approved a pull request: "depends: disable variables, rules and suffixes."
(https://github.com/bitcoin/bitcoin/pull/33045#pullrequestreview-3411327796)
ACK 3651344fedfd75ea1fd178e53de2607a3bdfb558.
> I was going to suggest:
>
> * Using `--no-builtin-rules` rather than `-r` to be a little more self-documenting
>
> ...
>
> But...
>
> BSD Make supports `-r`, so that pretty much moots both points. Also, I'm pretty sure we require GNU Make anyway.
We do [require](https://github.com/bitcoin/bitcoin/blob/master/depends/README.md) GNU Make, so I also support using the long option names for better self-documentation.
(https://github.com/bitcoin/bitcoin/pull/33045#pullrequestreview-3411327796)
ACK 3651344fedfd75ea1fd178e53de2607a3bdfb558.
> I was going to suggest:
>
> * Using `--no-builtin-rules` rather than `-r` to be a little more self-documenting
>
> ...
>
> But...
>
> BSD Make supports `-r`, so that pretty much moots both points. Also, I'm pretty sure we require GNU Make anyway.
We do [require](https://github.com/bitcoin/bitcoin/blob/master/depends/README.md) GNU Make, so I also support using the long option names for better self-documentation.
💬 optout21 commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2486708184)
If it's inlined, it can't be used as extern from the test :(
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2486708184)
If it's inlined, it can't be used as extern from the test :(
💬 hebasto commented on issue "depends: Evaluate potential improvements":
(https://github.com/bitcoin/bitcoin/issues/18821#issuecomment-3480893699)
> 2. Eliminating builtin rules and variables e.g. `MAKEFLAGS += --no-builtin-variables`: I've tried this, and it breaks libminiupnpc because it relies on builtin rules.
See https://github.com/bitcoin/bitcoin/pull/33045.
(https://github.com/bitcoin/bitcoin/issues/18821#issuecomment-3480893699)
> 2. Eliminating builtin rules and variables e.g. `MAKEFLAGS += --no-builtin-variables`: I've tried this, and it breaks libminiupnpc because it relies on builtin rules.
See https://github.com/bitcoin/bitcoin/pull/33045.
👍 hebasto approved a pull request: "ci: Fix lint runner selection (and docker cache)"
(https://github.com/bitcoin/bitcoin/pull/33744#pullrequestreview-3411360968)
ACK 0b3b8a3be1a0db0dfc634acca1d9305dc0fbfae6, I have reviewed the code and it looks OK.
(https://github.com/bitcoin/bitcoin/pull/33744#pullrequestreview-3411360968)
ACK 0b3b8a3be1a0db0dfc634acca1d9305dc0fbfae6, I have reviewed the code and it looks OK.
💬 optout21 commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2486733227)
I check the avg, which is computationally easier to obtain. It would make sense to check the median as well, and maybe compare the median with the average. However, I prefer to avoid the O(N log N) complexity of the sorting.
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2486733227)
I check the avg, which is computationally easier to obtain. It would make sense to check the median as well, and maybe compare the median with the average. However, I prefer to avoid the O(N log N) complexity of the sorting.
💬 optout21 commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2486736452)
Yes, nontheless I reduce the count from 30000 to 10000, that's also enough for the test.
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2486736452)
Yes, nontheless I reduce the count from 30000 to 10000, that's also enough for the test.
💬 optout21 commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2486781519)
Removed
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2486781519)
Removed
💬 optout21 commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2486783350)
I've adjusted the numbers used and added comments
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2486783350)
I've adjusted the numbers used and added comments