tests: Remove hardcoded addresstype in rpc_psbt.py #32701

1. 
   Christewart commented at 5:35 pm on June 7, 2025: contributor

   Taproot PSBT support was added in #22558 so I believe hard coding the -addresstype’s in the test setup is no longer necessary as the TODO indicates?
2. tests: Remove hardcoded addresstype in rpc_psbt.py e8a07765a3
3. 
   DrahtBot commented at 5:35 pm on June 7, 2025: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32701.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	b-l-u-e

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

4. DrahtBot added the label Tests on Jun 7, 2025
5. Christewart renamed this:
   tests: Remove hardcoded addresstype in rpc_psbt.py
   tests: Remove hardcoded addresstype in `rpc_psbt.py`
   on Jun 7, 2025
6. 
   b-l-u-e commented at 6:49 pm on June 8, 2025: none

   Code review ACK e8a0776

   I have tested the code. The change correctly removes the hardcoded addresstype restrictions that were added as a temporary measure before Taproot PSBT support was fully implemented.

   Testing performed:

   • Verified the test passes with the changes: test/functional/test_runner.py rpc_psbt
   • Confirmed the test also passes on master branch

   Rationale: The TODO comment explicitly stated this restriction should be removed “once taproot has psbt extensions”.

   Code quality:

   • Clean, minimal change that removes technical debt
   • No breaking changes or test modifications needed
   • Proper commit message following project conventions
7. fanquake requested review from achow101 on Jun 10, 2025
8. 
   danielabrozzoni commented at 4:02 pm on June 10, 2025: contributor

   I tried reviewing this PR, but I’m a bit confused about what’s going on, and I’m not sure what’s the best path forward :) My two sats:

   As I understand it, setting addresstype enforces a specific address format. If it’s not set, the default is used. Currently, the default is bech32, so removing the config option has no effect:

   https://github.com/bitcoin/bitcoin/blob/c1d4253d316ea627f46b26e395d7b48842258381/src/wallet/wallet.h#L149)

   While reviewing this PR, I expected that removing the addresstype before #22558 (commit 2111f32f2a6998531871e7792b5208992868ba7f) would make the test fail, but it doesn’t, exactly because of the default address type.

   However, if the default address type was changed to bech32m on master, this test would fail, as the error message for one transaction weight test would be different:

    02025-06-10T14:14:08.154577Z TestFramework (INFO): PRNG seed is: 3826738977563046413
    12025-06-10T14:14:08.155027Z TestFramework (INFO): Initializing test directory /tmp/nix-shell-117098-0/bitcoin_func_test_zxg20aw3
    22025-06-10T14:14:08.800678Z TestFramework (INFO): Test for invalid maximum transaction weights
    32025-06-10T14:14:08.838479Z TestFramework (ERROR): Assertion failed
    4Traceback (most recent call last):
    5  File "/home/daniela/Developer/bitcoin/test/functional/test_framework/util.py", line 164, in try_rpc
    6    fun(*args, **kwds)
    7  File "/home/daniela/Developer/bitcoin/test/functional/test_framework/coverage.py", line 50, in __call__
    8    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
    9                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   10  File "/home/daniela/Developer/bitcoin/test/functional/test_framework/authproxy.py", line 151, in __call__
   11    raise JSONRPCException(response['error'], status)
   12test_framework.authproxy.JSONRPCException: Maximum transaction weight is less than transaction weight without inputs (-4)
   13
   14During handling of the above exception, another exception occurred:
   15
   16Traceback (most recent call last):
   17  File "/home/daniela/Developer/bitcoin/test/functional/test_framework/test_framework.py", line 190, in main
   18    self.run_test()
   19  File "/home/daniela/Developer/bitcoin/./build/test/functional/rpc_psbt.py", line 405, in run_test
   20    assert_raises_rpc_error(-4, "Maximum transaction weight is too low, can not accommodate change output", self.nodes[0].walletcreatefundedpsbt, [], dest_arg_large, 0, {"max_tx_weight": (large_tx_vsize_without_inputs + 1) * WITNESS_SCALE_FACTOR})
   21  File "/home/daniela/Developer/bitcoin/test/functional/test_framework/util.py", line 155, in assert_raises_rpc_error
   22    assert try_rpc(code, message, fun, *args, **kwds), "No exception raised"
   23           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   24  File "/home/daniela/Developer/bitcoin/test/functional/test_framework/util.py", line 170, in try_rpc
   25    raise AssertionError(
   26AssertionError: Expected substring not found in error message:
   27substring: 'Maximum transaction weight is too low, can not accommodate change output'
   28error message: 'Maximum transaction weight is less than transaction weight without inputs'.
   292025-06-10T14:14:08.895803Z TestFramework (INFO): Not stopping nodes as test failed. The dangling processes will be cleaned up later.
   302025-06-10T14:14:08.896268Z TestFramework (WARNING): Not cleaning up dir /tmp/nix-shell-117098-0/bitcoin_func_test_zxg20aw3
   312025-06-10T14:14:08.896518Z TestFramework (ERROR): Test failed. Test logging available at /tmp/nix-shell-117098-0/bitcoin_func_test_zxg20aw3/test_framework.log
   322025-06-10T14:14:08.896955Z TestFramework (ERROR): 
   332025-06-10T14:14:08.897465Z TestFramework (ERROR): Hint: Call /home/daniela/Developer/bitcoin/test/functional/combine_logs.py '/tmp/nix-shell-117098-0/bitcoin_func_test_zxg20aw3' to consolidate all logs
   342025-06-10T14:14:08.897740Z TestFramework (ERROR): 
   352025-06-10T14:14:08.898000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log.
   362025-06-10T14:14:08.898268Z TestFramework (ERROR): https://github.com/bitcoin/bitcoin/issues
   372025-06-10T14:14:08.898478Z TestFramework (ERROR): 
   38[node 2] Cleaning up leftover process
   39[node 1] Cleaning up leftover process
   40[node 0] Cleaning up leftover process
   

   So, it seems that setting the address type still serves a purpose: it explicitly ensures that tests run with bech32 rather than any other format.

   I wonder if it’s okay to get rid of it entirely, at the cost of being a little bit less explicit (and whoever updates the default address type will then have to fix the test :)), if instead we should update the getnewaddress calls to include address_type where needed, or if we should simply remove the TODO and turn it into a comment explaining why addresstype is needed.

9. 
   achow101 commented at 5:59 pm on June 10, 2025: member

   if instead we should update the getnewaddress calls to include address_type where needed

   I think getnewaddress calls should be explicit about address types when a specific type is needed.

Contributors
Christewart DrahtBot b-l-u-e danielabrozzoni achow101

Review Requested
achow101

Labels
Tests