💬 TheCharlatan commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#issuecomment-2934750745)
Rebased 15ff9cb1789b7fe69c0ad24d737712aa6955e9cb -> df0eb65bd98f300618cccd667720f1ccc6145865 ([spendblock_4](https://github.com/TheCharlatan/bitcoin/tree/spendblock_4) -> [spendblock_5](https://github.com/TheCharlatan/bitcoin/tree/spendblock_5), [compare](https://github.com/TheCharlatan/bitcoin/compare/spendblock_4..spendblock_5))
* Fixed conflict with #32644
(https://github.com/bitcoin/bitcoin/pull/32317#issuecomment-2934750745)
Rebased 15ff9cb1789b7fe69c0ad24d737712aa6955e9cb -> df0eb65bd98f300618cccd667720f1ccc6145865 ([spendblock_4](https://github.com/TheCharlatan/bitcoin/tree/spendblock_4) -> [spendblock_5](https://github.com/TheCharlatan/bitcoin/tree/spendblock_5), [compare](https://github.com/TheCharlatan/bitcoin/compare/spendblock_4..spendblock_5))
* Fixed conflict with #32644
💬 hebasto commented on issue "windows: depends config fails":
(https://github.com/bitcoin/bitcoin/issues/32578#issuecomment-2934759550)
You could:
1. Check the free disk space on your machine.
2. Check whether the issue is resolved as described in https://github.com/bitcoin/bitcoin/blob/master/doc/build-windows-msvc.md#4-building-with-static-linking-with-gui
As a last resort, please report the issue [upstream](https://github.com/microsoft/vcpkg).
(https://github.com/bitcoin/bitcoin/issues/32578#issuecomment-2934759550)
You could:
1. Check the free disk space on your machine.
2. Check whether the issue is resolved as described in https://github.com/bitcoin/bitcoin/blob/master/doc/build-windows-msvc.md#4-building-with-static-linking-with-gui
As a last resort, please report the issue [upstream](https://github.com/microsoft/vcpkg).
📝 Jokacar10 opened a pull request: "b9de2eb "
(https://github.com/bitcoin/bitcoin/pull/32671)
b9de2eb
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests
...
(https://github.com/bitcoin/bitcoin/pull/32671)
b9de2eb
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests
...
✅ fanquake closed a pull request: "b9de2eb"
(https://github.com/bitcoin/bitcoin/pull/32671)
(https://github.com/bitcoin/bitcoin/pull/32671)
📝 m3dwards opened a pull request: "ci: update pwsh to use custom shell that fails-fast"
(https://github.com/bitcoin/bitcoin/pull/32672)
Github by default sets fail fast behaviour on pswh shell which means that if any powershell cmdlet fails the script will stop and exit. The problem is that this behaviour doesn't apply when calling native executables, it only applies to powershell cmdlets.
I think the safest thing is to whenever we use pwsh to enable `$PSNativeCommandUseErrorActionPreference = $true` which will also fail calling any exe that returns a non-zero exit code.
Technically the step `Adjust paths in test/config.in
...
(https://github.com/bitcoin/bitcoin/pull/32672)
Github by default sets fail fast behaviour on pswh shell which means that if any powershell cmdlet fails the script will stop and exit. The problem is that this behaviour doesn't apply when calling native executables, it only applies to powershell cmdlets.
I think the safest thing is to whenever we use pwsh to enable `$PSNativeCommandUseErrorActionPreference = $true` which will also fail calling any exe that returns a non-zero exit code.
Technically the step `Adjust paths in test/config.in
...
💬 m3dwards commented on pull request "ci: remove 3rd party js from windows dll gha job":
(https://github.com/bitcoin/bitcoin/pull/32513#discussion_r2123614842)
This PR enables failing fast for executables in pwsh:
https://github.com/bitcoin/bitcoin/pull/32672
(https://github.com/bitcoin/bitcoin/pull/32513#discussion_r2123614842)
This PR enables failing fast for executables in pwsh:
https://github.com/bitcoin/bitcoin/pull/32672
💬 i-am-yuvi commented on pull request "depends: Bump boost to 1.88.0 and use new CMake buildsystem":
(https://github.com/bitcoin/bitcoin/pull/32665#issuecomment-2934979547)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/32665#issuecomment-2934979547)
Concept ACK
💬 ryanofsky commented on pull request "thread-safety: fix annotations with REVERSE_LOCK":
(https://github.com/bitcoin/bitcoin/pull/32465#issuecomment-2935063889)
re: theuni https://github.com/bitcoin/bitcoin/pull/32465#issuecomment-2923398293
> For context, my primary motivation here is preparing for the follow-up commits that will actually remove the `lock`/`unlock` methods completely, in order to force the exception-safe RAII wrappers. See these two commits: [theuni@d4b0918](https://github.com/theuni/bitcoin/commit/d4b09184ae8ce38862a758423653f606e8b96045) and [theuni@1f2a9af](https://github.com/theuni/bitcoin/commit/1f2a9afdb302dd3f1686fe3461b9d791
...
(https://github.com/bitcoin/bitcoin/pull/32465#issuecomment-2935063889)
re: theuni https://github.com/bitcoin/bitcoin/pull/32465#issuecomment-2923398293
> For context, my primary motivation here is preparing for the follow-up commits that will actually remove the `lock`/`unlock` methods completely, in order to force the exception-safe RAII wrappers. See these two commits: [theuni@d4b0918](https://github.com/theuni/bitcoin/commit/d4b09184ae8ce38862a758423653f606e8b96045) and [theuni@1f2a9af](https://github.com/theuni/bitcoin/commit/1f2a9afdb302dd3f1686fe3461b9d791
...
💬 m3dwards commented on pull request "ci: remove 3rd party js from windows dll gha job":
(https://github.com/bitcoin/bitcoin/pull/32513#discussion_r2123711340)
@davidgumberg force pushed a change so that it behaves how you expected it to: https://github.com/bitcoin/bitcoin/compare/a453e66c827f260da597bf70273bcd0da2313774..7f8393ad745e6bc44dc9481428ea5979ed31a3c9
(https://github.com/bitcoin/bitcoin/pull/32513#discussion_r2123711340)
@davidgumberg force pushed a change so that it behaves how you expected it to: https://github.com/bitcoin/bitcoin/compare/a453e66c827f260da597bf70273bcd0da2313774..7f8393ad745e6bc44dc9481428ea5979ed31a3c9
🤔 hodlinator reviewed a pull request: "test: enabling wallet migration functional test on windows"
(https://github.com/bitcoin/bitcoin/pull/32219#pullrequestreview-2891238265)
Concept ACK 12248ee08026263b70b2a0e6857552c475614207
(https://github.com/bitcoin/bitcoin/pull/32219#pullrequestreview-2891238265)
Concept ACK 12248ee08026263b70b2a0e6857552c475614207
💬 hodlinator commented on pull request "test: enabling wallet migration functional test on windows":
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2123683812)
Regarding removing this support for building from source, I'm not sure about doing that. Yes, we use CMake for newer releases, but the older ones use Auto-tools, no?
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2123683812)
Regarding removing this support for building from source, I'm not sure about doing that. Yes, we use CMake for newer releases, but the older ones use Auto-tools, no?
💬 hodlinator commented on pull request "test: enabling wallet migration functional test on windows":
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2123124268)
nit: Why rename `platform` to `host`? Seems better to keep them separate to avoid conflating with args.host, or at least leave a small motivation in the commit message.
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2123124268)
nit: Why rename `platform` to `host`? Seems better to keep them separate to avoid conflating with args.host, or at least leave a small motivation in the commit message.
💬 hodlinator commented on pull request "test: enabling wallet migration functional test on windows":
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2123092808)
nit: Could print to stderr here and elsewhere for touched lines with error messages.
```suggestion
print(f"Failed to extract the {tag} tarball", file=sys.stderr)
```
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2123092808)
nit: Could print to stderr here and elsewhere for touched lines with error messages.
```suggestion
print(f"Failed to extract the {tag} tarball", file=sys.stderr)
```
💬 hodlinator commented on pull request "test: enabling wallet migration functional test on windows":
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2123700602)
nit: Could use more modern f-strings:
```suggestion
archive = f'bitcoin-{tag[1:]}-{host}.{archive_format}'
archiveUrl = f'https://bitcoincore.org/{bin_path}/{archive}'
print(f'Fetching: {archiveUrl}')
```
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2123700602)
nit: Could use more modern f-strings:
```suggestion
archive = f'bitcoin-{tag[1:]}-{host}.{archive_format}'
archiveUrl = f'https://bitcoincore.org/{bin_path}/{archive}'
print(f'Fetching: {archiveUrl}')
```
🤔 rkrux reviewed a pull request: "wallet, rpc: Return normalized descriptor in parent_descs"
(https://github.com/bitcoin/bitcoin/pull/32594#pullrequestreview-2892155604)
ACK 0def84d407facd319b52826d013cad0d5fc8dbf5
Apparently I didn't have the fdabfc7b01ec2cff5f2356e1b13b5537d30dbaf8 commit in my local, maybe I forgot to check it out earlier.
```bash
git range-diff a759a22...0def84d
```
Good job replacing the loop in the test with the regex.
(https://github.com/bitcoin/bitcoin/pull/32594#pullrequestreview-2892155604)
ACK 0def84d407facd319b52826d013cad0d5fc8dbf5
Apparently I didn't have the fdabfc7b01ec2cff5f2356e1b13b5537d30dbaf8 commit in my local, maybe I forgot to check it out earlier.
```bash
git range-diff a759a22...0def84d
```
Good job replacing the loop in the test with the regex.
💬 rkrux commented on pull request "wallet, rpc: Return normalized descriptor in parent_descs":
(https://github.com/bitcoin/bitcoin/pull/32594#discussion_r2123684230)
Nit for the comment: This _removes_ the xpub by substituting it with emptiness, the resultant `desc_verify` contains everything in the descriptor but the xpub.
(https://github.com/bitcoin/bitcoin/pull/32594#discussion_r2123684230)
Nit for the comment: This _removes_ the xpub by substituting it with emptiness, the resultant `desc_verify` contains everything in the descriptor but the xpub.
💬 rkrux commented on pull request "wallet, rpc: Return normalized descriptor in parent_descs":
(https://github.com/bitcoin/bitcoin/pull/32594#discussion_r2123692569)
```diff
- aspostrophe
+ apostrophe
```
(https://github.com/bitcoin/bitcoin/pull/32594#discussion_r2123692569)
```diff
- aspostrophe
+ apostrophe
```
💬 rkrux commented on pull request "wallet, rpc: Return normalized descriptor in parent_descs":
(https://github.com/bitcoin/bitcoin/pull/32594#discussion_r2123691895)
I checked that this Regex is fine because the origin fingerprint can only be a hex string `(\da-f)` and the `origin_part` is found even if the `desc_verify` doesn't contain hardened marker such as in `wpkh([58d258be/84/1/0]/0/*`. That's why I believe the subsequent `"h" in origin_part` check is required.
(https://github.com/bitcoin/bitcoin/pull/32594#discussion_r2123691895)
I checked that this Regex is fine because the origin fingerprint can only be a hex string `(\da-f)` and the `origin_part` is found even if the `desc_verify` doesn't contain hardened marker such as in `wpkh([58d258be/84/1/0]/0/*`. That's why I believe the subsequent `"h" in origin_part` check is required.
💬 rkrux commented on pull request "wallet, rpc: Return normalized descriptor in parent_descs":
(https://github.com/bitcoin/bitcoin/pull/32594#discussion_r2123713913)
`r"tpub\w+?(?=/)"`
The `\w` will accept an xpub with underscore as well that is not allowed, but ok.
(https://github.com/bitcoin/bitcoin/pull/32594#discussion_r2123713913)
`r"tpub\w+?(?=/)"`
The `\w` will accept an xpub with underscore as well that is not allowed, but ok.
👍 willcl-ark approved a pull request: "depends: Bump boost to 1.88.0 and use new CMake buildsystem"
(https://github.com/bitcoin/bitcoin/pull/32665#pullrequestreview-2892263099)
ACK 3a350c8a1b51e140e2689e10dca338470e8deef2
Tested that this works. I additionally tested rebasing #32595 on it, where it also works well.
(https://github.com/bitcoin/bitcoin/pull/32665#pullrequestreview-2892263099)
ACK 3a350c8a1b51e140e2689e10dca338470e8deef2
Tested that this works. I additionally tested rebasing #32595 on it, where it also works well.