💬 instagibbs commented on pull request "test: Fix reorg patterns in tests to use proper fork-based approach":
(https://github.com/bitcoin/bitcoin/pull/32587#issuecomment-3083794957)
reACK a161f058089dfe5f3aa7d3b9635f0400f0147b5d
spelling fix
(https://github.com/bitcoin/bitcoin/pull/32587#issuecomment-3083794957)
reACK a161f058089dfe5f3aa7d3b9635f0400f0147b5d
spelling fix
💬 instagibbs commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3083796940)
> Edit: Oh nvm, the other GetSigOpCount() recurses a P2SH script.
I wouldn't mind a comment spelling out what it's doing, it is a peculiar function
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3083796940)
> Edit: Oh nvm, the other GetSigOpCount() recurses a P2SH script.
I wouldn't mind a comment spelling out what it's doing, it is a peculiar function
💬 instagibbs commented on pull request "validation: docs and cleanups for MemPoolAccept coins views":
(https://github.com/bitcoin/bitcoin/pull/32973#issuecomment-3083835263)
reACK https://github.com/bitcoin/bitcoin/pull/32973/commits/b6d4688f77df9e31fd64e2be300f55bb8e944bd0
(https://github.com/bitcoin/bitcoin/pull/32973#issuecomment-3083835263)
reACK https://github.com/bitcoin/bitcoin/pull/32973/commits/b6d4688f77df9e31fd64e2be300f55bb8e944bd0
🤔 hebasto reviewed a pull request: "Migrate CI to hosted Cirrus Runners"
(https://github.com/bitcoin/bitcoin/pull/32989#pullrequestreview-3029285837)
Concept ACK.
> we qualify for an open source discount of 50%.
> We would be dependent on Cirrus infra...
We shouldn't be [surprised](https://cirrus-ci.org/blog/2023/07/17/limiting-free-usage-of-cirrus-ci/) when Cirrus suddenly changes its modus operandi, including its advertised open-source discount or general availability.
(https://github.com/bitcoin/bitcoin/pull/32989#pullrequestreview-3029285837)
Concept ACK.
> we qualify for an open source discount of 50%.
> We would be dependent on Cirrus infra...
We shouldn't be [surprised](https://cirrus-ci.org/blog/2023/07/17/limiting-free-usage-of-cirrus-ci/) when Cirrus suddenly changes its modus operandi, including its advertised open-source discount or general availability.
📝 maflcko opened a pull request: "ci: Only pass documented env vars"
(https://github.com/bitcoin/bitcoin/pull/33002)
The CI currently inherits almost all env vars from the host. This was problematic in the past and causing non-determinism, e.g. the fix in commit fa12558d21aa03c22407a1458a0345d8a7ab6a4b. It is still problematic today, see e.g. https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-2586828644, or https://github.com/bitcoin/bitcoin/issues/32935
This fixes https://github.com/bitcoin/bitcoin/issues/32935 by only passing env vars documented in `./ci/test/00_setup_env.sh`.
Implementation-wi
...
(https://github.com/bitcoin/bitcoin/pull/33002)
The CI currently inherits almost all env vars from the host. This was problematic in the past and causing non-determinism, e.g. the fix in commit fa12558d21aa03c22407a1458a0345d8a7ab6a4b. It is still problematic today, see e.g. https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-2586828644, or https://github.com/bitcoin/bitcoin/issues/32935
This fixes https://github.com/bitcoin/bitcoin/issues/32935 by only passing env vars documented in `./ci/test/00_setup_env.sh`.
Implementation-wi
...
💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3083905684)
> Concept ACK.
>
> > we qualify for an open source discount of 50%.
>
> > We would be dependent on Cirrus infra...
>
> We shouldn't be [surprised](https://cirrus-ci.org/blog/2023/07/17/limiting-free-usage-of-cirrus-ci/) when Cirrus suddenly changes its modus operandi, including its advertised open-source discount or general availability.
Certainly, it is good to be wary of that. I think this is equally true for all cloud providers though.
It's my belief that if we complete this mi
...
(https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3083905684)
> Concept ACK.
>
> > we qualify for an open source discount of 50%.
>
> > We would be dependent on Cirrus infra...
>
> We shouldn't be [surprised](https://cirrus-ci.org/blog/2023/07/17/limiting-free-usage-of-cirrus-ci/) when Cirrus suddenly changes its modus operandi, including its advertised open-source discount or general availability.
Certainly, it is good to be wary of that. I think this is equally true for all cloud providers though.
It's my belief that if we complete this mi
...
💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3083928304)
> 2\. We could always revert back to a self-hosted solution
Agree that this may happen with any third party (including GitHub itself). If we want to switch back to the self-hosted runners, it should be as trivial as `git revert $the_merge_commit_of_this_pull`. Alternatively, switch to GHA-based self-hosted runners. Though, it would be good to create a proof-of-concept pull request to switch to self-hosted runners in GHA, or a different hosted alternative (e.g. warp-build), after this pull is
...
(https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3083928304)
> 2\. We could always revert back to a self-hosted solution
Agree that this may happen with any third party (including GitHub itself). If we want to switch back to the self-hosted runners, it should be as trivial as `git revert $the_merge_commit_of_this_pull`. Alternatively, switch to GHA-based self-hosted runners. Though, it would be good to create a proof-of-concept pull request to switch to self-hosted runners in GHA, or a different hosted alternative (e.g. warp-build), after this pull is
...
💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2213251672)
Ugh good catch.
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2213251672)
Ugh good catch.
💬 dergoegge commented on pull request "fuzz: Make process_message(s) more deterministic":
(https://github.com/bitcoin/bitcoin/pull/32822#issuecomment-3083944969)
This needs a rebase, but should be good to go otherwise.
(https://github.com/bitcoin/bitcoin/pull/32822#issuecomment-3083944969)
This needs a rebase, but should be good to go otherwise.
🤔 marcofleon reviewed a pull request: "validation: docs and cleanups for MemPoolAccept coins views"
(https://github.com/bitcoin/bitcoin/pull/32973#pullrequestreview-3029414103)
ACK b6d4688f77df9e31fd64e2be300f55bb8e944bd0
Verified that the two functional tests fail with the suggested buggy patches. Seems fine to remove this test if it doesn't provide any new assurances.
> I think there is a low likelihood of accidentally reintroducing this problem and not immediately hitting e.g. a fuzzer crash on this line
I also tried to see if we'd get a fuzz crash after applying the first patch and it doesn't seem to be happening. I'm running `tx_package_eval` with a small
...
(https://github.com/bitcoin/bitcoin/pull/32973#pullrequestreview-3029414103)
ACK b6d4688f77df9e31fd64e2be300f55bb8e944bd0
Verified that the two functional tests fail with the suggested buggy patches. Seems fine to remove this test if it doesn't provide any new assurances.
> I think there is a low likelihood of accidentally reintroducing this problem and not immediately hitting e.g. a fuzzer crash on this line
I also tried to see if we'd get a fuzz crash after applying the first patch and it doesn't seem to be happening. I'm running `tx_package_eval` with a small
...
💬 josibake commented on issue "ci: improve "test each commit" job to handle more complex scenarios":
(https://github.com/bitcoin/bitcoin/issues/32991#issuecomment-3083977467)
> you'll want to do `git rebase --interactive c42c7351a6316805fdfb20163762c1ab8293dd0d` and then delete the old subtree commits
Apologies if I'm being dense, but what you're suggesting doesn't make sense to me. What I'm doing currently:
```shell
# starting from master
git checkout -b refresh-secp256k1
# git subtree pull --prefix src/secp256k1/ <myfork> <mybranch> --squash
git subtree pull --prefix src/secp256k1/ josie-secp256k1 bip352-silentpayments-module-2025 --squash
# rebase my bitcoin c
...
(https://github.com/bitcoin/bitcoin/issues/32991#issuecomment-3083977467)
> you'll want to do `git rebase --interactive c42c7351a6316805fdfb20163762c1ab8293dd0d` and then delete the old subtree commits
Apologies if I'm being dense, but what you're suggesting doesn't make sense to me. What I'm doing currently:
```shell
# starting from master
git checkout -b refresh-secp256k1
# git subtree pull --prefix src/secp256k1/ <myfork> <mybranch> --squash
git subtree pull --prefix src/secp256k1/ josie-secp256k1 bip352-silentpayments-module-2025 --squash
# rebase my bitcoin c
...
💬 glozow commented on pull request "validation: docs and cleanups for MemPoolAccept coins views":
(https://github.com/bitcoin/bitcoin/pull/32973#issuecomment-3083993965)
> Fwiw if I add LimitMempoolSize() right before ClearSubPackageState() in AcceptSubPackage there is an almost immediate crash.
That's the kind of scenario I was thinking - somehow reintroducing a trim somewhere mid-subpackage.
(https://github.com/bitcoin/bitcoin/pull/32973#issuecomment-3083993965)
> Fwiw if I add LimitMempoolSize() right before ClearSubPackageState() in AcceptSubPackage there is an almost immediate crash.
That's the kind of scenario I was thinking - somehow reintroducing a trim somewhere mid-subpackage.
💬 glozow commented on pull request "validation: docs and cleanups for MemPoolAccept coins views":
(https://github.com/bitcoin/bitcoin/pull/32973#discussion_r2213299485)
Will change if I need to retouch
(https://github.com/bitcoin/bitcoin/pull/32973#discussion_r2213299485)
Will change if I need to retouch
💬 hebasto commented on pull request "Bump SCRIPT_VERIFY flags to 64 bit":
(https://github.com/bitcoin/bitcoin/pull/32998#issuecomment-3084054356)
> CI failure seems to be due to [a bug in qt6 6.4](https://bugreports.qt.io/browse/QTBUG-31496?focusedId=888930&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-888930):
Although both failed CI job use Clang 20.1.7, the error can also be reproduced with GCC 13.3.
(https://github.com/bitcoin/bitcoin/pull/32998#issuecomment-3084054356)
> CI failure seems to be due to [a bug in qt6 6.4](https://bugreports.qt.io/browse/QTBUG-31496?focusedId=888930&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-888930):
Although both failed CI job use Clang 20.1.7, the error can also be reproduced with GCC 13.3.
🤔 yuvicc reviewed a pull request: "validation: remove BLOCK_FAILED_CHILD"
(https://github.com/bitcoin/bitcoin/pull/32950#pullrequestreview-3029521987)
Concept ACK
The CI failure looks like due to #32855
(https://github.com/bitcoin/bitcoin/pull/32950#pullrequestreview-3029521987)
Concept ACK
The CI failure looks like due to #32855
💬 maflcko commented on issue "ci: improve "test each commit" job to handle more complex scenarios":
(https://github.com/bitcoin/bitcoin/issues/32991#issuecomment-3084061052)
> git rebase --onto refresh-secp256k1 702eb96c46904288f72f02f968339c27c6524195^ implement-bip352
Yeah, that looks correct. It should also be identical to my version. The only difference is that yours works also non-iteractively. That is, the two commands give the same result:
```
git rebase --interactive --onto=14224fd2f3b31ed869397989d88306df2a3cfe6f da3494063f^ 8c073407aa # no interactive changes needed
```
```
git checkout 8c073407aa && git rebase --interactive 14224fd2f3b31ed869397989d88
...
(https://github.com/bitcoin/bitcoin/issues/32991#issuecomment-3084061052)
> git rebase --onto refresh-secp256k1 702eb96c46904288f72f02f968339c27c6524195^ implement-bip352
Yeah, that looks correct. It should also be identical to my version. The only difference is that yours works also non-iteractively. That is, the two commands give the same result:
```
git rebase --interactive --onto=14224fd2f3b31ed869397989d88306df2a3cfe6f da3494063f^ 8c073407aa # no interactive changes needed
```
```
git checkout 8c073407aa && git rebase --interactive 14224fd2f3b31ed869397989d88
...
💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2213349114)
Fixed. Added the check in three instances where P2SH inputs are added.
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2213349114)
Fixed. Added the check in three instances where P2SH inputs are added.
💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3084072448)
> > Edit: Oh nvm, the other GetSigOpCount() recurses a P2SH script.
>
> I wouldn't mind a comment spelling out what it's doing, it is a peculiar function
Added a comment to this effect on the call site.
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3084072448)
> > Edit: Oh nvm, the other GetSigOpCount() recurses a P2SH script.
>
> I wouldn't mind a comment spelling out what it's doing, it is a peculiar function
Added a comment to this effect on the call site.
💬 instagibbs commented on pull request "Bump SCRIPT_VERIFY flags to 64 bit":
(https://github.com/bitcoin/bitcoin/pull/32998#issuecomment-3084085825)
agreed, build issue also on my end with gcc 13.3.0
(https://github.com/bitcoin/bitcoin/pull/32998#issuecomment-3084085825)
agreed, build issue also on my end with gcc 13.3.0
💬 instagibbs commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3084094225)
reACK https://github.com/bitcoin/bitcoin/pull/32521/commits/96da68a38fa295d2414685739c41b8626e198d27
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3084094225)
reACK https://github.com/bitcoin/bitcoin/pull/32521/commits/96da68a38fa295d2414685739c41b8626e198d27