test: raise explicit error if any of the needed release binaries is missing #31462

theStack commented at 6:53 PM on December 10, 2024: contributor

If the releases directory exists, but still only a subset of the necessary previous release binaries are available, the test fails by throwing an exception (sometimes leading to follow-up exceptions like AssertionError: [node 0] Error: no RPC connection) and printing out a stack trace, which can be confusing and at a first glance suggests that the node crashed or some alike. Improve this by checking and printing out all of the missing release binaries and failing with an explicit error in this case. Also add an info on how to download previous releases binaries. Noticed while testing #30328.

Can be tested by e.g.

$ rm -rf ./releases
$ ./test/get_previous_releases.py -b
$ rm -rf ./releases/v28.0/
$ ./build/test/functional/wallet_migration.py

master:

<details> <summary>Long test fail output</summary>

...
2024-12-10T18:48:54.067000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/home/thestack/bitcoin/test/functional/test_framework/test_framework.py", line 590, in start_nodes
    node.start(extra_args[i], *args, **kwargs)
  File "/home/thestack/bitcoin/test/functional/test_framework/test_node.py", line 257, in start
    self.process = subprocess.Popen(self.args + extra_args, env=subp_env, stdout=stdout, stderr=stderr, cwd=cwd, **kwargs)
  File "/usr/lib/python3.10/subprocess.py", line 971, in __init__
    self._execute_child(args, executable, preexec_fn, close_fds,
  File "/usr/lib/python3.10/subprocess.py", line 1863, in _execute_child
    raise child_exception_type(errno_num, err_msg, err_filename)
FileNotFoundError: [Errno 2] No such file or directory: '/home/thestack/bitcoin/releases/v28.0/bin/bitcoind'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/thestack/bitcoin/test/functional/test_framework/test_framework.py", line 131, in main
    self.setup()
  File "/home/thestack/bitcoin/test/functional/test_framework/test_framework.py", line 315, in setup
    self.setup_network()
  File "/home/thestack/bitcoin/test/functional/test_framework/test_framework.py", line 409, in setup_network
    self.setup_nodes()
  File "/home/thestack/bitcoin/./build/test/functional/wallet_migration.py", line 54, in setup_nodes
    self.start_nodes()
  File "/home/thestack/bitcoin/test/functional/test_framework/test_framework.py", line 595, in start_nodes
    self.stop_nodes()
  File "/home/thestack/bitcoin/test/functional/test_framework/test_framework.py", line 610, in stop_nodes
    node.stop_node(wait=wait, wait_until_stopped=False)
  File "/home/thestack/bitcoin/test/functional/test_framework/test_node.py", line 396, in stop_node
    self.stop(wait=wait)
  File "/home/thestack/bitcoin/test/functional/test_framework/test_node.py", line 215, in __getattr__
    assert self.rpc_connected and self.rpc is not None, self._node_msg("Error: no RPC connection")
AssertionError: [node 0] Error: no RPC connection
2024-12-10T18:48:54.118000Z TestFramework (INFO): Stopping nodes
Traceback (most recent call last):
  File "/home/thestack/bitcoin/./build/test/functional/wallet_migration.py", line 1097, in <module>
    WalletMigrationTest(__file__).main()
  File "/home/thestack/bitcoin/test/functional/test_framework/test_framework.py", line 159, in main
    exit_code = self.shutdown()
  File "/home/thestack/bitcoin/test/functional/test_framework/test_framework.py", line 331, in shutdown
    self.stop_nodes()
  File "/home/thestack/bitcoin/test/functional/test_framework/test_framework.py", line 610, in stop_nodes
    node.stop_node(wait=wait, wait_until_stopped=False)
  File "/home/thestack/bitcoin/test/functional/test_framework/test_node.py", line 396, in stop_node
    self.stop(wait=wait)
  File "/home/thestack/bitcoin/test/functional/test_framework/test_node.py", line 215, in __getattr__
    assert self.rpc_connected and self.rpc is not None, self._node_msg("Error: no RPC connection")
AssertionError: [node 0] Error: no RPC connection
[node 0] Cleaning up leftover process
...

</details>

PR:

...
2025-01-01T20:26:27.999000Z TestFramework (INFO): PRNG seed is: 4570383538468804512
2025-01-01T20:26:28.000000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_lz66_zcu
2025-01-01T20:26:28.003000Z TestFramework (ERROR): Binary not found: /home/thestack/bitcoin/releases/v28.0/bin/bitcoind
2025-01-01T20:26:28.003000Z TestFramework (ERROR): Binary not found: /home/thestack/bitcoin/releases/v28.0/bin/bitcoin-cli
2025-01-01T20:26:28.003000Z TestFramework (INFO): Previous releases binaries can be downloaded via `test/get_previous_releases.py -b`.
2025-01-01T20:26:28.003000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/home/thestack/bitcoin/test/functional/test_framework/test_framework.py", line 131, in main
    self.setup()
  File "/home/thestack/bitcoin/test/functional/test_framework/test_framework.py", line 315, in setup
    self.setup_network()
  File "/home/thestack/bitcoin/test/functional/test_framework/test_framework.py", line 409, in setup_network
    self.setup_nodes()
  File "/home/thestack/bitcoin/./build/test/functional/wallet_migration.py", line 50, in setup_nodes
    self.add_nodes(self.num_nodes, versions=[
  File "/home/thestack/bitcoin/test/functional/test_framework/test_framework.py", line 537, in add_nodes
    raise AssertionError("At least one release binary is missing")
AssertionError: At least one release binary is missing
2025-01-01T20:26:28.061000Z TestFramework (INFO): Stopping nodes
...

DrahtBot commented at 6:53 PM on December 10, 2024: 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/31462.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	fjahr, kevkevinpal, maflcko, pablomartin4btc, achow101

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#31375 (multiprocess: Add bitcoin wrapper executable by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Tests on Dec 10, 2024

maflcko commented at 7:12 PM on December 10, 2024: member

Not sure about the implementation. This will turn an error (a missing previous release bin) into a silently passing CI.

FileNotFoundError: [Errno 2] No such file or directory: '/home/thestack/bitcoin/releases/v28.0/bin/bitcoind'

Seems pretty self-explanatory and doesn't need more code to write another error?

My understanding from reading the code is that the Error: no RPC connection shouldn't happen on stop. Since stop checks for self.running, and self.running = True is only set after Popen, which throws for you.

If this isn't the case, it would be better to fix that bug.

kevkevinpal commented at 7:38 PM on December 10, 2024: contributor

I agree that at first glance the log might be confusing but I agree with @maflcko that we should not be silently passing the ci

I would prefer we raise an AssertionError instead, so we keep the same functionality of the error but a better output

Output with AssertionError

2024-12-10T19:35:01.177000Z TestFramework (INFO): PRNG seed is: 5297365276077502746
2024-12-10T19:35:01.178000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_thepg93c
2024-12-10T19:35:01.178000Z TestFramework (ERROR): Binary not found: /mnt/shared_drive/DEVDIR/bitcoin/releases/v28.0/bin/bitcoind
2024-12-10T19:35:01.178000Z TestFramework (ERROR): Binary not found: /mnt/shared_drive/DEVDIR/bitcoin/releases/v28.0/bin/bitcoin-cli
2024-12-10T19:35:01.178000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/mnt/shared_drive/DEVDIR/bitcoin/test/functional/test_framework/test_framework.py", line 131, in main
    self.setup()
  File "/mnt/shared_drive/DEVDIR/bitcoin/test/functional/test_framework/test_framework.py", line 315, in setup
    self.setup_network()
  File "/mnt/shared_drive/DEVDIR/bitcoin/test/functional/test_framework/test_framework.py", line 409, in setup_network
    self.setup_nodes()
  File "/mnt/shared_drive/DEVDIR/bitcoin/./build/test/functional/wallet_migration.py", line 50, in setup_nodes
    self.add_nodes(self.num_nodes, versions=[
  File "/mnt/shared_drive/DEVDIR/bitcoin/test/functional/test_framework/test_framework.py", line 536, in add_nodes
    raise AssertionError("At least one release binary is missing")
AssertionError: At least one release binary is missing
2024-12-10T19:35:01.229000Z TestFramework (INFO): Stopping nodes
2024-12-10T19:35:01.229000Z TestFramework (WARNING): Not cleaning up dir /tmp/bitcoin_func_test_thepg93c
2024-12-10T19:35:01.229000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/bitcoin_func_test_thepg93c/test_framework.log
2024-12-10T19:35:01.229000Z TestFramework (ERROR):
2024-12-10T19:35:01.230000Z TestFramework (ERROR): Hint: Call /mnt/shared_drive/DEVDIR/bitcoin/test/functional/combine_logs.py '/tmp/bitcoin_func_test_thepg93c' to consolidate all logs
2024-12-10T19:35:01.230000Z TestFramework (ERROR):
2024-12-10T19:35:01.230000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log.
2024-12-10T19:35:01.230000Z TestFramework (ERROR): https://github.com/bitcoin/bitcoin/issues
2024-12-10T19:35:01.230000Z TestFramework (ERROR):

nit: in the PR description to reproduce, I get No such file or directory ./test/get_previous_releases -b -> ./test/get_previous_releases.py -b

fjahr commented at 3:54 PM on December 30, 2024: contributor

I agree that the output isn't ideal, I always forget what to run to get the missing binaries when I run into this. But I also think the test should still fail.

theStack force-pushed on Jan 1, 2025

theStack renamed this:
~~test: skip test if any of the needed release binaries is missing~~
test: raise explicit error if any of the needed release binaries is missing
on Jan 1, 2025

theStack commented at 8:33 PM on January 1, 2025: contributor

Thanks for the reviews! I agree that skipping the test is a bad idea, switched to to an explicit error message instead as suggested, also added a log.info with instructions on how to obtain the binaries for previous releases in this case.

theStack force-pushed on Jan 1, 2025

DrahtBot added the label CI failed on Jan 1, 2025

in test/functional/test_framework/test_framework.py:529 in e8795f4c11 outdated

 525 | @@ -526,6 +526,15 @@ def get_bin_from_version(version, bin_name, bin_default):
 526 |              binary = [get_bin_from_version(v, 'bitcoind', self.options.bitcoind) for v in versions]
 527 |          if binary_cli is None:
 528 |              binary_cli = [get_bin_from_version(v, 'bitcoin-cli', self.options.bitcoincli) for v in versions]
 529 | +        # Skip test if any of the needed release binaries is missing

fjahr commented at 9:24 PM on January 1, 2025:

        # Fail test if any of the needed release binaries is missing

theStack commented at 12:26 AM on January 7, 2025:

Fixed.

DrahtBot removed the label CI failed on Jan 1, 2025

in test/functional/test_framework/test_framework.py:537 in e8795f4c11 outdated

 532 | +            if shutil.which(bin_path) is None:
 533 | +                self.log.error(f"Binary not found: {bin_path}")
 534 | +                bins_missing = True
 535 | +        if bins_missing:
 536 | +            self.log.info("Previous releases binaries can be downloaded via `test/get_previous_releases.py -b`.")
 537 | +            raise AssertionError("At least one release binary is missing")

fjahr commented at 10:50 AM on January 2, 2025:

nit: Could also been put into the assert message IMO

            raise AssertionError("At least one release binary is missing. Previous releases binaries can be downloaded via `test/get_previous_releases.py -b`.")

furszy commented at 6:56 PM on January 6, 2025:

+1. Something like this would be helpful. I've received a few messages sharing these logs without knowing what to do next.

theStack commented at 12:27 AM on January 7, 2025:

Thanks, moved the download instructions directly into the assertion message as suggested.

pablomartin4btc commented at 6:31 PM on January 8, 2025:

+1. Something like this would be helpful. I've received a few messages sharing these logs without knowing what to do next.

I was one of them :raising_hand:, I stepped upon this issue while testing #31451 and I found this answer from @achow101 in stack-overflow before checking /test/README.md.

I'd put a reference to the README file in the error description instead of the .py and I'd clarify this issue there in the readme (if you agree with it but you don't want to update the PR I can create a new one). Also, because I didn't know about the -b [option], the script started to compile all versions specified in SHA256_SUMS (perhaps we should update the script and remove the old ones as they don't compile... or even switch the defautl to -b and create another option in order to compile the missing version?).

Last thing is that "At least one release" is not entirely correct, we need the last previous to the current, if you delete the v.28.0 from the releases folder (as specified in the description of the PR) and you still have v25.0 and/ or v24.0.1, you will get the same error (I think it's how add_nodes() combined with get_bin_from_version() works from the test_framework works).

theStack commented at 10:07 AM on January 9, 2025:

Will leave the PR as-is, but happy to review a follow-up PR which refines test/README.md about previous releases and/or adds improvements to the previous release download script itself.

Last thing is that "At least one release" is not entirely correct, we need the last previous to the current, if you delete the v.28.0 from the releases folder (as specified in the #31462#issue-2730936530 of the PR) and you still have v25.0 and/ or v24.0.1, you will get the same error (I think it's how add_nodes() combined with get_bin_from_version() works from the test_framework works).

Note that the concrete list of needed releases is individual for each functional test (passed as versions parameter to the add_nodes call), wallet_migration.py needing v28.0 is just one example. Stating "At least one needed release binary is missing" might be slightly more accurate, will adapt if I have to retouch.

pablomartin4btc commented at 2:16 PM on January 9, 2025:

Oh, I see... yeah, ok... in the case of WalletMigrationTest I can see the required versions defined in the setup. Thank you!

fjahr commented at 11:23 AM on January 2, 2025: contributor

tACK e8795f4c11622c26cf303b759ba717351a8fba45

Would be good to fix the comment but it's not a big deal.

Here is the output I see, I think it would great if the backtrace could be suppressed in this case because it's mostly noise distracting from the helpful logs. But it might be too much hassle, so not a must.

<details> <summary>Output</summary>

2025-01-01T21:24:42.885000Z TestFramework (INFO): Initializing test directory /var/folders/9z/n7rz_6cj3bq__11k5kcrsvvm0000gn/T/bitcoin_func_test_n24yl9c0
2025-01-01T21:24:42.886000Z TestFramework (ERROR): Binary not found: /Users/XXX/projects/clones/bitcoin/releases/v28.0/bin/bitcoind
2025-01-01T21:24:42.886000Z TestFramework (ERROR): Binary not found: /Users/XX/projects/clones/bitcoin/releases/v28.0/bin/bitcoin-cli
2025-01-01T21:24:42.886000Z TestFramework (INFO): Previous releases binaries can be downloaded via `test/get_previous_releases.py -b`.
2025-01-01T21:24:42.886000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/Users/XXX/projects/clones/bitcoin/test/functional/test_framework/test_framework.py", line 131, in main
    self.setup()
    ~~~~~~~~~~^^
  File "/Users/XXX/projects/clones/bitcoin/test/functional/test_framework/test_framework.py", line 315, in setup
    self.setup_network()
    ~~~~~~~~~~~~~~~~~~^^
  File "/Users/XXX/projects/clones/bitcoin/test/functional/test_framework/test_framework.py", line 409, in setup_network
    self.setup_nodes()
    ~~~~~~~~~~~~~~~~^^
  File "/Users/XXX/projects/clones/bitcoin/build/test/functional/wallet_migration.py", line 50, in setup_nodes
    self.add_nodes(self.num_nodes, versions=[
    ~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^
        None,
        ^^^^^
        280000,
        ^^^^^^^
    ])
    ^^
  File "/Users/XXX/projects/clones/bitcoin/test/functional/test_framework/test_framework.py", line 537, in add_nodes
    raise AssertionError("At least one release binary is missing")
AssertionError: At least one release binary is missing
2025-01-01T21:24:42.942000Z TestFramework (INFO): Stopping nodes

</details>

test: raise explicit error if any of the needed release binaries is missing

If the `releases` directory exists, but still only a subset of the
necessary previous release binaries are available, the test fails by
throwing an exception (sometimes leading to follow-up exceptions like
"AssertionError: [node 0] Error: no RPC connection") and printing out
a stack trace, which can be confusing and at a first glance suggests
that the node crashed or some alike.
Improve this by checking and printing out *all* of the missing release
binaries and failing with an explicit error in this case. Also add an
info on how to download previous releases binaries.
Noticed while testing #30328.

Can be tested by e.g.

$ ./test/get_previous_releases.py -b
$ rm -rf ./releases/v28.0/
$ ./build/test/functional/wallet_migration.py

1ea7e45a1f

theStack force-pushed on Jan 7, 2025

fjahr commented at 8:53 PM on January 7, 2025: contributor

re-ACK 1ea7e45a1f445d32a2b690d52befb2e63418653b

kevkevinpal commented at 9:22 PM on January 7, 2025: contributor

ACK 1ea7e45

maflcko commented at 10:40 AM on January 8, 2025: member

I've created #31620 to reduce the long, redundant and confusing error message observed originally a bit.

But the two changes are unrelated otherwise.

lgtm ACK 1ea7e45a1f445d32a2b690d52befb2e63418653b

pablomartin4btc commented at 6:31 PM on January 8, 2025: member

Concept ACK.

Left a few suggestions but other than that I'm happy with this step forward.

pablomartin4btc commented at 2:17 PM on January 9, 2025: member

tACK 1ea7e45a1f445d32a2b690d52befb2e63418653b

achow101 commented at 11:21 PM on January 9, 2025: member

ACK 1ea7e45a1f445d32a2b690d52befb2e63418653b

achow101 merged this on Jan 9, 2025

achow101 closed this on Jan 9, 2025

theStack deleted the branch on Jan 10, 2025

sedited referenced this in commit 230a439a4a on Jan 17, 2025

stickies-v referenced this in commit d760fd3dda on Mar 17, 2025

stickies-v referenced this in commit cc83553352 on Mar 17, 2025

stickies-v referenced this in commit 2614933f06 on Mar 17, 2025

stickies-v referenced this in commit b70418c5fc on Mar 17, 2025

stickies-v referenced this in commit 69f8a1fe50 on Mar 17, 2025

bug-castercv502 referenced this in commit 44b075fe71 on Sep 28, 2025

bitcoin locked this on Jan 10, 2026