-maxapsfee follow-up #19743

pull kallewoof wants to merge 2 commits into bitcoin:master from kallewoof:20200817-maxapsfee-followup changing 3 files +61 −15
  1. kallewoof commented at 5:44 AM on August 17, 2020: member

    Addresses feedback from jonatack and meshcollider in #14582.

  2. kallewoof force-pushed on Aug 17, 2020
  3. fanquake added the label Wallet on Aug 17, 2020
  4. in doc/release-notes-14582.md:7 in 920006ff37 outdated 
       0 | @@ -0,0 +1,14 @@
   1 | +Configuration
   2 | +-------------
   3 | +
   4 | +A new configuration flag `-maxapsfee` has been added, which sets the max allowed
   5 | +avoid partial spends (APS) fee. It defaults to 0 (i.e. fee is the same with
   6 | +and without APS). Setting it to -1 will disable APS, unless `-avoidpartialspends`
   7 | +is set.
    jonatack commented at 6:34 AM on August 17, 2020:

    set. (#14582)

  5. in doc/release-notes-14582.md:14 in 920006ff37 outdated 
       9 | +Wallet
  10 | +------
  11 | +
  12 | +The wallet will now avoid partial spends (APS) by default, if this does not result
  13 | +in a difference in fees compared to the non-APS variant. The allowed fee threshold
  14 | +can be adjusted using the new `-maxapsfee` configuration option.
    jonatack commented at 6:35 AM on August 17, 2020:

    option. (#14582)

  6. in test/functional/wallet_groups.py:100 in 920006ff37 outdated 
      89 | @@ -90,8 +90,8 @@ def run_test(self):
  90 |          # ~0.1 and 1.4 and should come from the same destination
  91 |          values = [vout["value"] for vout in tx3["vout"]]
  92 |          values.sort()
  93 | -        assert_approx(values[0], 0.1, 0.0001)
  94 | -        assert_approx(values[1], 1.4)
  95 | +        assert_approx(values[0], vexp=0.1, vspan=0.0001)
  96 | +        assert_approx(values[1], vexp=1.4, vspan=0.0001)
    jonatack commented at 6:40 AM on August 17, 2020:

    while here, there are six other assert_approx assertions without keyword args (two also in the new test), and two of the others which could optionally also have an explicit default vspan added

    kallewoof commented at 7:52 AM on August 17, 2020:

    Fair point. Fixed all of them.

  7. jonatack commented at 6:44 AM on August 17, 2020: contributor

    Concept ACK! Do you plan to add @meshcollider's suggestion in #14582 (review)?

  8. kallewoof force-pushed on Aug 17, 2020
  9. kallewoof commented at 7:53 AM on August 17, 2020: member

    @jonatack Thanks for review! Added a test to check the cap is not exceeded. [Edited: Will → Did]

  10. kallewoof force-pushed on Aug 17, 2020
  12. kallewoof force-pushed on Aug 17, 2020
  13. fjahr commented at 12:22 PM on August 17, 2020: contributor

    Code review ACK 198c4434a2870c67e9b1b7b39800c10ec0404fb5

    Edit: not sure about the CI failures, tests succeed for me locally.

  14. in test/functional/wallet_groups.py:126 in 198c4434a2 outdated 
     115 |  
 116 | +        addr_aps2 = self.nodes[3].getnewaddress()
 117 | +        [self.nodes[0].sendtoaddress(addr_aps2, 1.0) for _ in range(5)]
 118 | +        self.nodes[0].generate(1)
 119 | +        with self.nodes[3].assert_debug_log(['Fee non-grouped = 5520, grouped = 8240, using non-grouped']):
 120 | +            txid5 = self.nodes[3].sendtoaddress(self.nodes[0].getnewaddress(), 2.95)
    jonatack commented at 12:53 PM on August 17, 2020:

    Verified the tests all pass if -maxapsfee=0.00002719, and that they fail at this line if -maxapsfee=0.00002720 (8240 - 5520 = 2720):

    AssertionError: [node 3] Expected messages "['Fee non-grouped = 5520, grouped = 8240, using non-grouped']"

    Actual log

    Fee non-grouped = 5520, grouped = 8240, using grouped

    So if you want to test the limit, could set maxapsfee to 0.00002719.

    MarcoFalke commented at 4:58 PM on August 17, 2020:

    Test fails here:

    https://cirrus-ci.com/task/6483042137014272?command=ci#L5047

     test  2020-08-17T11:25:30.841000Z TestFramework (ERROR): JSONRPC error 
                                   Traceback (most recent call last):
                                     File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 118, in main
                                       self.run_test()
                                     File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/wallet_groups.py", line 119, in run_test
                                       txid5 = self.nodes[3].sendtoaddress(self.nodes[0].getnewaddress(), 2.95)
                                     File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/coverage.py", line 47, in __call__
                                       return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
                                     File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/authproxy.py", line 146, in __call__
                                       raise JSONRPCException(response['error'], status)
                                   test_framework.authproxy.JSONRPCException: Insufficient funds (-6)
    jonatack commented at 5:14 PM on August 17, 2020:

    image

    This may be a second one? I saw it at line 106. Edit: in the reported issue WRT master.

  15. jonatack commented at 12:54 PM on August 17, 2020: contributor

    ACK 198c4434a28

  16. jonatack cross-referenced this on Aug 17, 2020 from issue Intermittent CI failure: wallet_groups.py by practicalswift
  17. in test/functional/wallet_groups.py:112 in 198c4434a2 outdated 
     102 | @@ -103,13 +103,25 @@ def run_test(self):
 103 |          self.nodes[0].sendtoaddress(addr_aps, 1.0)
 104 |          self.nodes[0].sendtoaddress(addr_aps, 1.0)
 105 |          self.nodes[0].generate(1)
 106 | -        txid4 = self.nodes[3].sendtoaddress(self.nodes[0].getnewaddress(), 0.1)
 107 | +        self.sync_all()
    jonatack commented at 3:15 PM on August 17, 2020:

    Hopefully this line will fix #19749

  18. kallewoof force-pushed on Aug 18, 2020
  19. kallewoof cross-referenced this on Aug 18, 2020 from issue tests: add sync_all to fix race condition in wallet groups test by kallewoof
  20. kallewoof commented at 1:10 AM on August 18, 2020: member

    ~This should fix; if waiting for this to be merge-ready is undesired, #19756 was opened.~

  21. kallewoof force-pushed on Aug 18, 2020
  22. MarcoFalke referenced this in commit e6e277f9ed on Aug 18, 2020
  23. doc: release notes for -maxapsfee 9f77b82176
  24. kallewoof force-pushed on Aug 18, 2020
  25. jonatack commented at 6:49 AM on August 18, 2020: contributor

    ACK 19cf0d3 per git range-diff e6e277f9 198c443 19cf0d3 provided the CI agrees; the test passes for me locally

  26. kallewoof commented at 7:55 AM on August 18, 2020: member

    Weirdly, it's now giving a different message.

    AssertionError: [node 3] Expected messages "['Fee non-grouped = 2820, grouped = 4160, using grouped']" does not partially match log: [...] 2020-08-18T07:17:37.634268Z [httpworker.1] [default wallet] Fee non-grouped = 2820, grouped = 2820, using grouped

  27. meshcollider commented at 8:06 AM on August 18, 2020: contributor

    I get the same error locally ^

  28. kallewoof commented at 8:12 AM on August 18, 2020: member

    Rebase screw-up. Fixing.

  29. kallewoof force-pushed on Aug 18, 2020
  30. jonatack commented at 9:39 AM on August 18, 2020: contributor

    ACK 19cf0d3 per git range-diff e6e277f9 198c443 19cf0d3 provided the CI agrees; the test passes for me locally

    I think I mistakenly ran the test while still on commit 198c443. Re-reviewing.

  31. in test/functional/wallet_groups.py:124 in b740b7637c outdated 
     120 | +        with self.nodes[3].assert_debug_log(['Fee non-grouped = 5520, grouped = 8240, using non-grouped']):
 121 | +            txid5 = self.nodes[3].sendtoaddress(self.nodes[0].getnewaddress(), 2.95)
 122 | +        tx5 = self.nodes[3].getrawtransaction(txid5, True)
 123 | +        # tx5 should have 3 inputs (1.0, 1.0, 1.0) and 2 outputs
 124 | +        assert_equal(3, len(tx5["vin"]))
 125 | +        assert_equal(2, len(tx4["vout"]))
    jonatack commented at 9:40 AM on August 18, 2020:

    Should this be tx5?

            assert_equal(2, len(tx5["vout"]))
    kallewoof commented at 10:22 AM on August 18, 2020:

    It should! Gah thanks.

  32. meshcollider commented at 9:46 AM on August 18, 2020: contributor

    functional test run ACK b740b7637c101d1aee4de98b43bbb36c35cbe351 modulo jon's comment (i.e. passes with his suggested change)

    Btw Kalle, we edit out the '@' symbol tags in the OP because they get added to the merge commit and it spams users every time any other fork of the repo cherry-picks the PR :)

  33. jonatack commented at 10:12 AM on August 18, 2020: contributor

    @kallewoof further to #19743 (review), here is a proposed additional test commit if you feel like pulling it in:

    https://github.com/jonatack/bitcoin/commits/maxapsfee-tests

  34. -maxapsfee: follow-up fixes
    Co-authored-by: Jon Atack <jon@atack.com>
Co-authored-by: Samuel Dobson <dobsonsa68@gmail.com>
    7e31ea9fa0
  35. kallewoof force-pushed on Aug 18, 2020
  36. kallewoof commented at 10:27 AM on August 18, 2020: member

    Squashed @jonatack's improvements (including tx4 → tx5 typo) into main commit (already co-authored-by tagged).

  37. jonatack commented at 10:32 AM on August 18, 2020: contributor

    ACK 7e31ea9fa0a

    (if Travis acks too)

  38. meshcollider commented at 10:49 AM on August 18, 2020: contributor

    re-ACK 7e31ea9fa0a59ced2293057acb14c71ec97db689

    Test passes locally but I'll wait for Travis

  39. meshcollider merged this on Aug 18, 2020
  40. meshcollider closed this on Aug 18, 2020
  41. sidhujag referenced this in commit 0e46a26711 on Aug 18, 2020
  42. deadalnix referenced this in commit 53106ad513 on Sep 14, 2021
  43. bitcoin locked this on Feb 15, 2022
Contributors
kallewoofjonatackfjahrMarcoFalkemeshcollider
Labels
Wallet
Linked (view graph)
#14582 wallet: always do avoid partial spends if fees are within a specified range#19749 Intermittent CI failure: wallet_groups.py#19756 tests: add sync_all to fix race condition in wallet groups test
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-05-19 06:53 UTC