tests: Remove hardcoded addresstype in `rpc

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?

tests: Remove hardcoded addresstype in rpc_psbt.py e8a07765a3

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.

DrahtBot added the label Tests on Jun 7, 2025

Christewart renamed this:
~~tests: Remove hardcoded addresstype in rpc_psbt.py~~
tests: Remove hardcoded addresstype in `rpc_psbt.py`
on Jun 7, 2025

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

fanquake requested review from achow101 on Jun 10, 2025

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:

2025-06-10T14:14:08.154577Z TestFramework (INFO): PRNG seed is: 3826738977563046413
2025-06-10T14:14:08.155027Z TestFramework (INFO): Initializing test directory /tmp/nix-shell-117098-0/bitcoin_func_test_zxg20aw3
2025-06-10T14:14:08.800678Z TestFramework (INFO): Test for invalid maximum transaction weights
2025-06-10T14:14:08.838479Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/home/daniela/Developer/bitcoin/test/functional/test_framework/util.py", line 164, in try_rpc
    fun(*args, **kwds)
  File "/home/daniela/Developer/bitcoin/test/functional/test_framework/coverage.py", line 50, in __call__
    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/daniela/Developer/bitcoin/test/functional/test_framework/authproxy.py", line 151, in __call__
    raise JSONRPCException(response['error'], status)
test_framework.authproxy.JSONRPCException: Maximum transaction weight is less than transaction weight without inputs (-4)

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/daniela/Developer/bitcoin/test/functional/test_framework/test_framework.py", line 190, in main
    self.run_test()
  File "/home/daniela/Developer/bitcoin/./build/test/functional/rpc_psbt.py", line 405, in run_test
    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})
  File "/home/daniela/Developer/bitcoin/test/functional/test_framework/util.py", line 155, in assert_raises_rpc_error
    assert try_rpc(code, message, fun, *args, **kwds), "No exception raised"
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/daniela/Developer/bitcoin/test/functional/test_framework/util.py", line 170, in try_rpc
    raise AssertionError(
AssertionError: Expected substring not found in error message:
substring: 'Maximum transaction weight is too low, can not accommodate change output'
error message: 'Maximum transaction weight is less than transaction weight without inputs'.
2025-06-10T14:14:08.895803Z TestFramework (INFO): Not stopping nodes as test failed. The dangling processes will be cleaned up later.
2025-06-10T14:14:08.896268Z TestFramework (WARNING): Not cleaning up dir /tmp/nix-shell-117098-0/bitcoin_func_test_zxg20aw3
2025-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
2025-06-10T14:14:08.896955Z TestFramework (ERROR): 
2025-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
2025-06-10T14:14:08.897740Z TestFramework (ERROR): 
2025-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.
2025-06-10T14:14:08.898268Z TestFramework (ERROR): https://github.com/bitcoin/bitcoin/issues
2025-06-10T14:14:08.898478Z TestFramework (ERROR): 
[node 2] Cleaning up leftover process
[node 1] Cleaning up leftover process
[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.

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.

Christewart commented at 12:08 AM on June 26, 2025: contributor

@danielabrozzoni thank you for the kick ass review! You are exactly right.

I made half hearted attempt to take yours and @achow101 's suggestions to make this test case work, but I think this is a bit more work that I would like to invest in this currently: 874064c781e822c8f7d49873e275e77f63acde86

AFAICT, the test suite only works for bech32 address types and is broken for all other address types - so a large overhaul for this test suite is needed to thoroughly test PSBT operations for all of our different utxo types (legacy, p2sh-segwit, bech32, and bech32m as of this writing).

There will also need to be new calculations done for hard coded values in the test suite. An example of that is here when calculating approximate fee rates for transactions.

I'm closing this for now as I am not going to continue working on it, if anyone else wants to pick up where I left off -- feel free.

Christewart closed this on Jun 26, 2025

tests: Remove hardcoded addresstype in `rpc_psbt.py` #32701

Code Coverage & Benchmarks

Reviews