Bitcoin Core Github
44 subscribers
119K links
Download Telegram
💬 rkrux commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2387893415)
In 793d727791fe042bbb4a9ff5df3db012a77a40fe "sign: Add CreateMuSig2Nonce"

Wouldn't hurt to add this assert imo. I'm inclining to fail early the whole process in case of any such scenarios.

```diff
diff --git a/src/key.cpp b/src/key.cpp
index a952acb260..f7e065c8da 100644
--- a/src/key.cpp
+++ b/src/key.cpp
@@ -372,6 +372,8 @@ std::vector<uint8_t> CKey::CreateMuSig2Nonce(MuSig2SecNonce& secnonce, const uin
if (!secp256k1_musig_nonce_gen(secp256k1_context_sign, secnonce.Get(), &p
...
💬 rkrux commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2387925458)
In 2806f8152c751641257f3f1ab0d886d9c8ece1e1 "pubkey: Return tweaks from BIP32 derivation"

Reconsider updating the commit description retouched, I can foresee future readers would want to know the intent of this commit, following up on https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-3282694791
💬 rkrux commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2387919138)
In 36f83554a2dc5b397ee3b4495d8bf2aff36cedc6 "test: Test MuSig2 in the wallet"

I feel it'd be prudent to be comprehensive in testing here. A more practical scenario would be where the `MuSig2` unspents are combined with non-MuSig2/Taproot unspents, and/or there could be multiple MuSig2 unspents in the PSBT. The following diff tests for:
1. 2 MuSig2 inputs in the PSBT along with 1 SegWit unspent. A secondary reason is to avoid seeing `dec_psbt["inputs"][0]` in several places.
2. Iterates all
...
🤔 janb84 reviewed a pull request: "refactor: unify container presence checks"
(https://github.com/bitcoin/bitcoin/pull/33192#pullrequestreview-3279795119)
reACK d870caca33ac9f0fdd76969a7341535c5722417e
📝 hebasto opened a pull request: "depends: Update URL for `qrencode` package source tarball"
(https://github.com/bitcoin/bitcoin/pull/33494)
The https://fukuchi.org/ homepage no longer links to the source tarball, and previously available files appear to have been removed. The homepage now instructs users to download source tarballs from the GitHub [releases](https://github.com/fukuchi/libqrencode/releases) page instead.

The diff between the source trees is immaterial:
```diff
--- old
+++ new
@@ -1,19 +1,16 @@
27e7deccd2925c94e4190ee64794a051199f215f145f76fd664cdebedbbf8a35 acinclude.m4
-e1e35b1309482f699a9700a2065a0bce09c
...
📝 JaroslawDomagala opened a pull request: "Create masteron"
(https://github.com/bitcoin/bitcoin/pull/33495)
<!--
*** 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 that improv
...
💬 JaroslawDomagala commented on pull request "Create masteron":
(https://github.com/bitcoin/bitcoin/pull/33495#issuecomment-3347001048)
22
👍 vasild approved a pull request: "Introduce SockMan ("lite"): low-level socket handling for HTTP"
(https://github.com/bitcoin/bitcoin/pull/32747#pullrequestreview-3279884844)
ACK e007e1b57d5d42c2a8d932d5b91eec8a3ca76e14
🤔 janb84 reviewed a pull request: "build: Move CMAKE_SKIP_INSTALL_RPATH from CMake to Guix script"
(https://github.com/bitcoin/bitcoin/pull/33470#pullrequestreview-3279929540)
ACK dd5c517757f97b68f7eb07628222c958b47f742b

PR removes CMAKE flag from CMakeLists to config flag section of build script. This change is a follow up on #33247 and seems like a good followup to move the Guix special flag to the Guix special build script.


**Commit:** `dd5c517757f9`

```shell
207120a771188474820e1a98f20780154f53037f12251d08f49582754819ab50 guix-build-dd5c517757f9/output/aarch64-linux-gnu/SHA256SUMS.part
7fd9043d0fcb6f58f3401968f83e47c54f94bd7fd5880951db1918f027ed85
...
💬 vasild commented on pull request "depends: Update URL for `qrencode` package source tarball":
(https://github.com/bitcoin/bitcoin/pull/33494#issuecomment-3347129804)
For me, the checksum is the same as in the PR:
```sh
$ wget -q -O - https://github.com/fukuchi/libqrencode/archive/refs/tags/v4.1.1.tar.gz |sha256
5385bc1b8c2f20f3b91d258bf8ccc8cf62023935df2d2676b5b67049f31a049c
$
```
📝 MirekLabada opened a pull request: "222 "
(https://github.com/bitcoin/bitcoin/pull/33496)
<!--
*** 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 that improv
...
💬 MirekLabada commented on pull request "222":
(https://github.com/bitcoin/bitcoin/pull/33496#issuecomment-3347144943)
? fix this please
📝 MirekLabada opened a pull request: "?"
(https://github.com/bitcoin/bitcoin/pull/33497)
<!--
*** 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 that improv
...
💬 ryanofsky commented on pull request "multiprocess: Add capnp wrapper for Chain interface":
(https://github.com/bitcoin/bitcoin/pull/29409#issuecomment-3347241863)
> Err, right. Updated the description with your suggested trace filtering.

Thanks created https://github.com/bitcoin-core/libmultiprocess/issues/219 to track this (feel free to create new issues directly in the future). Thanks for updating the rust client, too!
💬 hebasto commented on pull request "build: Move CMAKE_SKIP_INSTALL_RPATH from CMake to Guix script":
(https://github.com/bitcoin/bitcoin/pull/33470#discussion_r2388170464)
> Can this just be `CMAKE_SKIP_RPATH`?

With this suggestion having been adopted, the following lines of code can be dropped as well: https://github.com/bitcoin/bitcoin/blob/dd5c517757f97b68f7eb07628222c958b47f742b/CMakeLists.txt#L632 and https://github.com/bitcoin/bitcoin/blob/d8fe258cd6105704bf4427eda048dd7085ca516d/src/CMakeLists.txt#L420

I understand that this overlaps with https://github.com/bitcoin/bitcoin/pull/33247, so it's OK to leave this branch as is.
👍 hebasto approved a pull request: "build: Move CMAKE_SKIP_INSTALL_RPATH from CMake to Guix script"
(https://github.com/bitcoin/bitcoin/pull/33470#pullrequestreview-3280122695)
ACK dd5c517757f97b68f7eb07628222c958b47f742b, I have reviewed the code and it looks OK.
💬 hebasto commented on pull request "depends: Update URL for `qrencode` package source tarball":
(https://github.com/bitcoin/bitcoin/pull/33494#issuecomment-3347322127)
> For me, the checksum is the same as in the PR:
>
> ```shell
> $ wget -q -O - https://github.com/fukuchi/libqrencode/archive/refs/tags/v4.1.1.tar.gz |sha256
> 5385bc1b8c2f20f3b91d258bf8ccc8cf62023935df2d2676b5b67049f31a049c
> $
> ```
>
> but CI:
>
> ```
> sha256sum: WARNING: 1 computed checksum did NOT match
> /home/admin/actions-runner/_work/_temp/depends/sources/qrencode-4.1.1.tar.gz: FAILED
> ```

I believe it's a cache issue.

@willcl-ark @m3dwards @maflcko

Can the d
...
💬 maflcko commented on pull request "depends: Update URL for `qrencode` package source tarball":
(https://github.com/bitcoin/bitcoin/pull/33494#issuecomment-3347363844)
> Can the dependency CI cache be invalidated: (1) for this PR; (2) globally?

I wouldn't know how, other than by changing the cache key value (both for restore and save): https://github.com/bitcoin/bitcoin/blob/d8fe258cd6105704bf4427eda048dd7085ca516d/.github/actions/restore-caches/action.yml#L21
💬 hebasto commented on pull request "depends: Update URL for `qrencode` package source tarball":
(https://github.com/bitcoin/bitcoin/pull/33494#issuecomment-3347417118)
> I believe it's a cache issue.

Resolved by using the new `$(package)_file_name`.
💬 m3dwards commented on pull request "depends: Update URL for `qrencode` package source tarball":
(https://github.com/bitcoin/bitcoin/pull/33494#issuecomment-3347455469)
> Can the `depends` CI cache be invalidated: (1) for this PR; (2) globally?

I would have thought the cache should have been invalidated automatically, looking into it.
💬 kevkevinpal commented on pull request "ci: check if file or directory already exists for ${HOME}/.bitcoin instead of failing":
(https://github.com/bitcoin/bitcoin/pull/33486#issuecomment-3347489891)
> I don't think it is supported or even encouraged to run this internal CI script on an outside machine.

Yea I understand this, I was just playing around with the scripts to understand better how the CI works

> Also, I fail to see how this could even work, as you did not include any steps how to reproduce.

Yes, I updated the description. Basically, just create a directory in `$HOME/.bitcoin` and then try to run `./ci/test/03_test_script.sh` and you should see the error.

For me it is
...