test: test the >10 UTXO case for output groups #17838

kallewoof commented at 5:40 AM on December 31, 2019: member

#17603 notes behavior where avoidreuse will only spend 1 of the UTXO:s ~and mark all of them as spent~. This is ~correct~ currently expected behavior, but the reasoning is non-obvious: the user creates 31 outputs, and the code groups these into 4 groups, 3x10 and 1x1. It then ends up using the 1x1 group because it is smaller => fewer fees.

This PR adds a test that explicitly checks this case, but in a more direct manner; it creates 30 outputs of 1 btc each, all sending to the same destination, and the receiver then sends 11 btc back. From [10, 10, 10] coin select will pick 2, leave 1 alone, and create 11 out/9 change. The user will end up with ~~10 btc used, 9 btc unused.~~ 19 btc unused.*

(* this is because the system is currently set up to mark things as they come into the wallet; already existing stuff is only marked if the user does a rescan, which the caveat states when enabling the flag)

DrahtBot added the label Tests on Dec 31, 2019

DrahtBot commented at 8:22 AM on December 31, 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:

#17889 (wallet: Improve CWallet:MarkDestinationsDirty by promag)
#17843 (wallet: Reset reused transactions cache by fjahr)
#17824 (wallet: Improve coin selection for destination groups >10 by fjahr)

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:252 in b3ad0cd69c outdated

 247 | +        Test the case where [1] generates a new address A, then
 248 | +        [0] sends 1 BTC to A 30 times.
 249 | +        [1] spends 11 BTC from A
 250 | +        20 of the outputs in A should be used, leaving 10 intact.
 251 | +        Those 10 should all be marked used.
 252 | +        A single output of 9 btc should be change back to self, and should not be marked used.

jonatack commented at 9:06 AM on December 31, 2019:

nits: s/change/changed/, and s/self/itself/ ?

kallewoof commented at 10:42 AM on December 31, 2019:

It's change, as in money given back, so I think it's correct as it is, but I'll make it clearer.

jonatack commented at 11:14 AM on December 31, 2019:

Oh, of course. Yes, I read it wrong.

in test/functional/wallet_avoidreuse.py:275 in b3ad0cd69c outdated

 270 | +        self.sync_all()
 271 | +
 272 | +        # listunspent should show 1 unused 9 btc output, and 10 used 1 btc outputs
 273 | +        assert_unspent(self.nodes[1], total_count=11, total_sum=19, reused_supported=True, reused_count=10)
 274 | +        # getbalances should show 10 used, 9 btc trusted
 275 | +        assert_balances(self.nodes[1], mine={"used": 10, "trusted": 9})

jonatack commented at 9:26 AM on December 31, 2019:

This assert is failing for me; will provide details in the review summary.

kallewoof commented at 11:26 AM on December 31, 2019:

Thanks for testing -- I have modified the test to work according to the implementation, which has the limitation that it only marks new utxos as used when it sees them -- to get the 100% correct behavior the user would have to run a rescan. To be honest, this should probably be fixed..

jonatack commented at 9:29 AM on December 31, 2019: member

Thanks for adding the test. The last assertion fails for me with 19 btc trusted instead of the expected 10 used, 9 trusted:

         # getbalances should show 10 used, 9 btc trusted
+        import pprint
+        pprint.pprint(self.nodes[0].getbalances())
+        pprint.pprint(self.nodes[1].getbalances())
         assert_balances(self.nodes[1], mine={"used": 10, "trusted": 9})

2019-12-31T09:24:55.314000Z TestFramework (INFO): Test many utxos one destination
{'mine': {'immature': Decimal('2262.50308860'),
          'trusted': Decimal('5419.99691140'),
          'untrusted_pending': Decimal('11.00000000')}}
{'mine': {'immature': Decimal('0E-8'),
          'trusted': Decimal('18.99971440'),
          'untrusted_pending': Decimal('0E-8'),
          'used': Decimal('0E-8')}}
2019-12-31T09:25:01.212000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 112, in main
    self.run_test()
  File "test/functional/wallet_avoidreuse.py", line 91, in run_test
    self.test_many_utxos_one_dest()
  File "test/functional/wallet_avoidreuse.py", line 278, in test_many_utxos_one_dest
    assert_balances(self.nodes[1], mine={"used": 10, "trusted": 9})
  File "test/functional/wallet_avoidreuse.py", line 64, in assert_balances
    assert_approx(got[k], v, 0.001)
  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/util.py", line 31, in assert_approx
    raise AssertionError("%s < [%s..%s]" % (str(v), str(vexp - vspan), str(vexp + vspan)))
AssertionError: 0E-8 < [9.999..10.001]

kallewoof commented at 10:41 AM on December 31, 2019: member

