qa: `wallet_importdescriptors` failure "TypeError: 'bool' object is not subscriptable" #31881

hebasto commented at 1:36 PM on February 16, 2025: member

From https://github.com/bitcoin/bitcoin/actions/runs/13355177559/job/37296904714:

244/317 - wallet_importdescriptors.py --descriptors failed, Duration: 13 s

stdout:
2025-02-16T12:59:56.602000Z TestFramework (INFO): PRNG seed is: 3316204795374696824
2025-02-16T12:59:56.602000Z TestFramework (INFO): Initializing test directory /Users/runner/work/bitcoin/bitcoin/ci/scratch/test_runner/test_runner_₿_🏃_20250216_125556/wallet_importdescriptors_99
2025-02-16T12:59:57.273000Z TestFramework (INFO): Setting up wallets
2025-02-16T12:59:57.317000Z TestFramework (INFO): Mining coins
2025-02-16T12:59:58.510000Z TestFramework (INFO): Import should fail if a descriptor is not provided
2025-02-16T12:59:58.519000Z TestFramework (INFO): Should import a p2pkh descriptor
2025-02-16T12:59:58.537000Z TestFramework (INFO): Test can import same descriptor with public key twice
2025-02-16T12:59:58.543000Z TestFramework (INFO): Test can update descriptor label
2025-02-16T12:59:58.550000Z TestFramework (INFO): Internal addresses cannot have labels
2025-02-16T12:59:58.552000Z TestFramework (INFO): Internal addresses should be detected as such
2025-02-16T12:59:58.564000Z TestFramework (INFO): Should not import a p2sh-p2wpkh descriptor without checksum
2025-02-16T12:59:58.564000Z TestFramework (INFO): Should not import a p2sh-p2wpkh descriptor that has range specified
2025-02-16T12:59:58.565000Z TestFramework (INFO): Should not import a p2sh-p2wpkh descriptor and have it set to active
2025-02-16T12:59:58.566000Z TestFramework (INFO): Should import a (non-active) p2sh-p2wpkh descriptor
2025-02-16T12:59:58.582000Z TestFramework (INFO): Should import a 1-of-2 bare multisig from descriptor
2025-02-16T12:59:58.584000Z TestFramework (INFO): Should not treat individual keys from the imported bare multisig as watchonly
2025-02-16T12:59:58.585000Z TestFramework (INFO): Ranged descriptors cannot have labels
2025-02-16T12:59:58.586000Z TestFramework (INFO): Private keys required for private keys enabled wallet
2025-02-16T12:59:58.586000Z TestFramework (INFO): Ranged descriptor import should warn without a specified range
2025-02-16T12:59:58.590000Z TestFramework (INFO): Should not import a ranged descriptor that includes xpriv into a watch-only wallet
2025-02-16T12:59:58.590000Z TestFramework (INFO): Should not import a descriptor with hardened derivations when private keys are disabled
2025-02-16T12:59:58.595000Z TestFramework (INFO): Verify we can only extend descriptor's range
2025-02-16T12:59:58.619000Z TestFramework (INFO): Check we can change descriptor internal flag
2025-02-16T12:59:58.633000Z TestFramework (INFO): Key ranges should be imported in order
2025-02-16T12:59:58.665000Z TestFramework (INFO): Check we can change next_index
2025-02-16T12:59:58.678000Z TestFramework (INFO): Check imported descriptors are not active by default
2025-02-16T12:59:58.680000Z TestFramework (INFO): Check can activate inactive descriptor
2025-02-16T12:59:58.683000Z TestFramework (INFO): Check can deactivate active descriptor
2025-02-16T12:59:58.687000Z TestFramework (INFO): Verify activation state is persistent
2025-02-16T12:59:58.693000Z TestFramework (INFO): Should import a descriptor with a WIF private key as spendable
2025-02-16T12:59:58.696000Z TestFramework (INFO): Test can import same descriptor with private key twice
2025-02-16T12:59:58.713000Z TestFramework (INFO): Test that multisigs can be imported, signed for, and getnewaddress'd
2025-02-16T13:00:02.944000Z TestFramework (INFO): Multisig with distributed keys
2025-02-16T13:00:03.441000Z TestFramework (INFO): We can create and use a huge multisig under P2WSH
2025-02-16T13:00:04.656000Z TestFramework (INFO): Under P2SH, multisig are standard with up to 15 compressed keys
2025-02-16T13:00:05.714000Z TestFramework (INFO): Amending multisig with new private keys
2025-02-16T13:00:05.897000Z TestFramework (INFO): Combo descriptors cannot be active
2025-02-16T13:00:05.897000Z TestFramework (INFO): Descriptors with no type cannot be active
2025-02-16T13:00:05.900000Z TestFramework (INFO): Test importing a descriptor to an encrypted wallet
2025-02-16T13:00:09.233000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/Users/runner/work/bitcoin/bitcoin/ci/scratch/build-aarch64-apple-darwin23.6.0/test/functional/wallet_importdescriptors.py", line 711, in run_test
    duration = wallet_info["scanning"]["duration"]
               ~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^
