💬 l0rinc commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2487015862)
how so, what's the failure you're seeing? It was working for me locally
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2487015862)
how so, what's the failure you're seeing? It was working for me locally
💬 enirox001 commented on pull request "test: Refactor rpc_getblockstats.py to use MiniWallet":
(https://github.com/bitcoin/bitcoin/pull/33184#issuecomment-3481276496)
Changes made in https://github.com/bitcoin/bitcoin/commit/b89afc6e4d6e71df4d99619480d705e1d0708ba7
- Changed the title and description of the PR to better depict the purpose of PR
- Simplified the transaction data structure by replacing tuples with a list of amounts
- Fixed a redundant assertion to properly validate key consistency across stats
(https://github.com/bitcoin/bitcoin/pull/33184#issuecomment-3481276496)
Changes made in https://github.com/bitcoin/bitcoin/commit/b89afc6e4d6e71df4d99619480d705e1d0708ba7
- Changed the title and description of the PR to better depict the purpose of PR
- Simplified the transaction data structure by replacing tuples with a list of amounts
- Fixed a redundant assertion to properly validate key consistency across stats
💬 l0rinc commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2487024773)
median is less sensitive to outliers - and nlogn is fine in tests
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2487024773)
median is less sensitive to outliers - and nlogn is fine in tests
💬 purpleKarrot commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#issuecomment-3481365862)
> Consensus code should arrive at the same conclusion, regardless of the architecture it runs on.
True.
> Using architecture-specific types such as size_t can lead to issues.
Can it?
`sizeof(size_t)` may have different values on different architectures, so depending on `sizeof(size_t)` is problematic.
But if sizes are serialized as `uint32_t`, I assume only the value range of `uint32_t` is valid, even on 64bit machines. In what scenario can `size_t` lead to a problem?
(https://github.com/bitcoin/bitcoin/pull/33724#issuecomment-3481365862)
> Consensus code should arrive at the same conclusion, regardless of the architecture it runs on.
True.
> Using architecture-specific types such as size_t can lead to issues.
Can it?
`sizeof(size_t)` may have different values on different architectures, so depending on `sizeof(size_t)` is problematic.
But if sizes are serialized as `uint32_t`, I assume only the value range of `uint32_t` is valid, even on 64bit machines. In what scenario can `size_t` lead to a problem?
🤔 hebasto reviewed a pull request: "Add a menu action to restore then migrate a legacy wallet"
(https://github.com/bitcoin-core/gui/pull/877#pullrequestreview-3411832114)
Tested on Ubuntu 25.10 using a legacy wallet created in Bitcoin Core 25.2.
Please resolve a silent merge conflict with https://github.com/bitcoin/bitcoin/pull/33550.
(https://github.com/bitcoin-core/gui/pull/877#pullrequestreview-3411832114)
Tested on Ubuntu 25.10 using a legacy wallet created in Bitcoin Core 25.2.
Please resolve a silent merge conflict with https://github.com/bitcoin/bitcoin/pull/33550.
👍 stickies-v approved a pull request: "refactor: C++20 operators"
(https://github.com/bitcoin/bitcoin/pull/33771#pullrequestreview-3411834976)
utACK 48840bfc2d7beeac0ddf56a3c26b243156ec8936. Pretty straightforward cleanup taking advantage of C++20 improvements, nice.
(https://github.com/bitcoin/bitcoin/pull/33771#pullrequestreview-3411834976)
utACK 48840bfc2d7beeac0ddf56a3c26b243156ec8936. Pretty straightforward cleanup taking advantage of C++20 improvements, nice.
💬 stickies-v commented on pull request "refactor: C++20 operators":
(https://github.com/bitcoin/bitcoin/pull/33771#discussion_r2487080201)
nit (here and for all other new `operator<=>`): perhaps better to be explicit about the `std::strong_ordering` return type? And perhaps making it `constexpr` while touching?
<details>
<summary>git diff on 48840bfc2d</summary>
```diff
diff --git a/src/prevector.h b/src/prevector.h
index d4d90c7350..595be4a603 100644
--- a/src/prevector.h
+++ b/src/prevector.h
@@ -7,6 +7,7 @@
#include <algorithm>
#include <cassert>
+#include <compare>
#include <cstddef>
#include <cstdint>
...
(https://github.com/bitcoin/bitcoin/pull/33771#discussion_r2487080201)
nit (here and for all other new `operator<=>`): perhaps better to be explicit about the `std::strong_ordering` return type? And perhaps making it `constexpr` while touching?
<details>
<summary>git diff on 48840bfc2d</summary>
```diff
diff --git a/src/prevector.h b/src/prevector.h
index d4d90c7350..595be4a603 100644
--- a/src/prevector.h
+++ b/src/prevector.h
@@ -7,6 +7,7 @@
#include <algorithm>
#include <cassert>
+#include <compare>
#include <cstddef>
#include <cstdint>
...
👍 ryanofsky approved a pull request: "precalculate SipHash constant salt XORs"
(https://github.com/bitcoin/bitcoin/pull/30442#pullrequestreview-3411441889)
Code review ACK 89bbbbd257063118e6968c409e52632835b76ce8. Make sense to replace the optimized `SipHashUint256` and `SipHashUint256Extra` functions with something more generic and avoid the need for them to repeat the same preprocessing when called multiple times. I left various suggestions below but they are not important. This already seems like a clear improvement in its current form.
(https://github.com/bitcoin/bitcoin/pull/30442#pullrequestreview-3411441889)
Code review ACK 89bbbbd257063118e6968c409e52632835b76ce8. Make sense to replace the optimized `SipHashUint256` and `SipHashUint256Extra` functions with something more generic and avoid the need for them to repeat the same preprocessing when called multiple times. I left various suggestions below but they are not important. This already seems like a clear improvement in its current form.
💬 ryanofsky commented on pull request "precalculate SipHash constant salt XORs":
(https://github.com/bitcoin/bitcoin/pull/30442#discussion_r2486788376)
In commit "optimization: Introduce PresaltedSipHasher for repeated hashing" (8568340be2c679713703edc3d418272ac78d1147)
This code would feel safer if `v` vector was declared const, to provide some guarantee that `v[0]` will actually equal `C0 ^ k0` (and so on) at the time this is called.
EDIT: Implemented this const suggestion in diff below
(https://github.com/bitcoin/bitcoin/pull/30442#discussion_r2486788376)
In commit "optimization: Introduce PresaltedSipHasher for repeated hashing" (8568340be2c679713703edc3d418272ac78d1147)
This code would feel safer if `v` vector was declared const, to provide some guarantee that `v[0]` will actually equal `C0 ^ k0` (and so on) at the time this is called.
EDIT: Implemented this const suggestion in diff below
💬 ryanofsky commented on pull request "precalculate SipHash constant salt XORs":
(https://github.com/bitcoin/bitcoin/pull/30442#discussion_r2486797297)
In commit "optimization: Introduce PresaltedSipHasher for repeated hashing" (8568340be2c679713703edc3d418272ac78d1147)
Can the description of this class from commit description be added here so this has some documentation?
Also it seems like distinction between the `CSipHasher` and `PresaltedSipHasher` classes is not really that one is presalted and the other isn't. They seem both seem presalted and have the same members and do the same work in the constructor.
Would seem more accurate
...
(https://github.com/bitcoin/bitcoin/pull/30442#discussion_r2486797297)
In commit "optimization: Introduce PresaltedSipHasher for repeated hashing" (8568340be2c679713703edc3d418272ac78d1147)
Can the description of this class from commit description be added here so this has some documentation?
Also it seems like distinction between the `CSipHasher` and `PresaltedSipHasher` classes is not really that one is presalted and the other isn't. They seem both seem presalted and have the same members and do the same work in the constructor.
Would seem more accurate
...
💬 ryanofsky commented on pull request "precalculate SipHash constant salt XORs":
(https://github.com/bitcoin/bitcoin/pull/30442#discussion_r2486893190)
In commit "optimization: Introduce PresaltedSipHasher for repeated hashing" (8568340be2c679713703edc3d418272ac78d1147)
Would be good to document that calling this is equivalent to calling `SipHasher(k0, k1).Write(val).Write(extra).Finalize()`
(https://github.com/bitcoin/bitcoin/pull/30442#discussion_r2486893190)
In commit "optimization: Introduce PresaltedSipHasher for repeated hashing" (8568340be2c679713703edc3d418272ac78d1147)
Would be good to document that calling this is equivalent to calling `SipHasher(k0, k1).Write(val).Write(extra).Finalize()`
💬 ryanofsky commented on pull request "precalculate SipHash constant salt XORs":
(https://github.com/bitcoin/bitcoin/pull/30442#discussion_r2486998959)
In commit "optimization: Introduce PresaltedSipHasher for repeated hashing" (8568340be2c679713703edc3d418272ac78d1147)
I can see this code is just being moved, not duplicated again, but since this does duplicate the CSipHasher constructor code, would seem good to just move and define once in a single place.
Would suggest a change like the following (this change also makes PresaltedSipHasher state constant as suggested earlier):
```diff
--- a/src/crypto/siphash.cpp
+++ b/src/crypto/si
...
(https://github.com/bitcoin/bitcoin/pull/30442#discussion_r2486998959)
In commit "optimization: Introduce PresaltedSipHasher for repeated hashing" (8568340be2c679713703edc3d418272ac78d1147)
I can see this code is just being moved, not duplicated again, but since this does duplicate the CSipHasher constructor code, would seem good to just move and define once in a single place.
Would suggest a change like the following (this change also makes PresaltedSipHasher state constant as suggested earlier):
```diff
--- a/src/crypto/siphash.cpp
+++ b/src/crypto/si
...
💬 optout21 commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2487089474)
If `GetSkipHeight` is `inline`, I get the error:
```
[ 75%] Built target bitcoin-node
/usr/bin/ld: CMakeFiles/test_bitcoin.dir/chain_tests.cpp.o: in function `chain_tests::get_skip_height_test::test_method()':
/home/bitcoin/bitcoin-core/build/src/test/./test/chain_tests.cpp:50:(.text+0x68b): undefined reference to `GetSkipHeight(int)'
/usr/bin/ld: /home/bitcoin/bitcoin-core/build/src/test/./test/chain_tests.cpp:51:(.text+0x7cf): undefined reference to `GetSkipHeight(int)'
/usr/bin/ld: /h
...
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2487089474)
If `GetSkipHeight` is `inline`, I get the error:
```
[ 75%] Built target bitcoin-node
/usr/bin/ld: CMakeFiles/test_bitcoin.dir/chain_tests.cpp.o: in function `chain_tests::get_skip_height_test::test_method()':
/home/bitcoin/bitcoin-core/build/src/test/./test/chain_tests.cpp:50:(.text+0x68b): undefined reference to `GetSkipHeight(int)'
/usr/bin/ld: /home/bitcoin/bitcoin-core/build/src/test/./test/chain_tests.cpp:51:(.text+0x7cf): undefined reference to `GetSkipHeight(int)'
/usr/bin/ld: /h
...
💬 purpleKarrot commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#issuecomment-3481391205)
nit: C++ defines types like uint64_t in the namespace std and then, optionally (!!!), also in the global namespace for compatibility with C. We should prefer std::uint64_t.
(https://github.com/bitcoin/bitcoin/pull/33724#issuecomment-3481391205)
nit: C++ defines types like uint64_t in the namespace std and then, optionally (!!!), also in the global namespace for compatibility with C. We should prefer std::uint64_t.
💬 optout21 commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2487099898)
Oh, I see. There are in fact n+1 elements. Will change.
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2487099898)
Oh, I see. There are in fact n+1 elements. Will change.
👍 ryanofsky approved a pull request: "ci: Update Clang in "tidy" job"
(https://github.com/bitcoin/bitcoin/pull/33445#pullrequestreview-3411868329)
Code review ACK 5d784bebaff5e3acc0b5180ee51d9a16aec0e356, just adding clang version comment since last review.
Would still be nice to remove ipc build code from the test directory as a followup too (https://github.com/bitcoin/bitcoin/pull/33445#discussion_r2379651340)
(https://github.com/bitcoin/bitcoin/pull/33445#pullrequestreview-3411868329)
Code review ACK 5d784bebaff5e3acc0b5180ee51d9a16aec0e356, just adding clang version comment since last review.
Would still be nice to remove ipc build code from the test directory as a followup too (https://github.com/bitcoin/bitcoin/pull/33445#discussion_r2379651340)
💬 maflcko commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#issuecomment-3481418578)
> But if sizes are serialized as `uint32_t`, I assume only the value range of `uint32_t` is valid, even on 64bit machines. In what scenario can `size_t` lead to a problem?
It is explained in the CVE (linked in the description):
"Before writing a block to disk, Bitcoin Core checks that its size is within a normal range. This check would overflow on 32-bit systems for blocks over 1GB..."
The basic idea is that the following code is brittle:
```
u32 value = result of some calculation,
...
(https://github.com/bitcoin/bitcoin/pull/33724#issuecomment-3481418578)
> But if sizes are serialized as `uint32_t`, I assume only the value range of `uint32_t` is valid, even on 64bit machines. In what scenario can `size_t` lead to a problem?
It is explained in the CVE (linked in the description):
"Before writing a block to disk, Bitcoin Core checks that its size is within a normal range. This check would overflow on 32-bit systems for blocks over 1GB..."
The basic idea is that the following code is brittle:
```
u32 value = result of some calculation,
...
👍 hebasto approved a pull request: "Add createwallet, createwalletdescriptor, and migratewallet to history filter"
(https://github.com/bitcoin-core/gui/pull/901#pullrequestreview-3411906243)
ACK 4e352efa2ce756c668664486c99d003eef530e0c.
(https://github.com/bitcoin-core/gui/pull/901#pullrequestreview-3411906243)
ACK 4e352efa2ce756c668664486c99d003eef530e0c.
🚀 hebasto merged a pull request: "Add createwallet, createwalletdescriptor, and migratewallet to history filter"
(https://github.com/bitcoin-core/gui/pull/901)
(https://github.com/bitcoin-core/gui/pull/901)
🤔 hodlinator reviewed a pull request: "log: reduce excessive "rolling back/forward" messages during block replay"
(https://github.com/bitcoin/bitcoin/pull/33443#pullrequestreview-3411788483)
Concept ACK 1fc7a81f1f5f30ba3ebe305ac2a520c7b4afb596
(https://github.com/bitcoin/bitcoin/pull/33443#pullrequestreview-3411788483)
Concept ACK 1fc7a81f1f5f30ba3ebe305ac2a520c7b4afb596