Skip to content

Conversation

Sjors
Copy link
Member

@Sjors Sjors commented Jul 9, 2025

Because stdout and stderr are consumed by the node, the mock external signers can't use them for logging.

Instead have them print directly into test_framework.log, which can then be be retrieved via combine_logs.py.

In preparation (and addtion) this PR starts with a few refactors:

  • move mock_signer_path helpers to util and reuse them between rpc_signer.py and wallet_signer.py
  • move the mocks from test/functional/mocks to test/functional: this is needed to avoid Python import hell in the next commits
  • have the mocks take advantage of the test_framework by extracting (and reusing) perform_pre_checks and mock_signer_psbt_path into util
  • extract the debug.log style Formatter into util so we can use it in the final commit

Originally this PR was intended to aid in debugging #32855, which has been solved since.

@DrahtBot DrahtBot added the Tests label Jul 9, 2025
@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 9, 2025

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/32928.

Reviews

See the guideline for information on the review process.

Type Reviewers
Concept ACK naiyoma

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:

  • #33230 (cli: Handle arguments that can be either JSON or string by achow101)
  • #33142 (test: Run bench sanity checks in parallel with functional tests by maflcko)

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.

@Sjors Sjors force-pushed the 2025/07/test-signer branch from 4f8075b to 3ff8e66 Compare July 9, 2025 14:51
@Sjors Sjors changed the title test: add logging to mock signers test: add logging to mock external signers Jul 9, 2025
@DrahtBot DrahtBot removed the CI failed label Jul 9, 2025
@Sammie05
Copy link

Sammie05 commented Jul 13, 2025

Adding mock_signer_log and logging “Started” / “Finished” makes debugging much clearer and great improvement, especially since stdout/stderr can’t be used here.
Thanks for cleaning this up and making it easier for future test debugging

Copy link
Contributor

@naiyoma naiyoma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK ->
I attempted to debug -> #32855 (comment) after pulling this branch, and so far I can see some useful logs in combined.log

mock_external_signer” logs from this diff

grep "mock_external_signer" combined.log
node1 2025-07-19T19:35:11.455260Z [init] [common/args.cpp:850] [logArgsPrefix] Command-line arg: signer="/usr/bin/python3 /home/ubuntu/Projects/bitcoin/test/functional/test_framework/../mock_external_signer.py"
test 2025-07-19T19:35:11.823136Z TestFramework (DEBUG): -signer=/usr/bin/python3 /home/ubuntu/Projects/bitcoin/test/functional/test_framework/../mock_external_signer.py
node1 2025-07-19T19:35:11.846121Z [httpworker.1] [common/run_command.cpp:29] [RunCommandParseJSON] RunCommandParseJSON: command='/usr/bin/python3 /home/ubuntu/Projects/bitcoin/test/functional/test_framework/../mock_external_signer.py enumerate', stdin_length=0, stdin_empty=true
test 2025-07-19T19:35:11.963244Z TestFramework.mock_external_signer (DEBUG): Started
test 2025-07-19T19:35:11.969415Z TestFramework.mock_external_signer (DEBUG): Finished
node1 2025-07-19T19:35:11.986061Z [httpworker.1] [common/run_command.cpp:29] [RunCommandParseJSON] RunCommandParseJSON: command='/usr/bin/python3 /home/ubuntu/Projects/bitcoin/test/functional/test_framework/../mock_external_signer.py --fingerprint 00000001 --chain regtest getdescriptors --account 0', stdin_length=0, stdin_empty=true
test 2025-07-19T19:35:12.078454Z TestFramework.mock_external_signer (DEBUG): Started
test 2025-07-19T19:35:12.084684Z TestFramework.mock_external_signer (DEBUG): Naiyoma HANGING: stdin hang for getdescriptors

- reuse perform_pre_checks helper
- add mock_psbt_path helper
Because stdout and stderr are consumed by the node, we can't use them for logging. Instead have the mocks print directly into test_framework.log, which can then be be retrieved via combine_logs.py.
@Sjors Sjors force-pushed the 2025/07/test-signer branch from 3ff8e66 to b1edd6d Compare September 1, 2025 07:10
@maflcko
Copy link
Member

maflcko commented Sep 1, 2025

This will hopefully aid in debugging #32855.

For reference, the issue was something else, and has been fixed now, in the meantime.

@Sjors
Copy link
Member Author

Sjors commented Sep 1, 2025

@maflcko updated the PR description. I assume more logging is still useful though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants