Skip to content

Conversation

Sjors
Copy link
Member

@Sjors Sjors commented Jun 3, 2025

Fixes #783

Based on #795

@Sjors Sjors force-pushed the 2025/06/psbt branch 2 times, most recently from fc57dfb to 46ceff9 Compare June 3, 2025 07:44
@Sjors
Copy link
Member Author

Sjors commented Jun 3, 2025

@Sjors Sjors marked this pull request as ready for review June 4, 2025 09:56
@Sjors
Copy link
Member Author

Sjors commented Jun 4, 2025

I ended copying @bigspider's code. However there's no test coverage.

I also can't manually test it on a device without passing the BIP-388 policy policy and (no BIP?) HMAC, which is a separate issue.

@Sjors
Copy link
Member Author

Sjors commented Jul 21, 2025

Fixed a few bugs.

I wanted to test this using #792 and taking some code from MooSig, but the latter changed the sign_psbt signature to return List[Tuple[int, SignPsbtYieldedObject]] instead of List[Tuple[int, bytes, bytes]].

@Sjors
Copy link
Member Author

Sjors commented Jul 21, 2025

I think I get the gist of the sign_psbt change, so I made a standalone change in #793 use the SignPsbtYieldedObject (only PartialSignature, leaving out the MuSig2 specific MusigPubNonce and MusigPartialSignature).

@Sjors Sjors mentioned this pull request Jul 21, 2025
1 task
Sjors and others added 19 commits July 22, 2025 10:54
```
error: "/home/runner/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/Cargo.lock" does not exist, unable to build with the standard library, try:
        rustup component add rust-src --toolchain nightly-x86_64-unknown-linux-gnu
```

https://github.com/bitcoin-core/HWI/actions/runs/16440824447/job/46462763374?pr=795
We can now use the main esp-idf toochain to install the appropriate
version of the qemu emulator, rather than building it ourselves from
source.  Mirrors change recently made to Jade repo.
See e.g. https://github.com/bitcoin-core/HWI/actions/runs/16440824447/job/46463441252

```
work/bitcoin/build/bin/bitcoind: /lib/x86_64-linux-gnu/libc.so.6: version `GLIBC_2.38' not found (required by work/bitcoin/build/bin/bitcoind)
````

Job lingered for 45 minutes.
Manually re-applied the patch after the original code seems to have
moved around a bit.
```
ERROR: coldcard: test_signtx (test_device.TestSignTx.test_signtx) (addrtypes=['legacy'], multisig_types=['legacy'], external=True, op_return=False)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/__w/HWI/HWI/test/test_device.py", line 588, in test_signtx
    self._test_signtx(addrtypes, multisig_types, external, op_return)
  File "/__w/HWI/HWI/test/test_device.py", line 576, in _test_signtx
    self._generate_and_finalize(True, psbt)
  File "/__w/HWI/HWI/test/test_device.py", line 403, in _generate_and_finalize
    self.assertTrue(first_sign_res["signed"])
                    ~~~~~~~~~~~~~~^^^^^^^^^^
KeyError: 'signed'
```
Sjors added 7 commits July 24, 2025 10:03
This reverts commit edab2af.

The always() option is too powerfull. The next commit implements
an alternative solution to the original issue.
This ensures the failure to build a simulator for one device
doesn't abort running jobs for the others. They're still grouped
by manufacturer.

Alternative to bitcoin-core#743.
Build failure on ubuntu-latest:

```
../py/stackctrl.c: In function ‘mp_stack_ctrl_init’:
../py/stackctrl.c:32:32: error: storing the address of local variable ‘stack_dummy’ in ‘mp_state_ctx.thread.stack_top’ [-Werror=dangling-pointer=]
   32 |     MP_STATE_THREAD(stack_top) = (char *)&stack_dummy;
../py/stackctrl.c:31:18: note: ‘stack_dummy’ declared here
   31 |     volatile int stack_dummy;
      |                  ^~~~~~~~~~~
In file included from ../py/runtime.h:29,
                 from ../py/stackctrl.c:27:
../py/mpstate.h:282:23: note: ‘mp_state_ctx’ declared here
  282 | extern mp_state_ctx_t mp_state_ctx;
      |                       ^~~~~~~~~~~~
cc1: all warnings being treated as errors
make: *** [../py/mkrules.mk:77: build/py/stackctrl.o] Error 1
```

Test failure (after downgrading build sim):

```
 File "/github/home/.cache/pypoetry/virtualenvs/hwi-crEDFiR--py3.9/lib/python3.9/site-packages/sdl2/dll.py", line 362, in <module>
    dll = DLL("SDL2", ["SDL2", "SDL2-2.0", "SDL2-2.0.0"], os.getenv("PYSDL2_DLL_PATH"))
  File "/github/home/.cache/pypoetry/virtualenvs/hwi-crEDFiR--py3.9/lib/python3.9/site-packages/sdl2/dll.py", line 253, in __init__
    raise RuntimeError("could not find any library for %s (%s)" %
RuntimeError: could not find any library for SDL2 (PYSDL2_DLL_PATH: unset)
```

https://github.com/bitcoin-core/HWI/actions/runs/16466809973/job/46548656293?pr=795
The build on ubuntu-latest succeeds, but the resulting binary uses
a too recent version of glibc for the test runners to handle.

This only seems to impact Trezor 1, but just downgrade for Trezor T
as well.
NanoS support has been dropped: LedgerHQ/app-bitcoin-new#262

NanoX also makes it possible to test MuSig2 in the future.

Keep NanoS for legacy.
@Sjors
Copy link
Member Author

Sjors commented Jul 30, 2025

I've been testing this with various wallets and I'm pretty sure it works now.

Rebasing on #795 to make sure CI passes.

Co-Authored-By: Salvatore Ingala <6681844+bigspider@users.noreply.github.com>
@Sjors
Copy link
Member Author

Sjors commented Jul 30, 2025

Fixed linter E226 missing whitespace around arithmetic operator

Looks like I accidentally dropped PSBT_IN_TAP_INTERNAL_KEY deserialization code bit, brought it back.

I also added the new test vectors, copied from Bitcoin Core.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add MuSig2 PSBT fields
1 participant