Bitcoin Core Github
44 subscribers
120K links
Download Telegram
🤔 jonatack reviewed a pull request: "Docs: fix typos in documentation files"
(https://github.com/bitcoin/bitcoin/pull/32011#pullrequestreview-2666130196)
ACK 01c14b3511610c5d5c6f509aadee1836220753ac modulo failing CI (see comment below)
💬 jonatack commented on pull request "Docs: fix typos in documentation files":
(https://github.com/bitcoin/bitcoin/pull/32011#discussion_r1984377258)
This change is on a subtree directory and would need to be made upstream, not here. You can verify this locally by running `./test/lint/git-subtree-check.sh src/leveldb` from repo root.
💬 wgyt commented on pull request "Docs: fix typos in documentation files":
(https://github.com/bitcoin/bitcoin/pull/32011#discussion_r1984387492)
Thanks for sharing this, I have reverted this change.
🤔 jonatack reviewed a pull request: "Remove Taproot activation height"
(https://github.com/bitcoin/bitcoin/pull/26201#pullrequestreview-2666162948)
Light approach ACK 301e41985ef8cc8e3098d1d737864a5ae1f5108e
💬 jonatack commented on pull request "Remove Taproot activation height":
(https://github.com/bitcoin/bitcoin/pull/26201#discussion_r1984399781)
Should this be like https://github.com/bitcoin/bitcoin/pull/26201/files#diff-decae4be02fb8a47ab4557fe74a9cb853bdfa3ec0fa1b515c0a1e5de91f4ad0bR1418?

```suggestion
* Consensus changes for which the new rules are enforced from genesis are not listed here.
```
💬 jonatack commented on pull request "Docs: fix typos in documentation files":
(https://github.com/bitcoin/bitcoin/pull/32011#discussion_r1984404706)
You'll need to squash the fixup commit into the first one (see https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits).
💬 wgyt commented on pull request "Docs: fix typos in documentation files":
(https://github.com/bitcoin/bitcoin/pull/32011#discussion_r1984410203)
OK, I have squashed the commit. :)
💬 tnndbtc commented on pull request "miniscript: fixes #29098 by only use first k valid signatures":
(https://github.com/bitcoin/bitcoin/pull/31719#discussion_r1984419437)
Agree, added.
💬 tnndbtc commented on pull request "miniscript: fixes #29098 by only use first k valid signatures":
(https://github.com/bitcoin/bitcoin/pull/31719#discussion_r1984420241)
Agree both. Provided comment on why adding the loop to generate number of ZEROs for nsat_return. Also, squashed multiple commits into one commit.
⚠️ jirijakes opened an issue: "test: No unit test covers BIP342 tapscript signatures"
(https://github.com/bitcoin/bitcoin/issues/32012)
The following change does not make any unit test fail:

```diff
diff --git i/src/script/interpreter.cpp w/src/script/interpreter.cpp
index a35306b693..d9c119ae77 100644
--- i/src/script/interpreter.cpp
+++ w/src/script/interpreter.cpp
@@ -1484,7 +1484,7 @@ bool SignatureHashSchnorr(uint256& hash_out, ScriptExecutionData& execdata, cons
// key_version is not used and left uninitialized.
break;
case SigVersion::TAPSCRIPT:
- ext_flag = 1;
+ ext_flag = 42;

...
💬 tnndbtc commented on pull request "miniscript: fixes #29098 by only use first k valid signatures":
(https://github.com/bitcoin/bitcoin/pull/31719#issuecomment-2705493881)
@hodlinator Appreciate your valuable comments and reviews. I've made the changes accordingly so you can check them out one by one.

As for the choice "_I think it would be good if this PR did one of either:
a) Directly included https://github.com/bitcoin/bitcoin/commit/387c12e0813a863bc9728777719acbafe7b12a34 from https://github.com/bitcoin/bitcoin/pull/28212, or
b) Not mention https://github.com/bitcoin/bitcoin/commit/387c12e0813a863bc9728777719acbafe7b12a34/https://github.com/bitcoin/bitc
...
💬 hebasto commented on issue "[rfc] build: Reject unclean configure?":
(https://github.com/bitcoin/bitcoin/issues/31942#issuecomment-2705675642)
> > even though the check is being re-run
>
> No, the check is not rerun, [the result is cached](https://cmake.org/cmake/help/latest/module/CheckCXXSourceCompiles.html).
>
> In this particular case, where `check_cxx_source_compiles` is used not to check for compiler capability, but for checking the availability of dependencies, where it is expected that users will want to rerun the check after fixing the problem, it may make sense to reset the cache entry by adding the following in `TryAppendL
...
💬 hebasto commented on pull request "RFC contrib: turn off compression of macOS SDK to fix determinism (across distros)":
(https://github.com/bitcoin/bitcoin/pull/32009#issuecomment-2705678071)
Concept ACK for the same reasoning as in https://github.com/bitcoin/bitcoin/pull/32009#issuecomment-2704999209.
💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2705689094)
> Unsure if a release note was written, apologies if I missed it. @hebasto @theuni @fanquake could you add to https://github.com/bitcoin-core/bitcoin-devwiki/wiki/29.0-Release-Notes-draft?

Added.
🤔 i-am-yuvi reviewed a pull request: "qa: clarify and document one assumeutxo test case with malleated snapshot"
(https://github.com/bitcoin/bitcoin/pull/31907#pullrequestreview-2666431885)
tACK e5ff4e416ecc8a51367022eb8a7a291f8cbc0c65

- Ran all functional and unit test for assume utxo
- Tested with different invalid amounts
- Verified the serialization
💬 hebasto commented on issue "build: cmake --install fails after --target deploy":
(https://github.com/bitcoin/bitcoin/issues/32007#issuecomment-2705751725)
I think this behaviour is expected since `test_bitcoin-qt.exe` is not part of the Windows installer and is not required for the `deploy` target.

Additionally, in [WINDOWS BUILD NOTES](https://github.com/bitcoin/bitcoin/blob/a9a2b669f3e07266ae739574e9c1cef5af711db7/doc/build-windows.md), `cmake --install build` follows `cmake --build build`, and this sequence always works.
⚠️ saikiran57 opened an issue: "Check if the wallet already contains the descriptor GetDescriptorScriptPubKeyMan"
(https://github.com/bitcoin/bitcoin/issues/32013)
### Is there an existing issue for this?

- [x] I have searched the existing issues

### Current behaviour

In importdescriptors rpc call when we try import descriptor address we are internally calling ProcessDescriptorImport() method which checking

` // Check if the wallet already contains the descriptor
auto existing_spk_manager = wallet.GetDescriptorScriptPubKeyMan(w_desc);
if (existing_spk_manager) {
if (!existing_spk_manager->CanUpdateToWa
...
🤔 hebasto reviewed a pull request: "doc: add note to Windows build about stripping bins"
(https://github.com/bitcoin/bitcoin/pull/32002#pullrequestreview-2666503323)
Approach ACK 9d0311d5bda104afaa8a89aa6505d683610609de.
💬 hebasto commented on pull request "doc: add note to Windows build about stripping bins":
(https://github.com/bitcoin/bitcoin/pull/32002#discussion_r1984606195)
```suggestion
After building using the Windows subsystem, it can be useful to copy the compiled
```
💬 hebasto commented on pull request "doc: add note to Windows build about stripping bins":
(https://github.com/bitcoin/bitcoin/pull/32002#discussion_r1984606657)
```suggestion
an install prefix using `CMAKE_INSTALL_PREFIX`. For example, to install to `c:\workspace\bitcoin`,
```