From 53dcf2b407f8a2b4ebe372a8f28b7ba073b9eda7 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 1a14d7af0..2ad82b2e0 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1607,8 +1607,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 2a5cc40dc428e9024142ebebeaa97a3a3a4a1fa6 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 f44311b0b..c474547b7 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -16,6 +16,7 @@ #include #include #include +#include #include #include @@ -374,4 +375,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 2ad82b2e0..060325921 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2830,7 +2830,14 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CTransac 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 57b22c0e4..7b791ae41 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -64,6 +64,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 7f2c0c1bd..7c8c6c9af 100755 --- a/test/functional/rpc_psbt.py +++ b/test/functional/rpc_psbt.py @@ -169,6 +169,10 @@ class PSBTTest(BitcoinTestFramework): 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) + # BIP 174 Test Vectors # Check that unknown values are just passed through