From 9aec5409b909846ab5d40ac3959c1b12b38a1382 Mon Sep 17 00:00:00 2001 From: Andrew Kozlik Date: Wed, 24 Feb 2021 17:28:57 +0100 Subject: [PATCH] feat(legacy): Allow decreasing output amount in RBF transactions. --- legacy/firmware/CHANGELOG.md | 2 ++ legacy/firmware/layout2.c | 31 +++++++++++++++++ legacy/firmware/layout2.h | 3 ++ legacy/firmware/signing.c | 67 ++++++++++++++++++++---------------- 4 files changed, 74 insertions(+), 29 deletions(-) diff --git a/legacy/firmware/CHANGELOG.md b/legacy/firmware/CHANGELOG.md index de9a4811d..922d0870f 100644 --- a/legacy/firmware/CHANGELOG.md +++ b/legacy/firmware/CHANGELOG.md @@ -10,6 +10,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). - Public key to ECDHSessionKey. [#1518] ### Changed +- Allow decreasing the output value in RBF transactions. [#1491] ### Deprecated @@ -380,3 +381,4 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). [#1402]: https://github.com/trezor/trezor-firmware/pull/1402 [#1415]: https://github.com/trezor/trezor-firmware/pull/1415 [#1518]: https://github.com/trezor/trezor-firmware/pull/1518 +[#1491]: https://github.com/trezor/trezor-firmware/issues/1491 diff --git a/legacy/firmware/layout2.c b/legacy/firmware/layout2.c index 59fb0349f..7acb62c90 100644 --- a/legacy/firmware/layout2.c +++ b/legacy/firmware/layout2.c @@ -503,6 +503,37 @@ void layoutConfirmReplacement(const char *description, uint8_t txid[32]) { description, str[0], str[1], str[2], str[3], NULL); } +void layoutConfirmModifyOutput(const CoinInfo *coin, AmountUnit amount_unit, + TxOutputType *out, TxOutputType *orig_out, + int page) { + if (page == 0) { + render_address_dialog(coin, out->address, _("Modify amount for"), + _("address:"), NULL); + } else { + char *question = NULL; + uint64_t amount_change = 0; + if (orig_out->amount < out->amount) { + question = _("Increase amount by:"); + amount_change = out->amount - orig_out->amount; + } else { + question = _("Decrease amount by:"); + amount_change = orig_out->amount - out->amount; + } + + char str_amount_change[32] = {0}; + format_coin_amount(amount_change, NULL, coin, amount_unit, + str_amount_change, sizeof(str_amount_change)); + + char str_amount_new[32] = {0}; + format_coin_amount(out->amount, NULL, coin, amount_unit, str_amount_new, + sizeof(str_amount_new)); + + layoutDialogSwipe(&bmp_icon_question, _("Cancel"), _("Confirm"), NULL, + question, str_amount_change, NULL, _("New amount:"), + str_amount_new, NULL); + } +} + void layoutConfirmModifyFee(const CoinInfo *coin, AmountUnit amount_unit, uint64_t fee_old, uint64_t fee_new) { char str_fee_change[32] = {0}; diff --git a/legacy/firmware/layout2.h b/legacy/firmware/layout2.h index 7f02a259a..df7abeeba 100644 --- a/legacy/firmware/layout2.h +++ b/legacy/firmware/layout2.h @@ -54,6 +54,9 @@ void layoutConfirmTx(const CoinInfo *coin, AmountUnit amount_unit, uint64_t total_in, uint64_t total_out, uint64_t change_out); void layoutConfirmReplacement(const char *description, uint8_t txid[32]); +void layoutConfirmModifyOutput(const CoinInfo *coin, AmountUnit amount_unit, + TxOutputType *out, TxOutputType *orig_out, + int page); void layoutConfirmModifyFee(const CoinInfo *coin, AmountUnit amount_unit, uint64_t fee_old, uint64_t fee_new); void layoutFeeOverThreshold(const CoinInfo *coin, AmountUnit amount_unit, diff --git a/legacy/firmware/signing.c b/legacy/firmware/signing.c index 5081ad0d9..762e3b1a5 100644 --- a/legacy/firmware/signing.c +++ b/legacy/firmware/signing.c @@ -296,6 +296,10 @@ static bool add_amount(uint64_t *dest, uint64_t amount) { return true; } +static bool is_rbf_enabled(TxInfo *tx_info) { + return tx_info->min_sequence <= MAX_BIP125_RBF_SEQUENCE; +} + void send_req_1_input(void) { signing_stage = STAGE_REQUEST_1_INPUT; resp.has_request_type = true; @@ -527,6 +531,21 @@ void phase1_request_next_input(void) { return; } + char *description = NULL; + if (!is_rbf_enabled(&info) && is_rbf_enabled(&orig_info)) { + description = _("Finalize TXID:"); + } else { + description = _("Update TXID:"); + } + + // Confirm original TXID. + layoutConfirmReplacement(description, orig_hash); + if (!protectButton(ButtonRequestType_ButtonRequest_SignTx, false)) { + fsm_sendFailure(FailureType_Failure_ActionCancelled, NULL); + signing_abort(); + return; + } + idx2 = 0; } @@ -1449,15 +1468,28 @@ static bool signing_check_orig_output(TxOutputType *orig_output) { return false; } - // Replacement transactions must not decrease the value of any external - // outputs. Furthermore, the only way to increase the amount would be by - // supplying an external input, which is currently not supported, so the - // external output amounts must remain unchanged. if (!is_change) { - if (output.amount != orig_output->amount) { + if (output.amount < orig_output->amount) { + // Replacement transactions may need to decrease the value of external + // outputs to bump the fee. This is needed if the original transaction + // transfers the entire account balance ("Send Max"). + for (int page = 0; page < 2; ++page) { + layoutConfirmModifyOutput(coin, amount_unit, &output, orig_output, + page); + if (!protectButton(ButtonRequestType_ButtonRequest_ConfirmOutput, + false)) { + fsm_sendFailure(FailureType_Failure_ActionCancelled, NULL); + signing_abort(); + return false; + } + } + } else if (output.amount > orig_output->amount) { + // Only PayJoin transactions may increase the value of external outputs + // by supplying an external input. However, external inputs are + // currently not supported. fsm_sendFailure( FailureType_Failure_ProcessError, - _("Changing original output amounts is not supported.")); + _("Increasing original output amounts is not supported.")); signing_abort(); return false; } @@ -1544,29 +1576,6 @@ static bool signing_confirm_tx(void) { return false; } - bool rbf_disabled = info.min_sequence > MAX_BIP125_RBF_SEQUENCE; - bool orig_rbf_disabled = orig_info.min_sequence > MAX_BIP125_RBF_SEQUENCE; - char *description = NULL; - if (rbf_disabled && !orig_rbf_disabled) { - description = _("Finalize TXID:"); - } else if (fee != orig_fee) { - description = _("Modify fee for TXID:"); - } else { - // The host might want to modify nSequence on some inputs (e.g. to - // re-enable RBF on a transaction that was dropped from the mempool), add - // more inputs and consolidate them in a change-output or use a different - // change-output address. - description = _("Update TXID:"); - } - - // Confirm original TXID. - layoutConfirmReplacement(description, orig_hash); - if (!protectButton(ButtonRequestType_ButtonRequest_SignTx, false)) { - fsm_sendFailure(FailureType_Failure_ActionCancelled, NULL); - signing_abort(); - return false; - } - // Fee modification. if (fee != orig_fee) { layoutConfirmModifyFee(coin, amount_unit, orig_fee, fee);