test: use default address type (bech32) for wallet_bumpfee tests #17199

1. 
   theStack commented at 0:47 am on October 20, 2019: member

   The use of native segwit addresses (pure p2wpkh instead of p2sh-p2wpkh) leads to smaller transaction sizes, needing adaption of some constants in the following test cases:

   • test_dust_to_fee(): adaption of dust calculation (p2wpkh spend estimate of 67 is taken from src/policy/policy.cpp:GetDustThreshold())
   • test_maxtxfee_fails(): lowering -maxtxfee setting to trigger fail
2. fanquake added the label Tests on Oct 20, 2019
3. fanquake requested review from instagibbs on Oct 20, 2019
4. 
   laanwj commented at 10:12 am on October 21, 2019: member

   ACK 8ca1c1cd008b489735e7b24302375220f40edd38
5. 
   instagibbs commented at 2:21 pm on October 21, 2019: member

   cursory ACK https://github.com/bitcoin/bitcoin/pull/17199/commits/8ca1c1cd008b489735e7b24302375220f40edd38

   didn’t use pencil and paper to confirm all the numbers, but at least a full explanation was added for the latter magic number.

6. test: use default address type (bech32) for wallet_bumpfee tests

   The use of native segwit addresses (pure p2wpkh instead of p2sh-p2wpkh) leads
   to smaller transaction sizes, needing adaption of some constants in the
   following test cases:
       - test_dust_to_fee(): adaption of dust calculation
             (p2wpkh spend estimate of 67 is taken from src/policy/policy.cpp:GetDustThreshold())
       - test_maxtxfee_fails(): lowering -maxtxfee setting to trigger fail

   8d8e5a79d0
7. in test/functional/wallet_bumpfee.py:41 in 8ca1c1cd00 outdated

   37@@ -38,7 +38,6 @@ def set_test_params(self):
   38             "-walletrbf={}".format(i),
   39             "-mintxfee=0.00002",
   40             "-deprecatedrpc=totalFee",
   41-            "-addresstype=p2sh-segwit", # TODO update constants in test and remove
   

   ---

   ---

   

   MarcoFalke commented at 2:01 pm on October 22, 2019:

   I don’t understand why we only test this for the default addresstype. I’d prefer to either hardcode the addresstype, so that it doesn’t break when the default changes, or to test all relevant address types.

   ---

   

   theStack commented at 2:41 pm on October 22, 2019:

   Fair point – from the comment and issue #17043 I was concluding that it was even desired if tests break after the default address changes, to increase the pressure to write/correct tests for it (which could otherwise be forgotten), but then again I have to admit that this is kind of a weak argument. Of course it would be best to have tests for all relevant address types. For now I will change the type to bech32.
8. theStack force-pushed on Oct 22, 2019
9. 
   luke-jr commented at 4:21 pm on November 4, 2019: member

   Concept ACK
10. MarcoFalke referenced this in commit 6cb10c14c6 on Nov 4, 2019
11. MarcoFalke merged this on Nov 4, 2019
12. MarcoFalke closed this on Nov 4, 2019

    ---

13. sidhujag referenced this in commit bae1f2dcc4 on Nov 7, 2019
14. sidhujag referenced this in commit d186c4ff9d on Nov 10, 2020
15. theStack deleted the branch on Dec 1, 2020
16. DrahtBot locked this on Feb 15, 2022

Contributors
theStack laanwj instagibbs MarcoFalke luke-jr

Review Requested
instagibbs

Labels
Tests