[test] Add test for listaddressgroupings #10255

pull jimmysong wants to merge 1 commits into bitcoin:master from jimmysong:test_listaddressgroupings changing 1 files +51 −9
  1. jimmysong commented at 8:05 PM on April 21, 2017: contributor

    Test added as part of wallet-accounts.py. Only the most basic grouping is tested. Made the file flake8 compliant. Most of the diff is whitespace.

  2. jonasschnelli added the label Tests on Apr 21, 2017
  3. in test/functional/wallet-accounts.py:33 in 19e9a262a9 outdated 
      28 | @@ -27,74 +29,74 @@ def __init__(self):
  29 |          self.node_args = [[]]
  30 |  
  31 |      def setup_network(self):
  32 | -        self.nodes = start_nodes(self.num_nodes, self.options.tmpdir, self.node_args)
  33 | +        self.nodes = start_nodes(self.num_nodes, self.options.tmpdir,
  34 | +                                 self.node_args)
    MarcoFalke commented at 5:43 PM on April 23, 2017:

    nit: As mentioned here, I don't think it makes sense to blindly apply pep8 to all python code. Also, changing white space in unrelated commits makes them harder to cherry-pick to other branches.

    jimmysong commented at 10:42 PM on April 23, 2017:

    understood, reverted most of the whitespace diff.

  4. jimmysong force-pushed on Apr 23, 2017
  5. jimmysong commented at 2:49 PM on April 25, 2017: contributor

    Reverted some of the whitespace diff.

  6. TheBlueMatt commented at 12:41 AM on April 26, 2017: member

    Hmm, it would be nice to get some more thorough testing of this in. Useful to have tests that "just call it", but to really qualify it as "tests listaddressgroupings" you might want to also create a few test addresses, create some raw transactions to them, test that addresses shared in the input are grouped which weren't previously, and then test that the groupings are transitive (ie if you have a transaction with inputs from address A & B, and another from addresses B & C, there is now one group with all three grouped, despite it being in separate transactions). If you dont want to bite that much off, still good to have these tests, but maybe dont add it to the list of "tested RPCs" at the top, and maybe we should have a blacklist on the coverage meter at the end.

  7. jimmysong commented at 1:30 AM on April 26, 2017: contributor

    @TheBlueMatt Thanks for the suggestion. I'm going to have to study a bit more on exactly how addresses group together in the wallet-accounts.py test to get a handle on what the groups should be and shouldn't be. I'm fine with removing that this RPC is tested for now or making this task a bigger one with a more thorough test of listaddressgroupings.

  8. jnewbery commented at 9:51 PM on April 28, 2017: member

    @jimmysong - I haven't looked at the complete test, so I don't know if this breaks things further down, but I imagine you can do this by:

    • generate 102 blocks instead of 101. That means the node has 2 coinbase rewards of 50BTC, which are unlinkable and so should be in different groupings.
    • construct a single transaction which sends 25 BTC from each address to an external address. The two originating addresses are now linkable and so should be in the same grouping.

    If you make this change, there shouldn't be a change in the balance, so :crossed_fingers: it shouldn't break anything further down in the test.

  9. jimmysong force-pushed on Apr 29, 2017
  10. jimmysong force-pushed on Apr 29, 2017
  11. jimmysong commented at 3:36 AM on April 29, 2017: contributor

    @jnewbery @TheBlueMatt I've changed the test per jnewbery's suggestion. Hopefully, this is enough to test? Note it was kind of tricky because of fees.

  12. jimmysong force-pushed on Apr 29, 2017
  13. in test/functional/wallet-accounts.py:60 in 59687cc1cd outdated 
      61 | +        linked_addresses.sort()
  62 | +
  63 | +        # send 50 from each address to a third address not in this wallet
  64 | +        # fee is 0.0000612, which is why we need to spend 99.9999388
  65 | +        common_address = "msf4WtN1YQKXvNtvdFYt9JBnUD2FB41kjr"
  66 | +        node.sendfrom("", common_address, 99.9999388)
    MarcoFalke commented at 12:38 PM on April 29, 2017:

    You might want to try if subtractfromfee helps here.

    node.sendmany(fromaccount="",amounts={ common_address:100},subtractfeefrom=[common_address], minconf=1)
    jimmysong commented at 9:46 PM on April 29, 2017:

    Thanks! That works great!

  14. jimmysong force-pushed on Apr 29, 2017
  15. jimmysong commented at 9:46 PM on April 29, 2017: contributor

    changed so that there's no need to hard-code fees per @MarcoFalke's suggestion.

  16. jnewbery commented at 5:42 PM on May 1, 2017: member

    Looks good @jimmysong . Thanks for persisting with this.

    I would personally prefer to do the second listaddressgroupings before you generate a new block, so the only differences between the first and second calls are:

    • in the first call, the addresses are in two separate groupings and in the second call they're in the same grouping
    • in the first call, both addresses have balance 50 and in the second call both addresses have balance 0.

    I have an example here that does that: https://github.com/jnewbery/bitcoin/tree/pr10255 . Feel free to take it for this PR.

    Another suggestion: you may be able to remove the fee entirely by using the {create,fund,send}rawtransaction RPCs instead of sendtomany. I'm not sure if that makes the test more straightforward. Up to you if you want to try that out.

  17. jimmysong force-pushed on May 1, 2017
  18. [test] Add test for listaddressgroupings
    Test added as part of wallet-accounts.py.
