Remove legacy btc tx signing by amendelzon · Pull Request #491 · rsksmart/rsk-powhsm

amendelzon · 2025-12-24T07:35:56Z

Removing legacy signing logic from powHSM firmware
Removing legacy signing logic from middleware
Updated firmware test suite after legacy signing logic removal
Updated middleware unit tests after legacy signing logic removal
Updated protocol documentation

PR has been split into several commits in order to make reviewing easier.

github-actions · 2025-12-24T07:36:08Z

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

- Incidentally removing no longer used bitcoin utility functions

- Updated sign_auth logic to test for specific no-match in sighash generation wrt legacy sighashes - Kept back some legacy signing logic test cases just for the sake of making sure legacy sighash signing is no longer possible

- Removed legacy signing from from authorized signing spec - Reworded segwit signing form as the sole authorized signing form

Copilot

Pull request overview

This PR removes legacy (non-segwit) BTC authorized signing support across the firmware and middleware, standardizing authorized signing on a single transaction hashing/signing flow and updating tests + protocol documentation accordingly.

Changes:

Middleware: removes legacy sighashComputationMode handling and requires witnessScript + outpointValue for authorized signing.
Firmware: removes legacy signing-path parsing/logic and updates the sign-authorized APDU format (no sighash mode byte; extradata required).
Tests/docs: deletes legacy-specific test suites, updates fixtures, and refreshes protocol documentation for the new request shape.

Reviewed changes

