π€ w0xlt reviewed a pull request: "log: always print initial signature verification state"
(https://github.com/bitcoin/bitcoin/pull/33336#pullrequestreview-3209275659)
Approach ACK
Using `std::optional<bool>` introduces a third state (unset), which guarantees exactly one initial log messageβregardless of whether the first real value is true or false.
For reviewers, hereβs a simple way to verify the behavior change:
```
mkdir temp_data_dir
./build/bin/bitcoind -regtest -datadir=temp_data_dir -daemon
./build/bin/bitcoin-cli -regtest -datadir=temp_data_dir createwallet w
./build/bin/bitcoin-cli -regtest -datadir=temp_data_dir getnewaddress
./build/b
...
(https://github.com/bitcoin/bitcoin/pull/33336#pullrequestreview-3209275659)
Approach ACK
Using `std::optional<bool>` introduces a third state (unset), which guarantees exactly one initial log messageβregardless of whether the first real value is true or false.
For reviewers, hereβs a simple way to verify the behavior change:
```
mkdir temp_data_dir
./build/bin/bitcoind -regtest -datadir=temp_data_dir -daemon
./build/bin/bitcoin-cli -regtest -datadir=temp_data_dir createwallet w
./build/bin/bitcoin-cli -regtest -datadir=temp_data_dir getnewaddress
./build/b
...
π jalateras opened a pull request: "Fix #25980: Validate transactions in combinerawtransaction"
(https://github.com/bitcoin/bitcoin/pull/33361)
## Summary
This PR fixes issue #25980 where `combinerawtransaction` would silently accept unrelated transactions and only use the first one, ignoring the rest.
## Problem
When passed two or more unrelated transactions (transactions with different inputs/outputs), `combinerawtransaction` would:
- Take the first transaction as the base
- Silently ignore the unrelated parts of subsequent transactions
- Only merge signatures for matching inputs
This behavior was confusing and could lead to unexp
...
(https://github.com/bitcoin/bitcoin/pull/33361)
## Summary
This PR fixes issue #25980 where `combinerawtransaction` would silently accept unrelated transactions and only use the first one, ignoring the rest.
## Problem
When passed two or more unrelated transactions (transactions with different inputs/outputs), `combinerawtransaction` would:
- Take the first transaction as the base
- Silently ignore the unrelated parts of subsequent transactions
- Only merge signatures for matching inputs
This behavior was confusing and could lead to unexp
...
π¬ kannapoix commented on pull request "doc: update multisig tutorial to use multipath descriptors":
(https://github.com/bitcoin/bitcoin/pull/33286#discussion_r2339166383)
It looks like the bitcoin binary might not have been built.
As far as I know, BUILD_BITCOIN_BIN=ON is the default, but maybe you built with a non-default setting.
If you build with bitcoin binary explicitly enabled, it should be generated at the path:
```
cmake -B build -DBUILD_BITCOIN_BIN=ON
```
(https://github.com/bitcoin/bitcoin/pull/33286#discussion_r2339166383)
It looks like the bitcoin binary might not have been built.
As far as I know, BUILD_BITCOIN_BIN=ON is the default, but maybe you built with a non-default setting.
If you build with bitcoin binary explicitly enabled, it should be generated at the path:
```
cmake -B build -DBUILD_BITCOIN_BIN=ON
```
π¬ kannapoix commented on pull request "doc: update multisig tutorial to use multipath descriptors":
(https://github.com/bitcoin/bitcoin/pull/33286#issuecomment-3278940886)
reACK 5424b4f1ce74c82b7ae01034bbc1592088048128
Tested and reviewed the new functional test.
(https://github.com/bitcoin/bitcoin/pull/33286#issuecomment-3278940886)
reACK 5424b4f1ce74c82b7ae01034bbc1592088048128
Tested and reviewed the new functional test.
π vasild opened a pull request: "Run feature_bind_port_(discover|externalip).py in CI"
(https://github.com/bitcoin/bitcoin/pull/33362)
This PR includes https://github.com/bitcoin/bitcoin/pull/32757 (first two commits here).
Then, on top of that:
`feature_bind_port_discover.py` and `feature_bind_port_externalip.py` require a routable address on the machine to run. Since that was not predictably available on CI, those tests required a manual setting up of IP addresses (e.g. using `ifconfig`) and then running the tests with a command line option telling them that the addresses are set up. The tests were not run in CI and got
...
(https://github.com/bitcoin/bitcoin/pull/33362)
This PR includes https://github.com/bitcoin/bitcoin/pull/32757 (first two commits here).
Then, on top of that:
`feature_bind_port_discover.py` and `feature_bind_port_externalip.py` require a routable address on the machine to run. Since that was not predictably available on CI, those tests required a manual setting up of IP addresses (e.g. using `ifconfig`) and then running the tests with a command line option telling them that the addresses are set up. The tests were not run in CI and got
...
π¬ vasild commented on pull request "net: Fix Discover() not running when using -bind=0.0.0.0:port":
(https://github.com/bitcoin/bitcoin/pull/32757#issuecomment-3279026280)
> I would really like to know if its possible to run this test in CI ...
> `IFF_LOOPBACK` addresses are being filtered out [here](https://github.com/bitcoin/bitcoin/blob/e17fb86382eafb7ccfddb56bca981f6a12201c85/src/common/netif.cpp#L340). So I don't think the problem from https://github.com/bitcoin/bitcoin/issues/31336 is fixed by this as the OP says.
Both resolved in https://github.com/bitcoin/bitcoin/pull/33362
(https://github.com/bitcoin/bitcoin/pull/32757#issuecomment-3279026280)
> I would really like to know if its possible to run this test in CI ...
> `IFF_LOOPBACK` addresses are being filtered out [here](https://github.com/bitcoin/bitcoin/blob/e17fb86382eafb7ccfddb56bca981f6a12201c85/src/common/netif.cpp#L340). So I don't think the problem from https://github.com/bitcoin/bitcoin/issues/31336 is fixed by this as the OP says.
Both resolved in https://github.com/bitcoin/bitcoin/pull/33362
π€ Eunovo reviewed a pull request: "log: always print initial signature verification state"
(https://github.com/bitcoin/bitcoin/pull/33336#pullrequestreview-3209656734)
ACK https://github.com/bitcoin/bitcoin/pull/33336/commits/39f41135c764c6c4705ce2cd36781ca8d43f0114:
Previously, users could only verify that script checks were enabled by confirming the absence of a "Disabling signature validations at block #1" message in the logs, which is a confusing negative confirmation pattern.
This PR adds explicit positive feedback by ensuring an "Enabling signature validations at block #1" message appears when script checks are enabled, making the system state cleare
...
(https://github.com/bitcoin/bitcoin/pull/33336#pullrequestreview-3209656734)
ACK https://github.com/bitcoin/bitcoin/pull/33336/commits/39f41135c764c6c4705ce2cd36781ca8d43f0114:
Previously, users could only verify that script checks were enabled by confirming the absence of a "Disabling signature validations at block #1" message in the logs, which is a confusing negative confirmation pattern.
This PR adds explicit positive feedback by ensuring an "Enabling signature validations at block #1" message appears when script checks are enabled, making the system state cleare
...
π¬ Eunovo commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#issuecomment-3279099513)
> After `reindex`, my node has ~841k headers, a lot lower than the current tip of ~914k. The default `assumevalid` block (height 912,683) is in that missing range, so the header lookup fails since the block isn't found in the block index (even though the log claims "Assuming ancestors of block ... have valid signatures"). Even when I set a custom `assumevalid` to a block I actually have, my `chainwork` is still below the bumped `nMinimumChainWork` from [755152ac](https://github.com/bitcoin/bitco
...
(https://github.com/bitcoin/bitcoin/pull/33336#issuecomment-3279099513)
> After `reindex`, my node has ~841k headers, a lot lower than the current tip of ~914k. The default `assumevalid` block (height 912,683) is in that missing range, so the header lookup fails since the block isn't found in the block index (even though the log claims "Assuming ancestors of block ... have valid signatures"). Even when I set a custom `assumevalid` to a block I actually have, my `chainwork` is still below the bumped `nMinimumChainWork` from [755152ac](https://github.com/bitcoin/bitco
...
π hodlinator approved a pull request: "log: always print initial signature verification state"
(https://github.com/bitcoin/bitcoin/pull/33336#pullrequestreview-3209753464)
re-ACK 39f41135c764c6c4705ce2cd36781ca8d43f0114
Only made test less racy (https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2337774261) since the first ACK.
---
> > After `reindex`, my node has ~841k headers, a lot lower than the current tip of ~914k.
> > The default `assumevalid` block (height 912,683) is in that missing range, [would say: is outside that range]
>
> I think the behaviour here is probably fine -- it's a weak way of preventing you from triggering assumevalid
...
(https://github.com/bitcoin/bitcoin/pull/33336#pullrequestreview-3209753464)
re-ACK 39f41135c764c6c4705ce2cd36781ca8d43f0114
Only made test less racy (https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2337774261) since the first ACK.
---
> > After `reindex`, my node has ~841k headers, a lot lower than the current tip of ~914k.
> > The default `assumevalid` block (height 912,683) is in that missing range, [would say: is outside that range]
>
> I think the behaviour here is probably fine -- it's a weak way of preventing you from triggering assumevalid
...
π¬ vasild commented on pull request "Run feature_bind_port_(discover|externalip).py in CI":
(https://github.com/bitcoin/bitcoin/pull/33362#issuecomment-3279213012)
`69177404ba...f047fce8ab`: fix typo
(https://github.com/bitcoin/bitcoin/pull/33362#issuecomment-3279213012)
`69177404ba...f047fce8ab`: fix typo
π¬ vasild commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r2339623672)
`INTERNET_TRAFFIC_EXPECTED` is a means to turn this into a non-fatal error. For folks that want to run the CI shell scripts outside of CI, in environments where other programs on the same machine create internet traffic. It came from here: https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-2591928909
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r2339623672)
`INTERNET_TRAFFIC_EXPECTED` is a means to turn this into a non-fatal error. For folks that want to run the CI shell scripts outside of CI, in environments where other programs on the same machine create internet traffic. It came from here: https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-2591928909
β οΈ vasild opened an issue: "Early detect non-loopback traffic from unit tests"
(https://github.com/bitcoin/bitcoin/issues/33363)
Tests should not attempt to create network connections over the internet because that has unpredictable timing and outcome. It could also reveal the location (IP address) where somebody is running Bitcoin Core tests, if the target address is very specific, e.g. not `github.com:443`, but `1.2.3.4:8333` for example. See [#31339](https://github.com/bitcoin/bitcoin/issues/31339).
https://github.com/bitcoin/bitcoin/pull/31349 is an attempt to detect such cases in CI. In the [discussions](https://git
...
(https://github.com/bitcoin/bitcoin/issues/33363)
Tests should not attempt to create network connections over the internet because that has unpredictable timing and outcome. It could also reveal the location (IP address) where somebody is running Bitcoin Core tests, if the target address is very specific, e.g. not `github.com:443`, but `1.2.3.4:8333` for example. See [#31339](https://github.com/bitcoin/bitcoin/issues/31339).
https://github.com/bitcoin/bitcoin/pull/31349 is an attempt to detect such cases in CI. In the [discussions](https://git
...
π¬ vasild commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-3279520674)
> Another idea could be to have unit test setup use CreateSock to throw errors ...
I think that is worth exploring. Opened https://github.com/bitcoin/bitcoin/issues/33363 to track it, so that it does not get forgotten.
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-3279520674)
> Another idea could be to have unit test setup use CreateSock to throw errors ...
I think that is worth exploring. Opened https://github.com/bitcoin/bitcoin/issues/33363 to track it, so that it does not get forgotten.
π¬ willcl-ark commented on pull request "ci: disable cirrus cache in 32bit arm job":
(https://github.com/bitcoin/bitcoin/pull/33302#discussion_r2339795470)
Done in ff18b6bbaf3.
LMK what you think of the new approach. I agree it's clearer.
(https://github.com/bitcoin/bitcoin/pull/33302#discussion_r2339795470)
Done in ff18b6bbaf3.
LMK what you think of the new approach. I agree it's clearer.
π¬ willcl-ark commented on pull request "ci: disable cirrus cache in 32bit arm job":
(https://github.com/bitcoin/bitcoin/pull/33302#discussion_r2339799367)
Run on my fork at: https://github.com/willcl-ark/bitcoin/actions/runs/17626151823
(https://github.com/bitcoin/bitcoin/pull/33302#discussion_r2339799367)
Run on my fork at: https://github.com/willcl-ark/bitcoin/actions/runs/17626151823
π fanquake opened a pull request: "ci: always use tag for LLVM checkout"
(https://github.com/bitcoin/bitcoin/pull/33364)
Rather than trying to match the apt installed clang version, which is prone to intermittent issues. i.e #33345.
(https://github.com/bitcoin/bitcoin/pull/33364)
Rather than trying to match the apt installed clang version, which is prone to intermittent issues. i.e #33345.
π¬ Sjors commented on pull request "doc: update multisig tutorial to use multipath descriptors":
(https://github.com/bitcoin/bitcoin/pull/33286#discussion_r2339906587)
It should be built by default, as of a few months ago.
(https://github.com/bitcoin/bitcoin/pull/33286#discussion_r2339906587)
It should be built by default, as of a few months ago.
π willcl-ark opened a pull request: "ci: Skip Centos job on GHA runners"
(https://github.com/bitcoin/bitcoin/pull/33365)
Currently when running on GH free runners, this jobs runs out of space. This does not happen on Cirrus runners in the main repo (bitcoin/bitcoin) but does happen on forks.
Rather than include code to free up space on the runner image, disable this job on forks.
Fixes #33293
(https://github.com/bitcoin/bitcoin/pull/33365)
Currently when running on GH free runners, this jobs runs out of space. This does not happen on Cirrus runners in the main repo (bitcoin/bitcoin) but does happen on forks.
Rather than include code to free up space on the runner image, disable this job on forks.
Fixes #33293
π¬ willcl-ark commented on pull request "ci: Skip Centos job on GHA runners":
(https://github.com/bitcoin/bitcoin/pull/33365#issuecomment-3279713173)
I did trial freeing up space on the runner as suggested by @maflcko in #33293, and it is possible to free up 18GB realtively easily, using e.g. https://github.com/willcl-ark/bitcoin/blob/f5885d05160fe0ac167976f2b36b6d5bec373e08/.github/actions/clear-files/action.yml
But a one-line disable feels much cleaner, and also removes us from maintaing code in this repo, which isn't for our own use.
(https://github.com/bitcoin/bitcoin/pull/33365#issuecomment-3279713173)
I did trial freeing up space on the runner as suggested by @maflcko in #33293, and it is possible to free up 18GB realtively easily, using e.g. https://github.com/willcl-ark/bitcoin/blob/f5885d05160fe0ac167976f2b36b6d5bec373e08/.github/actions/clear-files/action.yml
But a one-line disable feels much cleaner, and also removes us from maintaing code in this repo, which isn't for our own use.
π willcl-ark approved a pull request: "ci: always use tag for LLVM checkout"
(https://github.com/bitcoin/bitcoin/pull/33364#pullrequestreview-3210692825)
ACK b736052e39f1f466f63f261ace3dd2deba171e8a
I did wonder about defining in the *.env files, something like:
```patch
diff --git a/ci/test/00_setup_env_native_fuzz_with_msan.sh b/ci/test/00_setup_env_native_fuzz_with_msan.sh
index c71772b8e2c..47497ed4c81 100755
--- a/ci/test/00_setup_env_native_fuzz_with_msan.sh
+++ b/ci/test/00_setup_env_native_fuzz_with_msan.sh
@@ -26,6 +26,7 @@ export BITCOIN_CONFIG="\
-DAPPEND_CPPFLAGS='-DBOOST_MULTI_INDEX_ENABLE_SAFE_MODE -U_FORTIFY_SOURCE' \
...
(https://github.com/bitcoin/bitcoin/pull/33364#pullrequestreview-3210692825)
ACK b736052e39f1f466f63f261ace3dd2deba171e8a
I did wonder about defining in the *.env files, something like:
```patch
diff --git a/ci/test/00_setup_env_native_fuzz_with_msan.sh b/ci/test/00_setup_env_native_fuzz_with_msan.sh
index c71772b8e2c..47497ed4c81 100755
--- a/ci/test/00_setup_env_native_fuzz_with_msan.sh
+++ b/ci/test/00_setup_env_native_fuzz_with_msan.sh
@@ -26,6 +26,7 @@ export BITCOIN_CONFIG="\
-DAPPEND_CPPFLAGS='-DBOOST_MULTI_INDEX_ENABLE_SAFE_MODE -U_FORTIFY_SOURCE' \
...