💬 romanz commented on pull request "A few followups after introducing `/rest/blockpart/` endpoint":
(https://github.com/bitcoin/bitcoin/pull/34074#discussion_r2624937872)
Thanks! Fixed https://github.com/bitcoin/bitcoin/pull/34074/commits/e9065275c5655ed1b20efc5b1f25d0d6d4fa473e and added https://github.com/bitcoin/bitcoin/pull/34074/commits/584f0cbf7932b4e5cf629c90fc7453d94a890abe.
(https://github.com/bitcoin/bitcoin/pull/34074#discussion_r2624937872)
Thanks! Fixed https://github.com/bitcoin/bitcoin/pull/34074/commits/e9065275c5655ed1b20efc5b1f25d0d6d4fa473e and added https://github.com/bitcoin/bitcoin/pull/34074/commits/584f0cbf7932b4e5cf629c90fc7453d94a890abe.
💬 maflcko commented on pull request "A few followups after introducing `/rest/blockpart/` endpoint":
(https://github.com/bitcoin/bitcoin/pull/34074#issuecomment-3662702660)
review ACK 584f0cbf7932b4e5cf629c90fc7453d94a890abe 🐟
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 584f0cbf7932
...
(https://github.com/bitcoin/bitcoin/pull/34074#issuecomment-3662702660)
review ACK 584f0cbf7932b4e5cf629c90fc7453d94a890abe 🐟
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 584f0cbf7932
...
💬 boxtob commented on pull request "cmake: Add fail pattern to `try_append_cxx_flags`":
(https://github.com/bitcoin/bitcoin/pull/34047#discussion_r2624973413)
Technically it does negate the switch but the result/outcome is the same as already mentioned by @fanquake and @maflcko.
Expected behavior:
```
$ env CXX=clang++ CC=clang CXXFLAGS="-Wno-error=unknown-warning-option" cmake -B build 2>&1 | grep WREALLY_UNKNOWN_WARNING
-- Performing Test CXX_SUPPORTS__WREALLY_UNKNOWN_WARNING
-- Performing Test CXX_SUPPORTS__WREALLY_UNKNOWN_WARNING - Success
```
Non expected behavior:
```
$ env CXX=clang++ CC=clang CXXFLAGS="-Wno-error=unknown-warning-o
...
(https://github.com/bitcoin/bitcoin/pull/34047#discussion_r2624973413)
Technically it does negate the switch but the result/outcome is the same as already mentioned by @fanquake and @maflcko.
Expected behavior:
```
$ env CXX=clang++ CC=clang CXXFLAGS="-Wno-error=unknown-warning-option" cmake -B build 2>&1 | grep WREALLY_UNKNOWN_WARNING
-- Performing Test CXX_SUPPORTS__WREALLY_UNKNOWN_WARNING
-- Performing Test CXX_SUPPORTS__WREALLY_UNKNOWN_WARNING - Success
```
Non expected behavior:
```
$ env CXX=clang++ CC=clang CXXFLAGS="-Wno-error=unknown-warning-o
...
💬 davidgumberg commented on pull request "test: sock: Enable socket pair tests on Windows":
(https://github.com/bitcoin/bitcoin/pull/33506#issuecomment-3662730391)
> > My gut reaction is that it would not be worth the extra code to do this, but I don't feel very strongly and am happy to add this if you think it's better.
>
> Not 100% sure. As long as this is only used for the tests, like now, i don't mind it being like this. But if it were to be used for anything in the actual code, i think a socket pair should be a socket pair. Using a fallback work-around for functionality that's plainly supported seems strange.
100% agree that if we were using `so
...
(https://github.com/bitcoin/bitcoin/pull/33506#issuecomment-3662730391)
> > My gut reaction is that it would not be worth the extra code to do this, but I don't feel very strongly and am happy to add this if you think it's better.
>
> Not 100% sure. As long as this is only used for the tests, like now, i don't mind it being like this. But if it were to be used for anything in the actual code, i think a socket pair should be a socket pair. Using a fallback work-around for functionality that's plainly supported seems strange.
100% agree that if we were using `so
...
💬 l0rinc commented on pull request "A few followups after introducing `/rest/blockpart/` endpoint":
(https://github.com/bitcoin/bitcoin/pull/34074#discussion_r2625015165)
I think this was supposed to be a `BIN` to explain it's not a json failure
```suggestion
res = self.test_rest_request(f"/blockpart/{blockhash}", status=400, req_type=ReqType.BIN, ret_type=RetType.OBJ)
```
(https://github.com/bitcoin/bitcoin/pull/34074#discussion_r2625015165)
I think this was supposed to be a `BIN` to explain it's not a json failure
```suggestion
res = self.test_rest_request(f"/blockpart/{blockhash}", status=400, req_type=ReqType.BIN, ret_type=RetType.OBJ)
```
💬 romanz commented on pull request "A few followups after introducing `/rest/blockpart/` endpoint":
(https://github.com/bitcoin/bitcoin/pull/34074#discussion_r2625055321)
Updated 7fe94a0493
(https://github.com/bitcoin/bitcoin/pull/34074#discussion_r2625055321)
Updated 7fe94a0493
💬 l0rinc commented on pull request "A few followups after introducing `/rest/blockpart/` endpoint":
(https://github.com/bitcoin/bitcoin/pull/34074#issuecomment-3662873909)
ACK 59b93f11e8600d5224359f4d05619c0f56aef1e6
[Since last ack](https://github.com/bitcoin/bitcoin/compare/823d4d8a5b71b7c740a6552f72fbfdb9b2813ca6..59b93f11e8600d5224359f4d05619c0f56aef1e6) the reason was added for status failure and non-json test was added for missing offset
(https://github.com/bitcoin/bitcoin/pull/34074#issuecomment-3662873909)
ACK 59b93f11e8600d5224359f4d05619c0f56aef1e6
[Since last ack](https://github.com/bitcoin/bitcoin/compare/823d4d8a5b71b7c740a6552f72fbfdb9b2813ca6..59b93f11e8600d5224359f4d05619c0f56aef1e6) the reason was added for status failure and non-json test was added for missing offset
💬 achow101 commented on issue "sqlite legacy descriptor wallet migration fails":
(https://github.com/bitcoin/bitcoin/issues/33468#issuecomment-3663281115)
> it doesn't let me :(
Open the dump file, change the `bdb` at the top to `sqlite` and do `createfromdump`. You'll get a checksum error that gives you the correct checksum. Copy that checksum, open the dump file again, go to the bottom and replace the checksum. `createfromdump` should now work.
(https://github.com/bitcoin/bitcoin/issues/33468#issuecomment-3663281115)
> it doesn't let me :(
Open the dump file, change the `bdb` at the top to `sqlite` and do `createfromdump`. You'll get a checksum error that gives you the correct checksum. Copy that checksum, open the dump file again, go to the bottom and replace the checksum. `createfromdump` should now work.
💬 2knwhzwkm6-max commented on pull request "contrib: Add zsh completion scripts":
(https://github.com/bitcoin/bitcoin/pull/33402#issuecomment-3663286905)
Tested PR #33402 zsh completions on macOS 26.1 (arm64), zsh 5.9 (arm64-apple-darwin25.0).
Bitcoin Core tools: bitcoin-cli v30.99.0-0f1f02995b71
PR commit: 5f2c0cdf07
Summary:
- Completion functions are discoverable via fpath/compinit (contrib/completions/zsh/).
- Command->completion bindings are set:
- bitcoin-cli -> _bitcoin-cli
- bitcoind -> _bitcoind
- bitcoin-tx -> _bitcoin-tx
- TAB key is bound to completion: "^I" expand-or-complete
(https://github.com/bitcoin/bitcoin/pull/33402#issuecomment-3663286905)
Tested PR #33402 zsh completions on macOS 26.1 (arm64), zsh 5.9 (arm64-apple-darwin25.0).
Bitcoin Core tools: bitcoin-cli v30.99.0-0f1f02995b71
PR commit: 5f2c0cdf07
Summary:
- Completion functions are discoverable via fpath/compinit (contrib/completions/zsh/).
- Command->completion bindings are set:
- bitcoin-cli -> _bitcoin-cli
- bitcoind -> _bitcoind
- bitcoin-tx -> _bitcoin-tx
- TAB key is bound to completion: "^I" expand-or-complete
🤔 ajtowns reviewed a pull request: "Add initial vectorized chacha20 implementation for 2-3x speedup"
(https://github.com/bitcoin/bitcoin/pull/34083#pullrequestreview-3585574031)
Some nits. Approach looks very nice, and at least going by the bench results, gives a good improvement.
(https://github.com/bitcoin/bitcoin/pull/34083#pullrequestreview-3585574031)
Some nits. Approach looks very nice, and at least going by the bench results, gives a good improvement.
💬 ajtowns commented on pull request "Add initial vectorized chacha20 implementation for 2-3x speedup":
(https://github.com/bitcoin/bitcoin/pull/34083#discussion_r2625334750)
`static_assert(sizeof(vec256) == 32)` ? (Or `size(vec256) == 32 || ALL_MULTI_STATES_DISABLED`)
(https://github.com/bitcoin/bitcoin/pull/34083#discussion_r2625334750)
`static_assert(sizeof(vec256) == 32)` ? (Or `size(vec256) == 32 || ALL_MULTI_STATES_DISABLED`)
💬 ajtowns commented on pull request "Add initial vectorized chacha20 implementation for 2-3x speedup":
(https://github.com/bitcoin/bitcoin/pull/34083#discussion_r2625248846)
Why separate this into an .ipp file if it's only included in a single .cpp file?
(https://github.com/bitcoin/bitcoin/pull/34083#discussion_r2625248846)
Why separate this into an .ipp file if it's only included in a single .cpp file?
💬 ajtowns commented on pull request "Add initial vectorized chacha20 implementation for 2-3x speedup":
(https://github.com/bitcoin/bitcoin/pull/34083#discussion_r2625293528)
Using `#if !defined` for these seems old school. Why not static constexpr bools, and `if constexpr (..)`?
Writing:
```c++
#if defined(__x86_64__) || defined(__amd64__)
# define CHACHA20_VEC_DISABLE_STATES_16 true
# define CHACHA20_VEC_DISABLE_STATES_8 true
# define CHACHA20_VEC_DISABLE_STATES_6 true
# define CHACHA20_VEC_DISABLE_STATES_4 false
# define CHACHA20_VEC_DISABLE_STATES_2 false
#elif defined(__ARM_NEON)
...
static constexpr bool CHACHA20_VEC_ALL_MULTI_STATES_D
...
(https://github.com/bitcoin/bitcoin/pull/34083#discussion_r2625293528)
Using `#if !defined` for these seems old school. Why not static constexpr bools, and `if constexpr (..)`?
Writing:
```c++
#if defined(__x86_64__) || defined(__amd64__)
# define CHACHA20_VEC_DISABLE_STATES_16 true
# define CHACHA20_VEC_DISABLE_STATES_8 true
# define CHACHA20_VEC_DISABLE_STATES_6 true
# define CHACHA20_VEC_DISABLE_STATES_4 false
# define CHACHA20_VEC_DISABLE_STATES_2 false
#elif defined(__ARM_NEON)
...
static constexpr bool CHACHA20_VEC_ALL_MULTI_STATES_D
...
💬 ajtowns commented on pull request "Add initial vectorized chacha20 implementation for 2-3x speedup":
(https://github.com/bitcoin/bitcoin/pull/34083#discussion_r2625335880)
`if constexpr (ITER + 1 < I)` (we have a space after `if constexpr` elsewhere)
(https://github.com/bitcoin/bitcoin/pull/34083#discussion_r2625335880)
`if constexpr (ITER + 1 < I)` (we have a space after `if constexpr` elsewhere)
⚠️ BenWestgate opened an issue: "doc: Use multi-path descriptors in descriptors.md and linked tests."
(https://github.com/bitcoin/bitcoin/issues/34086)
### Motivation
These files should use multipath descriptors as per https://github.com/bitcoin/bitcoin/commit/2a46e94a1600a4f28e01db23a89f039acaa2c45e and https://github.com/bitcoin/bitcoin/pull/33286:
**Motivation:**
A single multipath descriptor is the most convenient pattern for multisig; our documentation should use it.
**Locations:**
- [ ] [doc/descriptors.md](https://github.com/bitcoin/bitcoin/tree/master/doc/descriptors.md):
- [ ] "Basic Multisig Example"
- [ ] "Examples" with this
...
(https://github.com/bitcoin/bitcoin/issues/34086)
### Motivation
These files should use multipath descriptors as per https://github.com/bitcoin/bitcoin/commit/2a46e94a1600a4f28e01db23a89f039acaa2c45e and https://github.com/bitcoin/bitcoin/pull/33286:
**Motivation:**
A single multipath descriptor is the most convenient pattern for multisig; our documentation should use it.
**Locations:**
- [ ] [doc/descriptors.md](https://github.com/bitcoin/bitcoin/tree/master/doc/descriptors.md):
- [ ] "Basic Multisig Example"
- [ ] "Examples" with this
...
💬 ajtowns commented on pull request "Add initial vectorized chacha20 implementation for 2-3x speedup":
(https://github.com/bitcoin/bitcoin/pull/34083#discussion_r2625986598)
Here's a branch that replaces the #defines and abstracts the recursive template stuff a bit more for your perusal https://github.com/ajtowns/bitcoin/commits/202512-pr34083-templates/
(https://github.com/bitcoin/bitcoin/pull/34083#discussion_r2625986598)
Here's a branch that replaces the #defines and abstracts the recursive template stuff a bit more for your perusal https://github.com/ajtowns/bitcoin/commits/202512-pr34083-templates/
💬 l0rinc commented on pull request "Add initial vectorized chacha20 implementation for 2-3x speedup":
(https://github.com/bitcoin/bitcoin/pull/34083#issuecomment-3664158140)
Looking forward to reviewing it - quick question before I do: was it tested on big-endian systems?
(https://github.com/bitcoin/bitcoin/pull/34083#issuecomment-3664158140)
Looking forward to reviewing it - quick question before I do: was it tested on big-endian systems?
💬 ajtowns commented on pull request "Add initial vectorized chacha20 implementation for 2-3x speedup":
(https://github.com/bitcoin/bitcoin/pull/34083#discussion_r2626127018)
My understanding is that you're inverting the logic here: instead of
```c++
#define QUARTERROUND(a,b,c,d) \
X(a,b,d,16) X(c,d,b,12) X(a,d,b,8) X(c,d,b,7)
```
This does the `X(a,b,d,16)` all four times via simd, then `X(c,d,b,12)` all four times, etc. And the simd part relies on the data for those four steps being exactly +4 units apart, which is why the shuffling and unshuffling is necessary for the second round. (Okay, not four times but eight times because each vec256 is two block
...
(https://github.com/bitcoin/bitcoin/pull/34083#discussion_r2626127018)
My understanding is that you're inverting the logic here: instead of
```c++
#define QUARTERROUND(a,b,c,d) \
X(a,b,d,16) X(c,d,b,12) X(a,d,b,8) X(c,d,b,7)
```
This does the `X(a,b,d,16)` all four times via simd, then `X(c,d,b,12)` all four times, etc. And the simd part relies on the data for those four steps being exactly +4 units apart, which is why the shuffling and unshuffling is necessary for the second round. (Okay, not four times but eight times because each vec256 is two block
...
💬 ajtowns commented on pull request "Add initial vectorized chacha20 implementation for 2-3x speedup":
(https://github.com/bitcoin/bitcoin/pull/34083#discussion_r2626160716)
Overflow happens every 274GB I guess, which presumably isn't worth putting much effort in to optimising around.
(https://github.com/bitcoin/bitcoin/pull/34083#discussion_r2626160716)
Overflow happens every 274GB I guess, which presumably isn't worth putting much effort in to optimising around.
💬 laanwj commented on pull request "test: sock: Enable socket pair tests on Windows":
(https://github.com/bitcoin/bitcoin/pull/33506#issuecomment-3664344101)
> Even though it's probably a bad idea to make changes in the present to suit a future use case that might never come about, I think it is generally useful to be able to create and connect the sockets separately in sock_tests.cpp and to be able to create TCP sockets like the application does.
f it's intentionally TCP then calling it `tcp_socket_pair` might be even better. So that no one decides to make this improvement in the meantime.
(https://github.com/bitcoin/bitcoin/pull/33506#issuecomment-3664344101)
> Even though it's probably a bad idea to make changes in the present to suit a future use case that might never come about, I think it is generally useful to be able to create and connect the sockets separately in sock_tests.cpp and to be able to create TCP sockets like the application does.
f it's intentionally TCP then calling it `tcp_socket_pair` might be even better. So that no one decides to make this improvement in the meantime.