Bitcoin Core Github
43 subscribers
122K links
Download Telegram
💬 Eunovo commented on pull request "descriptors: inference process, do not return unparsable multisig descriptors":
(https://github.com/bitcoin/bitcoin/pull/31404#discussion_r1998654566)
> For example, if the script is a P2SH output and the provider lacks the redeem script (watch-only scenario), the expected behavior is to return a `addr()`. But with your changes, it would return `nullptr instead.

Are you certain? I checked this and I couldn't verify this claim. If the redeem script is not available in the provider, `provider.GetCScript(scriptid, subscript)` should evaluate to `false` and the expected `addr()` desc will be returned. I tested this by adding the following test
...
👍 rkrux approved a pull request: "test: avoid disk space warning for non-regtest"
(https://github.com/bitcoin/bitcoin/pull/32057#pullrequestreview-2690371123)
ACK 20fe41e9e83d510fd467f5a999d55a614b16ef89

The last statement in the PR description seems to be contradicting with the title & the code comment.

> This PR fixes the issue by allowing the space warning on both testnet3 and testnet4.
💬 l0rinc commented on pull request "refactor: Use std::span over Span":
(https://github.com/bitcoin/bitcoin/pull/31519#issuecomment-2729392547)
Rebased it against latest [master](https://github.com/bitcoin/bitcoin/commit/db2c57ae9eebdb75c58cd165ac929919969c19a9), ran all tests locally, searched for remaining `\bSpan\b` and compared to previously acked fab51b26d68b37f5914314ad29eeb930538ea7ae the only diff was @theuni's [comment](https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1980199782).

<details>
<summary>Details</summary>

```diff
---
diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
--- a/src/test/uti
...
💬 ryanofsky commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1998673375)
> @hebasto should we unset `CMAKE_RUNTIME_OUTPUT_DIRECTORY` here to avoid `mpgen` and `mpgen` landing in the `bin/` directory? Maybe it would be better to move the `libmultiprocess` subtree up one level?

I like the idea of moving the add_subdirectory call up a level so to avoid this problem so will experiment with this.
💬 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
🤔 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
l0rinc closed a pull request: "doc: use `libfuzzer-nosan` for macOS"
(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.
💬 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.
💬 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
...
💬 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.
💬 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).
💬 ismaelsadeeq commented on pull request "wallet, rpc: deprecate settxfee and paytxfee":
(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?
🚀 hebasto merged a pull request: "test: Check datadir cleanup after assumeutxo was successful"
(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
...
💬 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.
💬 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?
💬 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).
```
👍 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