💬 Sjors commented on pull request "test: avoid disk space warning for non-regtest":
(https://github.com/bitcoin/bitcoin/pull/32057#issuecomment-2729404900)
@rkrux updated PR description
(https://github.com/bitcoin/bitcoin/pull/32057#issuecomment-2729404900)
@rkrux updated PR description
🤔 hebasto reviewed a pull request: "build: Drop option to disable hardening."
(https://github.com/bitcoin/bitcoin/pull/32071#pullrequestreview-2690444929)
Approach ACK ecf2046d4b5c43ddf64f62f09cd3ed70dd5caafb.
The only case I'm aware of where some hardening flags cause an issue is on NetBSD 10, and this is expected to be fixed in NetBSD 11.
For more details, see:
- https://github.com/bitcoin/bitcoin/pull/28724#issuecomment-2585683053
- https://github.com/hebasto/bitcoin-core-nightly/pull/30
(https://github.com/bitcoin/bitcoin/pull/32071#pullrequestreview-2690444929)
Approach ACK ecf2046d4b5c43ddf64f62f09cd3ed70dd5caafb.
The only case I'm aware of where some hardening flags cause an issue is on NetBSD 10, and this is expected to be fixed in NetBSD 11.
For more details, see:
- https://github.com/bitcoin/bitcoin/pull/28724#issuecomment-2585683053
- https://github.com/hebasto/bitcoin-core-nightly/pull/30
✅ l0rinc closed a pull request: "doc: use `libfuzzer-nosan` for macOS"
(https://github.com/bitcoin/bitcoin/pull/32084)
(https://github.com/bitcoin/bitcoin/pull/32084)
💬 l0rinc commented on pull request "doc: use `libfuzzer-nosan` for macOS":
(https://github.com/bitcoin/bitcoin/pull/32084#issuecomment-2729481654)
Thanks for the hint @brunoerg, I did a bisect to see if it's the commits or the update, which gave me contradictory results.
Clearing all build caches however fixed it for master as well:
```bash
ccache -C && git clean -fxd && git reset --hard
```
Guess we can't rely on cache invalidation here.
(https://github.com/bitcoin/bitcoin/pull/32084#issuecomment-2729481654)
Thanks for the hint @brunoerg, I did a bisect to see if it's the commits or the update, which gave me contradictory results.
Clearing all build caches however fixed it for master as well:
```bash
ccache -C && git clean -fxd && git reset --hard
```
Guess we can't rely on cache invalidation here.
💬 hebasto commented on pull request "Shuffle depends instructions and recommend modern make for macOS":
(https://github.com/bitcoin/bitcoin/pull/32086#issuecomment-2729483431)
> Tell users / developers who use the depends system to install a modern version of `make`.
Concept ACK on that.
(https://github.com/bitcoin/bitcoin/pull/32086#issuecomment-2729483431)
> Tell users / developers who use the depends system to install a modern version of `make`.
Concept ACK on that.
💬 rkrux commented on pull request "docs: remove CI passing requirement for PR merging":
(https://github.com/bitcoin/bitcoin/pull/32006#discussion_r1998727102)
IMO, this section should be kept by rewording it a bit as suggested in this comment https://github.com/bitcoin/bitcoin/pull/32006#discussion_r1998121846. Though it is common sense now to ensure a passing CI, there could be new contributors who might not be aware of it and this section is something that can be shared with them if needed. Also, I've seen few instances of CI (beyond this repo) where the CI checks are not required to be passing for the PR to be merged, having this section would set
...
(https://github.com/bitcoin/bitcoin/pull/32006#discussion_r1998727102)
IMO, this section should be kept by rewording it a bit as suggested in this comment https://github.com/bitcoin/bitcoin/pull/32006#discussion_r1998121846. Though it is common sense now to ensure a passing CI, there could be new contributors who might not be aware of it and this section is something that can be shared with them if needed. Also, I've seen few instances of CI (beyond this repo) where the CI checks are not required to be passing for the PR to be merged, having this section would set
...
💬 hebasto commented on pull request "doc: use `libfuzzer-nosan` for macOS":
(https://github.com/bitcoin/bitcoin/pull/32084#issuecomment-2729530235)
> Thanks for the hint @brunoerg, I did a bisect to see if it's the commits or the update, which gave me contradictory results. Clearing all build caches however fixed it for master as well:
>
> ```shell
> ccache -C && git clean -fxd && git reset --hard
> ```
>
> Guess we can't rely on cache invalidation here.
I'd appreciate it if you could provide steps to reproduce the scenario where Ccache cache causes a build issue.
(https://github.com/bitcoin/bitcoin/pull/32084#issuecomment-2729530235)
> Thanks for the hint @brunoerg, I did a bisect to see if it's the commits or the update, which gave me contradictory results. Clearing all build caches however fixed it for master as well:
>
> ```shell
> ccache -C && git clean -fxd && git reset --hard
> ```
>
> Guess we can't rely on cache invalidation here.
I'd appreciate it if you could provide steps to reproduce the scenario where Ccache cache causes a build issue.
💬 l0rinc commented on pull request "doc: use `libfuzzer-nosan` for macOS":
(https://github.com/bitcoin/bitcoin/pull/32084#issuecomment-2729550053)
I can't, I deleted everything as the above steps show.
But it was likely the local build folder and not ccache (based on how git bisect behaved, i.e. after the first build passed, everything did).
(https://github.com/bitcoin/bitcoin/pull/32084#issuecomment-2729550053)
I can't, I deleted everything as the above steps show.
But it was likely the local build folder and not ccache (based on how git bisect behaved, i.e. after the first build passed, everything did).
💬 ismaelsadeeq commented on pull request "wallet, rpc: deprecate settxfee and paytxfee":
(https://github.com/bitcoin/bitcoin/pull/31278#issuecomment-2729551466)
re-ACK 2f2ab47bf74f4da37aad75a186cb0bb16e8af579
(https://github.com/bitcoin/bitcoin/pull/31278#issuecomment-2729551466)
re-ACK 2f2ab47bf74f4da37aad75a186cb0bb16e8af579
🤔 ismaelsadeeq reviewed a pull request: "test: Add test coverage for rpcwhitelistdefault when unset"
(https://github.com/bitcoin/bitcoin/pull/32079#pullrequestreview-2690555367)
Concept ACK, nice follow-up!
Why is it in draft?
(https://github.com/bitcoin/bitcoin/pull/32079#pullrequestreview-2690555367)
Concept ACK, nice follow-up!
Why is it in draft?
🚀 hebasto merged a pull request: "test: Check datadir cleanup after assumeutxo was successful"
(https://github.com/bitcoin/bitcoin/pull/32033)
(https://github.com/bitcoin/bitcoin/pull/32033)
💬 Prabhat1308 commented on pull request "fuzz: Speed up *_package_eval fuzz targets a bit":
(https://github.com/bitcoin/bitcoin/pull/31457#issuecomment-2729581927)
> Is this a correct way to measure the speed difference (on a Mac)?
>
> ```shell
> git clone --depth=1 https://github.com/bitcoin-core/qa-assets
> cmake --preset=libfuzzer\
> -DCMAKE_C_COMPILER="$(brew --prefix llvm)/bin/clang" \
> -DCMAKE_CXX_COMPILER="$(brew --prefix llvm)/bin/clang++" \
> -DCMAKE_EXE_LINKER_FLAGS="-fuse-ld=lld"
> cmake --build build_fuzz
> time FUZZ=tx_package_eval build_fuzz/bin/fuzz qa-assets/fuzz_corpora/tx_package_eval
> ```
Running this I run into
...
(https://github.com/bitcoin/bitcoin/pull/31457#issuecomment-2729581927)
> Is this a correct way to measure the speed difference (on a Mac)?
>
> ```shell
> git clone --depth=1 https://github.com/bitcoin-core/qa-assets
> cmake --preset=libfuzzer\
> -DCMAKE_C_COMPILER="$(brew --prefix llvm)/bin/clang" \
> -DCMAKE_CXX_COMPILER="$(brew --prefix llvm)/bin/clang++" \
> -DCMAKE_EXE_LINKER_FLAGS="-fuse-ld=lld"
> cmake --build build_fuzz
> time FUZZ=tx_package_eval build_fuzz/bin/fuzz qa-assets/fuzz_corpora/tx_package_eval
> ```
Running this I run into
...
💬 furszy commented on issue "listdescriptors true fails with 'Can't get descriptor string' in non-watch-only descriptor wallet":
(https://github.com/bitcoin/bitcoin/issues/32078#issuecomment-2729618600)
This occurs when the wallet does not contain all key/script paths key material (e.g. taproot descriptor with an internal pubkey). Test exercising the behavior and explaining the issue further: https://github.com/furszy/bitcoin-core/commit/970efeb0fb2632d714e0f444b4ac89278eb80b28.
(https://github.com/bitcoin/bitcoin/issues/32078#issuecomment-2729618600)
This occurs when the wallet does not contain all key/script paths key material (e.g. taproot descriptor with an internal pubkey). Test exercising the behavior and explaining the issue further: https://github.com/furszy/bitcoin-core/commit/970efeb0fb2632d714e0f444b4ac89278eb80b28.
💬 suiyuan1314 commented on pull request "docs: improve development guidelines for PR merging":
(https://github.com/bitcoin/bitcoin/pull/32006#discussion_r1998815319)
I have reworded the guidelines, is it okay?
(https://github.com/bitcoin/bitcoin/pull/32006#discussion_r1998815319)
I have reworded the guidelines, is it okay?
💬 willcl-ark commented on pull request "Shuffle depends instructions and recommend modern make for macOS":
(https://github.com/bitcoin/bitcoin/pull/32086#discussion_r1998797323)
```suggestion
Install Xcode Command Line Tools, Homebrew Package Manager, and
the Required Dependencies from [build-osx.md](../doc/build-osx.md).
```
(https://github.com/bitcoin/bitcoin/pull/32086#discussion_r1998797323)
```suggestion
Install Xcode Command Line Tools, Homebrew Package Manager, and
the Required Dependencies from [build-osx.md](../doc/build-osx.md).
```
👍 willcl-ark approved a pull request: "Shuffle depends instructions and recommend modern make for macOS"
(https://github.com/bitcoin/bitcoin/pull/32086#pullrequestreview-2690626911)
ACK 3ad36fce4be0aa413639468118d6dbd7379a2106
I confirm that the `make` brew formula [symlinks itself](https://github.com/Homebrew/homebrew-core/blob/5c4e0f50fa0cee5ae512f09ae887e3f5d0c05318/Formula/m/make.rb#L52-L55) to `gmake`.
Did a successful (`NO_QT=1`) depends build using `gmake` on macOS 15 and no longer see the errors from #30978.
I think it would be good to keep #30978 open if this is merged, to address https://github.com/bitcoin/bitcoin/issues/30978#issuecomment-2582535849
(https://github.com/bitcoin/bitcoin/pull/32086#pullrequestreview-2690626911)
ACK 3ad36fce4be0aa413639468118d6dbd7379a2106
I confirm that the `make` brew formula [symlinks itself](https://github.com/Homebrew/homebrew-core/blob/5c4e0f50fa0cee5ae512f09ae887e3f5d0c05318/Formula/m/make.rb#L52-L55) to `gmake`.
Did a successful (`NO_QT=1`) depends build using `gmake` on macOS 15 and no longer see the errors from #30978.
I think it would be good to keep #30978 open if this is merged, to address https://github.com/bitcoin/bitcoin/issues/30978#issuecomment-2582535849
💬 fanquake commented on issue "build: status code 503 for freetype":
(https://github.com/bitcoin/bitcoin/issues/32085#issuecomment-2729687435)
It'd be good if the Windows builds didn't compile all the Qt deps (which we don't even use) from source all the time, which would bypass this issue entirely, however outside of that there's no much we can do here.
(https://github.com/bitcoin/bitcoin/issues/32085#issuecomment-2729687435)
It'd be good if the Windows builds didn't compile all the Qt deps (which we don't even use) from source all the time, which would bypass this issue entirely, however outside of that there's no much we can do here.
✅ fanquake closed an issue: "build: status code 503 for freetype"
(https://github.com/bitcoin/bitcoin/issues/32085)
(https://github.com/bitcoin/bitcoin/issues/32085)
🤔 furszy reviewed a pull request: "wallet: removed duplicate call to GetDescriptorScriptPubKeyMan"
(https://github.com/bitcoin/bitcoin/pull/32023#pullrequestreview-2690742860)
Code review ACK c986b43f0c45388b87106bcbf2534c75eebe7b72.
(https://github.com/bitcoin/bitcoin/pull/32023#pullrequestreview-2690742860)
Code review ACK c986b43f0c45388b87106bcbf2534c75eebe7b72.
💬 furszy commented on pull request "wallet: removed duplicate call to GetDescriptorScriptPubKeyMan":
(https://github.com/bitcoin/bitcoin/pull/32023#discussion_r1998864194)
could make this more descriptive so that the user is aware of which specific descriptor failed when multiple are being imported.
```c++
throw std::runtime_error(strprintf("Can't update descriptor %s, error: %s", descriptor.descriptor->ToString(), error))
```
This will need modifications in the test side too.
(https://github.com/bitcoin/bitcoin/pull/32023#discussion_r1998864194)
could make this more descriptive so that the user is aware of which specific descriptor failed when multiple are being imported.
```c++
throw std::runtime_error(strprintf("Can't update descriptor %s, error: %s", descriptor.descriptor->ToString(), error))
```
This will need modifications in the test side too.