💬 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' \
...
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3280041954)
`4327327dd0...3f6a61e3c5`: rebase due to conflicts and take https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2328909174
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3280041954)
`4327327dd0...3f6a61e3c5`: rebase due to conflicts and take https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2328909174
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2340272853)
Done.
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2340272853)
Done.
💬 willcl-ark commented on pull request "Run feature_bind_port_(discover|externalip).py in CI":
(https://github.com/bitcoin/bitcoin/pull/33362#discussion_r2340540732)
What is the effect on a developer running CI locally on their machine, of adding NET_ADMIN capability? Is is possible for the CI to then modify/break the developer machine networking?
CAP_NET_ADMIN
Perform various network-related operations:
• interface configuration;
• administration of IP firewall, masquerading, and
accounting;
• modify routing tables;
• bind to any address for transparen
...
(https://github.com/bitcoin/bitcoin/pull/33362#discussion_r2340540732)
What is the effect on a developer running CI locally on their machine, of adding NET_ADMIN capability? Is is possible for the CI to then modify/break the developer machine networking?
CAP_NET_ADMIN
Perform various network-related operations:
• interface configuration;
• administration of IP firewall, masquerading, and
accounting;
• modify routing tables;
• bind to any address for transparen
...
💬 vasild commented on pull request "Run feature_bind_port_(discover|externalip).py in CI":
(https://github.com/bitcoin/bitcoin/pull/33362#discussion_r2340774782)
The privileges only apply to the guest, not to host. The guest ("container" in docker language) has no way to manipulate the host's networking.
https://docs.docker.com/engine/containers/run/#container-networking
https://docs.docker.com/engine/network/
> We use our own `--network` in CI ...
Hmm. Maybe that could be an easier way to add the dummy routable addresses. Then they will be available for the whole duration of the entire CI run, not only during the functional tests as in this PR
...
(https://github.com/bitcoin/bitcoin/pull/33362#discussion_r2340774782)
The privileges only apply to the guest, not to host. The guest ("container" in docker language) has no way to manipulate the host's networking.
https://docs.docker.com/engine/containers/run/#container-networking
https://docs.docker.com/engine/network/
> We use our own `--network` in CI ...
Hmm. Maybe that could be an easier way to add the dummy routable addresses. Then they will be available for the whole duration of the entire CI run, not only during the functional tests as in this PR
...
💬 instagibbs commented on pull request "txgraph: randomize order of same-feerate distinct-cluster transactions":
(https://github.com/bitcoin/bitcoin/pull/33335#discussion_r2340792959)
double checking this will happen IFF there's as siphash collision, i.e., on average every ~2^32 sequence comparisons?
(https://github.com/bitcoin/bitcoin/pull/33335#discussion_r2340792959)
double checking this will happen IFF there's as siphash collision, i.e., on average every ~2^32 sequence comparisons?
💬 instagibbs commented on pull request "txgraph: randomize order of same-feerate distinct-cluster transactions":
(https://github.com/bitcoin/bitcoin/pull/33335#discussion_r2340812332)
eyeballing this, I don't see this being initialized?
(https://github.com/bitcoin/bitcoin/pull/33335#discussion_r2340812332)
eyeballing this, I don't see this being initialized?
💬 instagibbs commented on pull request "txgraph: randomize order of same-feerate distinct-cluster transactions":
(https://github.com/bitcoin/bitcoin/pull/33335#issuecomment-3280624138)
concept ACK, I also think it removes the kinda weird behavior where same CFR clusters get mined/evicted based on the underlying cluster's original sequence value
(https://github.com/bitcoin/bitcoin/pull/33335#issuecomment-3280624138)
concept ACK, I also think it removes the kinda weird behavior where same CFR clusters get mined/evicted based on the underlying cluster's original sequence value