[wallet] Correctly limit output group size

sdaftuar commented at 3:38 pm on July 30, 2018: member

Also add a test to ensure that output groups are being limited, even if a wallet has many outputs corresponding to the same scriptPubKey (the test fails without the first commit).

[wallet] correctly limit output group size 57ec1c97b2

[qa] Add test for too-large wallet output groups a13647b8bd

sdaftuar force-pushed on Jul 30, 2018

kallewoof commented at 4:24 pm on July 30, 2018: member

utACK a13647b

Thanks for fixing this & adding test for it!

in src/wallet/wallet.cpp:4426 in a13647b8bd

4422@@ -4423,7 +4423,10 @@ std::vector<OutputGroup> CWallet::GroupOutputs(const std::vector<COutput>& outpu
4423             size_t ancestors, descendants;
4424             mempool.GetTransactionAncestry(output.tx->GetHash(), ancestors, descendants);
4425             if (!single_coin && ExtractDestination(output.tx->tx->vout[output.i].scriptPubKey, dst)) {
4426-                if (gmap.count(dst) == 10) {
4427+                // Limit output groups to no more than 10 entries, to protect

Empact commented at 4:27 pm on July 30, 2018:

9

kallewoof commented at 4:35 pm on July 30, 2018:

If it has 10 entries, …. so 10 is correct, no?

Empact commented at 4:45 pm on July 30, 2018:

I may be misreading but I see:

“no more than 10” != >= 10
“no more than 10” == > 10
“no more than 9” == >= 10

Make sense?

kallewoof commented at 5:21 pm on July 30, 2018:

I agree that no more than 10 is misleading, when using >=, but no more than 9 is not correct here, as it will continue adding even if there are 9 entries in the gmap entry.

Perhaps Limit output groups to roughly 10 entries to convey the >= (although in reality, every insertion is always 1 element, so it will stop at exactly 10).

Empact commented at 5:54 pm on July 30, 2018:

“no more than 9” means “9 or fewer”, so “will continue adding even if there are 9 entries in the gmap entry” is not incongruent.

kallewoof commented at 6:32 pm on July 30, 2018:

Huh… The statement “no more than 9” being equivalent to >= 10 is not how it works in my head but maybe I am confused about the expression. :)

Empact commented at 7:03 pm on July 30, 2018:

Hah. Ok I tend to think visually so here is an attempt at visualization:

0>= 10: (..., 11, 10), 9, ...
1> 10: (..., 11), 10, 9, ...
2"no more than 9": (..., 11, 10), 9, ...
3"no more than 10": (..., 11), 10, 9, ...

Note the “no more than” conditions are inverted.

MarcoFalke added this to the milestone 0.17.0 on Jul 30, 2018

MarcoFalke added the label Wallet on Jul 30, 2018

laanwj commented at 4:59 pm on July 30, 2018: member

utACK a13647b8bd667ca58d8e82682c1d46555dce42c9 Good catch!

in test/functional/wallet_groups.py:73 in a13647b8bd

64@@ -63,5 +65,29 @@ def run_test (self):
65         assert_approx(v[0], 0.2)
66         assert_approx(v[1], 1.3, 0.0001)
67 
68+        # Empty out node2's wallet
69+        self.nodes[2].sendtoaddress(address=self.nodes[0].getnewaddress(), amount=self.nodes[2].getbalance(), subtractfeefromamount=True)
70+        self.sync_all()
71+        self.nodes[0].generate(1)
72+
73+        # Fill node2's wallet with 10000 outputs corresponding to the same

promag commented at 11:48 pm on July 30, 2018:

nit, ... 10000 outputs (5 transactions, 2000 outputs each)....

in test/functional/wallet_groups.py:68 in a13647b8bd

64@@ -63,5 +65,29 @@ def run_test (self):
65         assert_approx(v[0], 0.2)
66         assert_approx(v[1], 1.3, 0.0001)
67 
68+        # Empty out node2's wallet

promag commented at 11:51 pm on July 30, 2018:

Add node3 instead?

in test/functional/wallet_groups.py:76 in a13647b8bd

71+        self.nodes[0].generate(1)
72+
73+        # Fill node2's wallet with 10000 outputs corresponding to the same
74+        # scriptPubKey
75+        for i in range(5):
76+            raw_tx = self.nodes[0].createrawtransaction([{"txid":"0"*64, "vout":0}], [{addr2[0]: 0.05}])

promag commented at 11:55 pm on July 30, 2018:

Side note, I wonder if createrawtransaction should allow duplicate addresses now that outputs is an array? cc @MarcoFalke

BTW, #12419 would break this — maybe I should close that?

Edit, inputs can be empty: createrawtransaction([], ... and remove tx.vin = [] below.

promag commented at 11:56 pm on July 30, 2018: member

Tested ACK a13647b.

MarcoFalke commented at 4:00 pm on August 1, 2018: member

utACK a13647b8bd667ca58d8e82682c1d46555dce42c9 (test only. confirmed that it is failing on master without recompiling. didn’t look at the cpp code changes)

MarcoFalke added this to the "Mergeable" column in a project

MarcoFalke removed this from the "Mergeable" column in a project

MarcoFalke merged this on Aug 1, 2018

MarcoFalke closed this on Aug 1, 2018

MarcoFalke referenced this in commit c88529a178 on Aug 1, 2018

UdjinM6 referenced this in commit 49a2b03116 on Jun 29, 2021

UdjinM6 referenced this in commit 473a075559 on Jun 29, 2021

UdjinM6 referenced this in commit b4b5ee583a on Jul 1, 2021

UdjinM6 referenced this in commit b33c0294e7 on Jul 2, 2021

UdjinM6 referenced this in commit 4bbf00a6e4 on Jul 2, 2021

MarcoFalke locked this on Sep 8, 2021

[wallet] Correctly limit output group size #13805