test: fix `--perf` profiling #35509

Functional test profiling with --perf (introduced in #14519) is currently broken, as TestNode._start_perf accesses the self.binary attribute which doesn't exist anymore since commit 0d2eefca8bf3cb64e3f2f912ed32118524430967 / PR #31866. Fix that by using the proper field self.binaries.paths.bitcoind.

Also, perf's error output for missing kernel permissions changed slightly in 2020 (s/tweaking/adjusting) (see https://github.com/torvalds/linux/commit/c1034eb069201f3f3c40f34f3d937ecb8049d0cf), so adapt our matching string as well in order to show the "couldn't collect data" warning if necessary. Relying on a certain output string is obviously brittle, so we might want to do something smarter there.

This is a minimum-diff-fix that works on my machine (arm64, Ubuntu 25.04, Kernel 6.17.0-8-qcom-x1e, perf 6.1.174). I think the feature is useful in principle, but considering that no one noticed for more than a year that it's broken, it seems that no one really uses it, and an alternative might also be to remove it. Curious to hear opinions.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--006a51241073e994b41acfe9ec718e94-->

Code Coverage & Benchmarks

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

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #35528 (test: doc: remove --perf profiling from functional test framework by theStack)

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.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

Concept ACK

Yeah, I think it can be removed (or moved to be doc-only), as it requires a manual diff anyway to make use of it, so that manual diff might as well just be a dedicated new functional test file with the test code, and a diff to test_node.py to force-inject perf.

The hints about compiling with debug symbols, the hint about dwarf/frame-pointers and the hint about perf_event_paranoid can be added to the docs instead.

Let's recall that we already have a micro-benchmark suite, so this is only meant for end-to-end performance testing that needs and expensive setup (like a specific blockchain, or txs generated with test utils). For end-to-end perf testing that only needs a trivial setup (like calling the echo RPC), a vanilla bash/Python script may be easier. One could even argue that one could use https://github.com/jamesob/verystable (or similar) in a vanilla script for more expensive setups, but I haven't tried this and I don't want to trail off too far.

@maflcko: Thanks for weighing in. I'm not opposed to removing it at all, but can you elaborate more on the "needing a manual diff" part? Is that referring to the usage with the profile_with_perf context manager? At least on my end when I invoke any functional test with the --perf flag, it seems to work out of the box with this PR (well, after installing perf and giving the kernel the needed permissions).

I'll keep the PR open for a few more days in any case, in the (seemingly unlikely?) case that anyone is strongly in favor of keeping it.

Is that referring to the usage with the profile_with_perf context manager? At least on my end when I invoke any functional test with the --perf flag, it seems to work out of the box with this PR

Ah, thx for correcting me. I've never used this feature and forgot.

Opened #35528 as an alternative PR to remove it. @w0xlt: I interpreted your "Concept ACK" as "makes sense to fix something broken" rather than a strong compelling reason to keep it, feel free to add more context in case you are e.g. using that feature regularly (or could see it being valuable).

Closing, since #35528 was merged.