Copilot reviewed 60 out of 62 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
middleware/tests/ledger/test_protocol.py	Updates ledger protocol tests to reject legacy message shape and assert segwit-style signing calls.
middleware/tests/ledger/test_hsm2dongle.py	Updates expected device error mapping for removed sighash-mode errors.
middleware/tests/ledger/hsm2dongle_cmds/test_hsm2dongle_sign_auth_legacy.py	Removes legacy sign-authorized command test suite.
middleware/tests/ledger/hsm2dongle_cmds/test_hsm2dongle_sign_auth.py	Renames/rewires sign-authorized command tests to the new unified format and error codes.
middleware/tests/comm/test_protocol.py	Updates comm protocol tests to match the new authorized signing message validation rules and version handling.
middleware/tests/comm/test_bitcoin.py	Replaces legacy unsigned-tx/hash tests with segwit-focused sighash test vectors and invalid-redeem-script coverage.
middleware/ledger/protocol.py	Removes legacy unsigned-tx “unsigning” flow; requires witnessScript/outpointValue for authorized signing requests.
middleware/ledger/hsm2dongle.py	Removes sighash computation mode enum/byte; always sends extradata (witnessScript length + witnessScript + outpointValue).
middleware/comm/protocol.py	Tightens authorized signing message validation to require `tx`, `input`, `witnessScript`, `outpointValue`.
middleware/comm/bitcoin.py	Removes legacy tx hash/unsigning helpers; adds additional redeem-script validation in P2SH sighash helper.
firmware/test/resources/912-no-onboard-sign-auth-fails.json	Updates signAuthorized fixture to include witnessScript/outpointValue and new tx sample.
firmware/test/resources/900-2-larger-than-apdu-command-fails-tcpsigner.json	Adds TCPSigner raw-command oversize APDU negative test.
firmware/test/resources/700-device-fake-receipt-root-fails.json	Adds witnessScript/outpointValue fields to match new signAuthorized fixture schema.
firmware/test/resources/620-get-after-success-sign.json	Adds a getState-after-sign regression/behavior test case.
firmware/test/resources/612-1-sign-3input-5output-testnet-legacy.json	Adds witnessScript/outpointValue fields to legacy-named fixture.
firmware/test/resources/611-1-sign-sample-testnet-tx-erp-legacy.json	Adds witnessScript/outpointValue fields to legacy-named fixture.
firmware/test/resources/610-2-sign-sample-mainnet-tx-legacy.json	Adds witnessScript/outpointValue fields to legacy-named fixture.
firmware/test/resources/610-1-sign-sample-mainnet-tx-i1.json	Adds new fixture for signing input 1 on a sample mainnet tx.
firmware/test/resources/610-0-sign-sample-mainnet-tx-i0.json	Adds new fixture for signing input 0 on a sample mainnet tx.
firmware/test/resources/606-1-sign-match-not-last-log-legacy.json	Adds witnessScript/outpointValue fields to legacy-named fixture.
firmware/test/resources/606-0-sign-match-not-last-log.json	Adds new segwit-style fixture for “matching log not last” scenario.
firmware/test/resources/604-1-sign-match-long-proof-legacy.json	Adds witnessScript/outpointValue fields to legacy-named fixture.
firmware/test/resources/604-0-sign-match-long-proof.json	Adds new segwit-style fixture for long merkle proof scenario.
firmware/test/resources/603-1-sign-long-redeemscript-legacy.json	Adds witnessScript/outpointValue fields to legacy-named fixture.
firmware/test/resources/603-0-sign-long-witnessscript.json	Removes `txType` and relies on explicit witnessScript/outpointValue fields.
firmware/test/resources/602-sign-basic.json	Removes `txType` and relies on explicit witnessScript/outpointValue fields.
firmware/test/resources/428-sign-auth-fail-disallow-receipt-multiple-top-level.json	Updates fixture tx and adds witnessScript/outpointValue for new extradata-based signing.
firmware/test/resources/425-sign-auth-fail-invalid-length.json	Updates fixture tx and adds witnessScript/outpointValue for new extradata-based signing.
firmware/test/resources/423-sign-auth-fail-invalid-path.json	Updates fixture tx and adds witnessScript/outpointValue for new extradata-based signing.
firmware/test/resources/422-sign-fail-proof-does-not-connect.json	Updates fixture tx and adds witnessScript/outpointValue for new extradata-based signing.
firmware/test/resources/421-sign-fail-proof-wrong-version-node.json	Updates fixture tx and adds witnessScript/outpointValue for new extradata-based signing.
firmware/test/resources/420-sign-fail-proof-leaf-nomatch.json	Updates fixture tx and adds witnessScript/outpointValue for new extradata-based signing.
firmware/test/resources/419-sign-fail-proof-first-non-leaf.json	Updates fixture tx and adds witnessScript/outpointValue for new extradata-based signing.
firmware/test/resources/418-sign-fail-proof-only-receipts-root.json	Updates fixture tx and adds witnessScript/outpointValue for new extradata-based signing.
firmware/test/resources/417-sign-fail-receipt-malformed-nologs.json	Updates fixture tx and adds witnessScript/outpointValue for new extradata-based signing.
firmware/test/resources/416-sign-fail-receipt-top-level-size-mismatch.json	Updates fixture tx and adds witnessScript/outpointValue for new extradata-based signing.
firmware/test/resources/415-1-sign-fail-receipt-no-match-single-log-legacy.json	Adds witnessScript/outpointValue fields to legacy-named fixture.
firmware/test/resources/415-0-sign-fail-receipt-no-match-single-log.json	Adds new segwit-style fixture for “no match single log” scenario.
firmware/test/resources/414-1-sign-fail-receipt-nomatch-legacy.json	Adds witnessScript/outpointValue fields to legacy-named fixture.
firmware/test/resources/414-0-sign-fail-receipt-nomatch.json	Adds new segwit-style fixture for “unmatched receipt” scenario.
firmware/test/resources/413-1-sign-fail-malformed-output-legacy.json	Adds witnessScript/outpointValue fields to legacy-named fixture.
firmware/test/resources/413-0-sign-fail-malformed-output.json	Adds new segwit-style fixture for malformed BTC output scenario.
firmware/test/resources/412-sign-fail-malformed-redeem-script-legacy.json	Adds witnessScript/outpointValue fields to legacy-named fixture.
firmware/test/resources/411-1-sign-fail-zero-byte-input-script-legacy.json	Adds witnessScript/outpointValue fields to legacy-named fixture and adjusts naming.
firmware/test/resources/411-0-sign-fail-zero-byte-input-script.json	Adds new segwit-style fixture for zero-byte input-script scenario.
firmware/test/resources/410-sign-fail-invalid-version.json	Removes legacy invalid-version fixture.
firmware/test/resources/302-sign.json	Adds witnessScript/outpointValue fields to sign fixture.
firmware/test/resources/216-sign-segwit-t2i1.json	Removes `txType` and relies on explicit witnessScript/outpointValue fields.
firmware/test/resources/215-sign-segwit-t2i0.json	Removes `txType` and adds witnessScript/outpointValue for legacy input in mixed tx.
firmware/test/resources/214-sign-segwit-t1i1.json	Removes `txType` and relies on explicit witnessScript/outpointValue fields.
firmware/test/resources/213-sign-segwit-t1i0.json	Removes `txType` and relies on explicit witnessScript/outpointValue fields.
firmware/test/resources/202-sign-iris.json	Adds witnessScript/outpointValue fields to sign fixture.
firmware/test/cases/sign_auth.py	Removes legacy/segwit branching; always computes segwit-style sighash and adds a legacy-vs-segwit mismatch sanity check.
firmware/test/cases/case.py	Wraps JSON loading with contextual error message.
firmware/src/powhsm/src/auth_tx.h	Removes legacy fields and sighash-mode handling from authorized signing context.
firmware/src/powhsm/src/auth_tx.c	Removes legacy sighash/scriptSig parsing path; requires extradata and finalizes a single sighash flow.
firmware/src/powhsm/src/auth_receipt.c	Fixes control-flow/bracing and a typo in a comment.
firmware/src/powhsm/src/auth.h	Removes invalid sighash mode error and renumbers invalid extradata size error code.
docs/protocol.md	Updates sign-authorized request format to remove `sighashComputationMode` and require witnessScript/outpointValue.
.github/workflows/run-tests.yml	Switches integration test dependency checkout to the `enhancement/add-segwit-tests` ref.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-06T19:09:29Z

