diff --git a/releases/Next-ChangeLog.md b/releases/Next-ChangeLog.md index f7ad4e3e..877c91b0 100644 --- a/releases/Next-ChangeLog.md +++ b/releases/Next-ChangeLog.md @@ -4,7 +4,9 @@ This lists the new changes that have not yet been published in a normal release. # Shared Improvements - Both Mk4 and Q - +- Bugfix: If all change outputs have `nValue=0` they're not shown in UX +- Bugfix: Disallow negative input/output amounts in PSBT +- Enhancement: Add warning for zero value outputs if not OP_RETURNs # Mk4 Specific Changes diff --git a/shared/auth.py b/shared/auth.py index 5fe4729d..8cfa4dab 100644 --- a/shared/auth.py +++ b/shared/auth.py @@ -358,6 +358,7 @@ class ApproveTransaction(UserAuthorizedAction): #print('FatalPSBTIssue: ' + exc.args[0]) return await self.failure(exc.args[0]) except BaseException as exc: + # sys.print_exception(exc) del self.psbt gc.collect() @@ -622,10 +623,12 @@ class ApproveTransaction(UserAuthorizedAction): largest_outs = [] largest_change = [] total_change = 0 + has_change = False for idx, tx_out in self.psbt.output_iter(): outp = self.psbt.outputs[idx] if outp.is_change: + has_change = True total_change += tx_out.nValue if len(largest_change) < MAX_VISIBLE_CHANGE: largest_change.append((tx_out.nValue, self.chain.render_address(tx_out.scriptPubKey))) @@ -677,7 +680,7 @@ class ApproveTransaction(UserAuthorizedAction): msg.write("\n") # change outputs - verified to be coming back to our wallet - if total_change > 0: + if has_change: msg.write("Change back:\n%s %s\n" % self.chain.render_value(total_change)) visible_change_sum = 0 if len(largest_change) == 1: diff --git a/shared/history.py b/shared/history.py index 3c9dc3dd..479d2580 100644 --- a/shared/history.py +++ b/shared/history.py @@ -132,7 +132,7 @@ class OutptValueCache: # save new addition assert len(key) == ENCKEY_LEN - assert amount > 0 + # assert amount > 0 entry = key + cls.encode_value(prevout, amount) cls.runtime_cache.append(entry) diff --git a/shared/psbt.py b/shared/psbt.py index a59652c3..6cf88f1d 100644 --- a/shared/psbt.py +++ b/shared/psbt.py @@ -1395,7 +1395,7 @@ class psbtObject(psbtProxy): assert not self.has_goc, "v0 requires exclusion of global output count" assert not self.has_gtv, "v0 requires exclusion of global txn version" assert self.txn, "v0 requires inclusion of global unsigned tx" - assert self.txn[1] > 63, 'txn too short' + assert self.txn[1] > 61, 'txn too short' assert self.fallback_locktime is None, "v0 requires exclusion of global fallback locktime" assert self.txn_modifiable is None, "v0 requires exclusion of global txn modifiable" @@ -1494,13 +1494,20 @@ class psbtObject(psbtProxy): num_op_return = 0 num_op_return_size = 0 num_unknown_scripts = 0 + zero_val_outs = 0 # only those that are not OP_RETURN are considered self.num_change_outputs = 0 for idx, txo in self.output_iter(): output = self.outputs[idx] # perform output validation af = output.validate(idx, txo, self.my_xfp, self.active_multisig, self) + assert txo.nValue >= 0, "negative output value: o%d" % idx total_out += txo.nValue + + if (txo.nValue == 0) and (af != "op_return"): + # OP_RETURN outputs have nValue=0 standard + zero_val_outs += 1 + if output.is_change: self.num_change_outputs += 1 total_change += txo.nValue @@ -1526,16 +1533,16 @@ class psbtObject(psbtProxy): '%s != %s' % (self.total_change_value, total_change) # check fee is reasonable - if self.total_value_out == 0: - per_fee = 100 - else: - the_fee = self.calculate_fee() - if the_fee is None: - return - if the_fee < 0: - raise FatalPSBTIssue("Outputs worth more than inputs!") + the_fee = self.calculate_fee() + if the_fee is None: + return + if the_fee < 0: + raise FatalPSBTIssue("Outputs worth more than inputs!") + if self.total_value_out: per_fee = the_fee * 100 / self.total_value_out + else: + per_fee = 100 fee_limit = settings.get('fee_limit', DEFAULT_MAX_FEE_PERCENTAGE) @@ -1562,6 +1569,12 @@ class psbtObject(psbtProxy): 'Sending to %d not well understood script(s).' % num_unknown_scripts) ) + if zero_val_outs: + self.warnings.append( + ('Zero Value', + 'Non-standard zero value output(s).') + ) + self.consolidation_tx = (self.num_change_outputs == self.num_outputs) # Enforce policy related to change outputs @@ -1703,7 +1716,7 @@ class psbtObject(psbtProxy): # pull out just the CTXOut object (expensive) utxo = inp.get_utxo(txi.prevout.n) - assert utxo.nValue > 0 + assert utxo.nValue >= 0, "negative input value: i%d" % i total_in += utxo.nValue # Look at what kind of input this will be, and therefore what @@ -1723,7 +1736,7 @@ class psbtObject(psbtProxy): if not foreign: # no foreign inputs, we can calculate the total input value - assert total_in > 0 + assert total_in > 0, "zero value txn" self.total_value_in = total_in else: # 1+ inputs don't belong to us, we can't calculate the total input value diff --git a/testing/test_sign.py b/testing/test_sign.py index 44e6195e..70698cc8 100644 --- a/testing/test_sign.py +++ b/testing/test_sign.py @@ -3135,6 +3135,97 @@ def test_null_data_op_return(fake_txn, start_sign, end_sign, reset_seed_words): assert "null-data" in story assert "OP_RETURN" in story +def test_smallest_txn(fake_txn, start_sign, end_sign, reset_seed_words, settings_set): + # serialized txn has just 62 bytes and is the smallest that we support + # 1 input (iregardless of script type) and 1 zero value null OP_RETURN + reset_seed_words() + settings_set('fee_limit', -1) + psbt = fake_txn(1, 0, op_return=[(10, b"")], input_amount=10) + start_sign(psbt, False, stxn_flags=STXN_VISUALIZE) + story = end_sign(accept=None, expect_txn=False).decode() + assert "null-data" in story + assert "OP_RETURN" in story + + +@pytest.mark.parametrize("num_outs", [1, 12]) +@pytest.mark.parametrize("change", [True, False]) +def test_zero_value_outputs(num_outs, change, fake_txn, start_sign, end_sign, reset_seed_words, + settings_set): + reset_seed_words() + # user needs to disable fee limit checks to be able to do this + settings_set("fee_limit", -1) + change_outs = list(range(num_outs)) if change else [] + psbt = fake_txn(1, num_outs, outvals=num_outs*[0], change_outputs=change_outs, input_amount=1) + start_sign(psbt, False, stxn_flags=STXN_VISUALIZE) + story = end_sign(accept=None, expect_txn=False).decode() + assert f"Zero Value: Non-standard zero value outputs: {num_outs}" in story + assert "1 input" in story + assert f"{num_outs} output{'' if num_outs == 1 else 's'}" in story + assert 'Network fee 0.00000001 XTN' in story + + if change: + assert "0.00000000 XTN" in story.split("\n\n")[4] # change back is zero + assert "Consolidating 0.00000000 XTN" in story + assert "Change back" in story + if num_outs > 1: + assert "to addresses" in story + else: + assert "to address" in story + else: + # even + if num_outs == 12: + # even tho we do not see 2 outputs, fee is also 0 and 2 smaller not shown here have also value o 0 + assert story.count('0.00000000 XTN') == 12 + else: + assert story.count('0.00000000 XTN') == 2 + assert "Change back" not in story + + +@pytest.mark.parametrize("change", [True, False]) +@pytest.mark.parametrize("num_ins", [True, False]) +def test_zero_value_input(change, num_ins, fake_txn, start_sign, end_sign, reset_seed_words, + cap_story): + # 0 value inputs - not allowed + reset_seed_words() + psbt = fake_txn(1, 1, fee=0, input_amount=0) + start_sign(psbt, False, stxn_flags=STXN_VISUALIZE) + with pytest.raises(Exception): + end_sign(accept=None, expect_txn=False).decode() + + _, story = cap_story() + assert "zero value txn" in story + + +@pytest.mark.parametrize("change", [True, False]) +def test_zero_value_inputs(change, fake_txn, start_sign, end_sign, reset_seed_words): + # one input is-non zero + # others are zero --> allowed + reset_seed_words() + invals = [0 for i in range(4)] + [100] + psbt = fake_txn(5, 1, invals=invals, outvals=[99], change_outputs=[0] if change else [], fee=20) + start_sign(psbt, False, stxn_flags=STXN_VISUALIZE) + end_sign(accept=None, expect_txn=False).decode() + + +def test_negative_amount_inputs(reset_seed_words, fake_txn, start_sign, end_sign, cap_story): + reset_seed_words() + psbt = fake_txn(1, 1, fee=0, input_amount=-1000) + start_sign(psbt, False, stxn_flags=STXN_VISUALIZE) + with pytest.raises(Exception): + end_sign(accept=None, expect_txn=False).decode() + + _, story = cap_story() + assert "negative input value: i0" in story + +def test_negative_amount_outputs(reset_seed_words, fake_txn, start_sign, end_sign, cap_story): + reset_seed_words() + psbt = fake_txn(1, 1, outvals=[-1000], fee=0) + start_sign(psbt, False, stxn_flags=STXN_VISUALIZE) + with pytest.raises(Exception): + end_sign(accept=None, expect_txn=False).decode() + + _, story = cap_story() + assert "negative output value: o0" in story def test_mk4_done_signing_infinite_loop(goto_home, try_sign, fake_txn, enable_hw_ux, settings_get, is_q1):