👍 TheCharlatan approved a pull request: "versionbits refactoring"
(https://github.com/bitcoin/bitcoin/pull/29039#pullrequestreview-2682297269)
ACK 7565563bc7a5bb98ebf03a7d6881912a74d3f302
(https://github.com/bitcoin/bitcoin/pull/29039#pullrequestreview-2682297269)
ACK 7565563bc7a5bb98ebf03a7d6881912a74d3f302
💬 TheCharlatan commented on pull request "versionbits refactoring":
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r1993721926)
nit: Since corecheck is reporting `IsActiveAfter` as not being covered, how about (though not very sure the checks make a lot of sense there):
```diff
diff --git a/src/test/versionbits_tests.cpp b/src/test/versionbits_tests.cpp
index bcacac025d..3218ed0811 100644
--- a/src/test/versionbits_tests.cpp
+++ b/src/test/versionbits_tests.cpp
@@ -407,11 +407,13 @@ void check_computeblockversion(VersionBitsCache& versionbitscache, const Consens
// check signalling continues while min_act
...
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r1993721926)
nit: Since corecheck is reporting `IsActiveAfter` as not being covered, how about (though not very sure the checks make a lot of sense there):
```diff
diff --git a/src/test/versionbits_tests.cpp b/src/test/versionbits_tests.cpp
index bcacac025d..3218ed0811 100644
--- a/src/test/versionbits_tests.cpp
+++ b/src/test/versionbits_tests.cpp
@@ -407,11 +407,13 @@ void check_computeblockversion(VersionBitsCache& versionbitscache, const Consens
// check signalling continues while min_act
...
💬 ryanofsky commented on pull request "test: avoid treating hash results as integers (part 1)":
(https://github.com/bitcoin/bitcoin/pull/32050#issuecomment-2721796758)
Changes in this PR seem reasonable to me although I don't agree with premise that it is an improvement to treat hashes as blobs, but then then arbitrarily decide to display those blobs as reverse-byte hex strings, when there is probably not other software that displays blobs as reverse-byte hex strings.
Status quo where hashes are simply interpreted as little endian numbers, and those numbers are represented as hex strings in the standard way python and other software represents numbers repre
...
(https://github.com/bitcoin/bitcoin/pull/32050#issuecomment-2721796758)
Changes in this PR seem reasonable to me although I don't agree with premise that it is an improvement to treat hashes as blobs, but then then arbitrarily decide to display those blobs as reverse-byte hex strings, when there is probably not other software that displays blobs as reverse-byte hex strings.
Status quo where hashes are simply interpreted as little endian numbers, and those numbers are represented as hex strings in the standard way python and other software represents numbers repre
...
💬 Prabhat1308 commented on pull request "contrib: Add deterministic-unittest-coverage":
(https://github.com/bitcoin/bitcoin/pull/31901#issuecomment-2721800574)
> Word of caution, i'm running in a nix-shell on a mac. So would like to see someone on pure mac env. confirm that this is the issue
Applied this change and I get deterministic results on all runs
```
#if defined(__clang__) && (defined(__linux__) || defined(__APPLE__))
```
<img width="1512" alt="Screenshot 2025-03-13 at 9 31 40 PM" src="https://github.com/user-attachments/assets/fb622440-8634-47ba-b21a-54fc567803df" />
> Confirmed. I still have the issue on master but I just tested
...
(https://github.com/bitcoin/bitcoin/pull/31901#issuecomment-2721800574)
> Word of caution, i'm running in a nix-shell on a mac. So would like to see someone on pure mac env. confirm that this is the issue
Applied this change and I get deterministic results on all runs
```
#if defined(__clang__) && (defined(__linux__) || defined(__APPLE__))
```
<img width="1512" alt="Screenshot 2025-03-13 at 9 31 40 PM" src="https://github.com/user-attachments/assets/fb622440-8634-47ba-b21a-54fc567803df" />
> Confirmed. I still have the issue on master but I just tested
...
🤔 jonatack reviewed a pull request: "[IBD] flush UXTOs in bigger batches"
(https://github.com/bitcoin/bitcoin/pull/31645#pullrequestreview-2682568821)
Concept ACK, testing
(https://github.com/bitcoin/bitcoin/pull/31645#pullrequestreview-2682568821)
Concept ACK, testing
👍 l0rinc approved a pull request: "CLI cleanups"
(https://github.com/bitcoin/bitcoin/pull/31887#pullrequestreview-2682594542)
I would prefer doing the modernizations here to get rid of more technical debt, but this is already better than before so:
ACK d423fd9ec83ae323ac29bdc1b677ed8260bd59c4
(https://github.com/bitcoin/bitcoin/pull/31887#pullrequestreview-2682594542)
I would prefer doing the modernizations here to get rid of more technical debt, but this is already better than before so:
ACK d423fd9ec83ae323ac29bdc1b677ed8260bd59c4
💬 l0rinc commented on pull request "CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1993873254)
the problem is likely that the method return `int8_t` for an index - unnecessary, should likely be `size_t` or `int` - not important
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1993873254)
the problem is likely that the method return `int8_t` for an index - unnecessary, should likely be `size_t` or `int` - not important
💬 jonatack commented on pull request "CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1993885246)
That's true about the index being int.
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1993885246)
That's true about the index being int.
🤔 BrandonOdiwuor reviewed a pull request: "contrib: Fix `gen-bitcoin-conf.sh`"
(https://github.com/bitcoin/bitcoin/pull/32049#pullrequestreview-2682713176)
crACK a24419f8bed5e1145ce171dbbdad957750585471
(https://github.com/bitcoin/bitcoin/pull/32049#pullrequestreview-2682713176)
crACK a24419f8bed5e1145ce171dbbdad957750585471
👍 dergoegge approved a pull request: "consensus: Remove checkpoints (take 2)"
(https://github.com/bitcoin/bitcoin/pull/31649#pullrequestreview-2682741320)
ACK 3c5d1a468199722da620f1f3d8ae3319980a46d5
(https://github.com/bitcoin/bitcoin/pull/31649#pullrequestreview-2682741320)
ACK 3c5d1a468199722da620f1f3d8ae3319980a46d5
💬 GarmashAlex commented on pull request "tests: Improve stderr validation in test_runner.py":
(https://github.com/bitcoin/bitcoin/pull/31966#issuecomment-2721999168)
@maflcko could i just create a new PR instead of this one . I don't know how to squash commits
(https://github.com/bitcoin/bitcoin/pull/31966#issuecomment-2721999168)
@maflcko could i just create a new PR instead of this one . I don't know how to squash commits
📝 janb84 opened a pull request: "contrib: Update coverage.cpp macro to support macOS"
(https://github.com/bitcoin/bitcoin/pull/32059)
In PR [#31901](https://github.com/bitcoin/bitcoin/pull/31901), Coverage.cpp was introduced as a separate utility file. However, the macro defined in Coverage.cpp was limited to Clang and Linux, which caused issues for users on macOS when using the newly introduced deterministic test tooling.
This change extends the macro test to include support for the Apple platform, ensuring that macOS users can utilize the coverage functionality so that the deterministic test tooling can run without encoun
...
(https://github.com/bitcoin/bitcoin/pull/32059)
In PR [#31901](https://github.com/bitcoin/bitcoin/pull/31901), Coverage.cpp was introduced as a separate utility file. However, the macro defined in Coverage.cpp was limited to Clang and Linux, which caused issues for users on macOS when using the newly introduced deterministic test tooling.
This change extends the macro test to include support for the Apple platform, ensuring that macOS users can utilize the coverage functionality so that the deterministic test tooling can run without encoun
...
💬 Sjors commented on pull request "contrib: Update coverage.cpp macro to support macOS":
(https://github.com/bitcoin/bitcoin/pull/32059#issuecomment-2722033464)
From the original PR it's not obviously how to use this, is there a tl&dr for what to test?
(https://github.com/bitcoin/bitcoin/pull/32059#issuecomment-2722033464)
From the original PR it's not obviously how to use this, is there a tl&dr for what to test?
📝 VolodymyrBg opened a pull request: "test: Add support for mainnet addresses in address_to_scriptpubkey"
(https://github.com/bitcoin/bitcoin/pull/32060)
This commit adds support for mainnet addresses (P2PKH and P2SH) in the address_to_scriptpubkey function in the test framework. Previously, the function only supported testnet addresses and segwit addresses.
The changes include:
- Adding support for mainnet P2PKH addresses (version 0)
- Adding support for mainnet P2SH addresses (version 5)
- Adding tests to verify the new functionality
- Removing the TODO comment as the requested functionality has been implemented
(https://github.com/bitcoin/bitcoin/pull/32060)
This commit adds support for mainnet addresses (P2PKH and P2SH) in the address_to_scriptpubkey function in the test framework. Previously, the function only supported testnet addresses and segwit addresses.
The changes include:
- Adding support for mainnet P2PKH addresses (version 0)
- Adding support for mainnet P2SH addresses (version 5)
- Adding tests to verify the new functionality
- Removing the TODO comment as the requested functionality has been implemented
💬 ryanofsky commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#issuecomment-2722056036)
Rebased 7fd7960389254c7a2ba50614e1cbdf8911ef6a7d -> 58f62c2221d82e22e7c2d9cccedd58e4390ace5a ([`pr/cstate.7`](https://github.com/ryanofsky/bitcoin/commits/pr/cstate.7) -> [`pr/cstate.8`](https://github.com/ryanofsky/bitcoin/commits/pr/cstate.8), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/cstate.7-rebase..pr/cstate.8)) due to various conflicts.
---
re: https://github.com/bitcoin/bitcoin/pull/30214#issuecomment-2720898462
> Shouldn't the block index be our fundamental sourc
...
(https://github.com/bitcoin/bitcoin/pull/30214#issuecomment-2722056036)
Rebased 7fd7960389254c7a2ba50614e1cbdf8911ef6a7d -> 58f62c2221d82e22e7c2d9cccedd58e4390ace5a ([`pr/cstate.7`](https://github.com/ryanofsky/bitcoin/commits/pr/cstate.7) -> [`pr/cstate.8`](https://github.com/ryanofsky/bitcoin/commits/pr/cstate.8), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/cstate.7-rebase..pr/cstate.8)) due to various conflicts.
---
re: https://github.com/bitcoin/bitcoin/pull/30214#issuecomment-2720898462
> Shouldn't the block index be our fundamental sourc
...
💬 janb84 commented on pull request "contrib: Update coverage.cpp macro to support macOS":
(https://github.com/bitcoin/bitcoin/pull/32059#issuecomment-2722068136)
> From the original PR it's not obvious how to use this, is there a tl&dr for what to test?
Use this config:
```shell
cmake -S . -B build -DCMAKE_C_COMPILER='clang' -DCMAKE_CXX_COMPILER='clang++' -DCMAKE_CXX_FLAGS='-fprofile-instr-generate -fcoverage-mapping'
```
build as normal :
```shell
cmake --build build
```
Run tooling for util_string_tests:
```shell
RUST_BACKTRACE=1 cargo run --manifest-path ./contrib/devtools/deterministic-unittest-coverage/Cargo.toml -- $PWD/build uti
...
(https://github.com/bitcoin/bitcoin/pull/32059#issuecomment-2722068136)
> From the original PR it's not obvious how to use this, is there a tl&dr for what to test?
Use this config:
```shell
cmake -S . -B build -DCMAKE_C_COMPILER='clang' -DCMAKE_CXX_COMPILER='clang++' -DCMAKE_CXX_FLAGS='-fprofile-instr-generate -fcoverage-mapping'
```
build as normal :
```shell
cmake --build build
```
Run tooling for util_string_tests:
```shell
RUST_BACKTRACE=1 cargo run --manifest-path ./contrib/devtools/deterministic-unittest-coverage/Cargo.toml -- $PWD/build uti
...
💬 rkrux commented on pull request "test: avoid treating hash results as integers (part 1)":
(https://github.com/bitcoin/bitcoin/pull/32050#issuecomment-2722093413)
Re: https://github.com/bitcoin/bitcoin/pull/32050#issuecomment-2721796758
These are quite subtle points that are raised, but important IMO.
Even if for some reason later it's decided to not do the cpp changes as suggested in the description, the functional test changes as done in this PR are good enough in their own right, and hence, I agree that "part 1" can be dropped from the title unless the author intends to do more type-conversion related functional test changes, which one can argue
...
(https://github.com/bitcoin/bitcoin/pull/32050#issuecomment-2722093413)
Re: https://github.com/bitcoin/bitcoin/pull/32050#issuecomment-2721796758
These are quite subtle points that are raised, but important IMO.
Even if for some reason later it's decided to not do the cpp changes as suggested in the description, the functional test changes as done in this PR are good enough in their own right, and hence, I agree that "part 1" can be dropped from the title unless the author intends to do more type-conversion related functional test changes, which one can argue
...
💬 instagibbs commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#issuecomment-2722117978)
reACK 3c5d1a468199722da620f1f3d8ae3319980a46d5
(https://github.com/bitcoin/bitcoin/pull/31649#issuecomment-2722117978)
reACK 3c5d1a468199722da620f1f3d8ae3319980a46d5
🤔 marcofleon reviewed a pull request: "fuzz: split `coinselection` harness"
(https://github.com/bitcoin/bitcoin/pull/31870#pullrequestreview-2683023589)
reACK ba82240553ddd534287845e10bc76b46b45329fe
Only thing that changed since last review is the fix to https://github.com/bitcoin/bitcoin/pull/31870#discussion_r1965528993. The order of the two checks has been switched but afaict it shouldn't matter.
(https://github.com/bitcoin/bitcoin/pull/31870#pullrequestreview-2683023589)
reACK ba82240553ddd534287845e10bc76b46b45329fe
Only thing that changed since last review is the fix to https://github.com/bitcoin/bitcoin/pull/31870#discussion_r1965528993. The order of the two checks has been switched but afaict it shouldn't matter.
💬 theStack commented on pull request "test: avoid treating hash results as integers":
(https://github.com/bitcoin/bitcoin/pull/32050#issuecomment-2722365413)
@ryanofsky @rkrux: Thanks for the feedback! Removed the "(part 1)" from the PR title as suggested and adapted the strong "almost never makes sense" wording in the description. I agree that displaying hash results as byte-reversed hex is an arbitrary decision, but one that stems from the Satoshi-era past and all Bitcoin protocol software that deals with user interface follows it (see also https://bitcoin.stackexchange.com/questions/116730/why-does-bitcoin-core-print-sha256-hashes-uint256-bytes-in
...
(https://github.com/bitcoin/bitcoin/pull/32050#issuecomment-2722365413)
@ryanofsky @rkrux: Thanks for the feedback! Removed the "(part 1)" from the PR title as suggested and adapted the strong "almost never makes sense" wording in the description. I agree that displaying hash results as byte-reversed hex is an arbitrary decision, but one that stems from the Satoshi-era past and all Bitcoin protocol software that deals with user interface follows it (see also https://bitcoin.stackexchange.com/questions/116730/why-does-bitcoin-core-print-sha256-hashes-uint256-bytes-in
...