From 4443c337f6337aa7fcb76bceb3987ad6a1fbbaf2 Mon Sep 17 00:00:00 2001 From: satoshiotomakan <127754187+satoshiotomakan@users.noreply.github.com> Date: Fri, 30 Jun 2023 10:54:17 +0200 Subject: [PATCH] fix(bitcoin): Fix `TWAnySignerPlan` (#3278) --- src/Bitcoin/TransactionBuilder.cpp | 24 +++---- src/Bitcoin/TransactionBuilder.h | 6 +- src/Bitcoin/TransactionPlan.h | 3 - src/Decred/TransactionBuilder.h | 7 +-- src/Zen/TransactionBuilder.h | 6 +- .../chains/Bitcoin/TWBitcoinSigningTests.cpp | 63 ++++++++++++++++++- tests/chains/Bitcoin/TxComparisonHelper.cpp | 1 + tests/chains/Zen/TransactionBuilderTests.cpp | 2 - 8 files changed, 77 insertions(+), 35 deletions(-) diff --git a/src/Bitcoin/TransactionBuilder.cpp b/src/Bitcoin/TransactionBuilder.cpp index 6cad21e6b52..d703ce64754 100644 --- a/src/Bitcoin/TransactionBuilder.cpp +++ b/src/Bitcoin/TransactionBuilder.cpp @@ -99,7 +99,7 @@ TransactionPlan TransactionBuilder::plan(const SigningInput& input) { auto inputSum = InputSelector::sum(input.utxos); // select UTXOs - plan.amount = input.totalAmount; + plan.amount = input.amount; // if amount requested is the same or more than available amount, it cannot be satisfied, but // treat this case as MaxAmount, and send maximum available (which will be less) @@ -114,14 +114,12 @@ TransactionPlan TransactionBuilder::plan(const SigningInput& input) { output_size = 2 + extraOutputs; // output + change if (input.useMaxUtxo) { selectedInputs = inputSelector.selectMaxAmount(input.byteFee); + } else if (input.utxos.size() <= SimpleModeLimit && + input.utxos.size() <= MaxUtxosHardLimit) { + selectedInputs = inputSelector.select(plan.amount, input.byteFee, output_size); } else { - if (input.utxos.size() <= SimpleModeLimit && - input.utxos.size() <= MaxUtxosHardLimit) { - selectedInputs = inputSelector.select(plan.amount, input.byteFee, output_size); - } else { - selectedInputs = - inputSelector.selectSimple(plan.amount, input.byteFee, output_size); - } + selectedInputs = + inputSelector.selectSimple(plan.amount, input.byteFee, output_size); } } else { output_size = 1 + extraOutputs; // output, no change @@ -137,9 +135,12 @@ TransactionPlan TransactionBuilder::plan(const SigningInput& input) { } } - if (plan.utxos.size() == 0) { + if (plan.utxos.empty()) { plan.amount = 0; plan.error = Common::Proto::Error_not_enough_utxos; + } else if (maxAmount && !input.extraOutputs.empty()) { + plan.amount = 0; + plan.error = Common::Proto::Error_invalid_params; } else { plan.availableAmount = InputSelector::sum(plan.utxos); @@ -147,9 +148,9 @@ TransactionPlan TransactionBuilder::plan(const SigningInput& input) { // must preliminary set change so that there is a second output if (!maxAmount) { assert(input.totalAmount <= plan.availableAmount); - plan.amount = input.totalAmount; + plan.amount = input.amount; plan.fee = 0; - plan.change = plan.availableAmount - plan.amount; + plan.change = plan.availableAmount - input.totalAmount; } else { plan.amount = plan.availableAmount; plan.fee = 0; @@ -167,7 +168,6 @@ TransactionPlan TransactionBuilder::plan(const SigningInput& input) { } else { // max available amount plan.amount = std::max(Amount(0), plan.availableAmount - plan.fee); - plan.useMaxAmount = true; } assert(plan.amount >= 0 && plan.amount <= plan.availableAmount); diff --git a/src/Bitcoin/TransactionBuilder.h b/src/Bitcoin/TransactionBuilder.h index 75331b8d860..aa1c4db4737 100644 --- a/src/Bitcoin/TransactionBuilder.h +++ b/src/Bitcoin/TransactionBuilder.h @@ -30,11 +30,7 @@ class TransactionBuilder { Transaction tx; tx.lockTime = input.lockTime; - auto outputToAmount = input.amount; - if (plan.useMaxAmount) { - outputToAmount = plan.amount; - } - auto outputTo = prepareOutputWithScript(input.toAddress, outputToAmount, input.coinType); + auto outputTo = prepareOutputWithScript(input.toAddress, plan.amount, input.coinType); if (!outputTo.has_value()) { return Result::failure(Common::Proto::Error_invalid_address); } diff --git a/src/Bitcoin/TransactionPlan.h b/src/Bitcoin/TransactionPlan.h index 1bc01a57a49..f41a93e19b5 100644 --- a/src/Bitcoin/TransactionPlan.h +++ b/src/Bitcoin/TransactionPlan.h @@ -41,9 +41,6 @@ struct TransactionPlan { Data outputOpReturn; - /// Check if we use max amount for output address - bool useMaxAmount = false; - Common::Proto::SigningError error = Common::Proto::SigningError::OK; TransactionPlan() = default; diff --git a/src/Decred/TransactionBuilder.h b/src/Decred/TransactionBuilder.h index 4d0257e6fdc..0fafce4195d 100644 --- a/src/Decred/TransactionBuilder.h +++ b/src/Decred/TransactionBuilder.h @@ -29,18 +29,13 @@ struct TransactionBuilder { static Transaction build(const Bitcoin::TransactionPlan& plan, const Bitcoin::SigningInput& input) { auto coin = TWCoinTypeDecred; - auto outputToAmount = input.amount; - if (plan.useMaxAmount) { - outputToAmount = plan.amount; - } - auto lockingScriptTo = Bitcoin::Script::lockScriptForAddress(input.toAddress, coin); if (lockingScriptTo.empty()) { return {}; } Transaction tx; - tx.outputs.emplace_back(TransactionOutput(outputToAmount, /* version: */ 0, lockingScriptTo)); + tx.outputs.emplace_back(TransactionOutput(plan.amount, /* version: */ 0, lockingScriptTo)); if (plan.change > 0) { auto lockingScriptChange = Bitcoin::Script::lockScriptForAddress(input.changeAddress, coin); diff --git a/src/Zen/TransactionBuilder.h b/src/Zen/TransactionBuilder.h index 641e1b42756..fddea3bfc48 100644 --- a/src/Zen/TransactionBuilder.h +++ b/src/Zen/TransactionBuilder.h @@ -37,11 +37,7 @@ struct TransactionBuilder { auto blockHash = plan.preBlockHash; auto blockHeight = plan.preBlockHeight; - auto outputToAmount = input.amount; - if (plan.useMaxAmount) { - outputToAmount = plan.amount; - } - auto outputTo = prepareOutputWithScript(input.toAddress, outputToAmount, input.coinType, blockHash, blockHeight); + auto outputTo = prepareOutputWithScript(input.toAddress, plan.amount, input.coinType, blockHash, blockHeight); if (!outputTo.has_value()) { return Result::failure(Common::Proto::Error_invalid_address); } diff --git a/tests/chains/Bitcoin/TWBitcoinSigningTests.cpp b/tests/chains/Bitcoin/TWBitcoinSigningTests.cpp index 9e370c77baf..75136722db7 100644 --- a/tests/chains/Bitcoin/TWBitcoinSigningTests.cpp +++ b/tests/chains/Bitcoin/TWBitcoinSigningTests.cpp @@ -82,6 +82,65 @@ SigningInput buildInputP2PKH(bool omitKey = false) { return input; } +/// This test only checks if the transaction output will have an expected value. +/// It doesn't check correctness of the encoded representation. +/// Issue: https://github.com/trustwallet/wallet-core/issues/3273 +TEST(BitcoinSigning, SignMaxAmount) { + const auto privateKey = parse_hex("4646464646464646464646464646464646464646464646464646464646464646"); + const auto ownAddress = "bc1qhkfq3zahaqkkzx5mjnamwjsfpq2jk7z00ppggv"; + + const auto revUtxoHash0 = + parse_hex("07c42b969286be06fae38528c85f0a1ce508d4df837eb5ac4cf5f2a7a9d65fa8"); + const auto inPubKey0 = + parse_hex("024bc2a31265153f07e70e0bab08724e6b85e217f8cd628ceb62974247bb493382"); + const auto inPubKeyHash0 = parse_hex("bd92088bb7e82d611a9b94fbb74a0908152b784f"); + const auto utxoScript0 = parse_hex("0014bd92088bb7e82d611a9b94fbb74a0908152b784f"); + + const auto initialAmount = 10'189'533; + const auto availableAmount = 10'189'534; + const auto fee = 110; + const auto amountWithoutFee = availableAmount - fee; + // There shouldn't be any change + const auto change = 0; + + Proto::SigningInput signingInput; + signingInput.set_coin_type(TWCoinTypeBitcoin); + signingInput.set_hash_type(TWBitcoinSigHashTypeAll); + signingInput.set_amount(initialAmount); + signingInput.set_byte_fee(1); + signingInput.set_to_address("bc1q2dsdlq3343vk29runkgv4yc292hmq53jedfjmp"); + signingInput.set_change_address(ownAddress); + signingInput.set_use_max_amount(true); + + *signingInput.add_private_key() = std::string(privateKey.begin(), privateKey.end()); + + // Add UTXO + auto utxo = signingInput.add_utxo(); + utxo->set_script(utxoScript0.data(), utxoScript0.size()); + utxo->set_amount(availableAmount); + utxo->mutable_out_point()->set_hash( + std::string(revUtxoHash0.begin(), revUtxoHash0.end())); + utxo->mutable_out_point()->set_index(0); + utxo->mutable_out_point()->set_sequence(UINT32_MAX); + + // Plan + Proto::TransactionPlan plan; + ANY_PLAN(signingInput, plan, TWCoinTypeBitcoin); + // Plan is checked, assume it is accepted + EXPECT_EQ(plan.amount(), amountWithoutFee); + EXPECT_EQ(plan.available_amount(), availableAmount); + EXPECT_EQ(plan.fee(), fee); + EXPECT_EQ(plan.change(), change); + + *signingInput.mutable_plan() = plan; + + Proto::SigningOutput output; + ANY_SIGN(signingInput, TWCoinTypeBitcoin); + + const auto& output0 = output.transaction().outputs().at(0); + EXPECT_EQ(output0.value(), amountWithoutFee); +} + TEST(BitcoinSigning, SignBRC20TransferCommit) { auto privateKey = parse_hex("e253373989199da27c48680e3a3fc0f648d50f9a727ef17a7fe6a4dc3b159129"); auto fullAmount = 26400; @@ -589,11 +648,11 @@ TEST(BitcoinSigning, SignP2WPKH_HashAnyoneCanPay_TwoInput) { TEST(BitcoinSigning, SignP2WPKH_MaxAmount) { auto input = buildInputP2WPKH(1'000, TWBitcoinSigHashTypeAll, 625'000'000, 600'000'000, true); - input.totalAmount = 1224999773; + input.totalAmount = 1'224'999'773; { // test plan (but do not reuse plan result) auto plan = TransactionBuilder::plan(input); - EXPECT_TRUE(verifyPlan(plan, {625'000'000, 600'000'000}, 1224999773, 227)); + EXPECT_TRUE(verifyPlan(plan, {625'000'000, 600'000'000}, 1'224'999'773, 227)); } // Sign diff --git a/tests/chains/Bitcoin/TxComparisonHelper.cpp b/tests/chains/Bitcoin/TxComparisonHelper.cpp index 1e7bec24a4e..8120401fa7f 100644 --- a/tests/chains/Bitcoin/TxComparisonHelper.cpp +++ b/tests/chains/Bitcoin/TxComparisonHelper.cpp @@ -43,6 +43,7 @@ UTXOs buildTestUTXOs(const std::vector& amounts) { SigningInput buildSigningInput(Amount amount, int byteFee, const UTXOs& utxos, bool useMaxAmount, enum TWCoinType coin, bool omitPrivateKey) { SigningInput input; + input.amount = amount; input.totalAmount = amount; input.byteFee = byteFee; input.useMaxAmount = useMaxAmount; diff --git a/tests/chains/Zen/TransactionBuilderTests.cpp b/tests/chains/Zen/TransactionBuilderTests.cpp index 8575d8ed089..3c6ea1df885 100644 --- a/tests/chains/Zen/TransactionBuilderTests.cpp +++ b/tests/chains/Zen/TransactionBuilderTests.cpp @@ -66,7 +66,6 @@ TEST(ZenTransactionBuilder, Build) { ASSERT_EQ(plan.fee, 294); plan.preBlockHash = blockHash; plan.preBlockHeight = blockHeight; - plan.useMaxAmount = true; // plan1 auto result = Zen::TransactionBuilder::build(plan, input).payload(); @@ -75,7 +74,6 @@ TEST(ZenTransactionBuilder, Build) { ASSERT_EQ(result.outputs[0].value, plan.amount); // plan2 - plan.useMaxAmount = false; result = Zen::TransactionBuilder::build(plan, input).payload(); ASSERT_EQ(result.outputs.size(), 4ul);