🤔 furszy reviewed a pull request: "qa: Avoid knock-on exception in assert_start_raises_init_error"
(https://github.com/bitcoin/bitcoin/pull/32929#pullrequestreview-3589512416)
Code ACK 356883f0e48be59bcb154096cef82cbf3f0dd9d8
(https://github.com/bitcoin/bitcoin/pull/32929#pullrequestreview-3589512416)
Code ACK 356883f0e48be59bcb154096cef82cbf3f0dd9d8
💬 hebasto commented on pull request "ci: Add IWYU job":
(https://github.com/bitcoin/bitcoin/pull/33810#issuecomment-3667066005)
@maflcko
Thank you for the review. The PR has been reworked per your feedback.
(https://github.com/bitcoin/bitcoin/pull/33810#issuecomment-3667066005)
@maflcko
Thank you for the review. The PR has been reworked per your feedback.
💬 hebasto commented on pull request "ci: Add IWYU job":
(https://github.com/bitcoin/bitcoin/pull/33810#discussion_r2628562127)
> edit: The full patch would be:
[Taken](https://github.com/bitcoin/bitcoin/pull/33810#issuecomment-3667066005).
(https://github.com/bitcoin/bitcoin/pull/33810#discussion_r2628562127)
> edit: The full patch would be:
[Taken](https://github.com/bitcoin/bitcoin/pull/33810#issuecomment-3667066005).
💬 jb55 commented on issue "sqlite legacy descriptor wallet migration fails":
(https://github.com/bitcoin/bitcoin/issues/33468#issuecomment-3667104351)
oh nice I instinctually tried that and ran into the checksum error, didn't realize I could replace the checksum. will try
(https://github.com/bitcoin/bitcoin/issues/33468#issuecomment-3667104351)
oh nice I instinctually tried that and ran into the checksum error, didn't realize I could replace the checksum. will try
💬 jb55 commented on issue "sqlite legacy descriptor wallet migration fails":
(https://github.com/bitcoin/bitcoin/issues/33468#issuecomment-3667153801)
that works! thanks boss.
(https://github.com/bitcoin/bitcoin/issues/33468#issuecomment-3667153801)
that works! thanks boss.
✅ jb55 closed an issue: "sqlite legacy descriptor wallet migration fails"
(https://github.com/bitcoin/bitcoin/issues/33468)
(https://github.com/bitcoin/bitcoin/issues/33468)
💬 stickies-v commented on issue "RFC: separate kernel logging infrastructure":
(https://github.com/bitcoin/bitcoin/issues/34062#issuecomment-3667169341)
I have implemented a new approach ("Layering") in https://github.com/bitcoin/bitcoin/compare/master...stickies-v:bitcoin:2025-12/kernel-logging-layering. The Layering approach turned out much less invasive than my initial attempts showed, and now has my preference.
In a nutshell:
- we distinguish between a (low-level) logger that produces log statements (`util::log::Logger`) and (higher-level) log sinks (`BCLog::Sink`, `btck_LoggingConnection`) that consume the logs by registering a callback on
...
(https://github.com/bitcoin/bitcoin/issues/34062#issuecomment-3667169341)
I have implemented a new approach ("Layering") in https://github.com/bitcoin/bitcoin/compare/master...stickies-v:bitcoin:2025-12/kernel-logging-layering. The Layering approach turned out much less invasive than my initial attempts showed, and now has my preference.
In a nutshell:
- we distinguish between a (low-level) logger that produces log statements (`util::log::Logger`) and (higher-level) log sinks (`BCLog::Sink`, `btck_LoggingConnection`) that consume the logs by registering a callback on
...
👍 ryanofsky approved a pull request: "refactor: unify container presence checks"
(https://github.com/bitcoin/bitcoin/pull/33192#pullrequestreview-3589700908)
Code review ACK d9319b06cf82664d55f255387a348135fd7f91c7. I manually reviewed the full change, and it seems there are a lot of positive comments about this and no more very significant conflicts, so I will merge it shortly.
I am wondering if there are plans to turn on the clang-tidy check and follow up with other replacements?
(https://github.com/bitcoin/bitcoin/pull/33192#pullrequestreview-3589700908)
Code review ACK d9319b06cf82664d55f255387a348135fd7f91c7. I manually reviewed the full change, and it seems there are a lot of positive comments about this and no more very significant conflicts, so I will merge it shortly.
I am wondering if there are plans to turn on the clang-tidy check and follow up with other replacements?
🚀 ryanofsky merged a pull request: "refactor: unify container presence checks"
(https://github.com/bitcoin/bitcoin/pull/33192)
(https://github.com/bitcoin/bitcoin/pull/33192)
💬 l0rinc commented on pull request "refactor: unify container presence checks":
(https://github.com/bitcoin/bitcoin/pull/33192#issuecomment-3667210907)
> I am wondering if there are plans to turn on the clang-tidy check and follow up with other replacements?
I couldn't use tidy for many of these changes, but I'm looking forward to doing a follow-up.
Thanks for the persistent reviewers, I know it was a boring one :)
(https://github.com/bitcoin/bitcoin/pull/33192#issuecomment-3667210907)
> I am wondering if there are plans to turn on the clang-tidy check and follow up with other replacements?
I couldn't use tidy for many of these changes, but I'm looking forward to doing a follow-up.
Thanks for the persistent reviewers, I know it was a boring one :)
📝 Dahka2321 opened a pull request: "chore: bump checkout to v6"
(https://github.com/bitcoin/bitcoin/pull/34094)
Updated actions/checkout from v5 to v6 in ci.yml workflow
(https://github.com/bitcoin/bitcoin/pull/34094)
Updated actions/checkout from v5 to v6 in ci.yml workflow
💬 waketraindev commented on pull request "[30.x] Finalise v30.1":
(https://github.com/bitcoin/bitcoin/pull/34092#issuecomment-3667331991)
ACK https://github.com/bitcoin/bitcoin/commit/2a21824b1190968ee8ba3120400c00e56be10591
(https://github.com/bitcoin/bitcoin/pull/34092#issuecomment-3667331991)
ACK https://github.com/bitcoin/bitcoin/commit/2a21824b1190968ee8ba3120400c00e56be10591
💬 theuni commented on pull request "Add initial vectorized chacha20 implementation for 2-3x speedup":
(https://github.com/bitcoin/bitcoin/pull/34083#issuecomment-3667333373)
Ok, after some experimenting, I am very hesitant to add the helpers as suggested above. For example, consider @ajtowns's seemingly innocuous Repeat function:
```c++
template <size_t REPS, typename Fn>
ALWAYS_INLINE void Repeat(Fn&& fn)
{
if constexpr (REPS > 0) {
fn();
Repeat<REPS-1>(std::forward<Fn>(fn));
}
}
```
This causes a 10% slowdown on this branch, but with avx2 the performance is abysmal:
Baseline(master):
```
| 1.38 | 723,27
...
(https://github.com/bitcoin/bitcoin/pull/34083#issuecomment-3667333373)
Ok, after some experimenting, I am very hesitant to add the helpers as suggested above. For example, consider @ajtowns's seemingly innocuous Repeat function:
```c++
template <size_t REPS, typename Fn>
ALWAYS_INLINE void Repeat(Fn&& fn)
{
if constexpr (REPS > 0) {
fn();
Repeat<REPS-1>(std::forward<Fn>(fn));
}
}
```
This causes a 10% slowdown on this branch, but with avx2 the performance is abysmal:
Baseline(master):
```
| 1.38 | 723,27
...
💬 sedited commented on issue "RFC: separate kernel logging infrastructure":
(https://github.com/bitcoin/bitcoin/issues/34062#issuecomment-3667358790)
> I'll open a PR if/when people are happy about the concept and initial approach, that's probably a better place to discuss code in more depth.
This newest approach looks good to me. I think some people might still have reservations about unconditionally logging all categories. I tend to agree with your opinion on this though. Once debug logging is activated, expectations on string serialization overhead should be the same, no matter the category selected.
(https://github.com/bitcoin/bitcoin/issues/34062#issuecomment-3667358790)
> I'll open a PR if/when people are happy about the concept and initial approach, that's probably a better place to discuss code in more depth.
This newest approach looks good to me. I think some people might still have reservations about unconditionally logging all categories. I tend to agree with your opinion on this though. Once debug logging is activated, expectations on string serialization overhead should be the same, no matter the category selected.
📝 l0rinc opened a pull request: "refactor: enable `readability-container-contains` clang-tidy rule"
(https://github.com/bitcoin/bitcoin/pull/34095)
Replace the last few instances of `.count() != 0` and `.count() == 0` patterns with the more expressive C++20 `.contains()` method:
* `std::set<std::string>` in `getblocktemplate` RPC;
* `std::map<std::string, ...>` in `transaction_tests`;
* `boost::multi_index_container` (w)tx checks in `CTxMemPool::exists()`
With no remaining violations, enable the `readability-container-contains` clang-tidy check to prevent future regressions.
Follow-up to https://github.com/bitcoin/bitcoin/pull/33
...
(https://github.com/bitcoin/bitcoin/pull/34095)
Replace the last few instances of `.count() != 0` and `.count() == 0` patterns with the more expressive C++20 `.contains()` method:
* `std::set<std::string>` in `getblocktemplate` RPC;
* `std::map<std::string, ...>` in `transaction_tests`;
* `boost::multi_index_container` (w)tx checks in `CTxMemPool::exists()`
With no remaining violations, enable the `readability-container-contains` clang-tidy check to prevent future regressions.
Follow-up to https://github.com/bitcoin/bitcoin/pull/33
...
💬 l0rinc commented on pull request "log: show reindex progress in `ImportBlocks`":
(https://github.com/bitcoin/bitcoin/pull/33353#issuecomment-3667507927)
rfm?
(https://github.com/bitcoin/bitcoin/pull/33353#issuecomment-3667507927)
rfm?
💬 arejula27 commented on pull request "build: Fix source paths for debugging in CMake":
(https://github.com/bitcoin/bitcoin/pull/34081#issuecomment-3667561373)
I have analysed how the different debug information is stored in the master branch, using my changes and removing the remaps (your proposal ). The observable difference is in how DWARF debug information records source file paths.
Here are the three scenarios I tested with `objdump -Wi` to extract the info:
```bash
readelf -wi ./build/bin/bitcoind | grep DW_AT_name | grep "\.cpp" >clean_readelf.txt
```
## First case (master branch – typical out-of-tree build)
```text
<3674904> DW_
...
(https://github.com/bitcoin/bitcoin/pull/34081#issuecomment-3667561373)
I have analysed how the different debug information is stored in the master branch, using my changes and removing the remaps (your proposal ). The observable difference is in how DWARF debug information records source file paths.
Here are the three scenarios I tested with `objdump -Wi` to extract the info:
```bash
readelf -wi ./build/bin/bitcoind | grep DW_AT_name | grep "\.cpp" >clean_readelf.txt
```
## First case (master branch – typical out-of-tree build)
```text
<3674904> DW_
...
👋 l0rinc's pull request is ready for review: "refactor: enable `readability-container-contains` clang-tidy rule"
(https://github.com/bitcoin/bitcoin/pull/34095)
(https://github.com/bitcoin/bitcoin/pull/34095)
💬 brunoerg commented on pull request "Implementation of SwiftSync":
(https://github.com/bitcoin/bitcoin/pull/34004#issuecomment-3667635232)
> Regarding the pruned-only feature. This would not be the case if there was a P2P message for undo-data. It would allow undo-data to be written in an un-trusted state and later confirmed as valid with a check of IsBalanced. Not to mention it would allow for full validation.
Are you intending to work on this P2P message? Is this part of any specification?
(https://github.com/bitcoin/bitcoin/pull/34004#issuecomment-3667635232)
> Regarding the pruned-only feature. This would not be the case if there was a P2P message for undo-data. It would allow undo-data to be written in an un-trusted state and later confirmed as valid with a check of IsBalanced. Not to mention it would allow for full validation.
Are you intending to work on this P2P message? Is this part of any specification?
💬 brunoerg commented on pull request "Implementation of SwiftSync":
(https://github.com/bitcoin/bitcoin/pull/34004#discussion_r2628960699)
86d009d0009a53bbb42923b1c1c062f11499d481: You could also assert that the block count is the same as `result["height"]`.
(https://github.com/bitcoin/bitcoin/pull/34004#discussion_r2628960699)
86d009d0009a53bbb42923b1c1c062f11499d481: You could also assert that the block count is the same as `result["height"]`.
💬 brunoerg commented on pull request "Implementation of SwiftSync":
(https://github.com/bitcoin/bitcoin/pull/34004#discussion_r2628927165)
77b2518e9903d0b1fffe1ca7cfd5f7e191af0b5a: nit: We could avoid this duplicated loop (we basically have the same thing for each unspent block) by creating an "util" function that does it.
(https://github.com/bitcoin/bitcoin/pull/34004#discussion_r2628927165)
77b2518e9903d0b1fffe1ca7cfd5f7e191af0b5a: nit: We could avoid this duplicated loop (we basically have the same thing for each unspent block) by creating an "util" function that does it.