-maxapsfee follow-up #19743

kallewoof commented at 5:44 AM on August 17, 2020: member

Addresses feedback from jonatack and meshcollider in #14582.

kallewoof force-pushed on Aug 17, 2020

fanquake added the label Wallet on Aug 17, 2020

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)

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)

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.

jonatack commented at 6:44 AM on August 17, 2020: member

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

kallewoof force-pushed on Aug 17, 2020

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]

kallewoof force-pushed on Aug 17, 2020

kallewoof commented at 8:13 AM on August 17, 2020: member

Added test for when cap is exceeded.

https://github.com/bitcoin/bitcoin/blob/452befa3999853383fc960d4982b244ab23d36b7/test/functional/wallet_groups.py#L114-L122

kallewoof force-pushed on Aug 17, 2020

fjahr commented at 12:22 PM on August 17, 2020: member

Code review ACK 198c4434a2870c67e9b1b7b39800c10ec0404fb5

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

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:

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

jonatack commented at 12:54 PM on August 17, 2020: member

ACK 198c4434a28

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

kallewoof force-pushed on Aug 18, 2020

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.~

kallewoof force-pushed on Aug 18, 2020

MarcoFalke referenced this in commit e6e277f9ed on Aug 18, 2020

doc: release notes for -maxapsfee 9f77b82176

kallewoof force-pushed on Aug 18, 2020

jonatack commented at 6:49 AM on August 18, 2020: member

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

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

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

I get the same error locally ^

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

Rebase screw-up. Fixing.

kallewoof force-pushed on Aug 18, 2020

jonatack commented at 9:39 AM on August 18, 2020: member

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.

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.

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 :)

jonatack commented at 10:12 AM on August 18, 2020: member

@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

-maxapsfee: follow-up fixes

Co-authored-by: Jon Atack <jon@atack.com>
Co-authored-by: Samuel Dobson <dobsonsa68@gmail.com>

7e31ea9fa0

kallewoof force-pushed on Aug 18, 2020

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).

jonatack commented at 10:32 AM on August 18, 2020: member

ACK 7e31ea9fa0a

(if Travis acks too)

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

re-ACK 7e31ea9fa0a59ced2293057acb14c71ec97db689

Test passes locally but I'll wait for Travis

meshcollider merged this on Aug 18, 2020

meshcollider closed this on Aug 18, 2020

sidhujag referenced this in commit 0e46a26711 on Aug 18, 2020

deadalnix referenced this in commit 53106ad513 on Sep 14, 2021

DrahtBot locked this on Feb 15, 2022