@jonatack I spoke^Wpushed too soon (I must have been absent-minded as I saw that error locally before pushing).

kallewoof force-pushed on Dec 31, 2019

in test/functional/wallet_avoidreuse.py:63 in d0cf6e29a1 outdated

  59 | @@ -60,6 +60,7 @@ def assert_unspent(node, total_count=None, total_sum=None, reused_supported=None
  60 |  def assert_balances(node, mine):
  61 |      '''Make assertions about a node's getbalances output'''
  62 |      got = node.getbalances()["mine"]
  63 | +    print(got)

jonatack commented at 5:22 PM on December 31, 2019:

Remove this print?

kallewoof force-pushed on Jan 1, 2020

kallewoof commented at 4:06 AM on January 1, 2020: member

Removed left-over print and tweaked comment.

kallewoof force-pushed on Jan 1, 2020

in test/functional/wallet_avoidreuse.py:260 in afc72f9e8c outdated

 255 | +        self.log.info("Test many utxos one destination")
 256 | +
 257 | +        fundaddr = self.nodes[1].getnewaddress()
 258 | +        retaddr = self.nodes[0].getnewaddress()
 259 | +
 260 | +        [self.nodes[0].sendtoaddress(fundaddr, 1) for _ in range(30)]

jimmysong commented at 5:56 PM on January 2, 2020:

Generally, this is not a pattern seen in the codebase. When list comprehensions are done, they're done to assign to a variable. It's also a little harder to read. A two-liner like this is easier.:

for _ in range(30):
    self.nodes[0].sendtoaddress(fundaddr, 1)

jimmysong commented at 5:57 PM on January 2, 2020:

Also, a comment above would be useful ( # send one bitcoin to the funding address 30 times)

kallewoof commented at 3:49 AM on January 3, 2020:

No, there are cases where this is done:

https://github.com/bitcoin/bitcoin/blob/17e14ac92fced92457945859463eda5d435bdd49/test/functional/feature_block.py#L1301

https://github.com/bitcoin/bitcoin/blob/17e14ac92fced92457945859463eda5d435bdd49/test/functional/p2p_compactblocks.py#L601

Exactly the same pattern:

https://github.com/bitcoin/bitcoin/blob/17e14ac92fced92457945859463eda5d435bdd49/test/functional/p2p_feefilter.py#L82

Probably more examples.

jimmysong commented at 4:13 AM on January 3, 2020:

OK, I missed those. I still don't think it's that clear, but if it's there already, I'm happy to have it be a pattern.

kallewoof commented at 4:38 AM on January 3, 2020:

Yeah, you're probably right. I'll change it to be more clear and add comment. Thanks for the feedback.

jimmysong commented at 5:58 PM on January 2, 2020: contributor

Looks good other than 1 nit. Thanks for your PR!

in test/functional/wallet_avoidreuse.py:295 in afc72f9e8c outdated

 272 | +
 273 | +        # listunspent should show 11 unused outputs (you might expect 1 unused 9
 274 | +        # btc output, and 10 used 1 btc outputs, but due to how things are set
 275 | +        # up this requires a rescan by the user; this should probably be fixed).
 276 | +        assert_unspent(self.nodes[1], total_count=11, total_sum=19, reused_supported=True, reused_count=10)
 277 | +        assert_balances(self.nodes[1], mine={"used": 0, "trusted": 19})

jonatack commented at 6:47 PM on January 2, 2020:

Verified what happens after a rescan: getbalances changes, but not listunspent.

+
+        # After -rescan, getbalances shows 1 unused 9 btc output, and 10 used 1 btc outputs.
+        self.restart_node(1, extra_args=self.extra_args[1] + ['-rescan'])
+        assert_unspent(self.nodes[1], total_count=11, total_sum=19, reused_supported=True, reused_count=10)
+        assert_balances(self.nodes[1], mine={"trusted": 9, "used": 10})

Worthwhile to add this assertion and update the comment in line 273?

I'd also like to compare and test this on the changes in the related PRs.

kallewoof commented at 4:40 AM on January 3, 2020:

Good idea! Thanks, added to test.

kallewoof force-pushed on Jan 3, 2020

DrahtBot added the label Needs rebase on Jan 7, 2020

test: test the >10 UTXO case for output groups 59d9b513e2

kallewoof force-pushed on Jan 8, 2020

DrahtBot removed the label Needs rebase on Jan 8, 2020

DrahtBot added the label Needs rebase on Jan 15, 2020

DrahtBot commented at 10:11 AM on January 15, 2020: member

Needs rebase

kallewoof commented at 10:45 AM on January 15, 2020: member

Rendered redundant with #17843.

kallewoof closed this on Jan 15, 2020

kallewoof deleted the branch on Jan 15, 2020

DrahtBot locked this on Feb 15, 2022