New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[wallet] Use destination groups instead of coins in coin select #12257

Merged
merged 10 commits into from Jul 24, 2018

Conversation

@kallewoof
Member

kallewoof commented Jan 24, 2018

This PR adds an optional (off by default) -avoidpartialspends flag, which changes coin select to use output groups rather than outputs, where each output group corresponds to all outputs with the same destination.

It is a privacy improvement, as each time you spend some output, any other output that is publicly associated with the destination (address) will also be spent at the same time, at the cost of fee increase for cases where coin select without group restriction would find a more optimal set of coins (see example below).

For regular use without address reuse, this PR should have no effect on the user experience whatsoever; it only affects users who, for some reason, have multiple outputs with the same destination (i.e. address reuse).

Nodes with this turned off will still try to avoid partial spending, if the fee of the resulting transaction is not greater than the fee of the original transaction.

Example: a node has four outputs linked to two addresses A and B:

  • 1.0 btc to A
  • 0.5 btc to A
  • 1.0 btc to B
  • 0.5 btc to B

The node sends 0.2 btc to C. Without -avoidpartialspends, the following coin selection will occur:

  • 0.5 btc to A or B is picked
  • 0.2 btc is output to C
  • 0.3 - fee is output to (unique change address)

With -avoidpartialspends, the following will instead happen:

  • Both of (0.5, 1.0) btc to A or B is picked (one or the other pair)
  • 0.2 btc is output to C
  • 1.3 - fee is output to (unique change address)

As noted, the pro here is that, assuming nobody sends to the address after you spend from it, you will only ever use one address once. The con is that the transaction becomes slightly larger in this case, because it is overpicking outputs to adhere to the no partial spending rule.

This complements #10386, in particular it addresses @luke-jr and @gmaxwell's concerns in #10386 (comment) and #10386 (comment).

Together with -avoidreuse, this fully addresses the concerns in #10065 I believe.

@fanquake fanquake added the Wallet label Jan 24, 2018

@luke-jr

This comment has been minimized.

Member

luke-jr commented Jan 24, 2018

It doesn't avoid reuse (you still need to sign for each input), and has security issues in the event of QC (but that's already a hypothetical big issue for us anyway), but seems useful anyway.

Is this affected by the current coin control grouping defect, where it includes change in the same group as unrelated outputs? That would be a serious privacy risk, I think, since it would basically tell the world which output was change.

@kallewoof

This comment has been minimized.

Member

kallewoof commented Jan 24, 2018

@luke-jr:

It doesn't avoid reuse (you still need to sign for each input), and has security issues in the event of QC (but that's already a hypothetical big issue for us anyway), but seems useful anyway.

Together with -avoidreuse in #10386 it avoids reuse, I think. Are there any cases I'm missing?

Is this affected by the current coin control grouping defect, where it includes change in the same group as unrelated outputs? That would be a serious privacy risk, I think, since it would basically tell the world which output was change.

I'm not sure what this defect is. Is it described somewhere?

@ziggamon

This comment has been minimized.

ziggamon commented Jan 24, 2018

Does this also keep track of the change outputs in case more coins arrive later to A?

Imagine your example, where the coins from A go to C, with a change output to what we now call D.

If more coins arrive at A later, will they be spent together with D?

@kallewoof

This comment has been minimized.

Member

kallewoof commented Jan 24, 2018

@ziggamon Change outputs will go to a different destination so they will be considered separate from their inputs. In other words, D in your case will not be considered related to A in this case, and will not be associated with any coins arriving at A afterwards.

@@ -3063,6 +3048,30 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletT
return true;
}
bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletTx& wtxNew, CReserveKey& reservekey, CAmount& nFeeRet,

This comment has been minimized.

@NicolasDorier

NicolasDorier Jan 30, 2018

Member

I think this is avoidable complexity, because avoidpartial will be always higher fees.
I think it is OK, you pay fee for privacy, while giving incentive to privacy over fee by default.

This comment has been minimized.

@kallewoof

kallewoof Jan 31, 2018

Member

Actually avoidpartial gives the same fee a lot of the times, from what I've seen so far. It depends on your utxo set, of course.

@NicolasDorier

This comment has been minimized.

Member

NicolasDorier commented Jan 30, 2018

