Trustworthy sign_psbt amount/fee display for non-default sighash#512
Trustworthy sign_psbt amount/fee display for non-default sighash#512iartemov-ledger wants to merge 5 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #512 +/- ##
===========================================
+ Coverage 72.36% 72.39% +0.02%
===========================================
Files 71 71
Lines 10347 10385 +38
Branches 1876 1888 +12
===========================================
+ Hits 7488 7518 +30
- Misses 2113 2118 +5
- Partials 746 749 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
elf sizes
Stack consumption summary
|
iartemov-ledger
force-pushed
the
negative_fee
branch
from
53758f0 to
c81d48f
Compare
There was a problem hiding this comment.
Pull request overview
This PR makes the on-device sign_psbt transaction amount/fee display trustworthy under non-default sighash rules by showing only information that the signed inputs provably commit to (full details for default sighash, “spent only” for ANYONECANPAY|ALL, or a notice-only screen when outputs/amounts can’t be trusted).
Changes:
- Introduces commit predicates and
decide_tx_display_mode()to determine which transaction details are safe to display given the effective sighash commitments. - Updates PSBT signing UX to support
FULL/SPENT_ONLY/UNAVAILABLEdisplay modes and to scope negative-fee rejection + high-fee warnings to cases where fee is meaningful. - Adds unit tests for the new predicates/mode decision and extends Python integration tests for negative-fee behavior under ANYONECANPAY vs default sighash.
Reviewed changes
Copilot reviewed 11 out of 252 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| unit-tests/test_sighash.c | Adds unit tests for sighash commit predicates and display-mode decision logic. |
| tests/test_sign_psbt_with_sighash_types.py | Imports IncorrectDataError and adds tests for negative-fee handling under ANYONECANPAY vs default sighash. |
| tests/conftest.py | Makes Nano settings navigation more robust by matching the start of the confirm button label text. |
| src/ui/display.h | Introduces tx_summary_t and threads display-mode state through UI transaction validation structs/APIs. |
| src/ui/display.c | Adds prepare_tx_summary() and updates simplified/streaming UI entrypoints to consume tx_summary_t. |
| src/ui/display_nbgl.c | Implements new NBGL review labeling/flow for SPENT_ONLY and UNAVAILABLE modes and gates high-fee warning on FULL. |
| src/handler/sign_psbt/transaction_display.c | Computes total_spent, decides display mode, and updates UI invocation + high-fee logic accordingly (including notice-only path). |
| src/handler/sign_psbt/preprocess_outputs.c | Restricts negative-fee rejection to fully-committed transactions only. |
| src/handler/sign_psbt/preprocess_inputs.c | Aggregates “inputs open/outputs open” flags over signed inputs and tracks internal input totals. |
| src/handler/sign_psbt.h | Extends signing state with internal_inputs_total_amount and aggregated sighash openness flags. |
| src/common/sighash.h | Adds commit predicate helpers, tx_display_mode_t, and decide_tx_display_mode(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
iartemov-ledger
force-pushed
the
negative_fee
branch
2 times, most recently
from
63b3e44 to
657a063
Compare
iartemov-ledger
force-pushed
the
negative_fee
branch
from
657a063 to
6a97e13
Compare
bigspider
left a comment
bigspider
left a comment
There was a problem hiding this comment.
Great initiative, I think this is going in a good direction.
As discussed offline, there are a few things that might deserve further thinking:
- It seems hopeless to try to provide a meaningful display when different inputs have different signing flags; but it is unclear if there are use cases for that. We could either forbid or automatically consider unsafe any such scenario (TBD), and focus on cases where all inputs have the same sighash flag (TL;DR: no sighash flag mixing)
- SIGHASH_SINGLE actually seems safe in the 1-input-1-output case, and since there are already deployed usecases for it, it might be worth including it
- The UX in the
TX_DISPLAY_SPENT_ONLYcase is not very clear, as it doesn't clearly distinguish between: (a) the amount spent/received from/to the wallet policy; (b) the amount going to each output. This needs to be clearly for the user. - When not signing with SIGHASH_ALL, but with no sighash flag mixing, it might be worth clearly showing what's the sighash flag used.
For anything not fitting these patterns, it is probably fine to fall back to TX_DISPLAY_UNAVAILABLE and just admit that we can't clear-sign it. Maybe the warning could be worded to be more explicitly scarier.
Possible add-ons that are fine to defer to future PRs:
- Even in the
TX_DISPLAY_UNAVAILABLEcase, if a certain registered wallet policy is involved in a transaction - An advanced transaction mode showing info also about each input (e.g.: if internal, its address, its amount, the sighash flag requested for signing if non-default).
I think only small changes are needed to fill these gaps, and it will make the display for such transactions a lot more meaningful.
| // False iff ANYONECANPAY is set (then others can add inputs, so the total input | ||
| // amount, and the fee, is unknown). Only valid for accepted sighash types. | ||
| static inline bool sighash_commits_all_inputs(uint32_t sighash_type) { | ||
| return (sighash_type & SIGHASH_ANYONECANPAY) == 0; | ||
| } | ||
|
|
||
| // True only for base-type ALL (ALL, or DEFAULT on taproot); NONE/SINGLE leave the | ||
| // outputs (and our change) free to change after signing. | ||
| static inline bool sighash_commits_all_outputs(uint32_t sighash_type) { | ||
| uint32_t base = sighash_type & ~(uint32_t) SIGHASH_ANYONECANPAY; | ||
| return base == SIGHASH_ALL || base == SIGHASH_DEFAULT; | ||
| } |
There was a problem hiding this comment.
I think what we care about in these functions is: does the signature commit to all the inputs/outputs that are provided in the PSBT that we are signing? That is because we base the display info based on the provided PSBT. We don't necessarily care if other inputs/outputs are added by someone else, as long as it doesn't invalidate what we committed to.
Therefore, we might generalize these functions a bit:
- if there is only 1 input, then
sighash_commits_all_inputscould always return true. All the sighash flags commit to the current input, whether or not ANYONECANPAY is present. - if there is only 1 input and output, then SIGHASH_SINGLE still commits to that all outputs, as trivially there is only one.
In particular, this would allow covering the SINGLE|ACP case among the 'safer' patterns (the one-input-one-output case is interesting as it is already used in some protocols). Note that we still don't know the fee in this case (and someone else might be paying for it!), but we do know exactly how much we're spending/receiving.
There was a problem hiding this comment.
Generalizing sighash_commits_all_inputs() to return true for a single input would be unsafe, because this function isn't just informational - it feeds sighash_inputs_open, which drives the negative-fee check skip in preprocess_outputs().
The subtlety is that two different ideas are being mixed up here:
- "commits to the inputs provided in the PSBT" - yes, true for 1 input
- "the input set is closed, i.e. no one can add inputs" =
!ANYONECANPAY- what this function actually means
A single ANYONECANPAY input satisfies the first but not the second: others can still append inputs. If sighash_commits_all_inputs() returned true there, sighash_inputs_open would become false, the negative-fee check would no longer be skipped, and a valid 1-input ANYONECANPAY maker PSBT (output > our input) would be wrongly rejected.
There was a problem hiding this comment.
Sure, the negative-fee check should be only done when the fee can be computed, which is basically only the case for SIGHASH_ALL/DEFAULT. Other than that, I don't think there is any other problem?
iartemov-ledger
force-pushed
the
negative_fee
branch
2 times, most recently
from
9c19708 to
e2d58b6
Compare
iartemov-ledger
force-pushed
the
negative_fee
branch
from
e2d58b6 to
d543499
Compare
Code coverage reportPer-file coverage
|
|
Normally the change-requests expressed all along the PR work have been addressed or postponned: sign_psbt non-default-sighash — change requests & resolutionBranch:
|
See the seequences and screens for different sighash combinations :