From b06483c96a83af3f7721d01c4cafe3edf5909552 Mon Sep 17 00:00:00 2001 From: Gregory Sanders Date: Wed, 3 Oct 2018 14:40:47 +0900 Subject: [PATCH 1/2] Remove stale comment in CalculateMaximumSignedInputSize --- src/wallet/wallet.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 8ea4c5c4957d1..79e3b5b22ba43 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1530,8 +1530,6 @@ int CalculateMaximumSignedInputSize(const CTxOut& txout, const CWallet* wallet, CMutableTransaction txn; txn.vin.push_back(CTxIn(COutPoint())); if (!wallet->DummySignInput(txn.vin[0], txout, use_max_sig)) { - // This should never happen, because IsAllFromMe(ISMINE_SPENDABLE) - // implies that we can sign for every input. return -1; } return GetVirtualTransactionInputSize(txn.vin[0]); From 0fb2e69815bd5146e601a7fd3585f21a1fdd6f5d Mon Sep 17 00:00:00 2001 From: Gregory Sanders Date: Wed, 3 Oct 2018 14:41:03 +0900 Subject: [PATCH 2/2] CreateTransaction: Assume minimum p2sh-p2wpkh spend size for unknown change --- src/wallet/test/wallet_tests.cpp | 44 ++++++++++++++++++++++++++++++++ src/wallet/wallet.cpp | 9 ++++++- src/wallet/wallet.h | 3 +++ test/functional/rpc_psbt.py | 4 +++ 4 files changed, 59 insertions(+), 1 deletion(-) diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index c6aac8aad5666..623c5c39a212e 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -17,6 +17,7 @@ #include #include #include +#include #include #include @@ -394,4 +395,47 @@ BOOST_FIXTURE_TEST_CASE(wallet_disableprivkeys, TestChain100Setup) BOOST_CHECK(!wallet->GetKeyFromPool(pubkey, false)); } +// Explicit calculation which is used to test the wallet constant +// We get the same virtual size due to rounding(weight/4) for both use_max_sig values +static size_t CalculateNestedKeyhashInputSize(bool use_max_sig) +{ + // Generate ephemeral valid pubkey + CKey key; + key.MakeNewKey(true); + CPubKey pubkey = key.GetPubKey(); + + // Generate pubkey hash + uint160 key_hash(Hash160(pubkey.begin(), pubkey.end())); + + // Create inner-script to enter into keystore. Key hash can't be 0... + CScript inner_script = CScript() << OP_0 << std::vector(key_hash.begin(), key_hash.end()); + + // Create outer P2SH script for the output + uint160 script_id(Hash160(inner_script.begin(), inner_script.end())); + CScript script_pubkey = CScript() << OP_HASH160 << std::vector(script_id.begin(), script_id.end()) << OP_EQUAL; + + // Add inner-script to key store and key to watchonly + CBasicKeyStore keystore; + keystore.AddCScript(inner_script); + keystore.AddKeyPubKey(key, pubkey); + + // Fill in dummy signatures for fee calculation. + SignatureData sig_data; + + if (!ProduceSignature(keystore, use_max_sig ? DUMMY_MAXIMUM_SIGNATURE_CREATOR : DUMMY_SIGNATURE_CREATOR, script_pubkey, sig_data)) { + // We're hand-feeding it correct arguments; shouldn't happen + assert(false); + } + + CTxIn tx_in; + UpdateInput(tx_in, sig_data); + return (size_t)GetVirtualTransactionInputSize(tx_in); +} + +BOOST_FIXTURE_TEST_CASE(dummy_input_size_test, TestChain100Setup) +{ + BOOST_CHECK_EQUAL(CalculateNestedKeyhashInputSize(false), DUMMY_NESTED_P2WPKH_INPUT_SIZE); + BOOST_CHECK_EQUAL(CalculateNestedKeyhashInputSize(true), DUMMY_NESTED_P2WPKH_INPUT_SIZE); +} + BOOST_AUTO_TEST_SUITE_END() diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 79e3b5b22ba43..eff94455db804 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2753,7 +2753,14 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std if (pick_new_inputs) { nValueIn = 0; setCoins.clear(); - coin_selection_params.change_spend_size = CalculateMaximumSignedInputSize(change_prototype_txout, this); + int change_spend_size = CalculateMaximumSignedInputSize(change_prototype_txout, this); + // If the wallet doesn't know how to sign change output, assume p2sh-p2wpkh + // as lower-bound to allow BnB to do it's thing + if (change_spend_size == -1) { + coin_selection_params.change_spend_size = DUMMY_NESTED_P2WPKH_INPUT_SIZE; + } else { + coin_selection_params.change_spend_size = (size_t)change_spend_size; + } coin_selection_params.effective_fee = nFeeRateNeeded; if (!SelectCoins(vAvailableCoins, nValueToSelect, setCoins, nValueIn, coin_control, coin_selection_params, bnb_used)) { diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index f96798201f1fa..4291163bea94a 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -85,6 +85,9 @@ static const bool DEFAULT_WALLET_RBF = false; static const bool DEFAULT_WALLETBROADCAST = true; static const bool DEFAULT_DISABLE_WALLET = false; +//! Pre-calculated constants for input size estimation in *virtual size* +static constexpr size_t DUMMY_NESTED_P2WPKH_INPUT_SIZE = 91; + class CBlockIndex; class CCoinControl; class COutput; diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py index ba3818bf24f5e..c651abcfb4096 100755 --- a/test/functional/rpc_psbt.py +++ b/test/functional/rpc_psbt.py @@ -207,6 +207,10 @@ def run_test(self): assert tx_in["sequence"] > MAX_BIP125_RBF_SEQUENCE assert_equal(decoded_psbt["tx"]["locktime"], 0) + # Make sure change address wallet does not have P2SH innerscript access to results in success + # when attempting BnB coin selection + self.nodes[0].walletcreatefundedpsbt([], [{self.nodes[2].getnewaddress():unspent["amount"]+1}], block_height+2, {"changeAddress":self.nodes[1].getnewaddress()}, False) + # Regression test for 14473 (mishandling of already-signed witness transaction): psbtx_info = self.nodes[0].walletcreatefundedpsbt([{"txid":unspent["txid"], "vout":unspent["vout"]}], [{self.nodes[2].getnewaddress():unspent["amount"]+1}]) complete_psbt = self.nodes[0].walletprocesspsbt(psbtx_info["psbt"])