Some notes I am leaving here but to think about:

  • Making it default is the right way, but we should keep in mind that people relying on address reuse will pay way higher fee, so they need to be notified appropriately.
    Virtually all japanese exchanges are using address reuse (1 address per customer), this change would impact them a lot. I am unsure if they depend on Bitcoin Core Wallet though.
  • I am unsure if Bitcoin Core filter uneconomical outputs (filtering outputs which are more costly than their value to spend), but if bitcoin core implements this, we need to make sure this rule is always enforced at the Output level and not at the OutputGroup level. (Need to be enforced before you create the group)
@@ -2575,17 +2556,21 @@ bool CWallet::SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAm
++it;
}
// form groups from remaining coins; note that preset coins will not
// automatically have their associated (same address) coins included
std::vector<OutputGroup> groups = group_outputs(vCoins, !(coinControl && coinControl->m_avoid_partial_spends > -1 ? coinControl->m_avoid_partial_spends : gArgs.GetBoolArg("-avoidpartialspends", DEFAULT_AVOIDPARTIALSPENDS)));

This comment has been minimized.

@NicolasDorier

NicolasDorier Jan 30, 2018

Member

I think coinControl->m_avoid_partial_spends should be a boolean, and coinControl should set its value based on the arg avoidpartialspends just after being created. (At worse, just before this call for now)

This will make the code easier to reason about.

This comment has been minimized.

@kallewoof

kallewoof Jan 31, 2018

Member

Unfortunately that means either adding a coincontrol.cpp file and moving the constructor there, or adding a dependency on the gArg thing in coincontrol.h, which seems a bit overkill just for this. I'm very open to suggestions though; the short thing is an undesired hack.

@@ -30,6 +30,8 @@ class CCoinControl
boost::optional<CFeeRate> m_feerate;
//! Override the default confirmation target if set
boost::optional<unsigned int> m_confirm_target;
//! Avoid partial use of funds sent to a given address: -1 means default
short m_avoid_partial_spends;

This comment has been minimized.

@NicolasDorier

NicolasDorier Jan 30, 2018

Member

what about std::optional<bool> ?

This comment has been minimized.

@MarcoFalke

MarcoFalke Jan 30, 2018

Member

(since C++17) 😛

This comment has been minimized.

@NicolasDorier

NicolasDorier Jan 30, 2018

Member

Ah I forgot we were using boost for that... I would still prefer boost than a short though.

This comment has been minimized.

@kallewoof

kallewoof Feb 23, 2018

Member

Changed to using a boost::optional.

@kallewoof

This comment has been minimized.

Member

kallewoof commented Jan 31, 2018

@NicolasDorier

Making it default is the right way, but we should keep in mind that people relying on address reuse will pay way higher fee, so they need to be notified appropriately.

I think keeping it off by default for now is reasonable, until we get enough of an idea of the general consensus among people. I suspect, though, that most people won't care, as long as it doesn't give overly huge fee increases.

Virtually all japanese exchanges are using address reuse (1 address per customer), this change would impact them a lot. I am unsure if they depend on Bitcoin Core Wallet though.

Also some people have a "permanent" address that they receive payments to over a long period of time (e.g. charities). They also care little about privacy, usually, as their address is publicly tied to them anyway.

I am unsure if Bitcoin Core filter uneconomical outputs (filtering outputs which are more costly than their value to spend), but if bitcoin core implements this, we need to make sure this rule is always enforced at the Output level and not at the OutputGroup level. (Need to be enforced before you create the group)

Good point. I think the dust limit check can be added to the grouping method fairly easily.

@@ -2421,103 +2421,84 @@ static void ApproximateBestSubset(const std::vector<CInputCoin>& vValue, const C
}
}
bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const int nConfMine, const int nConfTheirs, const uint64_t nMaxAncestors, std::vector<COutput> vCoins,

This comment has been minimized.

@NicolasDorier

NicolasDorier Jan 31, 2018

Member

I am unsure yet, but I think you can make the diff way smaller by keeping the old signature of this method and building the OutputGroup at the scope (local variable) of this function only.

This comment has been minimized.

@kallewoof

kallewoof Feb 1, 2018

Member

This method doesn't know whether you want single or grouped as it doesn't have coincontrol. Would have to add a bool for that at least. Not sure it'll become smaller though. It'll just move code around.

This comment has been minimized.

@NicolasDorier

NicolasDorier Feb 1, 2018

Member

So the way I did it with NBitcoin is to keep the concept of group internal to only the method which does coin packing.

If grouping is deactivated, I group by random, that way there is a single code path for everything.

I think it is a way smaller code change because all consumer of SelectCoinsMinConf do not have to change (you can make the bool default to false to keep original behavior)
The concept of group does not make lot's of sense anywhere outside of this method.

