GlcoinCore/elements
1
0
Fork 0
mirror of https://github.com/ElementsProject/elements.git synced 2026-08-16 13:01:19 +02:00
Code Issues Projects Releases Packages Wiki Activity Actions

Merge pull request #1172 from apoelstra/2022-09--wallet-fix

Browse source 
wallet: don't clear out all the blinding data when dropping change
This commit is contained in:
Pablo Greco 2022-09-26 11:43:30 -03:00 • committed by GitHub
commit f1dd3de8e1
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
5 changed files with 232 additions and 41 deletions
Download patch file Download diff file Expand all files Collapse all files

138
src/wallet/spend.cpp
View file

@ -718,25 +718,29 @@ static uint32_t GetLocktimeForNewTransaction(interfaces::Chain& chain, const uin
}
// Reset all non-global blinding details.
void resetBlindDetails(BlindDetails* det) {
static void resetBlindDetails(BlindDetails* det, bool preserve_output_data = false) {
det->i_amount_blinds.clear();
det->i_asset_blinds.clear();
det->i_assets.clear();
det->i_amounts.clear();
det->o_amounts.clear();
det->o_pubkeys.clear();
if (!preserve_output_data) {
det->o_pubkeys.clear();
}
det->o_amount_blinds.clear();
det->o_assets.clear();
det->o_asset_blinds.clear();
det->num_to_blind = 0;
det->change_to_blind = 0;
det->only_recipient_blind_index = -1;
det->only_change_pos = -1;
if (!preserve_output_data) {
det->num_to_blind = 0;
det->change_to_blind = 0;
det->only_recipient_blind_index = -1;
det->only_change_pos = -1;
}
}
bool fillBlindDetails(BlindDetails* det, CWallet* wallet, CMutableTransaction& txNew, std::vector<CInputCoin>& selected_coins, bilingual_str& error) {
static bool fillBlindDetails(BlindDetails* det, CWallet* wallet, CMutableTransaction& txNew, std::vector<CInputCoin>& selected_coins, bilingual_str& error) {
int num_inputs_blinded = 0;
// Fill in input blinding details
@ -1032,9 +1036,13 @@ bool CWallet::CreateTransactionInternal(
if (!coin_selection_params.m_subtract_fee_outputs) {
coin_selection_params.tx_noinputs_size = 11; // Static vsize overhead + outputs vsize. 4 nVersion, 4 nLocktime, 1 input count, 1 output count, 1 witness overhead (dummy, flag, stack size)
if (g_con_elementsmode) {
coin_selection_params.tx_noinputs_size += 44; // change output: 9 bytes value, 1 byte scriptPubKey, 33 bytes asset, 1 byte nonce
coin_selection_params.tx_noinputs_size += 46; // fee output: 9 bytes value, 1 byte scriptPubKey, 33 bytes asset, 1 byte nonce, 1 byte each for null rangeproof/surjectionproof
}
}
// ELEMENTS: If we have blinded inputs but no blinded outputs (which, since the wallet
// makes an effort to not produce change, is a common case) then we need to add a
// dummy output.
bool may_need_blinded_dummy = !!blind_details;
for (const auto& recipient : vecSend)
{
CTxOut txout(recipient.asset, recipient.nAmount, recipient.scriptPubKey);
@ -1056,6 +1064,7 @@ bool CWallet::CreateTransactionInternal(
if (blind_details) {
blind_details->o_pubkeys.push_back(recipient.confidentiality_key);
if (blind_details->o_pubkeys.back().IsFullyValid()) {
may_need_blinded_dummy = false;
blind_details->num_to_blind++;
blind_details->only_recipient_blind_index = txNew.vout.size()-1;
if (!coin_selection_params.m_subtract_fee_outputs) {
@ -1064,6 +1073,35 @@ bool CWallet::CreateTransactionInternal(
}
}
}
if (may_need_blinded_dummy && !coin_selection_params.m_subtract_fee_outputs) {
// dummy output: 33 bytes value, 2 byte scriptPubKey, 33 bytes asset, 1 byte nonce, 66 bytes dummy rangeproof, 1 byte null surjectionproof
// FIXME actually, we currently just hand off to BlindTransaction which will put
// a full rangeproof and surjectionproof. We should fix this when we overhaul
// the blinding logic.
coin_selection_params.tx_noinputs_size += 70 + 66 +(MAX_RANGEPROOF_SIZE + DEFAULT_SURJECTIONPROOF_SIZE + WITNESS_SCALE_FACTOR - 1)/WITNESS_SCALE_FACTOR;
}
// If we are going to issue an asset, add the issuance data to the noinputs_size so that
// we allocate enough coins for them.
if (issuance_details) {
size_t issue_count = 0;
for (unsigned int i = 0; i < txNew.vout.size(); i++) {
if (txNew.vout[i].nAsset.IsExplicit() && txNew.vout[i].nAsset.GetAsset() == CAsset(uint256S("1"))) {
issue_count++;
} else if (txNew.vout[i].nAsset.IsExplicit() && txNew.vout[i].nAsset.GetAsset() == CAsset(uint256S("2"))) {
issue_count++;
}
}
if (issue_count > 0) {
// Allocate space for blinding nonce, entropy, and whichever of nAmount/nInflationKeys is null
coin_selection_params.tx_noinputs_size += 2 * 32 + 2 * (2 - issue_count);
}
// Allocate non-null nAmount/nInflationKeys and rangeproofs
if (issuance_details->blind_issuance) {
coin_selection_params.tx_noinputs_size += issue_count * (33 * WITNESS_SCALE_FACTOR + MAX_RANGEPROOF_SIZE + WITNESS_SCALE_FACTOR - 1) / WITNESS_SCALE_FACTOR;
} else {
coin_selection_params.tx_noinputs_size += issue_count * 9;
}
}
// Include the fees for things that aren't inputs, excluding the change output
const CAmount not_input_fees = coin_selection_params.m_effective_feerate.GetFee(coin_selection_params.tx_noinputs_size);
@ -1087,6 +1125,17 @@ bool CWallet::CreateTransactionInternal(
return false;
}
// If all of our inputs are explicit, we don't need a blinded dummy
if (may_need_blinded_dummy) {
may_need_blinded_dummy = false;
for (const auto& coin : setCoins) {
if (!coin.txout.nValue.IsExplicit()) {
may_need_blinded_dummy = true;
break;
}
}
}
// Always make a change output
// We will reduce the fee from this change output later, and remove the output if it is too small.
// ELEMENTS: wrap this all in a loop, set nChangePosInOut specifically for policy asset
@ -1342,36 +1391,46 @@ bool CWallet::CreateTransactionInternal(
CAmount change_amount = change_position->nValue.GetAmount();
if (IsDust(*change_position, coin_selection_params.m_discard_feerate) || change_amount <= coin_selection_params.m_cost_of_change)
{
txNew.vout.erase(change_position);
bool was_blinded = blind_details && blind_details->o_pubkeys[nChangePosInOut].IsValid();
change_pos[nChangePosInOut] = std::nullopt;
tx_blinded.vout.erase(tx_blinded.vout.begin() + nChangePosInOut);
if (tx_blinded.witness.vtxoutwit.size() > (unsigned) nChangePosInOut) {
tx_blinded.witness.vtxoutwit.erase(tx_blinded.witness.vtxoutwit.begin() + nChangePosInOut);
}
if (blind_details) {
bool was_blinded = blind_details->o_pubkeys[nChangePosInOut].IsValid();
// If the change was blinded, and was the only blinded output, we cannot drop it
// without causing the transaction to fail to balance. So keep it, and merely
// zero it out.
if (was_blinded && blind_details->num_to_blind == 1) {
assert (may_need_blinded_dummy);
change_position->scriptPubKey = CScript() << OP_RETURN;
change_position->nValue = 0;
} else {
txNew.vout.erase(change_position);
blind_details->o_amounts.erase(blind_details->o_amounts.begin() + nChangePosInOut);
blind_details->o_assets.erase(blind_details->o_assets.begin() + nChangePosInOut);
blind_details->o_pubkeys.erase(blind_details->o_pubkeys.begin() + nChangePosInOut);
// If change_amount == 0, we did not increment num_to_blind initially
// and therefore do not need to decrement it here.
if (was_blinded) {
blind_details->num_to_blind--;
blind_details->change_to_blind--;
change_pos[nChangePosInOut] = std::nullopt;
tx_blinded.vout.erase(tx_blinded.vout.begin() + nChangePosInOut);
if (tx_blinded.witness.vtxoutwit.size() > (unsigned) nChangePosInOut) {
tx_blinded.witness.vtxoutwit.erase(tx_blinded.witness.vtxoutwit.begin() + nChangePosInOut);
}
if (blind_details) {
// FIXME: I promise this makes sense and fixes an actual problem
// with the wallet that users could encounter. But no human could
// follow the logic as to what this does or why it is safe. After
// the 22.0 rebase we need to double-back and replace the blinding
// logic to eliminate a bunch of edge cases and make this logic
// incomprehensible. But in the interest of minimizing diff during
// the rebase I am going to do this for now.
if (blind_details->num_to_blind == 1) {
resetBlindDetails(blind_details);
if (!fillBlindDetails(blind_details, this, txNew, selected_coins, error)) {
return false;
blind_details->o_amounts.erase(blind_details->o_amounts.begin() + nChangePosInOut);
blind_details->o_assets.erase(blind_details->o_assets.begin() + nChangePosInOut);
blind_details->o_pubkeys.erase(blind_details->o_pubkeys.begin() + nChangePosInOut);
// If change_amount == 0, we did not increment num_to_blind initially
// and therefore do not need to decrement it here.
if (was_blinded) {
blind_details->num_to_blind--;
blind_details->change_to_blind--;
// FIXME: If we drop the change *and* this means we have only one
// blinded output *and* we have no blinded inputs, then this puts
// us in a situation where BlindTransaction will fail. This is
// prevented in fillBlindDetails, which adds an OP_RETURN output
// to handle this case. So do this ludicrous hack to accomplish
// this. This whole lump of un-followable-logic needs to be replaced
// by a complete rewriting of the wallet blinding logic.
if (blind_details->num_to_blind < 2) {
resetBlindDetails(blind_details, true /* don't wipe output data */);
if (!fillBlindDetails(blind_details, this, txNew, selected_coins, error)) {
return false;
}
}
}
}
@ -1385,6 +1444,10 @@ bool CWallet::CreateTransactionInternal(
fee_needed = coin_selection_params.m_effective_feerate.GetFee(nBytes);
}
// The only time that fee_needed should be less than the amount available for fees (in change_and_fee - change_amount) is when
// we are subtracting the fee from the outputs. If this occurs at any other time, it is a bug.
assert(coin_selection_params.m_subtract_fee_outputs || fee_needed <= map_change_and_fee.at(policyAsset) - change_amount);
// Update nFeeRet in case fee_needed changed due to dropping the change output
if (fee_needed <= map_change_and_fee.at(policyAsset) - change_amount) {
nFeeRet = map_change_and_fee.at(policyAsset) - change_amount;
@ -1479,9 +1542,9 @@ bool CWallet::CreateTransactionInternal(
summary += strprintf("#%d: %s%s [%s] (%s [%s])\n", i,
txNew.vout[i].IsFee() ? "[fee] " : "",
unblinded.nValue.GetAmount(),
txNew.vout[i].nValue.IsExplicit() ? "explicit" : "blinded",
blind_details->o_pubkeys[i].IsValid() ? "blinded" : "explicit",
unblinded.nAsset.GetAsset().GetHex(),
txNew.vout[i].nAsset.IsExplicit() ? "explicit" : "blinded"
blind_details->o_pubkeys[i].IsValid() ? "blinded" : "explicit"
);
}
WalletLogPrintf(summary+"\n");
@ -1498,6 +1561,7 @@ bool CWallet::CreateTransactionInternal(
int ret = BlindTransaction(blind_details->i_amount_blinds, blind_details->i_asset_blinds, blind_details->i_assets, blind_details->i_amounts, blind_details->o_amount_blinds, blind_details->o_asset_blinds, blind_details->o_pubkeys, issuance_asset_keys, issuance_token_keys, txNew);
assert(ret != -1);
if (ret != blind_details->num_to_blind) {
WalletLogPrintf("ERROR: tried to blind %d outputs but only blinded %d\n", (int) blind_details->num_to_blind, (int) ret);
error = _("Unable to blind the transaction properly. This should not happen.");
return false;
}

1
test/functional/test_runner.py
View file

@ -109,6 +109,7 @@ BASE_SCRIPTS = [
'feature_initial_reissuance_token.py',
'feature_progress.py',
'rpc_getnewblockhex.py',
'wallet_elements_regression_1172.py',
# Longest test should go first, to favor running tests in parallel
'wallet_hd.py --legacy-wallet',
'wallet_hd.py --descriptors',

126
test/functional/wallet_elements_regression_1172.py Executable file
View file

@ -0,0 +1,126 @@
#!/usr/bin/env python3
# Copyright (c) 2017-2020 The Bitcoin Core developers
# Distributed under the MIT software license, see the accompanying
# file COPYING or http://www.opensource.org/licenses/mit-license.php.
"""Test blinding logic when change is dropped and we have only one other blinded input
Constructs a transaction with a sufficiently small change output that it
gets dropped, in which there is only one other blinded input. In the case
that we have no blinded inputs, we would need to add an OP_RETURN output
to the transaction, neccessitating special logic.
Check that this special logic still results in a correct transaction that
sends the money to the desired recipient (and that the recipient is able
to receive/spend the money).
"""
from decimal import Decimal
from test_framework.blocktools import COINBASE_MATURITY
from test_framework.test_framework import BitcoinTestFramework
from test_framework.util import (
assert_equal,
satoshi_round,
)
class WalletCtTest(BitcoinTestFramework):
def set_test_params(self):
self.setup_clean_chain = True
self.num_nodes = 3
self.extra_args = [[
"-blindedaddresses=1",
"-initialfreecoins=2100000000000000",
"-con_blocksubsidy=0",
"-con_connect_genesis_outputs=1",
"-txindex=1",
]] * self.num_nodes
self.extra_args[0].append("-anyonecanspendaremine=1") # first node gets the coins
def skip_test_if_missing_module(self):
self.skip_if_no_wallet()
def test_send(self, amt, from_idx, to_idx, confidential):
# Try to send those coins to yet another wallet, sending a large enough amount
# that the change output is dropped.
address = self.nodes[to_idx].getnewaddress()
if not confidential:
address = self.nodes[to_idx].getaddressinfo(address)['unconfidential']
txid = self.nodes[from_idx].sendtoaddress(address, amt)
self.log.info(f"Sent {amt} LBTC to node {to_idx} in {txid}")
self.nodes[from_idx].generate(2)
self.sync_all()
for i in range(self.num_nodes):
self.log.info(f"Finished with node {i} balance: {self.nodes[i].getbalance()}")
assert_equal(self.nodes[from_idx].getbalance(), { "bitcoin": Decimal(0) })
assert_equal(self.nodes[to_idx].getbalance(), { "bitcoin": amt })
def run_test(self):
# Mine 101 blocks to get the initial coins out of IBD
self.nodes[0].generate(COINBASE_MATURITY + 1)
self.nodes[0].syncwithvalidationinterfacequeue()
self.sync_all()
for i in range(self.num_nodes):
self.log.info(f"Starting with node {i} balance: {self.nodes[i].getbalance()}")
# Send 1 coin to a new wallet
txid = self.nodes[0].sendtoaddress(self.nodes[1].getnewaddress(), 1)
self.log.info(f"Sent one coin to node 1 in {txid}")
self.nodes[0].generate(2)
self.sync_all()
# Try to send those coins to yet another wallet, sending a large enough amount
# that the change output is dropped.
amt = satoshi_round(Decimal(0.9995))
self.test_send(amt, 1, 2, True)
# Repeat, sending to a non-confidential output
amt = satoshi_round(Decimal(amt - Decimal(0.00035)))
self.test_send(amt, 2, 1, False)
# Again, sending from non-confidential to non-confidential
amt = satoshi_round(Decimal(amt - Decimal(0.00033)))
self.test_send(amt, 1, 2, False)
# Finally sending from non-confidential to confidential
amt = satoshi_round(Decimal(amt - Decimal(0.0005)))
self.test_send(amt, 2, 1, True)
# Then send the coins again to make sure they're spendable
amt = satoshi_round(Decimal(amt - Decimal(0.0005)))
self.test_send(amt, 1, 2, True)
addresses = [ self.nodes[1].getnewaddress() for i in range(15) ] \
+ [ self.nodes[2].getnewaddress() for i in range(15) ]
txid = self.nodes[2].sendmany(amounts={address: satoshi_round(Decimal(0.00025)) for address in addresses})
self.log.info(f"Sent many small UTXOs to nodes 1 and 2 in {txid}")
self.nodes[2].generate(2)
self.sync_all()
self.log.info(f"Issuing some assets from node 1")
# Try issuing assets
amt = satoshi_round(Decimal(1))
res1 = self.nodes[1].issueasset(amt, amt, True);
res2 = self.nodes[1].issueasset(amt, amt, False);
assets = [ res1["asset"], res1["token"], res2["asset"], res2["token"] ]
addresses = [ self.nodes[2].getnewaddress() for i in range(len(assets)) ]
txid = self.nodes[1].sendmany(
amounts={address: amt for address in addresses},
output_assets={addresses[i]: assets[i] for i in range(len(assets))},
)
self.log.info(f"Sent them to node 2 in {txid}")
self.nodes[1].generate(2)
self.sync_all()
# Send them back
addresses = [ self.nodes[1].getnewaddress() for i in range(len(assets)) ]
txid = self.nodes[2].sendmany(
amounts={address: amt for address in addresses},
output_assets={addresses[i]: assets[i] for i in range(len(assets))},
)
self.log.info(f"Sent them back to node 1 in {txid}")
if __name__ == '__main__':
WalletCtTest().main()

4
test/functional/wallet_importdescriptors.py
View file

@ -404,10 +404,10 @@ class ImportDescriptorsTest(BitcoinTestFramework):
address,
solvable=True,
ismine=True)
txid = w0.sendtoaddress(address, 49.99993240)
txid = w0.sendtoaddress(address, 49.99965520)
w0.generatetoaddress(6, w0.getnewaddress())
self.sync_blocks()
tx = wpriv.createrawtransaction([{"txid": txid, "vout": 0}], [{w0.getnewaddress(): 49.999}, {"fee": 0.0009324}])
tx = wpriv.createrawtransaction([{"txid": txid, "vout": 0}], [{w0.getnewaddress(): 49.999}, {"fee": 0.00065520}])
signed_tx = wpriv.signrawtransactionwithwallet(tx)
w1.sendrawtransaction(signed_tx['hex'])

4
test/functional/wallet_keypool.py
View file

@ -174,11 +174,11 @@ class KeyPoolTest(BitcoinTestFramework):
res = w2.walletcreatefundedpsbt(inputs=[], outputs=[{destination: 0.00025000}], options={"subtractFeeFromOutputs": [0], "feeRate": 0.00010})
assert_equal("psbt" in res, True)
# should work without subtractFeeFromOutputs if the exact fee is subtracted from the amount
res = w2.walletcreatefundedpsbt(inputs=[], outputs=[{destination: 0.00021650}], options={"feeRate": 0.00010})
res = w2.walletcreatefundedpsbt(inputs=[], outputs=[{destination: 0.00008570}], options={"feeRate": 0.00010})
assert_equal("psbt" in res, True)
# dust change should be removed
res = w2.walletcreatefundedpsbt(inputs=[], outputs=[{destination: 0.00021000}], options={"feeRate": 0.00010})
res = w2.walletcreatefundedpsbt(inputs=[], outputs=[{destination: 0.00008200}], options={"feeRate": 0.00010})
assert_equal("psbt" in res, True)
# create a transaction without change at the maximum fee rate, such that the output is still spendable: