wallet: Prefer full destination groups in coin selection

fjahr commented at 1:19 am on December 29, 2019: member

Fixes #17603 (together with #17843)

In the case of destination groups of >10 outputs existing in a wallet with avoid_reuse enabled, the grouping algorithm is adding left-over outputs as an “incomplete” group to the list of groups even when a full group has already been added. This leads to the strange behavior that if there are >10 outputs for a destination the transaction spending from that will effectively use len(outputs) % 10 as inputs for that transaction.

From the original PR and the code comment I understand the correct behavior should be the usage of 10 outputs. I opted for minimal changes in the current code although there maybe optimizations possible for cases with >20 outputs on a destination this sounds like too much of an edge case right now.

fanquake added the label Wallet on Dec 29, 2019

fanquake requested review from instagibbs on Dec 29, 2019

fanquake assigned achow101 on Dec 29, 2019

fanquake unassigned achow101 on Dec 29, 2019

fanquake requested review from achow101 on Dec 29, 2019

DrahtBot commented at 4:48 am on December 29, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#18418 (wallet: Increase OUTPUT_GROUP_MAX_ENTRIES to 100 by fjahr)
#17355 (gui: grey out used address in address book by za-kk)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

in test/functional/wallet_avoidreuse.py:247 in 6004cce874 outdated

241@@ -240,5 +242,26 @@ def test_fund_send_fund_send(self):
242         assert_approx(self.nodes[1].getbalance(), 1, 0.001)
243         assert_approx(self.nodes[1].getbalance(avoid_reuse=False), 11, 0.001)
244 
245+    def test_destination_groupings(self):
246+        '''
247+        If a destination has more than 10 outputs, exactly 10 outputs should

instagibbs commented at 3:18 pm on December 30, 2019:

what assumptions does this test have?

Seems like it assumes the wallet has no more than 10*21 BTC? In other words, those sends completely drain the wallet of other UTXO addresses. This should be asserted and/or programmatically done.

kallewoof commented at 11:47 am on January 3, 2020:

It took me awhile to realize what is being stated here.

If a destination has more than 10 outputs, the 10-output groups should always be picked before the <10-output group, if any such exists.

I.e. if a destination has 19 outputs, it will result in one 10-output group and one 9-output group. The second should never ever be used unless the former has already been used (in the same or a previous transaction).

If it has 21 outputs, it will result in two 10-output groups and one 1-output group. The third should be left alone unless the first two were used.

fjahr commented at 1:04 am on January 6, 2020:

I have expanded the test and the comments and hope it’s more clear now.

in src/wallet/wallet.cpp:4190 in 6004cce874 outdated

3976@@ -3977,6 +3977,7 @@ std::vector<OutputGroup> CWallet::GroupOutputs(const std::vector<COutput>& outpu
3977     std::vector<OutputGroup> groups;
3978     std::map<CTxDestination, OutputGroup> gmap;
3979     CTxDestination dst;
3980+    std::set<CTxDestination> full_groups;

achow101 commented at 6:43 pm on December 30, 2019:

ISTM a cleaner approach would be to move groups down before the return and just keep everything in the map. When there are more than 10 outputs in a group, just stop adding to the group. This would also handle the case of 20 outputs.

Unless our goal is to have multiple groups when more than 10 outputs for a destination?

cc @kallewoof

kallewoof commented at 3:49 am on December 31, 2019:

The intention was that if there were a ton of spends to the same address, these would be chunked into randomized 10 UTXO chunks, to avoid creating too large transactions. So yeah, the intention was to have multiple groups for the same destination in cases where there were 11+ UTXOs for it.

kallewoof commented at 3:55 am on December 31, 2019: member

The reported issue seems like things are not working as they should for sure, but I don’t see how this code fixes it. Ultimately, if I want to send 10.5 bitcoin to you, and I have 500 UTXO:s of 1 BTC each all sending to A, the grouping part should result in 50 groups of 10 random UTXO:s each (each group worth 10 BTC), and two of these should be selected when sending, and ALL of the outputs should be marked as used afterwards.

Your code seems to change this behavior to only result in one 10 BTC group per destination, which means the above send will result in “insufficient funds”.

kallewoof commented at 5:34 am on December 31, 2019: member

I investigated this further. See #17603 (comment)

fjahr commented at 1:23 pm on December 31, 2019: member

I investigated this further. See #17603 (comment)

Thanks, that’s what I found as well, I should have described it in greater detail.

The reported issue seems like things are not working as they should for sure, but I don’t see how this code fixes it. Ultimately, if I want to send 10.5 bitcoin to you, and I have 500 UTXO:s of 1 BTC each all sending to A, the grouping part should result in 50 groups of 10 random UTXO:s each (each group worth 10 BTC), and two of these should be selected when sending, and ALL of the outputs should be marked as used afterwards.

Your code seems to change this behavior to only result in one 10 BTC group per destination, which means the above send will result in “insufficient funds”.

My code does handle this case correctly. You describe the behavior that would occur if I had implemented it as @achow101 suggests. The “full groups” (10 outputs) are already pushed into groups earlier and only the “incomplete” group of <10 outputs would be added later. So we are collecting all the full groups but skip the last one if it is not full.

However, your comment made me realize another shortcoming of the current code: If there are 11 outputs of 1 BTC and we want to send 10.5 BTC then we see the insufficient funds error because we are “forgetting” the incomplete group. To fix this I would suggest implementing the following: the last, incomplete group, is pushed into one of the full groups, resulting in one of the groups being up to 19 outputs large. “Worst case” scenario that I see: we want to send a very small amount from the destination and we have exactly 19 outputs, then we would use all the 19 outputs. I think this should be ok? I will wait for your feedback before pushing the code for it.

See adapted tests I ran with the current code for the examples discussed above: https://gist.github.com/fjahr/d56b68e58b4275ef52cd5f4eb50b1d4e

fjahr renamed this:
~~wallet: Fix coin selection for destination groups >10~~
wallet: Improve coin selection for destination groups >10
on Jan 2, 2020

kallewoof commented at 11:21 am on January 3, 2020: member

I’m not sure this is the right approach here. It seems like marking the output groups as ‘partial’ when there are >10 outputs would address this. A simple approach would be to explicitly think of <max groups for multi-group destinations as “0-conf”. The current code would then actively try to avoid using it except as a last resort.

Edit: I didn’t test, but I assume your example with “not working” above is for your patch, not the current master.

kallewoof commented at 11:31 am on January 3, 2020: member

I would do something like this

 0diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
 1index 3954f6626..3c88ff9c0 100644
 2--- a/src/wallet/wallet.cpp
 3+++ b/src/wallet/wallet.cpp
 4@@ -3976,6 +3976,7 @@ bool CWalletTx::IsImmatureCoinBase() const
 5 std::vector<OutputGroup> CWallet::GroupOutputs(const std::vector<COutput>& outputs, bool single_coin) const {
 6     std::vector<OutputGroup> groups;
 7     std::map<CTxDestination, OutputGroup> gmap;
 8+    std::set<CTxDestination> gfull;
 9     CTxDestination dst;
10     for (const auto& output : outputs) {
11         if (output.fSpendable) {
12@@ -3990,6 +3991,7 @@ std::vector<OutputGroup> CWallet::GroupOutputs(const std::vector<COutput>& outpu
13                 if (gmap[dst].m_outputs.size() >= OUTPUT_GROUP_MAX_ENTRIES) {
14                     groups.push_back(gmap[dst]);
15                     gmap.erase(dst);
16+                    gfull.insert(dst);
17                 }
18                 gmap[dst].Insert(input_coin, output.nDepth, output.tx->IsFromMe(ISMINE_ALL), ancestors, descendants);
19             } else {
20@@ -3998,7 +4000,16 @@ std::vector<OutputGroup> CWallet::GroupOutputs(const std::vector<COutput>& outpu
21         }
22     }
23     if (!single_coin) {
24-        for (const auto& it : gmap) groups.push_back(it.second);
25+        for (auto& it : gmap) {
26+            auto& group = it.second;
27+            if (gfull.count(it.first) > 0 && group.m_outputs.size() < OUTPUT_GROUP_MAX_ENTRIES) {
28+                // make this unattractive as we want coin selection to avoid it if possible
29+                group.m_depth = 0;
30+                group.m_from_me = false;
31+                group.m_ancestors = 9999;
32+            }
33+            groups.push_back(group);
34+        }
35     }
36     return groups;
37 }

fjahr force-pushed on Jan 6, 2020

fjahr commented at 1:06 am on January 6, 2020: member

Edit: I didn’t test, but I assume your example with “not working” above is for your patch, not the current master.

Yeah, that’s right.

I would do something like this

That is a good idea, I did not think of that possibility before. But I still see a problem there: if there are flags activated like -walletrejectlongchains or -txconfirmtarget then we will have the same issue as with my code described above. The incomplete group will be ignored and the user may get an insufficient funds error when there are actually enough funds available. And since DEFAULT_TX_CONFIRM_TARGET is set to 6 I think this would happen for every wallet that has not set this value to 0. If only using group.m_ancestors = 9999; at least this will work for the default wallet configuration. I am not sure how widely -walletrejectlongchains is used.

Honestly, I am not sure yet which solution I prefer. The upsides of my described approach:

No matter what the configuration of the wallet is, there will not be a false “Insufficient funds” error.
I think it does a better job of using funds from the same address together.

An example to explain 2.:

A wallet has 11 outputs of 1 BTC each at reused_address
It also has 1 output of 1 BTC at not_reused_address
The user wants to send 10.5 BTC and uses avoid_reuse
With the m_ancestors approach, the wallet would send 10 BTC from reused_address and 1 BTC from not_reused_address, indicating these addresses could be controlled by the same user
With my suggestion, there would be 11 BTC group only from reused_address selected for the inputs

This stands against the downsides of my approach:

At worst we are using 2 * max - 1 inputs when max could have been enough.
It is a more complicated change than yours.

Let me know if I am missing something. I am not sure how much weight each of these arguments carry. I think the perfect solution would only be possible by making a distinction between <max groups and max groups in coin selection and that seems like overkill given the impact of the issue.

I am pushing an updated version implementing your solution with only m_ancestors and and an improved test and hope to get some more feedback. But I am happy to make further changes.

DrahtBot added the label Needs rebase on Jan 7, 2020

kallewoof commented at 6:33 am on January 8, 2020: member

You’re probably right that it would not end up using the final group even if it could. I think that’s okay, in this case. As for the -walletrejectlongchains issue, you would replace 9999 with max_ancestors-1 where max_ancestors is defined as in the coin selection code.

fjahr force-pushed on Jan 8, 2020

fjahr commented at 9:54 pm on January 8, 2020: member

Rebased and implemented using max_ancestors - 1 base on feedback.

DrahtBot removed the label Needs rebase on Jan 8, 2020

fjahr force-pushed on Jan 10, 2020

meshcollider referenced this in commit ac61ec9da6 on Jan 15, 2020

DrahtBot added the label Needs rebase on Jan 15, 2020

sidhujag referenced this in commit a1b399c72d on Jan 15, 2020

fjahr force-pushed on Jan 16, 2020

fjahr commented at 8:03 pm on January 16, 2020: member

Rebased and gave test a less generic name.

DrahtBot removed the label Needs rebase on Jan 16, 2020

in src/wallet/wallet.h:808 in 87de015719 outdated

804@@ -805,7 +805,7 @@ class CWallet final : public WalletStorage, private interfaces::Chain::Notificat
805     bool IsUsedDestination(const uint256& hash, unsigned int n) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
806     void SetUsedDestinationState(WalletBatch& batch, const uint256& hash, unsigned int n, bool used, std::set<CTxDestination>& tx_destinations) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
807 
808-    std::vector<OutputGroup> GroupOutputs(const std::vector<COutput>& outputs, bool single_coin) const;
809+    std::vector<OutputGroup> GroupOutputs(const std::vector<COutput>& outputs, bool single_coin, const size_t max_ancestors = 10000) const;

jonatack commented at 5:36 pm on March 11, 2020:

Where does the value of 10000 come from? Could it be documented and maybe hoisted to a static constant?

kallewoof commented at 4:13 am on March 12, 2020:

I think this should be based on the wallet-determined max ancestor count, and should not have a default value here. (i.e. I suggest removing = 10000)

jonatack commented at 4:19 am on March 12, 2020:

Wallet-determined max ancestor count, as in getPackageLimits gArgs.GetArg("-limitancestorcount", DEFAULT_ANCESTOR_LIMIT), which defaults to 25?

kallewoof commented at 5:44 am on March 12, 2020:

Yep!

fjahr commented at 11:17 am on March 24, 2020:

I removed the default value (10000), thanks!

in test/functional/wallet_avoidreuse.py:293 in 87de015719 outdated

289@@ -288,5 +290,36 @@ def test_getbalances_used(self):
290         assert_unspent(self.nodes[1], total_count=2, total_sum=6, reused_count=1, reused_sum=1)
291         assert_balances(self.nodes[1], mine={"used": 1, "trusted": 5})
292 
293+    def test_large_destination_group(self):

jonatack commented at 5:39 pm on March 11, 2020:

Suggestion for line 202 if you have to retouch this commit (it’s unrelated but wouldn’t hurt to sneak it in, as this test is called several times):

0-  self.log.info("Test fund send fund send")
1+  self.log.info("Test fund send fund send {}".format(second_addr_type))

fjahr commented at 10:52 am on March 24, 2020:

Improved the naming overall in a separate commit. Thanks!

in test/functional/wallet_avoidreuse.py:327 in 87de015719 outdated

295+        Test the case where [1] only has 11 outputs of 1 BTC in the same reused
296+        address and tries to send a small payment of 0.5 BTC. The wallet
297+        should use 10 outputs from the reused address as inputs and not a
298+        single 1 BTC input, in order to join several outputs from the reused
299+        address.
300+        '''

kallewoof commented at 4:12 am on March 12, 2020:

I think having a test where the code DOES use the discouraged <10 leftover group would be helpful to ensure that this is correctly chosen when necessary.

fjahr commented at 10:51 am on March 24, 2020:

Added it, thanks.

jonatack commented at 5:48 pm on March 12, 2020: member

Concept ACK. Can this PR have a review club tag to help people know there is a resource?

Reviewers, review club notes and meeting logs of both sessions are here: https://bitcoincore.reviews/17824

Some of the suggestions from the sessions:

the PR title could use an improvement
the commit message could be a bit more elaborate
the changes could be better documented

(Also, there seemed to be some agreement in the sessions to consider raising OUTPUT_GROUP_MAX_ENTRIES.)

in src/wallet/wallet.cpp:2361 in 87de015719 outdated

2357@@ -2358,6 +2358,13 @@ bool CWallet::SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAm
2358             ++it;
2359     }
2360 
2361+    unsigned int limit_ancestor_count;

promag commented at 1:35 am on March 15, 2020:

I’m aware this is moved code, but I think these should be initialized.

fjahr commented at 10:51 am on March 24, 2020:

done

in src/wallet/wallet.cpp:2378 in 87de015719 outdated

2357@@ -2358,6 +2358,13 @@ bool CWallet::SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAm
2358             ++it;
2359     }
2360 
2361+    unsigned int limit_ancestor_count;
2362+    unsigned int limit_descendant_count;
2363+    chain().getPackageLimits(limit_ancestor_count, limit_descendant_count);
2364+    size_t max_ancestors = (size_t)std::max<int64_t>(1, limit_ancestor_count);

