💬 rkrux commented on pull request "wallet: Set migrated wallet name only on success":
(https://github.com/bitcoin/bitcoin/pull/32984#discussion_r2213133583)
In 060695c22ae7b2b0f2a1dd1417ed1b9d5a5ab542 "test: Failed load after migrate should restore backup"
Adding a review note because it took me some time to understand why this error pops up only after migration is complete and not during the process as this particular error comes from inside `CWallet::AttachChain` that is a part of `CWallet::Create`. The reason I found is that all the `CWallet::Create` calls inside the migration process pass an empty context that doesn't contain the `chain` due
...
(https://github.com/bitcoin/bitcoin/pull/32984#discussion_r2213133583)
In 060695c22ae7b2b0f2a1dd1417ed1b9d5a5ab542 "test: Failed load after migrate should restore backup"
Adding a review note because it took me some time to understand why this error pops up only after migration is complete and not during the process as this particular error comes from inside `CWallet::AttachChain` that is a part of `CWallet::Create`. The reason I found is that all the `CWallet::Create` calls inside the migration process pass an empty context that doesn't contain the `chain` due
...
💬 rkrux commented on pull request "test: refactor: overhaul block hash determination for `CBlock{,Header}` objects":
(https://github.com/bitcoin/bitcoin/pull/32868#issuecomment-3083793468)
re-ACK 5fa34951ead2eebcced919537f5e27526f61d909 modulo failing CI due to silent merge conflict.
(https://github.com/bitcoin/bitcoin/pull/32868#issuecomment-3083793468)
re-ACK 5fa34951ead2eebcced919537f5e27526f61d909 modulo failing CI due to silent merge conflict.
💬 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.