Make file a little more flake8 compliant
    dadfee38e8
  19. jimmysong force-pushed on May 1, 2017
  20. jimmysong commented at 6:14 PM on May 1, 2017: contributor

    Thanks @jnewbery. I've changed per your suggestions. Of course, I didn't see your tree until I had coded it up, but it's almost exactly the same. Also, rebased and squashed.

  21. jnewbery commented at 5:44 PM on May 2, 2017: member

    Looks great. utACK dadfee38e8fdf2b384d0991253865ebf9fbdb07f

  22. MarcoFalke commented at 6:17 PM on May 2, 2017: member

    utACK dadfee38e8fdf2b384d0991253865ebf9fbdb07f

  23. MarcoFalke merged this on May 2, 2017
  24. MarcoFalke closed this on May 2, 2017
  25. MarcoFalke referenced this in commit 3c5e6c94ca on May 2, 2017
  26. PastaPastaPasta referenced this in commit 974072169f on Jun 10, 2019
  27. PastaPastaPasta referenced this in commit 01d42f6d5f on Jun 10, 2019
  28. PastaPastaPasta referenced this in commit edcedba24e on Jun 10, 2019
  29. PastaPastaPasta referenced this in commit df71e82d17 on Jun 11, 2019
  30. PastaPastaPasta referenced this in commit 919704b297 on Jun 11, 2019
  31. PastaPastaPasta referenced this in commit 5188d9fa44 on Jun 15, 2019
  32. PastaPastaPasta referenced this in commit eb1ae3b87a on Jun 19, 2019
  33. PastaPastaPasta referenced this in commit e058d1d213 on Jun 19, 2019
  34. PastaPastaPasta referenced this in commit f4a74e7c86 on Jun 19, 2019
  35. PastaPastaPasta referenced this in commit d953416206 on Jun 19, 2019
  36. PastaPastaPasta referenced this in commit e2b08a2c61 on Jun 19, 2019
  37. PastaPastaPasta referenced this in commit 52de8b11d5 on Jun 19, 2019
  38. PastaPastaPasta referenced this in commit 13af34bf87 on Jun 19, 2019
  39. PastaPastaPasta referenced this in commit 26c63e626c on Jun 20, 2019
  40. barrystyle referenced this in commit 57d4af9017 on Jan 22, 2020
  41. MarcoFalke locked this on Sep 8, 2021
Contributors
jimmysongMarcoFalkeTheBlueMattjnewbery
Labels
Tests
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-27 15:16 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me