💬 glozow commented on pull request "test: add functional test for XORed block/undo files (`-blocksxor` option)":
(https://github.com/bitcoin/bitcoin/pull/30657#discussion_r1718786049)
It's jammy with clang 17, indeed it works with a non-`/tmp` folder.
(https://github.com/bitcoin/bitcoin/pull/30657#discussion_r1718786049)
It's jammy with clang 17, indeed it works with a non-`/tmp` folder.
💬 maflcko commented on pull request "test: add functional test for XORed block/undo files (`-blocksxor` option)":
(https://github.com/bitcoin/bitcoin/pull/30657#discussion_r1718816229)
Jammy doesn't have clang 17, so I guess it is installed from the nightly llvm apt and then `CC=clang-17 CXX=clang++-17 ../configure` (using the libstdc++-11, not libc++)?
(https://github.com/bitcoin/bitcoin/pull/30657#discussion_r1718816229)
Jammy doesn't have clang 17, so I guess it is installed from the nightly llvm apt and then `CC=clang-17 CXX=clang++-17 ../configure` (using the libstdc++-11, not libc++)?
🤔 maflcko reviewed a pull request: "test: [refactor] Use g_rng/m_rng directly"
(https://github.com/bitcoin/bitcoin/pull/30571#pullrequestreview-2241150302)
Sure, pushed your branch. A skim looks good. I left a nit and will do a real review later.
(https://github.com/bitcoin/bitcoin/pull/30571#pullrequestreview-2241150302)
Sure, pushed your branch. A skim looks good. I left a nit and will do a real review later.
💬 maflcko commented on pull request "test: [refactor] Use g_rng/m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1718879706)
nit in bb67ca09c88a46b99bf83e937fa761f0e25cba37: Not sure about this change. It seems overly restrictive to require a `FastRandomContext&` here, when a caller may not want to pass anything at all, or a different rng.
I think it would be better to drop this change.
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1718879706)
nit in bb67ca09c88a46b99bf83e937fa761f0e25cba37: Not sure about this change. It seems overly restrictive to require a `FastRandomContext&` here, when a caller may not want to pass anything at all, or a different rng.
I think it would be better to drop this change.
💬 TheCharlatan commented on pull request "fuzz: Faster utxo_snapshot fuzz target":
(https://github.com/bitcoin/bitcoin/pull/30644#discussion_r1718552423)
Nit: I find these callbacks a bit ugly, how about:
```diff
diff --git a/src/test/fuzz/utxo_snapshot.cpp b/src/test/fuzz/utxo_snapshot.cpp
index 0b4e0d183a..a30ac53f97 100644
--- a/src/test/fuzz/utxo_snapshot.cpp
+++ b/src/test/fuzz/utxo_snapshot.cpp
@@ -54,0 +55 @@ void initialize_chain()
+ .min_validation_cache = true,
diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
index cf47d16faf..62ff61b227 100644
--- a/src
...
(https://github.com/bitcoin/bitcoin/pull/30644#discussion_r1718552423)
Nit: I find these callbacks a bit ugly, how about:
```diff
diff --git a/src/test/fuzz/utxo_snapshot.cpp b/src/test/fuzz/utxo_snapshot.cpp
index 0b4e0d183a..a30ac53f97 100644
--- a/src/test/fuzz/utxo_snapshot.cpp
+++ b/src/test/fuzz/utxo_snapshot.cpp
@@ -54,0 +55 @@ void initialize_chain()
+ .min_validation_cache = true,
diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
index cf47d16faf..62ff61b227 100644
--- a/src
...
👍 TheCharlatan approved a pull request: "fuzz: Faster utxo_snapshot fuzz target"
(https://github.com/bitcoin/bitcoin/pull/30644#pullrequestreview-2240627537)
ACK fa69afd51879f8441df7a51144318a49e7dd27b9
The logic is a bit convoluted, but I don't have many suggestions for improving it and I think it is important to get this performance improvement deployed quickly.
(https://github.com/bitcoin/bitcoin/pull/30644#pullrequestreview-2240627537)
ACK fa69afd51879f8441df7a51144318a49e7dd27b9
The logic is a bit convoluted, but I don't have many suggestions for improving it and I think it is important to get this performance improvement deployed quickly.
💬 TheCharlatan commented on pull request "fuzz: Faster utxo_snapshot fuzz target":
(https://github.com/bitcoin/bitcoin/pull/30644#discussion_r1718893373)
Nit: The includes need to be corrected again.
(https://github.com/bitcoin/bitcoin/pull/30644#discussion_r1718893373)
Nit: The includes need to be corrected again.
💬 mzumsande commented on pull request "kernel: pre-28.x chainparams and headerssync update":
(https://github.com/bitcoin/bitcoin/pull/30658#discussion_r1718921534)
I see similar size to @Sjors, as of height=2873716:
```
du -sh ~/.bitcoin/testnet3/chainstate/ ~/.bitcoin/testnet3/blocks/
13G /home/martin/.bitcoin/testnet3/chainstate/
82G /home/martin/.bitcoin/testnet3/blocks/
```
(I wasn't continuously online during the fork shenanigans, so having fewer blocks makes sense).
(https://github.com/bitcoin/bitcoin/pull/30658#discussion_r1718921534)
I see similar size to @Sjors, as of height=2873716:
```
du -sh ~/.bitcoin/testnet3/chainstate/ ~/.bitcoin/testnet3/blocks/
13G /home/martin/.bitcoin/testnet3/chainstate/
82G /home/martin/.bitcoin/testnet3/blocks/
```
(I wasn't continuously online during the fork shenanigans, so having fewer blocks makes sense).
📝 hebasto opened a pull request: "CMake replaces Autotools"
(https://github.com/bitcoin/bitcoin/pull/30664)
This PR is based on https://github.com/bitcoin/bitcoin/pull/30454 with additional commits that delete both the Autotools-based build system and the legacy MSVC build system.
It aims reveal conflicts with other PRs.
For more details, please refer to today's IRC meeting discussion.
(https://github.com/bitcoin/bitcoin/pull/30664)
This PR is based on https://github.com/bitcoin/bitcoin/pull/30454 with additional commits that delete both the Autotools-based build system and the legacy MSVC build system.
It aims reveal conflicts with other PRs.
For more details, please refer to today's IRC meeting discussion.
💬 fjahr commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1718993291)
fixed
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1718993291)
fixed
💬 fjahr commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1718993817)
fixed (as part of one of the larger changes)
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1718993817)
fixed (as part of one of the larger changes)
💬 fjahr commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1718995086)
Adopted the type approach you suggested so this is resolved as well with that.
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1718995086)
Adopted the type approach you suggested so this is resolved as well with that.
💬 ryanofsky commented on pull request "test: [refactor] Use g_rng/m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1718994222)
> nit in [bb67ca0](https://github.com/bitcoin/bitcoin/commit/bb67ca09c88a46b99bf83e937fa761f0e25cba37): Not sure about this change. It seems overly restrictive to require a `FastRandomContext&` here, when a caller may not want to pass anything at all, or a different rng.
Makes sense. I hadn't really looked at this function closely and was just trying to keep behavior unchanged. Maybe it would be good to change signature to something like:
```c++
SeedRandomForTest(SeedRand seed = SeedRand:
...
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1718994222)
> nit in [bb67ca0](https://github.com/bitcoin/bitcoin/commit/bb67ca09c88a46b99bf83e937fa761f0e25cba37): Not sure about this change. It seems overly restrictive to require a `FastRandomContext&` here, when a caller may not want to pass anything at all, or a different rng.
Makes sense. I hadn't really looked at this function closely and was just trying to keep behavior unchanged. Maybe it would be good to change signature to something like:
```c++
SeedRandomForTest(SeedRand seed = SeedRand:
...
👍 ryanofsky approved a pull request: "test: [refactor] Use g_rng/m_rng directly"
(https://github.com/bitcoin/bitcoin/pull/30571#pullrequestreview-2241332588)
Code review ACK f1c1b37d2247eed49156c0467daa68aa38497bb8. Current version of this looks identical to the branch I pushed, except it is rebased and whitespace in the scripted diff was adjusted.
(https://github.com/bitcoin/bitcoin/pull/30571#pullrequestreview-2241332588)
Code review ACK f1c1b37d2247eed49156c0467daa68aa38497bb8. Current version of this looks identical to the branch I pushed, except it is rebased and whitespace in the scripted diff was adjusted.
💬 fjahr commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1718998488)
Thanks, I agree that the default wasn't very sensitive when looking at it this way. I have adopted the type approach that @ryanofsky suggested. I think it may be a bit more complicated to understand initially for users but it's also easier to hand because you don't have to deal with a hash or height in the most common use cases.
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1718998488)
Thanks, I agree that the default wasn't very sensitive when looking at it this way. I have adopted the type approach that @ryanofsky suggested. I think it may be a bit more complicated to understand initially for users but it's also easier to hand because you don't have to deal with a hash or height in the most common use cases.
💬 fjahr commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1719001528)
I have introduced your suggested fix as a separate commit because the main commit is already quite big and I think it's easier to review this way. I added some minor documentation changes since `CreateUTXOSnaphot()` is now only used in tests.
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1719001528)
I have introduced your suggested fix as a separate commit because the main commit is already quite big and I think it's easier to review this way. I added some minor documentation changes since `CreateUTXOSnaphot()` is now only used in tests.
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718988155)
As mentioned in https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717420847, who would do that?
Someone could theoretically do
```C++
constexpr char hex[] = {'a', 'b', '\0', '\0', '\0'};
VecFromHex(hex);
```
and have your version fail to compile.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718988155)
As mentioned in https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717420847, who would do that?
Someone could theoretically do
```C++
constexpr char hex[] = {'a', 'b', '\0', '\0', '\0'};
VecFromHex(hex);
```
and have your version fail to compile.
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718998801)
I see what you mean and tried it out, but ended up preferring input validation at the top of the function.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718998801)
I see what you mean and tried it out, but ended up preferring input validation at the top of the function.
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718978767)
Thank you!
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718978767)
Thank you!
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718976845)
I got lost around `SerializeMany` :), but agree, **serialize.h** does the `std::array` -> `Span` conversion internally.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718976845)
I got lost around `SerializeMany` :), but agree, **serialize.h** does the `std::array` -> `Span` conversion internally.