middleware/comm/bitcoin.py

    try:
        redeem_script = bitcoin.core.CScript(last_chunk_operand)
+        if not redeem_script.is_valid():
+            raise ValueError()
    except Exception:
        raise ValueError("Invalid redeem script: %s" % last_chunk_operand.hex())


redeem_script.is_valid() is not part of python-bitcoinlib's CScript API in the commonly used versions, so this will raise AttributeError at runtime and mask the intended "Invalid redeem script" error handling. Consider removing this call and relying on the existing parse/iteration checks (or validating by iterating/decoding opcodes) to detect malformed scripts.

Copilot · 2026-03-06T19:09:30Z

firmware/src/powhsm/src/auth_tx.c

            // Validate computation mode and init tx parsing context
-            switch (auth.tx.sighash_computation_mode) {
-            case SIGHASH_COMPUTE_MODE_LEGACY:
-                btctx_init(&auth.tx.ctx, &btctx_cb);
-                break;
-            case SIGHASH_COMPUTE_MODE_SEGWIT:
-                btctx_init(&auth.tx.ctx, &btctx_cb_segwit);
-                if (!auth.tx.segwit_extradata_size) {
-                    LOG("[E] Invalid extradata size for segwit");
-                    THROW(ERR_AUTH_INVALID_EXTRADATA_SIZE);
-                }
-                break;
-            default:
-                LOG("[E] Invalid sighash computation mode\n");
-                THROW(ERR_AUTH_INVALID_SIGHASH_COMPUTATION_MODE);
+            btctx_init(&auth.tx.ctx, &btctx_cb);
+            if (!auth.tx.extradata_size) {
+                LOG("[E] Invalid extradata size");
+                THROW(ERR_AUTH_INVALID_EXTRADATA_SIZE);
            }


By removing the legacy btctx_cb / BTCTX_EV_VIN_COUNT validation, the firmware no longer checks that auth.input_index_to_sign is < the transaction's VIN count. If an out-of-range index is provided, fields like ip_prevout/ip_seqno may never be populated but are still used when finalizing the sighash, leading to undefined behavior. Please reintroduce an explicit VIN-count vs input-index check (e.g., in the BTCTX_EV_VIN_COUNT handler) and throw ERR_AUTH_INVALID_TX_INPUT_INDEX on invalid indices.

Copilot · 2026-03-06T19:09:30Z

docs/protocol.md

+        "witnessScript": "hhhh", // (iv)
+        "outpointValue": i // (v)


The protocol doc doesn’t mention the stricter validation enforced by the middleware for authorized signing (e.g., witnessScript must be a non-empty hex string and outpointValue must be > 0 and <= 0xffffffffffffffff). To avoid client confusion/integration breakage, update this section to document these constraints explicitly.

Suggested change

"witnessScript": "hhhh", // (iv)

"outpointValue": i // (v)

"witnessScript": "hhhh", // (iv) non-empty hex-encoded witness script

"outpointValue": i // (v) outpoint amount; must be > 0 and <= 0xffffffffffffffff

Copilot · 2026-03-06T19:09:30Z

.github/workflows/run-tests.yml

        with:
          repository: rootstock/hsm-integration-test
-          ref: 5.4.0.plus
+          ref: enhancement/add-segwit-tests


The integration-test repo is now checked out from a moving branch name (enhancement/add-segwit-tests). For reproducible CI and to avoid unexpected breakages when that branch changes, prefer pinning to an immutable tag or commit SHA (and bump it intentionally when needed).

Suggested change

ref: enhancement/add-segwit-tests

ref: 0123456789abcdef0123456789abcdef01234567 # pinned commit SHA for reproducible CI

Copilot · 2026-03-06T19:09:31Z

.github/workflows/run-tests.yml

        with:
          repository: rootstock/hsm-integration-test
-          ref: 5.4.0.plus
+          ref: enhancement/add-segwit-tests


Same concern as above: the SGX integration tests also pin to a moving branch (enhancement/add-segwit-tests). Consider switching to a tag/commit SHA to keep CI deterministic.

Suggested change

ref: enhancement/add-segwit-tests

ref: 0123456789abcdef0123456789abcdef01234567

Copilot · 2026-03-06T19:09:31Z

firmware/test/cases/case.py

+            with open(path, "r") as f:
+                return cls.create(json.load(f))
+        except Exception as e:
+            raise RuntimeError(f"While loading test case from {path}: {e}")


The raised RuntimeError drops the original traceback context, which makes debugging malformed/invalid JSON fixtures harder. Prefer exception chaining (e.g., raise ... from e) so the underlying failure location is preserved.

Suggested change

raise RuntimeError(f"While loading test case from {path}: {e}")

raise RuntimeError(f"While loading test case from {path}: {e}") from e

amendelzon requested a review from italo-sampaio December 24, 2025 07:35

amendelzon marked this pull request as draft December 24, 2025 07:38

amendelzon force-pushed the improvement/remove-legacy-btc-tx-signing branch from 3891bee to 9b733bc Compare February 25, 2026 19:43

amendelzon added 4 commits February 26, 2026 08:44

Removing legacy signing logic from powHSM firmware

333343f

Removing legacy signing logic from middleware

5836222

- Incidentally removing no longer used bitcoin utility functions

Updated middleware unit tests after legacy signing logic removal

8ea4b4a

amendelzon force-pushed the improvement/remove-legacy-btc-tx-signing branch from 9b733bc to 86622da Compare February 25, 2026 19:44

amendelzon added 2 commits February 26, 2026 08:44

Updated protocol documentation

82996a6

- Removed legacy signing from from authorized signing spec - Reworded segwit signing form as the sole authorized signing form

Testing integration tests updates

86622da

amendelzon requested a review from Copilot March 6, 2026 19:02

Copilot started reviewing on behalf of amendelzon March 6, 2026 19:02 View session

Copilot AI reviewed Mar 6, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove legacy btc tx signing#491

Remove legacy btc tx signing#491
amendelzon wants to merge 6 commits intomasterfrom
improvement/remove-legacy-btc-tx-signing

amendelzon commented Dec 24, 2025

Uh oh!

github-actions bot commented Dec 24, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 6, 2026

Uh oh!

Copilot AI Mar 6, 2026

Uh oh!

Copilot AI Mar 6, 2026

Uh oh!

Copilot AI Mar 6, 2026

Uh oh!

Copilot AI Mar 6, 2026

Uh oh!

Copilot AI Mar 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	ref: enhancement/add-segwit-tests
	ref: 0123456789abcdef0123456789abcdef01234567 # pinned commit SHA for reproducible CI

	raise RuntimeError(f"While loading test case from {path}: {e}")
	raise RuntimeError(f"While loading test case from {path}: {e}") from e

Conversation

amendelzon commented Dec 24, 2025

Uh oh!

github-actions bot commented Dec 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Dependency Review

Scanned Files

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions bot commented Dec 24, 2025 •

edited

Loading