💬 Crypt-iQ commented on pull request "log: Use `__func__` for -logsourcelocations":
(https://github.com/bitcoin/bitcoin/pull/34088#issuecomment-3665671949)
Concept ACK. The debug logs are harder to read now.
(https://github.com/bitcoin/bitcoin/pull/34088#issuecomment-3665671949)
Concept ACK. The debug logs are harder to read now.
🤔 sipa reviewed a pull request: "contrib: asmap-tool.py - Don't write binary to TTY"
(https://github.com/bitcoin/bitcoin/pull/34089#pullrequestreview-3588088416)
ACK e7e51952dc24531932b6c06e4599be3a3d6bede8
(https://github.com/bitcoin/bitcoin/pull/34089#pullrequestreview-3588088416)
ACK e7e51952dc24531932b6c06e4599be3a3d6bede8
📝 hebasto opened a pull request: "net: Fix `-Wmissing-braces`"
(https://github.com/bitcoin/bitcoin/pull/34090)
On some non-POSIX platforms, Clang emits `-Wmissing-braces` warnings for the `IN6ADDR_ANY_INIT` and `IN6ADDR_LOOPBACK_INIT` macros. For example, on OpenIndiana / illumos:
```
$ uname -srv
SunOS 5.11 illumos-325e0fc8bb
$ clang --version
clang version 21.1.7 (https://github.com/OpenIndiana/oi-userland.git 36a81bf5e5d307d4e85893422600678d46328010)
Target: x86_64-pc-solaris2.11
Thread model: posix
InstalledDir: /usr/clang/21/bin
$ cmake -B build -DCMAKE_GENERATOR=Ninja -DCMAKE_C_COMPILER=cl
...
(https://github.com/bitcoin/bitcoin/pull/34090)
On some non-POSIX platforms, Clang emits `-Wmissing-braces` warnings for the `IN6ADDR_ANY_INIT` and `IN6ADDR_LOOPBACK_INIT` macros. For example, on OpenIndiana / illumos:
```
$ uname -srv
SunOS 5.11 illumos-325e0fc8bb
$ clang --version
clang version 21.1.7 (https://github.com/OpenIndiana/oi-userland.git 36a81bf5e5d307d4e85893422600678d46328010)
Target: x86_64-pc-solaris2.11
Thread model: posix
InstalledDir: /usr/clang/21/bin
$ cmake -B build -DCMAKE_GENERATOR=Ninja -DCMAKE_C_COMPILER=cl
...
💬 gmaxwell commented on pull request "fees: Introduce Mempool Based Fee Estimation to reduce overestimation":
(https://github.com/bitcoin/bitcoin/pull/34075#issuecomment-3665733085)
I think enabling it by default probably makes sense, that particular sub comment was more about having a ready way to choose not to use it when transacting.
As far as the initial defaults I think there I meant that that it shouldn't target too close to the bottom of the mempool as even if that currently gets high success that may not be true after wide deployment. Though on reflection I'm not sure that thats true as this only lowers feerates, so other people using this should only increase y
...
(https://github.com/bitcoin/bitcoin/pull/34075#issuecomment-3665733085)
I think enabling it by default probably makes sense, that particular sub comment was more about having a ready way to choose not to use it when transacting.
As far as the initial defaults I think there I meant that that it shouldn't target too close to the bottom of the mempool as even if that currently gets high success that may not be true after wide deployment. Though on reflection I'm not sure that thats true as this only lowers feerates, so other people using this should only increase y
...
🚀 fanquake merged a pull request: "A few followups after introducing `/rest/blockpart/` endpoint"
(https://github.com/bitcoin/bitcoin/pull/34074)
(https://github.com/bitcoin/bitcoin/pull/34074)
💬 fanquake commented on pull request "net: Fix `-Wmissing-braces`":
(https://github.com/bitcoin/bitcoin/pull/34090#issuecomment-3665787525)
https://github.com/bitcoin/bitcoin/actions/runs/20307057596/job/58327346295?pr=34090#step:11:495:
```bash
net_processing.cpp
netgroup.cpp
D:\a\bitcoin\bitcoin\src\net.cpp(3309,41): error C2440: '<function-style-cast>': cannot convert from 'initializer list' to 'in6_addr' [D:\a\bitcoin\bitcoin\build\src\bitcoin_node.vcxproj]
D:\a\bitcoin\bitcoin\src\net.cpp(3309,41):
'in6_addr::in6_addr': no overloaded function could convert all the argument types
C:\Program File
...
(https://github.com/bitcoin/bitcoin/pull/34090#issuecomment-3665787525)
https://github.com/bitcoin/bitcoin/actions/runs/20307057596/job/58327346295?pr=34090#step:11:495:
```bash
net_processing.cpp
netgroup.cpp
D:\a\bitcoin\bitcoin\src\net.cpp(3309,41): error C2440: '<function-style-cast>': cannot convert from 'initializer list' to 'in6_addr' [D:\a\bitcoin\bitcoin\build\src\bitcoin_node.vcxproj]
D:\a\bitcoin\bitcoin\src\net.cpp(3309,41):
'in6_addr::in6_addr': no overloaded function could convert all the argument types
C:\Program File
...
💬 fjahr commented on pull request "contrib: asmap-tool.py - Don't write binary to TTY":
(https://github.com/bitcoin/bitcoin/pull/34089#issuecomment-3665802808)
tACK e7e51952dc24531932b6c06e4599be3a3d6bede8
Thanks!
```
$ ./contrib/asmap/asmap-tool.py encode ../../python/kartograf/out/1765638546/final_result.txt
Not much use in writing binary to a TTY. Please specify an output file or pipe output to another process.
```
(https://github.com/bitcoin/bitcoin/pull/34089#issuecomment-3665802808)
tACK e7e51952dc24531932b6c06e4599be3a3d6bede8
Thanks!
```
$ ./contrib/asmap/asmap-tool.py encode ../../python/kartograf/out/1765638546/final_result.txt
Not much use in writing binary to a TTY. Please specify an output file or pipe output to another process.
```
💬 maflcko commented on pull request "log: Use `__func__` for -logsourcelocations":
(https://github.com/bitcoin/bitcoin/pull/34088#discussion_r2627467520)
Not sure if `__func__` is guaranteed to be consteval/constexpr?
(https://github.com/bitcoin/bitcoin/pull/34088#discussion_r2627467520)
Not sure if `__func__` is guaranteed to be consteval/constexpr?
💬 maflcko commented on pull request "log: Use `__func__` for -logsourcelocations":
(https://github.com/bitcoin/bitcoin/pull/34088#discussion_r2627471486)
Not sure about the implicit `operator const std::source_location&(){ return m_loc; };`. This seems like it could silently make some places use the wrong function name.
> function_name_short
Thx, done
(https://github.com/bitcoin/bitcoin/pull/34088#discussion_r2627471486)
Not sure about the implicit `operator const std::source_location&(){ return m_loc; };`. This seems like it could silently make some places use the wrong function name.
> function_name_short
Thx, done
🚀 fanquake merged a pull request: "ci: Pin native tests on cross-builds to same commit"
(https://github.com/bitcoin/bitcoin/pull/34080)
(https://github.com/bitcoin/bitcoin/pull/34080)
📝 brunoerg opened a pull request: "fuzz: doc: remove any mention to `address_deserialize_v2`"
(https://github.com/bitcoin/bitcoin/pull/34091)
We don't have `address_deserialize_v2` target anymore since fac81affb527132945773a5315bd27fec61ec52f (we used to have `address_deserialize_v1_notime`, `address_deserialize_v1_withtime` and `address_deserialize_v2` but now we only have a single `address_deserialize` target) so it removes any mention to it.
(https://github.com/bitcoin/bitcoin/pull/34091)
We don't have `address_deserialize_v2` target anymore since fac81affb527132945773a5315bd27fec61ec52f (we used to have `address_deserialize_v1_notime`, `address_deserialize_v1_withtime` and `address_deserialize_v2` but now we only have a single `address_deserialize` target) so it removes any mention to it.
💬 furszy commented on pull request "rpc, doc: clarify the response of listtransactions RPC":
(https://github.com/bitcoin/bitcoin/pull/32737#discussion_r2627466348)
Could use "transaction" instead of "blockchain transaction".
Also, I think the example could be improved, something like:
"For instance, a wallet transaction that sends funds to three addresses - one belonging to the wallet itself and two external— will produce four entries (one per output, including the change output).
As a result, the response of 'listransactions' will contain one entry in the 'receive' category and three entries in the 'send' category."
(feel free to add stuff to it
...
(https://github.com/bitcoin/bitcoin/pull/32737#discussion_r2627466348)
Could use "transaction" instead of "blockchain transaction".
Also, I think the example could be improved, something like:
"For instance, a wallet transaction that sends funds to three addresses - one belonging to the wallet itself and two external— will produce four entries (one per output, including the change output).
As a result, the response of 'listransactions' will contain one entry in the 'receive' category and three entries in the 'send' category."
(feel free to add stuff to it
...
🚀 fanquake merged a pull request: "contrib: asmap-tool.py - Don't write binary to TTY"
(https://github.com/bitcoin/bitcoin/pull/34089)
(https://github.com/bitcoin/bitcoin/pull/34089)
💬 brunoerg commented on pull request "fuzz: doc: remove any mention to `address_deserialize_v2`":
(https://github.com/bitcoin/bitcoin/pull/34091#issuecomment-3665817873)
cc @maflcko
(https://github.com/bitcoin/bitcoin/pull/34091#issuecomment-3665817873)
cc @maflcko
🤔 danielabrozzoni reviewed a pull request: "log: show reindex progress in `ImportBlocks`"
(https://github.com/bitcoin/bitcoin/pull/33353#pullrequestreview-3588239911)
tACK d7de5b109f69293b375729363b4e5038f3fb6b15 - code reviewed and tested on my archival node.
Nice to see this!
It’s true that not every file will take the same amount of time to reindex, but I think it’s better to have a rough estimate than no estimate at all.
(https://github.com/bitcoin/bitcoin/pull/33353#pullrequestreview-3588239911)
tACK d7de5b109f69293b375729363b4e5038f3fb6b15 - code reviewed and tested on my archival node.
Nice to see this!
It’s true that not every file will take the same amount of time to reindex, but I think it’s better to have a rough estimate than no estimate at all.
🤔 maflcko reviewed a pull request: "log: show reindex progress in `ImportBlocks`"
(https://github.com/bitcoin/bitcoin/pull/33353#pullrequestreview-3588272613)
review ACK d7de5b109f69293b375729363b4e5038f3fb6b15 💇
<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 d7de5b109f69293b37572
...
(https://github.com/bitcoin/bitcoin/pull/33353#pullrequestreview-3588272613)
review ACK d7de5b109f69293b375729363b4e5038f3fb6b15 💇
<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 d7de5b109f69293b37572
...
💬 furszy commented on issue "GUI crashes in regtest":
(https://github.com/bitcoin-core/gui/issues/918#issuecomment-3665898521)
Can you check the directory permissions please
(https://github.com/bitcoin-core/gui/issues/918#issuecomment-3665898521)
Can you check the directory permissions please
🤔 l0rinc requested changes to a pull request: "Add initial vectorized chacha20 implementation for 2-3x speedup"
(https://github.com/bitcoin/bitcoin/pull/34083#pullrequestreview-3586783659)
I went through the code roughly, I like that we're focusing on this and looking forward to specializing other parts of the code that are this critical (looking at you, SipHash!).
My biggest objection currently is that it's broken on big-endian systems - left a suggestion how to reproduce and fix it. This also reveals the lack of testing - we have to find a way to selectively enable and disable these optimizations to make sure we have tests that compare their outputs.
I agree with AJ that we
...
(https://github.com/bitcoin/bitcoin/pull/34083#pullrequestreview-3586783659)
I went through the code roughly, I like that we're focusing on this and looking forward to specializing other parts of the code that are this critical (looking at you, SipHash!).
My biggest objection currently is that it's broken on big-endian systems - left a suggestion how to reproduce and fix it. This also reveals the lack of testing - we have to find a way to selectively enable and disable these optimizations to make sure we have tests that compare their outputs.
I agree with AJ that we
...
💬 l0rinc commented on pull request "Add initial vectorized chacha20 implementation for 2-3x speedup":
(https://github.com/bitcoin/bitcoin/pull/34083#discussion_r2626253726)
can we assume that all big endian systems have `__builtin_bswap32` available.
https://github.com/bitcoin/bitcoin/blob/432b18ca8d0654318a8d882b28b20af2cb2d2e5d/src/compat/byteswap.h#L14-L16 indicates we could use our existing helpers here instead:
```C++
ALWAYS_INLINE void vec_byteswap(vec256& vec)
{
if constexpr (std::endian::native == std::endian::big) {
for (size_t i = 0; i < 8; ++i) {
vec[i] = internal_bswap_32(vec[i]);
}
}
}
```
A small, fixed-
...
(https://github.com/bitcoin/bitcoin/pull/34083#discussion_r2626253726)
can we assume that all big endian systems have `__builtin_bswap32` available.
https://github.com/bitcoin/bitcoin/blob/432b18ca8d0654318a8d882b28b20af2cb2d2e5d/src/compat/byteswap.h#L14-L16 indicates we could use our existing helpers here instead:
```C++
ALWAYS_INLINE void vec_byteswap(vec256& vec)
{
if constexpr (std::endian::native == std::endian::big) {
for (size_t i = 0; i < 8; ++i) {
vec[i] = internal_bswap_32(vec[i]);
}
}
}
```
A small, fixed-
...
💬 l0rinc commented on pull request "Add initial vectorized chacha20 implementation for 2-3x speedup":
(https://github.com/bitcoin/bitcoin/pull/34083#discussion_r2627477691)
Hmmm, it seems to me we're doing heavy recursive template instantiationshere to unroll loops.
My understanding (and experience with the mentioned Obfuscation PR) is that modern compilers are good at unrolling fixed-bound loops.
Can you please try if this also works and results in the same speedup?
```suggestion
template <size_t BITS, size_t I>
ALWAYS_INLINE void arr_add_xor_rot(std::array<vec256, I>& arr0, const std::array<vec256, I>& arr1, std::array<vec256, I>& arr2)
{
for (size_t
...
(https://github.com/bitcoin/bitcoin/pull/34083#discussion_r2627477691)
Hmmm, it seems to me we're doing heavy recursive template instantiationshere to unroll loops.
My understanding (and experience with the mentioned Obfuscation PR) is that modern compilers are good at unrolling fixed-bound loops.
Can you please try if this also works and results in the same speedup?
```suggestion
template <size_t BITS, size_t I>
ALWAYS_INLINE void arr_add_xor_rot(std::array<vec256, I>& arr0, const std::array<vec256, I>& arr1, std::array<vec256, I>& arr2)
{
for (size_t
...
💬 l0rinc commented on pull request "Add initial vectorized chacha20 implementation for 2-3x speedup":
(https://github.com/bitcoin/bitcoin/pull/34083#discussion_r2627310095)
Forcing vectorized operations on an emulated big-endian system (otherwise it falls back to the scalar implementation due to the safety checks):
```patch
diff --git a/src/crypto/chacha20_vec_base.cpp b/src/crypto/chacha20_vec_base.cpp
index 9fda9452a1..a2aa1e5552 100644
--- a/src/crypto/chacha20_vec_base.cpp
+++ b/src/crypto/chacha20_vec_base.cpp
@@ -20,7 +20,7 @@
# define CHACHA20_VEC_DISABLE_STATES_8
# define CHACHA20_VEC_DISABLE_STATES_6
# define CHACHA20_VEC_DISABLE_STATES_4
...
(https://github.com/bitcoin/bitcoin/pull/34083#discussion_r2627310095)
Forcing vectorized operations on an emulated big-endian system (otherwise it falls back to the scalar implementation due to the safety checks):
```patch
diff --git a/src/crypto/chacha20_vec_base.cpp b/src/crypto/chacha20_vec_base.cpp
index 9fda9452a1..a2aa1e5552 100644
--- a/src/crypto/chacha20_vec_base.cpp
+++ b/src/crypto/chacha20_vec_base.cpp
@@ -20,7 +20,7 @@
# define CHACHA20_VEC_DISABLE_STATES_8
# define CHACHA20_VEC_DISABLE_STATES_6
# define CHACHA20_VEC_DISABLE_STATES_4
...