promag commented at 1:35 am on March 15, 2020:

I’m aware this is moved code, but any idea why max<int64_t>?

fjahr commented at 11:23 am on March 24, 2020:

I don’t know. I did some digging and the type was introduced here and prior to that a uint64 was first introduced here in this context. I can only speculate that since this is a value from user input it was supposed to be as forgiving as possible if a ridiculously high number is provided, but even that does not make too much sense to me.

kallewoof commented at 6:43 am on March 25, 2020:

This may have been a way to safeguard against a user defined value that was negative, but the switch to getPackageLimits removed that safeguard. Not sure if that was wise or not, but that’s a separate question.

in src/wallet/wallet.cpp:4041 in 87de015719 outdated

4040@@ -4040,6 +4041,7 @@ std::vector<OutputGroup> CWallet::GroupOutputs(const std::vector<COutput>& outpu
4041                 if (gmap[dst].m_outputs.size() >= OUTPUT_GROUP_MAX_ENTRIES) {

promag commented at 1:49 am on March 15, 2020:

nit, could fix comment in L4038, s/10/OUTPUT_GROUP_MAX_ENTRIES.

fjahr commented at 10:51 am on March 24, 2020:

done

in src/wallet/wallet.cpp:4043 in 87de015719 outdated

4040@@ -4040,6 +4041,7 @@ std::vector<OutputGroup> CWallet::GroupOutputs(const std::vector<COutput>& outpu
4041                 if (gmap[dst].m_outputs.size() >= OUTPUT_GROUP_MAX_ENTRIES) {
4042                     groups.push_back(gmap[dst]);
4043                     gmap.erase(dst);

promag commented at 1:59 am on March 15, 2020:

nit, there are 3 dst lookups in gmap, one find would be better.

fjahr commented at 10:51 am on March 24, 2020:

done

in src/wallet/wallet.cpp:4029 in 87de015719 outdated

4022@@ -4023,10 +4023,11 @@ bool CWalletTx::IsImmatureCoinBase() const
4023     return GetBlocksToMaturity() > 0;
4024 }
4025 
4026-std::vector<OutputGroup> CWallet::GroupOutputs(const std::vector<COutput>& outputs, bool single_coin) const {
4027+std::vector<OutputGroup> CWallet::GroupOutputs(const std::vector<COutput>& outputs, bool single_coin, const size_t max_ancestors) const {
4028     std::vector<OutputGroup> groups;
4029     std::map<CTxDestination, OutputGroup> gmap;
4030     CTxDestination dst;

promag commented at 2:06 am on March 15, 2020:

IMO this should be in the loop.

fjahr commented at 10:50 am on March 24, 2020:

done

fjahr force-pushed on Mar 24, 2020

fjahr renamed this:
~~wallet: Improve coin selection for destination groups >10~~
wallet: Prefer full destination groups in coin selection
on Mar 24, 2020

fjahr force-pushed on Mar 24, 2020

fjahr commented at 11:25 am on March 24, 2020: member

Thanks for all the feedback! I should have addressed all the points here and also what was discussed in the review club. Also I tried to improve the naming of this PR/commit and expanded the commit message.

kallewoof approved

kallewoof commented at 6:49 am on March 25, 2020: member

Code review ACK 9dcafb06dfa3f69bc720b5a4f1134fab95f17e40

(Sorry about the horribly named test functions, btw. Thanks for fixing them.)

in src/wallet/wallet.cpp:4206 in 9dcafb06df outdated

4048+                if (it != gmap.end()) {
4049+                    // Limit output groups to no more than OUTPUT_GROUP_MAX_ENTRIES
4050+                    // number of entries, to protect against inadvertently creating
4051+                    // a too-large transaction when using -avoidpartialspends to
4052+                    // prevent breaking consensus or surprising users with a very
4053+                    // high amount of fees.

jonatack commented at 4:59 pm on March 30, 2020:

suggest: s/a very high amount of fees./high fees./

note this feedback by @Xekyo mitigating the high fee motivation here: https://bitcoincore.reviews/17824.html#l-329

in src/wallet/wallet.cpp:4225 in 9dcafb06df outdated

4069     if (!single_coin) {
4070-        for (const auto& it : gmap) groups.push_back(it.second);
4071+        for (auto& it : gmap) {
4072+            auto& group = it.second;
4073+            if (full_groups.count(it.first) > 0) {
4074+                // Make this unattractive as we want coin selection to avoid it if possible

jonatack commented at 5:10 pm on March 30, 2020:

Could perhaps expand on this documentation, like in your commit message, along the lines of:

0-                // Make this unattractive as we want coin selection to avoid it if possible
1+                // By assigning it one less than the maximum number of ancestors
2+                // allowed for this wallet, we move this smaller subgroup to the
3+                // bottom of the priority list for the coin selection algorithm.

in src/wallet/wallet.cpp:4228 in 9dcafb06df outdated

4072+            auto& group = it.second;
4073+            if (full_groups.count(it.first) > 0) {
4074+                // Make this unattractive as we want coin selection to avoid it if possible
4075+                group.m_ancestors = max_ancestors - 1;
4076+            }
4077+            groups.push_back(group);

jonatack commented at 10:22 pm on March 30, 2020:

Do you think it might be better to construct rather than copy?

 0-                        groups.push_back(it->second);
 1+                        groups.emplace_back(it->second);
 2                         it->second = OutputGroup{};
 3-                        full_groups.insert(dst);
 4+                        full_groups.emplace(dst);
 5@@ -4208,7 +4208,7 @@ std::vector<OutputGroup> CWallet::GroupOutputs(const std::vector<COutput>& outpu
 6                 // Make this unattractive as we want coin selection to avoid it if possible
 7                 group.m_ancestors = max_ancestors - 1;
 8             }
 9-            groups.push_back(group);
10+            groups.emplace_back(group);

jonatack commented at 11:18 pm on March 30, 2020: member

ACK 9dcafb06dfa code review and built/tested on the branch and rebased on master. Tried different values in the new tests to verify they hold with different edge cases.

A few comments below but please ignore to not lose review unless you need to retouch this. Thank you for improving the first commit message – it’s very good now; two nits if you have to retouch: s/use/user/ and s/smallet/smaller/. Finally, consider updating the PR description with information from the excellent first commit message.

fjahr commented at 1:51 pm on April 2, 2020: member

@jonatack Thanks for your comments! For now, I am saving the ACKs and included these cleanups in my followup PR #18418

DrahtBot added the label Needs rebase on Apr 7, 2020

wallet: Prefer full destination groups in coin selection

When a wallet uses avoid_reuse and has a large number of outputs in
a single destination, it groups these outputs in OutputGroups that
are no larger than OUTPUT_GROUP_MAX_ENTRIES. The goal is to spend
as many outputs as possible from the destination while not breaking
consensus due to a huge number of inputs and also not surprise the
use with high fees. If there are n outputs in a destination and
n > OUTPUT_GROUP_MAX_ENTRIES then this results in one or many groups
of size OUTPUT_GROUP_MAX_ENTRIES and possibly one group of size
< OUTPUT_GROUP_MAX_ENTRIES.

Prior to this commit the coin selection in the case where
n > OUTPUT_GROUP_MAX_ENTRIES was skewed towards the one group of
size < OUTPUT_GROUP_MAX_ENTRIES if it exists and the amount to be
spent by the transaction is smaller than the aggregate of those
of the group size < OUTPUT_GROUP_MAX_ENTRIES. The reason is that
the coin selection decides between the different groups based on
fees and mostly the smaller group will cause smaller fees.

The behavior that users of the avoid_reuse flag seek is that the
full groups of size OUTPUT_GROUP_MAX_ENTRIES get used first. This
commit implements this by pretending that the small group has
a large number of ancestors (one smallet than the maximum allowed
for this wallet). This dumps the small group to the bottom of the
list of priorities in the coin selection algorithm.

1abbdac677

test: Improve naming and logging of avoid_reuse tests a2324e4d3f

fjahr force-pushed on Apr 14, 2020

fjahr commented at 1:04 pm on April 14, 2020: member

Rebased, no code changes.

DrahtBot removed the label Needs rebase on Apr 14, 2020

jonatack commented at 10:04 am on April 16, 2020: member

Re-ACK a2324e4

kallewoof commented at 11:30 am on April 16, 2020: member

ACK a2324e4d3f47f084b07a364c9a360a0bf31e86a0

achow101 commented at 8:35 pm on April 16, 2020: member

ACK a2324e4d3f47f084b07a364c9a360a0bf31e86a0

meshcollider commented at 11:02 am on April 17, 2020: contributor

Tested ACK a2324e4d3f47f084b07a364c9a360a0bf31e86a0 (verified the new test fails on master without this change)

meshcollider merged this on Apr 17, 2020

meshcollider closed this on Apr 17, 2020

sidhujag referenced this in commit 155d8bdac1 on Apr 17, 2020

deadalnix referenced this in commit 948877df90 on Oct 9, 2020

sidhujag referenced this in commit 6376dfd958 on Nov 10, 2020

fanquake referenced this in commit ecddd12482 on May 26, 2021

DrahtBot locked this on Feb 15, 2022

wallet: Prefer full destination groups in coin selection #17824

Conflicts