forked from ElementsProject/elements
-
Notifications
You must be signed in to change notification settings - Fork 1
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Merge bitcoin/bitcoin#24584: wallet: avoid mixing different `OutputTy…
…pes` during coin selection 71d1d13 test: add unit test for AvailableCoins (josibake) da03cb4 test: functional test for new coin selection logic (josibake) 438e048 wallet: run coin selection by `OutputType` (josibake) 77b0707 refactor: use CoinsResult struct in SelectCoins (josibake) 2e67291 refactor: store by OutputType in CoinsResult (josibake) Pull request description: # Concept Following bitcoin/bitcoin#23789, Bitcoin Core wallet will now generate a change address that matches the payment address type. This improves privacy by not revealing which of the outputs is the change at the time of the transaction in scenarios where the input address types differ from the payment address type. However, information about the change can be leaked in a later transaction. This proposal attempts to address that concern. ## Leaking information in a later transaction Consider the following scenario:  1. Alice has a wallet with bech32 type UTXOs and pays Bob, who gives her a P2SH address 2. Alice's wallet generates a P2SH change output, preserving her privacy in `txid: a` 3. Alice then pays Carol, who gives her a bech32 address 4. Alice's wallet combines the P2SH UTXO with a bech32 UTXO and `txid: b` has two bech32 outputs From a chain analysis perspective, it is reasonable to infer that the P2SH input in `txid: b` was the change from `txid: a`. To avoid leaking information in this scenario, Alice's wallet should avoid picking the P2SH output and instead fund the transaction with only bech32 Outputs. If the payment to Carol can be funded with just the P2SH output, it should be preferred over the bech32 outputs as this will convert the P2SH UTXO to bech32 UTXOs via the payment and change outputs of the new transaction. **TLDR;** Avoid mixing output types, spend non-default `OutputTypes` when it is economical to do so. # Approach `AvailableCoins` now populates a struct, which makes it easier to access coins by `OutputType`. Coin selection tries to find a funding solution by each output type and chooses the most economical by waste metric. If a solution can't be found without mixing, coin selection runs over the entire wallet, allowing mixing, which is the same as the current behavior. I've also added a functional test (`test/functional/wallet_avoid_mixing_output_types.py`) and unit test (`src/wallet/test/availablecoins_tests.cpp`. ACKs for top commit: achow101: re-ACK 71d1d13 aureleoules: ACK 71d1d13. Xekyo: reACK 71d1d13 via `git range-diff master 6530d19 71d1d13` LarryRuane: ACK 71d1d13 Tree-SHA512: 2e0716efdae5adf5479446fabc731ae81d595131d3b8bade98b64ba323d0e0c6d964a67f8c14c89c428998bda47993fa924f3cfca1529e2bd49eaa4e31b7e426
- Loading branch information
Showing
11 changed files
with
612 additions
and
182 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,105 @@ | ||
| // Copyright (c) 2022 The Bitcoin Core developers | ||
| // Distributed under the MIT software license, see the accompanying | ||
| // file COPYING or https://www.opensource.org/licenses/mit-license.php. | ||
|
|
||
| #include <validation.h> | ||
| #include <wallet/coincontrol.h> | ||
| #include <wallet/spend.h> | ||
| #include <wallet/test/util.h> | ||
| #include <wallet/test/wallet_test_fixture.h> | ||
|
|
||
| #include <boost/test/unit_test.hpp> | ||
|
|
||
| namespace wallet { | ||
| BOOST_FIXTURE_TEST_SUITE(availablecoins_tests, WalletTestingSetup) | ||
| class AvailableCoinsTestingSetup : public TestChain100Setup | ||
| { | ||
| public: | ||
| AvailableCoinsTestingSetup() | ||
| { | ||
| CreateAndProcessBlock({}, {}); | ||
| wallet = CreateSyncedWallet(*m_node.chain, m_node.chainman->ActiveChain(), m_args, coinbaseKey); | ||
| } | ||
|
|
||
| ~AvailableCoinsTestingSetup() | ||
| { | ||
| wallet.reset(); | ||
| } | ||
| CWalletTx& AddTx(CRecipient recipient) | ||
| { | ||
| CTransactionRef tx; | ||
| CCoinControl dummy; | ||
| { | ||
| constexpr int RANDOM_CHANGE_POSITION = -1; | ||
| auto res = CreateTransaction(*wallet, {recipient}, RANDOM_CHANGE_POSITION, dummy); | ||
| BOOST_CHECK(res); | ||
| tx = res.GetObj().tx; | ||
| } | ||
| wallet->CommitTransaction(tx, {}, {}); | ||
| CMutableTransaction blocktx; | ||
| { | ||
| LOCK(wallet->cs_wallet); | ||
| blocktx = CMutableTransaction(*wallet->mapWallet.at(tx->GetHash()).tx); | ||
| } | ||
| CreateAndProcessBlock({CMutableTransaction(blocktx)}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())); | ||
|
|
||
| LOCK(wallet->cs_wallet); | ||
| wallet->SetLastBlockProcessed(wallet->GetLastBlockHeight() + 1, m_node.chainman->ActiveChain().Tip()->GetBlockHash()); | ||
| auto it = wallet->mapWallet.find(tx->GetHash()); | ||
| BOOST_CHECK(it != wallet->mapWallet.end()); | ||
| it->second.m_state = TxStateConfirmed{m_node.chainman->ActiveChain().Tip()->GetBlockHash(), m_node.chainman->ActiveChain().Height(), /*index=*/1}; | ||
| return it->second; | ||
| } | ||
|
|
||
| std::unique_ptr<CWallet> wallet; | ||
| }; | ||
|
|
||
| BOOST_FIXTURE_TEST_CASE(BasicOutputTypesTest, AvailableCoinsTestingSetup) | ||
| { | ||
| CoinsResult available_coins; | ||
| BResult<CTxDestination> dest; | ||
| LOCK(wallet->cs_wallet); | ||
|
|
||
| // Verify our wallet has one usable coinbase UTXO before starting | ||
| // This UTXO is a P2PK, so it should show up in the Other bucket | ||
| available_coins = AvailableCoins(*wallet); | ||
| BOOST_CHECK_EQUAL(available_coins.size(), 1U); | ||
| BOOST_CHECK_EQUAL(available_coins.other.size(), 1U); | ||
|
|
||
| // We will create a self transfer for each of the OutputTypes and | ||
| // verify it is put in the correct bucket after running GetAvailablecoins | ||
| // | ||
| // For each OutputType, We expect 2 UTXOs in our wallet following the self transfer: | ||
| // 1. One UTXO as the recipient | ||
| // 2. One UTXO from the change, due to payment address matching logic | ||
|
|
||
| // Bech32m | ||
| dest = wallet->GetNewDestination(OutputType::BECH32M, ""); | ||
| BOOST_ASSERT(dest.HasRes()); | ||
| AddTx(CRecipient{{GetScriptForDestination(dest.GetObj())}, 1 * COIN, /*fSubtractFeeFromAmount=*/true}); | ||
| available_coins = AvailableCoins(*wallet); | ||
| BOOST_CHECK_EQUAL(available_coins.bech32m.size(), 2U); | ||
|
|
||
| // Bech32 | ||
| dest = wallet->GetNewDestination(OutputType::BECH32, ""); | ||
| BOOST_ASSERT(dest.HasRes()); | ||
| AddTx(CRecipient{{GetScriptForDestination(dest.GetObj())}, 2 * COIN, /*fSubtractFeeFromAmount=*/true}); | ||
| available_coins = AvailableCoins(*wallet); | ||
| BOOST_CHECK_EQUAL(available_coins.bech32.size(), 2U); | ||
|
|
||
| // P2SH-SEGWIT | ||
| dest = wallet->GetNewDestination(OutputType::P2SH_SEGWIT, ""); | ||
| AddTx(CRecipient{{GetScriptForDestination(dest.GetObj())}, 3 * COIN, /*fSubtractFeeFromAmount=*/true}); | ||
| available_coins = AvailableCoins(*wallet); | ||
| BOOST_CHECK_EQUAL(available_coins.P2SH_segwit.size(), 2U); | ||
|
|
||
| // Legacy (P2PKH) | ||
| dest = wallet->GetNewDestination(OutputType::LEGACY, ""); | ||
| BOOST_ASSERT(dest.HasRes()); | ||
| AddTx(CRecipient{{GetScriptForDestination(dest.GetObj())}, 4 * COIN, /*fSubtractFeeFromAmount=*/true}); | ||
| available_coins = AvailableCoins(*wallet); | ||
| BOOST_CHECK_EQUAL(available_coins.legacy.size(), 2U); | ||
| } | ||
|
|
||
| BOOST_AUTO_TEST_SUITE_END() | ||
| } // namespace wallet |
Oops, something went wrong.