TypeError: 'bool' object is not subscriptable

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Users/runner/work/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 135, in main
    self.run_test()
    ~~~~~~~~~~~~~^^
  File "/Users/runner/work/bitcoin/bitcoin/ci/scratch/build-aarch64-apple-darwin23.6.0/test/functional/wallet_importdescriptors.py", line 714, in run_test
    assert "scanning" not in wallet_info
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError
2025-02-16T13:00:09.381000Z TestFramework (INFO): Not stopping nodes as test failed. The dangling processes will be cleaned up later.
2025-02-16T13:00:09.381000Z TestFramework (WARNING): Not cleaning up dir /Users/runner/work/bitcoin/bitcoin/ci/scratch/test_runner/test_runner_₿_🏃_20250216_125556/wallet_importdescriptors_99
2025-02-16T13:00:09.381000Z TestFramework (ERROR): Test failed. Test logging available at /Users/runner/work/bitcoin/bitcoin/ci/scratch/test_runner/test_runner_₿_🏃_20250216_125556/wallet_importdescriptors_99/test_framework.log

hebasto added the label Wallet on Feb 16, 2025

hebasto added the label Tests on Feb 16, 2025

hebasto commented at 3:50 PM on February 17, 2025: member

Another one: https://github.com/bitcoin/bitcoin/actions/runs/13373718736/job/37348002275

maflcko added the label CI failed on Feb 17, 2025

maflcko added this to the milestone 29.0 on Feb 17, 2025

maflcko commented at 4:03 PM on February 17, 2025: member

I haven't tried, but #31768 (review) by @hodlinator may fix it

furszy commented at 4:27 PM on February 17, 2025: member

I haven't tried, but #31768 (comment) by @hodlinator may fix it

It fixes it. Can test it by adding a small sleep(3) before the getwalletinfo() call.

But... this test is not deterministic. If the scanning process runs slower, the "0" duration check will also fail. I'm not sure we should keep it.

hodlinator commented at 9:20 PM on February 17, 2025: contributor

But... this test is not deterministic. If the scanning process runs slower, the "0" duration check will also fail. I'm not sure we should keep it.

Yeah, it seems like an overly harsh check to me. Curious what was the impulse behind #31768 @brunoerg?

brunoerg commented at 9:29 PM on February 17, 2025: contributor

I haven't tried, but #31768 (review) by @hodlinator may fix it

Yes, it fixes it.

Yeah, it seems like an overly harsh check to me. Curious what was the impulse behind #31768 @brunoerg?

I've tried it on different setups and besides the duration being different as expected for every setup/run I tried I didn't get a case where it's 0. Anyway, I'm fine on removing it or addressing the @hodlinator's suggestion.

hodlinator commented at 9:37 PM on February 17, 2025: contributor

Is the added code protecting some other code? Or just documenting the interface of the RPC, perhaps based on surviving mutants (from mutation testing)? If it's something like the former, I think it should be fixed. If it's the latter, I would lean towards reverting unless people agree we should start testing every aspect of the RPC interface (gradually or in larger PR(s)).

brunoerg commented at 9:47 PM on February 17, 2025: contributor

I just opened #31893 to remove it. It's better to avoid any possibility of intermittent issue.

Is the added code protecting some other code? Or just documenting the interface of the RPC, perhaps based on surviving mutants (from mutation testing)? If it's something like the former, I think it should be fixed. If it's the latter, I would lean towards reverting unless people agree we should start testing every aspect of the RPC interface (gradually or in larger PR(s)).

It's just a test coverage addition but not based on surviving mutants since it does not even have test coverage.

hodlinator commented at 9:56 PM on February 17, 2025: contributor

It's just a test coverage addition but not based on surviving mutants since it does not even have test coverage.

Ah, forgot that mutants are focused on modifying test code, not implementation. Edit: <-- this was wrong, see reply below

brunoerg commented at 9:58 PM on February 17, 2025: contributor

Ah, forgot that mutants are focused on modifying test code, not implementation.

No, mutants are focused on mutating the implementation to verify the tests. However, if there is no test coverage for the implementation, it does not make sense to modify it, there is nothing to verify.

fanquake referenced this in commit 63d625f761 on Feb 18, 2025

hebasto commented at 3:24 PM on February 18, 2025: member

Resolved in #31893.

hebasto closed this on Feb 18, 2025