π¬ 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.
π¬ brunoerg commented on pull request "Implementation of SwiftSync":
(https://github.com/bitcoin/bitcoin/pull/34004#discussion_r2628968775)
d019f9abe70ba9b488baac3454e9c853b04fe885: We could point here that it cannot be used without pruned mode.
(https://github.com/bitcoin/bitcoin/pull/34004#discussion_r2628968775)
d019f9abe70ba9b488baac3454e9c853b04fe885: We could point here that it cannot be used without pruned mode.
π¬ 2knwhzwkm6-max commented on pull request "rpc, doc: clarify the response of listtransactions RPC":
(https://github.com/bitcoin/bitcoin/pull/32737#discussion_r2629157315)
> Could use "transaction" instead of "blockchain transaction".
I agree with you
> Also, I think the example could be improved,
Iβd avoid βone per output / including the change outputβ here, since it can be misleading. listtransactions returns wallet entries, not necessarily one item per on-chain output.
In this example, the key point is that a payment to a wallet-owned address can show up as both a send entry (the wallet created that payment) and a receive entry (the wallet owns the
...
(https://github.com/bitcoin/bitcoin/pull/32737#discussion_r2629157315)
> Could use "transaction" instead of "blockchain transaction".
I agree with you
> Also, I think the example could be improved,
Iβd avoid βone per output / including the change outputβ here, since it can be misleading. listtransactions returns wallet entries, not necessarily one item per on-chain output.
In this example, the key point is that a payment to a wallet-owned address can show up as both a send entry (the wallet created that payment) and a receive entry (the wallet owns the
...
π¬ ajtowns commented on pull request "Add initial vectorized chacha20 implementation for 2-3x speedup":
(https://github.com/bitcoin/bitcoin/pull/34083#discussion_r2629157441)
Sorry, I meant just so you get an error if you're using a compiler ignores the `__attribute__` entirely but still passes the `#if` in _base.cpp.
(https://github.com/bitcoin/bitcoin/pull/34083#discussion_r2629157441)
Sorry, I meant just so you get an error if you're using a compiler ignores the `__attribute__` entirely but still passes the `#if` in _base.cpp.
π¬ ajtowns commented on pull request "Add initial vectorized chacha20 implementation for 2-3x speedup":
(https://github.com/bitcoin/bitcoin/pull/34083#discussion_r2629159079)
Yeah, I was thinking about whether the code should just do the non-vectorized stuff to get past the overflow then immediately go back to vectorizing, rather than waiting for the next call. I think what you've got makes sense.
(https://github.com/bitcoin/bitcoin/pull/34083#discussion_r2629159079)
Yeah, I was thinking about whether the code should just do the non-vectorized stuff to get past the overflow then immediately go back to vectorizing, rather than waiting for the next call. I think what you've got makes sense.