This comment has been minimized.

@kallewoof

kallewoof Feb 13, 2018

Member

I discussed with @NicolasDorier and we concluded that the current way this is done is okay (NBitcoin does it differently so it made sense there).

\fB\-avoidpartialspends\fR
.IP
Group outputs by address, selecting all or none, instead of selecting on
a per\-output basis. Improved privacy as an address is only used

This comment has been minimized.

@NicolasDorier

NicolasDorier Feb 5, 2018

Member

would say Privacy is improved as an address...

This comment has been minimized.

@kallewoof

kallewoof Feb 5, 2018

Member

Changed

@@ -462,6 +462,8 @@ std::string HelpMessage(HelpMessageMode mode)
strUsage += HelpMessageOpt("-maxsigcachesize=<n>", strprintf("Limit sum of signature cache and script execution cache sizes to <n> MiB (default: %u)", DEFAULT_MAX_SIG_CACHE_SIZE));
strUsage += HelpMessageOpt("-maxtipage=<n>", strprintf("Maximum tip age in seconds to consider node in initial block download (default: %u)", DEFAULT_MAX_TIP_AGE));
}
strUsage += HelpMessageOpt("-avoidpartialspends", strprintf(_("Group outputs by address, selecting all or none, instead of selecting on a per-output basis. Privacy is improved as an address is only used once (unless someone sends to it "

This comment has been minimized.

@luke-jr

luke-jr Feb 27, 2018

Member

"by key". Outputs don't have addresses.

This comment has been minimized.

@kallewoof

kallewoof Feb 27, 2018

Member

I see what you're saying, but in this case I think 'address' is fine; we speak of address reuse, after all. While outputs themselves don't have addresses, the funding to each output was done to a specific address; if that address was the same, then this PR will group them together.

Maybe Group outputs by key (address), selecting all or... would be fine, though?

@@ -47,6 +47,8 @@ OutputType g_change_type = OUTPUT_TYPE_NONE;
const char * DEFAULT_WALLET_DAT = "wallet.dat";
const uint32_t BIP32_HARDENED_KEY_LIMIT = 0x80000000;
#define COINCONTROL_AVOID_PARTIAL_SPENDS(cc) ((cc) && (cc)->m_avoid_partial_spends ? (cc)->m_avoid_partial_spends.get() : gArgs.GetBoolArg("-avoidpartialspends", DEFAULT_AVOIDPARTIALSPENDS))

This comment has been minimized.

@luke-jr

luke-jr Feb 27, 2018

Member

Seems this would be better as a method on CCoinControl...

This comment has been minimized.

@kallewoof

kallewoof Feb 27, 2018

Member

Coin control needs gArgs in that case, and currently there is no .cpp file so it'd have to be included in the header.

I'm actually starting to think maybe coin control could use a .cpp file...

@kallewoof

This comment has been minimized.

Member

kallewoof commented Feb 27, 2018

I added coincontrol.cpp and moved SetNull into it, setting it to the default given in DEFAULT_AVOIDPARTIALSPENDS. This cleans up use as it is always a bool with an up-to-date value even when default is used.

@@ -462,6 +462,8 @@ std::string HelpMessage(HelpMessageMode mode)
strUsage += HelpMessageOpt("-maxsigcachesize=<n>", strprintf("Limit sum of signature cache and script execution cache sizes to <n> MiB (default: %u)", DEFAULT_MAX_SIG_CACHE_SIZE));
strUsage += HelpMessageOpt("-maxtipage=<n>", strprintf("Maximum tip age in seconds to consider node in initial block download (default: %u)", DEFAULT_MAX_TIP_AGE));
}
strUsage += HelpMessageOpt("-avoidpartialspends", strprintf(_("Group outputs by address, selecting all or none, instead of selecting on a per-output basis. Privacy is improved as an address is only used once (unless someone sends to it "

This comment has been minimized.

@luke-jr

luke-jr Feb 28, 2018

Member

Also, this should be in wallet/init

This comment has been minimized.

@kallewoof
@@ -53,6 +53,8 @@ static const bool DEFAULT_WHITELISTFORCERELAY = true;
static const unsigned int DEFAULT_MIN_RELAY_TX_FEE = 1000;
//! -maxtxfee default
static const CAmount DEFAULT_TRANSACTION_MAXFEE = 0.1 * COIN;
//! -avoidpartialspends default
static const bool DEFAULT_AVOIDPARTIALSPENDS = false;

This comment has been minimized.

@luke-jr

luke-jr Feb 28, 2018

Member

Similarly, in wallet/wallet.h

(bSpendZeroConfChange && SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, 0, 1, std::min((size_t)4, nMaxChainLength/3), groups, setCoinsRet, nValueRet)) ||
(bSpendZeroConfChange && SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, 0, 1, nMaxChainLength/2, groups, setCoinsRet, nValueRet)) ||
(bSpendZeroConfChange && SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, 0, 1, nMaxChainLength, groups, setCoinsRet, nValueRet)) ||
(bSpendZeroConfChange && !fRejectLongChains && SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, 0, 1, std::numeric_limits<int64_t>::max(), groups, setCoinsRet, nValueRet));

This comment has been minimized.

@luke-jr

luke-jr Feb 28, 2018

Member

Change from uint64_t max to int64_t max should be documented somewhere, probably done alone in a separate bugfix commit.

This comment has been minimized.

@kallewoof

kallewoof Feb 28, 2018

Member

Split into its own commit, and documented change in commit message.

// we prioritize the lowest larger if
// 1. it has multiple outputs (i.e. it is the result of address reuse)
// 2. it is not substantially (>2x) larger than the target value
if (nTotalLower < nTargetValue || (lowest_larger && lowest_larger->m_outputs.size() > 1 && lowest_larger->m_value < 2 * nTargetValue)) {

This comment has been minimized.

@luke-jr

luke-jr Feb 28, 2018

Member

This policy change should be documented in the commit message.

This comment has been minimized.

@kallewoof

kallewoof Feb 28, 2018

Member

Split into its own commit, and documented policy change.

@laanwj

This comment has been minimized.

Member

laanwj commented Mar 5, 2018

Concept ACK

src/util.h Outdated
@@ -357,4 +358,14 @@ std::unique_ptr<T> MakeUnique(Args&&... args)
return std::unique_ptr<T>(new T(std::forward<Args>(args)...));
}
//! Simplification of std insertion
template <typename Tdst, typename Tsrc>
void insert(Tdst& dst, const Tsrc&& src) {

This comment has been minimized.

@NicolasDorier

NicolasDorier Mar 7, 2018

Member

As we discussed you might not need && because you can pass temporary object to the function with const Tsrc&

@@ -1055,11 +1055,10 @@ void CTxMemPool::TrimToSize(size_t sizelimit, std::vector<COutPoint>* pvNoSpends
}
}

This comment has been minimized.

@NicolasDorier

NicolasDorier Mar 7, 2018

Member

nit: You don't have to do this, but I think it would be better if this commit was self contained, as now the clients of TransactionWithinChainLimit are not compiling anymore. To verify your change is valid we need to check other commits later. This make it harder to review.

@NicolasDorier

This comment has been minimized.

Member

NicolasDorier commented Mar 7, 2018

Code Review ACK, though it would be nice to have the remaining TODO in the test done.

I am also wondering potential performance problem for wallet with lots of UTXOs. I already heard complaints that core was getting quite slow at doing transactions when for big wallets. (though this could be worked around by asking people to turn on avoidpartialspends)

@kallewoof

This comment has been minimized.

Member

kallewoof commented Mar 7, 2018

I am also wondering potential performance problem for wallet with lots of UTXOs. I already heard complaints that core was getting quite slow at doing transactions when for big wallets. (though this could be worked around by asking people to turn on avoidpartialspends)

I think wallets with a ton of outputs would want to use avoidpartialspends as it would affect them minimally fee-wise, and it would improve privacy for odd-ball reuse.

@kallewoof

This comment has been minimized.

Member

kallewoof commented Mar 15, 2018

I rebased this on top of the new coin selection algorithm.

@achow101

Light utACK. I kind of scanned over the functional tests.

@@ -4387,7 +4387,7 @@ std::vector<COutput>::iterator OutputGroup::discard(const COutput& output) {
std::vector<CInputCoin> OutputGroup::input_coins() const {
std::vector<CInputCoin> coins;
for (const auto& output : m_outputs) {
coins.emplace_back(output.tx, output.i);
coins.emplace_back(output.tx->tx, output.i);

This comment has been minimized.

@achow101

achow101 Apr 11, 2018

Member

Shouldn't this change be part of 27872fa?

This comment has been minimized.

@kallewoof

kallewoof Apr 12, 2018

Member

Yes! Thanks for noticing.

kallewoof added some commits Jul 17, 2018

@laanwj

This comment has been minimized.

Member

laanwj commented Jul 24, 2018

utACK 232f96f

@laanwj laanwj merged commit 232f96f into bitcoin:master Jul 24, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jul 24, 2018

Merge #12257: [wallet] Use destination groups instead of coins in coi…
…n select

232f96f doc: Add release notes for -avoidpartialspends (Karl-Johan Alm)
e00b469 clean-up: Remove no longer used ivars from CInputCoin (Karl-Johan Alm)
43e04d1 wallet: Remove deprecated OutputEligibleForSpending (Karl-Johan Alm)
0128121 test: Add basic testing for wallet groups (Karl-Johan Alm)
59d6f7b wallet: Switch to using output groups instead of coins in coin selection (Karl-Johan Alm)
87ebce2 wallet: Add output grouping (Karl-Johan Alm)
bb629cb Add -avoidpartialspends and m_avoid_partial_spends (Karl-Johan Alm)
65b3eda wallet: Add input bytes to CInputCoin (Karl-Johan Alm)
a443d7a moveonly: CoinElegibilityFilter into coinselection.h (Karl-Johan Alm)
173e18a utils: Add insert() convenience templates (Karl-Johan Alm)

Pull request description:

  This PR adds an optional (off by default) `-avoidpartialspends` flag, which changes coin select to use output groups rather than outputs, where each output group corresponds to all outputs with the same destination.

  It is a privacy improvement, as each time you spend some output, any other output that is publicly associated with the destination (address) will also be spent at the same time, at the cost of fee increase for cases where coin select without group restriction would find a more optimal set of coins (see example below).

  For regular use without address reuse, this PR should have no effect on the user experience whatsoever; it only affects users who, for some reason, have multiple outputs with the same destination (i.e. address reuse).

  Nodes with this turned off will still try to avoid partial spending, if the fee of the resulting transaction is not greater than the fee of the original transaction.

  Example: a node has four outputs linked to two addresses `A` and `B`:

  * 1.0 btc to `A`
  * 0.5 btc to `A`
  * 1.0 btc to `B`
  * 0.5 btc to `B`

  The node sends 0.2 btc to `C`. Without `-avoidpartialspends`, the following coin selection will occur:
  * 0.5 btc to `A` or `B` is picked
  * 0.2 btc is output to `C`
  * 0.3 - fee is output to (unique change address)

  With `-avoidpartialspends`, the following will instead happen:
  * Both of (0.5, 1.0) btc to `A` or `B` is picked (one or the other pair)
  * 0.2 btc is output to `C`
  * 1.3 - fee is output to (unique change address)

  As noted, the pro here is that, assuming nobody sends to the address after you spend from it, you will only ever use one address once. The con is that the transaction becomes slightly larger in this case, because it is overpicking outputs to adhere to the no partial spending rule.

  This complements #10386, in particular it addresses @luke-jr and @gmaxwell's concerns in #10386 (comment) and #10386 (comment).

  Together with `-avoidreuse`, this fully addresses the concerns in #10065 I believe.

Tree-SHA512: 24687a4490ba59cf4198ed90052944ff4996653a4257833bb52ed24d058b3e924800c9b3790aeb6be6385b653b49e304453e5d7ff960e64c682fc23bfc447621

@kallewoof kallewoof deleted the kallewoof:feature-addrgrouped-coinselect branch Jul 24, 2018

@sdaftuar

Oops, I was mid-review and never posted these comments before merge. I address one of these in #13805.

// m_ancestors is currently the max ancestor count for all coins in the group; however, this is
// not ideal, as a wallet will consider e.g. thirty 2-ancestor coins as having two ancestors,
// when in reality it has 60 ancestors.
m_ancestors = std::max(m_ancestors, ancestors);

This comment has been minimized.

@sdaftuar

sdaftuar Jul 30, 2018

Member

Rather than use std::max, it seems like we could just sum the ancestor values -- that risks overcounting, but what's the downside? I believe the goal of tracking this stuff is so that calls to SelectCoinsMinConf has a better chance of producing a transaction that passes the mempool chain limits, so if we err on the side of caution here I think that's just strictly better.

This comment has been minimized.

@kallewoof

kallewoof Jul 30, 2018

Member

Yeah, worst case the user has to wait for the inputs to confirm before they can spend them. (Is there a case where that could be a pontential problem? Can't think of one.)

This comment has been minimized.

@kallewoof
size_t ancestors, descendants;
mempool.GetTransactionAncestry(output.tx->GetHash(), ancestors, descendants);
if (!single_coin && ExtractDestination(output.tx->tx->vout[output.i].scriptPubKey, dst)) {
if (gmap.count(dst) == 10) {

This comment has been minimized.

@sdaftuar

sdaftuar Jul 30, 2018

Member

I think you meant if (gmap[dst].m_outputs.size() == 10)? gmap.count(dst) should always be 0 or 1.

Also -- I'm not sure if there's some negative side effect from deterministically creating these subgroups of outputs. Intuitively it seems safer to me if we randomly constructed these groups somehow to prevent someone from triggering some kind of coinselection issue by giving us coins that sort in a particular way. But maybe this concern is overblown.

This comment has been minimized.

@kallewoof

kallewoof Jul 30, 2018

Member

I'm not sure it's a big risk, but I also think it's better to be safe than sorry. I envision a replacement of push_back with insert(begin() + random(size()) (forgive pseudo-code-abbrev) in

m_outputs.push_back(output);

Will make a PR unless someone beats me to it.

This comment has been minimized.

@kallewoof

kallewoof Jul 30, 2018

Member

Hm.. actually, that's not sufficient due to the capping at 10. I guess shuffling the entire vector at the top in CWallet::GroupOutputs is the way to go.

This comment has been minimized.

@kallewoof

kallewoof Jul 30, 2018

Member

Adding shuffling in #13808. I chose to put it in the GroupOutputs method since it can skip the shuffle for is_single and size() <= 10 cases. Moved outside, due to constness.

This comment has been minimized.

@promag

promag Jul 30, 2018

Member

gmap.count(dst) == 10 is always false and is not fixed in #13808.

This comment has been minimized.

@kallewoof

kallewoof Jul 30, 2018

Member

It's fixed in #13805

kallewoof added a commit to kallewoof/bitcoin that referenced this pull request Jul 30, 2018

kallewoof added a commit to kallewoof/bitcoin that referenced this pull request Jul 30, 2018

kallewoof added a commit to kallewoof/bitcoin that referenced this pull request Jul 30, 2018

laanwj added a commit that referenced this pull request Aug 7, 2018

Merge #13812: wallet: sum ancestors rather than taking max in output …
…groups

23fbbb1 wallet: sum ancestors rather than taking max in output groups (Karl-Johan Alm)

Pull request description:

  This is pointed out in #12257 (comment).

  Basically, the ancestors gives an indication as to how many ancestors the resulting transaction will have, which is more precise when summing up the values, rather than taking the maximum, since all the coins in the group will become ancestors if selected.

Tree-SHA512: 0588c4b6059669650614817e041526a2ab89dda8c07fca8e077c7669dca1fed51cd164f7df56340840ab60285d48f3b140dcee64f64bf696b2dd4ab16d556a13

kallewoof added a commit to kallewoof/bitcoin that referenced this pull request Aug 9, 2018

kallewoof added a commit to kallewoof/bitcoin that referenced this pull request Aug 10, 2018

kallewoof added a commit to kallewoof/bitcoin that referenced this pull request Aug 10, 2018

ken2812221 pushed a commit to ken2812221/bitcoin that referenced this pull request Aug 13, 2018

Merge bitcoin#13808: wallet: shuffle coins before grouping, where war…
…ranted

18f690e wallet: shuffle coins before grouping, where warranted (Karl-Johan Alm)

Pull request description:

  Coins are randomly shuffled in coin selection to avoid unintentional privacy leaks regarding the user's coin set. For the case where a user has a lot of coins with the same destination, these will be grouped into groups of 10 *before* the shuffling.

  It is unclear whether this has any implications at all, but this PR plugs the potential issue, if there ever is one, by shuffling the coins before they are grouped.

  Issue brought up in bitcoin#12257 (comment)

Tree-SHA512: fb50ed4b5fc03ab4853d45b76e1c64476ad5bcd797497179bc37b9262885c974ed6811159fd8e581f1461b6cc6d0a66146f4b70a2777c0f5e818d1322e0edb89

Bushstar added a commit to Bushstar/Feathercoin that referenced this pull request Aug 13, 2018

uhliksk added a commit to fxtc/fxtc that referenced this pull request Aug 29, 2018

HashUnlimited added a commit to ToDoThings/bitcoin that referenced this pull request Sep 10, 2018

HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this pull request Sep 14, 2018

joemphilips added a commit to joemphilips/bitcoin-1 that referenced this pull request Nov 9, 2018

fish-en pushed a commit to fish-en/bitcoin that referenced this pull request Nov 11, 2018

fish-en pushed a commit to fish-en/bitcoin that referenced this pull request Nov 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment