🚀 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.
💬 ajtowns commented on pull request "Add initial vectorized chacha20 implementation for 2-3x speedup":
(https://github.com/bitcoin/bitcoin/pull/34083#issuecomment-3667913915)
> So.. I'd _really_ prefer to keep things as simple here as possible. I know the recursive template functions aren't exactly pretty, but I don't think they're _THAT_ bad? Plus, with c++26, it all goes away in favor of `template for` :)
Could we get some comments in the code as to compiler versions (and architecture) that failed to be efficient enough with lambdas, to hopefully avoid future people modernizing the code without checking that these problems have gone away? Presumably will be a lo
...
(https://github.com/bitcoin/bitcoin/pull/34083#issuecomment-3667913915)
> So.. I'd _really_ prefer to keep things as simple here as possible. I know the recursive template functions aren't exactly pretty, but I don't think they're _THAT_ bad? Plus, with c++26, it all goes away in favor of `template for` :)
Could we get some comments in the code as to compiler versions (and architecture) that failed to be efficient enough with lambdas, to hopefully avoid future people modernizing the code without checking that these problems have gone away? Presumably will be a lo
...
💬 ajtowns commented on pull request "Add initial vectorized chacha20 implementation for 2-3x speedup":
(https://github.com/bitcoin/bitcoin/pull/34083#discussion_r2629220635)
Writing this as:
```c++
template<size_t... ITER> using iseq = std::index_sequence<ITER>;
static constexpr ISEQ = std::make_index_sequence<I>();
template<size_t... ITER>
ALWAYS_INLINE void arr_set_vec256(iseq<ITER>, std::array<vec256, I>& arr, const vec256& vec)
{
((std::get<ITER>(arr) = vec), ...);
}
...
arr_set_vec256(ISEQ, arr0, num256);
```
doesn't seem too bad, and avoids lambdas? Possibly still a bit clumsy if you can't capt
...
(https://github.com/bitcoin/bitcoin/pull/34083#discussion_r2629220635)
Writing this as:
```c++
template<size_t... ITER> using iseq = std::index_sequence<ITER>;
static constexpr ISEQ = std::make_index_sequence<I>();
template<size_t... ITER>
ALWAYS_INLINE void arr_set_vec256(iseq<ITER>, std::array<vec256, I>& arr, const vec256& vec)
{
((std::get<ITER>(arr) = vec), ...);
}
...
arr_set_vec256(ISEQ, arr0, num256);
```
doesn't seem too bad, and avoids lambdas? Possibly still a bit clumsy if you can't capt
...
🤔 ajtowns reviewed a pull request: "policy: Remove individual transaction <minrelay restriction"
(https://github.com/bitcoin/bitcoin/pull/33892#pullrequestreview-3590460575)
Subject is a bit confusing; this leaves the checks for ***individual transactions*** considered on their own, but removes the check for transactions ***within a package***, where the transaction's feerate is below minfee, even if its got a CPFP relationship that bumps the fee. We still check the overall package fee, and we require the package to be a child plus direct parents via `IsChildWithParents`, so shouldn't end up with us adding garbage to the mempool that's immediately discarded, and the
...
(https://github.com/bitcoin/bitcoin/pull/33892#pullrequestreview-3590460575)
Subject is a bit confusing; this leaves the checks for ***individual transactions*** considered on their own, but removes the check for transactions ***within a package***, where the transaction's feerate is below minfee, even if its got a CPFP relationship that bumps the fee. We still check the overall package fee, and we require the package to be a child plus direct parents via `IsChildWithParents`, so shouldn't end up with us adding garbage to the mempool that's immediately discarded, and the
...
💬 ajtowns commented on pull request "policy: Remove individual transaction <minrelay restriction":
(https://github.com/bitcoin/bitcoin/pull/33892#discussion_r2629291543)
I think package relay is still restricted to 1P1C, so this only means that a single child can CPFP a below-minfee parent even if the child is non-TRUC. Might be good to be clearer about that.
I think the submitpackage RPC is slightly more general, in that it will accept n+1 transactions made up of a single child and n parents, and will now support parents below minfee. However, these will not relay if there is more than one parent that is below minfee. This might be something of a foot-gun.
(https://github.com/bitcoin/bitcoin/pull/33892#discussion_r2629291543)
I think package relay is still restricted to 1P1C, so this only means that a single child can CPFP a below-minfee parent even if the child is non-TRUC. Might be good to be clearer about that.
I think the submitpackage RPC is slightly more general, in that it will accept n+1 transactions made up of a single child and n parents, and will now support parents below minfee. However, these will not relay if there is more than one parent that is below minfee. This might be something of a foot-gun.