This adds support to the functional test framework to run the multiprocess bitcoin-node binary, and then tests it in a new interface_ipc.py functional test through the pycapnp module.
Add functional test for IPC interface #33201
pull sipa wants to merge 6 commits into bitcoin:master from sipa:202508_ipc_test changing 14 files +290 −20-
sipa commented at 8:10 PM on August 15, 2025: member
-
DrahtBot commented at 8:10 PM on August 15, 2025: contributor
<!--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/33201.
<!--021abf342d371248e50ceaed478a90ca-->
Reviews
See the guideline for information on the review process.
Type Reviewers ACK ryanofsky, TheCharlatan, Sjors Stale ACK josibake, janb84, ismaelsadeeq If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
<!--174a7506f384e20aa4161008e828411d-->
Conflicts
Reviewers, this pull request conflicts with the following ones:
- #33142 (test: Run bench sanity checks in parallel with functional tests by maflcko)
- #32297 (bitcoin-cli: Add -ipcconnect option by ryanofsky)
- #31723 (qa debug: Add --debug_runs/-waitfordebugger [DRAFT] by hodlinator)
- #31349 (ci: detect outbound internet traffic generated while running tests by vasild)
- #30437 (ipc: add bitcoin-mine test program by ryanofsky)
- #29838 (Feature: Use different datadirs for different signets by BrandonOdiwuor)
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-->
- sipa force-pushed on Aug 15, 2025
- DrahtBot added the label CI failed on Aug 15, 2025
-
DrahtBot commented at 8:14 PM on August 15, 2025: contributor
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed. <sub>Task
lint: https://github.com/bitcoin/bitcoin/runs/48194727681</sub> <sub>LLM reason (✨ experimental): The CI failure is caused by errors detected during the linting process, specifically fromruffand mypy, indicating code issues and missing module imports.</sub><details><summary>Hints</summary>
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
</details>
- sipa force-pushed on Aug 15, 2025
-
ryanofsky commented at 8:46 PM on August 15, 2025: contributor
Concept ACK. 27bd67857dd0e145d7539b949df2b7aaa53c6b1b is really nice and simpler than I would have expected.
In case it's useful #30437 adds an interface_ipc_mining.py test, and #32297 adds an interface_ipc_cli.py test which are similar to this test and have similar test framework updates. Code from those PRs might be useful here or vice versa.
- sipa force-pushed on Aug 15, 2025
- sipa force-pushed on Aug 15, 2025
- sipa force-pushed on Aug 15, 2025
- sipa force-pushed on Aug 16, 2025
-
Sjors commented at 7:06 AM on August 16, 2025: member
Concept ACK
So far I achieved test coverage for the Mining interface with a combination of unit tests and by having RPC methods such as
getblocktemplateuse the interface internally. But the latter has side-effects (see #32547) and could be avoided once we have enough direct coverage.CentOS CI fails with
capnp.lib.capnp.KjException: mp/proxy.capnp:6: failed: Import failed: /capnp/c++.capnhttps://cirrus-ci.com/task/4824947408764928?logs=ci#L3320 -
in test/functional/interface_ipc.py:127 in b87de46c7a outdated
69 | + mining = init.makeMining(ctx) 70 | + opts = modules['mining'].BlockCreateOptions() 71 | + opts.useMempool = True 72 | + opts.blockReservedWeight = 4000 73 | + opts.coinbaseOutputMaxAdditionalSigops = 0 74 | + template = mining.result.createNewBlock(opts)
Sjors commented at 7:09 AM on August 16, 2025:It would be great to cover the
waitNext()method ontemplate.
sipa commented at 1:31 PM on August 17, 2025:Done.
sipa force-pushed on Aug 16, 2025sipa force-pushed on Aug 16, 2025sipa force-pushed on Aug 16, 2025DrahtBot removed the label CI failed on Aug 16, 2025sipa force-pushed on Aug 16, 2025sipa force-pushed on Aug 16, 2025DrahtBot added the label CI failed on Aug 16, 2025sipa force-pushed on Aug 17, 2025sipa force-pushed on Aug 17, 2025DrahtBot removed the label CI failed on Aug 17, 2025in ci/test/01_base_install.sh:55 in eb482c1f34 outdated
51 | @@ -52,7 +52,7 @@ fi 52 | 53 | if [ -n "$PIP_PACKAGES" ]; then 54 | # shellcheck disable=SC2086 55 | - ${CI_RETRY_EXE} pip3 install --user $PIP_PACKAGES 56 | + ${CI_RETRY_EXE} pip3 install --break-system-packages --user $PIP_PACKAGES
sipa commented at 11:38 AM on August 17, 2025:@Sjors I'll expand the functional tests with some more methods now that CI works. It took many tries to find something that works on a reasonable number of platforms.
I have tried
pip3 install pycapnpon macOS CI, but that gives a strange error that I also got when running locally. Locally, I fixed that by installing pycapnp from source, but in macOS CI, that resulted in C++ errors about Python functions that were not found. At this point, I'm going to give up on getting it to work there; someone else can work on adding that.Regarding you running locally, where is your
c++.capnpinstalled? Theinterface_i2p.pyscript currently just assumes it's in/usr/include/capnp/c++.capnp, but that will not work for custom installations.
Sjors commented at 1:05 PM on August 17, 2025:where is your c++.capnp installed?
/opt/homebrew/bin/capnpc-c++This makes the test pass:
diff --git a/test/functional/interface_ipc.py b/test/functional/interface_ipc.py index 759fbbbc74..6f34a0dca6 100755 --- a/test/functional/interface_ipc.py +++ b/test/functional/interface_ipc.py @@ -33,7 +33,10 @@ class IPCInterfaceTest(BitcoinTestFramework): def capnp_modules(self): src_dir = Path(self.config['environment']['SRCDIR']) / "src" mp_dir = src_dir / "ipc" / "libmultiprocess" / "include" - imports = ["/usr/include", str(src_dir), str(mp_dir)] + # c++.capnp path: + # https://github.com/capnproto/pycapnp/issues/279#issuecomment-2477709244 + cpp_capnp_dir = Path(capnp.__path__[0]).parent + imports = ["/usr/include", str(src_dir), str(mp_dir), str(cpp_capnp_dir)] return { "proxy": capnp.load(str(mp_dir / "mp" / "proxy.capnp"), imports=imports), "init": capnp.load(str(src_dir / "ipc" / "capnp" / "init.capnp"), imports=imports),
sipa commented at 1:31 PM on August 17, 2025:Done.
ryanofsky commented at 5:51 PM on September 3, 2025:In commit "ci: enable IPC tests in CI" (7eb71775dce6f3f2fd0e4b62f439aab86d1b65a0)
Would seem good to drop --break-system-packages here and only add it on the platforms where it's needed following:
It's also not clear what the problem is if --break-system-packages is not used. Does the package fail to install otherwise?
sipa commented at 6:34 PM on September 3, 2025:I've removed it; let's see where CI breaks.
ryanofsky commented at 7:28 PM on September 3, 2025:re: #33201 (review)
I've removed it; let's see where CI breaks.
Thanks! For reference, the error on ubuntu seems to be:
+ retry -- pip3 install --user pycapnp error: externally-managed-environment × This environment is externally managed ╰─> To install Python packages system-wide, try apt install python3-xyz, where xyz is the package you are trying to install. If you wish to install a non-Debian-packaged Python package, create a virtual environment using python3 -m venv path/to/venv. Then use path/to/venv/bin/python and path/to/venv/bin/pip. Make sure you have python3-full installed. If you wish to install a non-Debian packaged Python application, it may be easiest to use pipx install xyz, which will manage a virtual environment for you. Make sure you have pipx installed. See /usr/share/doc/python3.12/README.venv for more information. note: If you believe this is a mistake, please contact your Python installation or OS distribution provider. You can override this, at the risk of breaking your Python installation or OS, by passing --break-system-packages. hint: See PEP 668 for the detailed specification.
sipa commented at 7:43 PM on September 3, 2025:Ok, added
--break-system-packagesback to the 4 CI jobs that failed as a result of removing it globally.Sjors commented at 7:08 AM on August 17, 2025: memberOn macOS 15.6 with capnp 1.2.0 installed via Homebrew and Python 3.10.14 installed via PyEnv I get:
pip3 install pycapnp ... Successfully installed pycapnp-2.0.0 cmake -B build cmake --build build ... % build/test/functional/interface_ipc.py 2025-08-17T07:05:49.321522Z TestFramework (INFO): PRNG seed is: 6039316184654231320 2025-08-17T07:05:49.322566Z TestFramework (INFO): Initializing test directory /var/folders/8p/qqdl2fsj7rd6q096jtmsfydc0000gn/T/bitcoin_func_test_z3jtr5u1 2025-08-17T07:05:50.595292Z TestFramework (ERROR): Unexpected exception Traceback (most recent call last): File "/Users/sjors/dev/bitcoin/test/functional/test_framework/test_framework.py", line 200, in main self.run_test() File "/Users/sjors/dev/bitcoin/build/test/functional/interface_ipc.py", line 82, in run_test self.run_echo_test() File "/Users/sjors/dev/bitcoin/build/test/functional/interface_ipc.py", line 63, in run_echo_test asyncio.run(capnp.run(async_routine())) File "/Users/sjors/.pyenv/versions/3.10.14/lib/python3.10/asyncio/runners.py", line 44, in run return loop.run_until_complete(main) File "/Users/sjors/.pyenv/versions/3.10.14/lib/python3.10/asyncio/base_events.py", line 649, in run_until_complete return future.result() File "capnp/lib/capnp.pyx", line 1944, in run File "capnp/lib/capnp.pyx", line 1945, in capnp.lib.capnp.run File "/Users/sjors/dev/bitcoin/build/test/functional/interface_ipc.py", line 57, in async_routine init, ctx = await self.make_capnp_init_ctx() File "/Users/sjors/dev/bitcoin/build/test/functional/interface_ipc.py", line 45, in make_capnp_init_ctx modules = self.capnp_modules() File "/Users/sjors/dev/bitcoin/build/test/functional/interface_ipc.py", line 38, in capnp_modules "proxy": capnp.load(str(mp_dir / "mp" / "proxy.capnp"), imports=imports), File "capnp/lib/capnp.pyx", line 4385, in capnp.lib.capnp.load File "capnp/lib/capnp.pyx", line 3574, in capnp.lib.capnp.SchemaParser.load capnp.lib.capnp.KjException: mp/proxy.capnp:6: failed: Import failed: /capnp/c++.capnpMaybe related: https://github.com/capnproto/pycapnp/issues/377 (though
pycapnp-1.3.0doesn't work at all, since it doesn't have therunmethod)Suggested test doc improvement:
diff --git a/test/README.md b/test/README.md index 37f2c07217..3b2979cdf6 100644 --- a/test/README.md +++ b/test/README.md @@ -34,6 +34,9 @@ The ZMQ functional test requires a python ZMQ library. To install it: - on Unix, run `sudo apt-get install python3-zmq` - on mac OS, run `pip3 install pyzmq` +The IPC functional test requires a python IPC library. To install it: + +- `pip3 install pycapnp` On Windows the `PYTHONUTF8` environment variable must be set to 1:sipa force-pushed on Aug 17, 2025janb84 commented at 3:30 PM on August 17, 2025: contributorOn macOS 15.6 with capnp 1.2.0 installed via Homebrew and Python 3.10.14 installed via PyEnv I get:
% build/test/functional/interface_ipc.py 2025-08-17T07:05:49.321522Z TestFramework (INFO): PRNG seed is: 6039316184654231320 2025-08-17T07:05:49.322566Z TestFramework (INFO): Initializing test directoryMaybe related: capnproto/pycapnp#377 (though
pycapnp-1.3.0doesn't work at all, since it doesn't have therunmethod)Experiencing related errors in a nix-shell on macos (the change as suggested by Sjors did not fix it)
build/test/functional/test_runner.py -j8 interface_ipc.py Temporary test directory at /private/tmp/nix-shell-46943-2562808985/test_runner_₿_🏃_20250817_171423 Remaining jobs: [interface_ipc.py] 1/1 - interface_ipc.py failed, Duration: 1 s stdout: 2025-08-17T15:14:23.339367Z TestFramework (INFO): PRNG seed is: 8709597786033281775 2025-08-17T15:14:23.339826Z TestFramework (INFO): Initializing test directory /private/tmp/nix-shell-46943-2562808985/test_runner_₿_🏃_20250817_171423/interface_ipc_0 2025-08-17T15:14:24.140276Z TestFramework (INFO): Running echo test stderr: libc++abi: terminating due to uncaught exception of type kj::ExceptionImpl: mp/proxy.capnp:6: failed: Import failed: /capnp/c++.capnp TEST | STATUS | DURATION interface_ipc.py | ✖ Failed | 1 slocation of
c++.capnp:/nix/store/qfb31v9la93rp92krx05ndzm5nidsrzd-capnproto-1.2.0/include/capnpjanb84 commented at 3:37 PM on August 17, 2025: contributor@janb84 Yeah, the include paths used in the python test are a guess. It'd be nice to pass through the location of the capnp installation from the build system to the tests, but I don't know how.
I'm also looking into it to get it to work, just a heads-up that there is (possibly) some trouble on nixos/ nix-shells
in test/functional/interface_ipc.py:48 in 3f7ff4849e outdated
43 | + "echo": capnp.load(str(src_dir / "ipc" / "capnp" / "echo.capnp"), imports=imports), 44 | + "mining": capnp.load(str(src_dir / "ipc" / "capnp" / "mining.capnp"), imports=imports), 45 | + } 46 | + 47 | + async def make_capnp_init_ctx(self): 48 | + modules = self.capnp_modules()
ismaelsadeeq commented at 3:24 PM on August 18, 2025:In "ci: add functional test for IPC interface" 3f7ff4849e6165a6bcc4d5977c35a467fd7fc232
Should modules be a test parameter initialised in
set_test_paramsso that we can later reuse in mining test?
sipa commented at 5:17 PM on August 18, 2025:Good idea, done.
in test/functional/interface_ipc.py:26 in 3f7ff4849e outdated
34 | + @functools.cache 35 | + def capnp_modules(self): 36 | + cpp_capnp_dir = Path(capnp.__path__[0]).parent 37 | + src_dir = Path(self.config['environment']['SRCDIR']) / "src" 38 | + mp_dir = src_dir / "ipc" / "libmultiprocess" / "include" 39 | + imports = ["/usr/include", str(cpp_capnp_dir), str(src_dir), str(mp_dir)]
ismaelsadeeq commented at 3:30 PM on August 18, 2025:In "ci: add functional test for IPC interface" https://github.com/bitcoin/bitcoin/commit/3f7ff4849e6165a6bcc4d5977c35a467fd7fc232
This currently does not work on macOs when the pycapnp uses system installed capnproto installed via homebrew.
Editing it to works for me.
imports = ["/opt/homebrew/opt/capnp/include", str(src_dir), str(mp_dir)]I thought of two ways to fix it but was not successful.
- Whether we can get the path to the include directly from
pycapnp - Search for the location of
capnprotoinstallation for supported platforms and add it.
josibake commented at 3:57 PM on August 18, 2025:FWIW, this worked for me on macos: #33201 (review). I'd recommend we have the docs tell users to install
pycapnpfrom source, since that should "just work" everywhere due to this line:cpp_capnp_dir = Path(capnp.__path__[0]).parent.Eventually, we can replace this recommendation once we have CMake automatically finding the include directory for capnproto and making the test framework aware.
ismaelsadeeq commented at 4:09 PM on August 18, 2025:@josibake I did that initially but was having some errors
<details> <summary>logs </summary>
(.venv) abubakarismail@Abubakars-MacBook-Pro pycapnp % cmake --version cmake version 4.1.0 CMake suite maintained and supported by Kitware (kitware.com/cmake). (.venv) abubakarismail@Abubakars-MacBook-Pro pycapnp % pip install . -C force-bundled-libcapnp=True Processing /Users/abubakarismail/Desktop/Work/bitcoin-dev/py-bitcoin-ipc-client/pycapnp Installing build dependencies ... done Getting requirements to build wheel ... done Preparing metadata (pyproject.toml) ... done Building wheels for collected packages: pycapnp Building wheel for pycapnp (pyproject.toml) ... error error: subprocess-exited-with-error × Building wheel for pycapnp (pyproject.toml) did not run successfully. │ exit code: 1 ╰─> [89 lines of output] running build_ext CMake Deprecation Warning at CMakeLists.txt:1 (cmake_minimum_required): Compatibility with CMake < 3.10 will be removed from a future version of CMake. Update the VERSION argument <min> value. Or, use the <min>...<max> syntax to tell CMake that the project requires at least <min> but has been updated to work with policies introduced by <max> or earlier. CMake Error at CMakeLists.txt:2 (project): Running '/opt/homebrew/bin/ninja' '--version' failed with: Traceback (most recent call last): File "/opt/homebrew/bin/ninja", line 5, in <module> from ninja import ninja ModuleNotFoundError: No module named 'ninja' CMake Error: CMAKE_CXX_COMPILER not set, after EnableLanguage -- Configuring incomplete, errors occurred! *WARNING* no libcapnp detected or rebuild forced. Attempting to build it from source now. If you have C++ Cap'n Proto installed, it may be out of date or is not being detected. This may take a while... already have /Users/abubakarismail/Desktop/Work/bitcoin-dev/py-bitcoin-ipc-client/pycapnp/bundled/capnproto-c++ Traceback (most recent call last): File "/Users/abubakarismail/Desktop/Work/bitcoin-dev/py-bitcoin-ipc-client/.venv/lib/python3.13/site-packages/pip/_vendor/pyproject_hooks/_in_process/_in_process.py", line 389, in <module> main() ~~~~^^ File "/Users/abubakarismail/Desktop/Work/bitcoin-dev/py-bitcoin-ipc-client/.venv/lib/python3.13/site-packages/pip/_vendor/pyproject_hooks/_in_process/_in_process.py", line 373, in main json_out["return_val"] = hook(**hook_input["kwargs"]) ~~~~^^^^^^^^^^^^^^^^^^^^^^^^ File "/Users/abubakarismail/Desktop/Work/bitcoin-dev/py-bitcoin-ipc-client/.venv/lib/python3.13/site-packages/pip/_vendor/pyproject_hooks/_in_process/_in_process.py", line 280, in build_wheel return _build_backend().build_wheel( ~~~~~~~~~~~~~~~~~~~~~~~~~~~~^ wheel_directory, config_settings, metadata_directory ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ) ^ File "/Users/abubakarismail/Desktop/Work/bitcoin-dev/py-bitcoin-ipc-client/pycapnp/_custom_build/backend.py", line 28, in build_wheel return super().build_wheel(wheel_directory, config_settings, metadata_directory) ~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/private/var/folders/dj/d8p8jhd172n7wnq81ryfl6rc0000gn/T/pip-build-env-xr1q9l8l/overlay/lib/python3.13/site-packages/setuptools/build_meta.py", line 435, in build_wheel return _build(['bdist_wheel', '--dist-info-dir', str(metadata_directory)]) File "/private/var/folders/dj/d8p8jhd172n7wnq81ryfl6rc0000gn/T/pip-build-env-xr1q9l8l/overlay/lib/python3.13/site-packages/setuptools/build_meta.py", line 423, in _build return self._build_with_temp_dir( ~~~~~~~~~~~~~~~~~~~~~~~~~^ cmd, ^^^^ ...<3 lines>... self._arbitrary_args(config_settings), ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ) ^ File "/private/var/folders/dj/d8p8jhd172n7wnq81ryfl6rc0000gn/T/pip-build-env-xr1q9l8l/overlay/lib/python3.13/site-packages/setuptools/build_meta.py", line 404, in _build_with_temp_dir self.run_setup() ~~~~~~~~~~~~~~^^ File "/Users/abubakarismail/Desktop/Work/bitcoin-dev/py-bitcoin-ipc-client/pycapnp/_custom_build/backend.py", line 22, in run_setup return super().run_setup(setup_script) ~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^ File "/private/var/folders/dj/d8p8jhd172n7wnq81ryfl6rc0000gn/T/pip-build-env-xr1q9l8l/overlay/lib/python3.13/site-packages/setuptools/build_meta.py", line 317, in run_setup exec(code, locals()) ~~~~^^^^^^^^^^^^^^^^ File "<string>", line 212, in <module> File "/private/var/folders/dj/d8p8jhd172n7wnq81ryfl6rc0000gn/T/pip-build-env-xr1q9l8l/overlay/lib/python3.13/site-packages/setuptools/__init__.py", line 115, in setup return distutils.core.setup(**attrs) ~~~~~~~~~~~~~~~~~~~~^^^^^^^^^ File "/private/var/folders/dj/d8p8jhd172n7wnq81ryfl6rc0000gn/T/pip-build-env-xr1q9l8l/overlay/lib/python3.13/site-packages/setuptools/_distutils/core.py", line 186, in setup return run_commands(dist) File "/private/var/folders/dj/d8p8jhd172n7wnq81ryfl6rc0000gn/T/pip-build-env-xr1q9l8l/overlay/lib/python3.13/site-packages/setuptools/_distutils/core.py", line 202, in run_commands dist.run_commands() ~~~~~~~~~~~~~~~~~^^ File "/private/var/folders/dj/d8p8jhd172n7wnq81ryfl6rc0000gn/T/pip-build-env-xr1q9l8l/overlay/lib/python3.13/site-packages/setuptools/_distutils/dist.py", line 1002, in run_commands self.run_command(cmd) ~~~~~~~~~~~~~~~~^^^^^ File "/private/var/folders/dj/d8p8jhd172n7wnq81ryfl6rc0000gn/T/pip-build-env-xr1q9l8l/overlay/lib/python3.13/site-packages/setuptools/dist.py", line 1102, in run_command super().run_command(command) ~~~~~~~~~~~~~~~~~~~^^^^^^^^^ File "/private/var/folders/dj/d8p8jhd172n7wnq81ryfl6rc0000gn/T/pip-build-env-xr1q9l8l/overlay/lib/python3.13/site-packages/setuptools/_distutils/dist.py", line 1021, in run_command cmd_obj.run() ~~~~~~~~~~~^^ File "<string>", line 169, in run File "/Users/abubakarismail/Desktop/Work/bitcoin-dev/py-bitcoin-ipc-client/pycapnp/buildutils/build.py", line 65, in build_libcapnp raise RuntimeError("CMake failed {}".format(returncode)) RuntimeError: CMake failed 1 [end of output] note: This error originates from a subprocess, and is likely not a problem with pip. ERROR: Failed building wheel for pycapnp Failed to build pycapnp [notice] A new release of pip is available: 25.1.1 -> 25.2 [notice] To update, run: pip install --upgrade pip ERROR: Failed to build installable wheels for some pyproject.toml based projects (pycapnp)</details>
So I opted into the other approach of
pip install .which by default, the setup script will automatically use the locally installed Cap'n Proto. If Cap'n Proto is not installed, it will bundle and build the matching Cap'n Proto library.So it picked up my homebrew capnproto installation.
fanquake commented at 4:14 PM on August 18, 2025:I did that initially but was having some errors
It looks like the
pycapnppackage doesn't currently work with Python 3.13 (https://github.com/capnproto/pycapnp/issues/372).
ismaelsadeeq commented at 4:50 PM on August 18, 2025:It looks like the pycapnp package doesn't currently work with Python 3.13 (https://github.com/capnproto/pycapnp/issues/372)
This issue seems unrelated.
The actual issue was that my ninja installation was not linked.
brew link --overwrite ninjaAnd then @josibake recommended documentation should work.
Thanks @josibake for helping me debug.
ryanofsky commented at 5:13 PM on August 18, 2025:re: #33201 (review)
On nixos, I needed to change this as follows:
- cpp_capnp_dir = Path(capnp.__path__[0]).parent + capnp_dir = Path(shutil.which("capnp")).parent.parent / "include" src_dir = Path(self.config['environment']['SRCDIR']) / "src" mp_dir = src_dir / "ipc" / "libmultiprocess" / "include" - imports = ["/usr/include", str(cpp_capnp_dir), str(src_dir), str(mp_dir)] + imports = [str(capnp_dir), str(src_dir), str(mp_dir)]Maybe something like this could work with homebrew too.
sipa commented at 5:17 PM on August 18, 2025:I added a comment to explain that Python 3.13 doesn't currently work.
Sjors commented at 9:10 AM on August 19, 2025:As of cd86ddaa1dac104415ab16cc8068fd50d068c12f the test passes for me on macOS with Python 3.10.14 with pycapnp installed via pip and capnp installed via homebrew.
I also tested Python 3.12.11. With Python 3.13.17
pip3 install pycapnpfails. We can worry about that later.in test/functional/interface_ipc.py:114 in 3f7ff4849e outdated
109 | + # Test some inspectors of template. 110 | + header = await template.result.getBlockHeader(ctx) 111 | + assert len(header.result) == 80 112 | + block_data = BytesIO((await template.result.getBlock(ctx)).result) 113 | + block = CBlock() 114 | + block.deserialize(block_data)
ismaelsadeeq commented at 3:32 PM on August 18, 2025:In "ci: add functional test for IPC interface" https://github.com/bitcoin/bitcoin/commit/3f7ff4849e6165a6bcc4d5977c35a467fd7fc232
Should be abstracted away as a helper to be reused ?
async def parse_and_deserialize_block(self, block_template, ctx): block_data = BytesIO((await block_template.result.getBlock(ctx)).result) block = CBlock() block.deserialize(block_data) return blockblock = await self.parse_and_deserialize_block(template, ctx)
sipa commented at 5:17 PM on August 18, 2025:Done.
in test/functional/interface_ipc.py:175 in 3f7ff4849e outdated
137 | + waitnext = template2.result.waitNext(ctx, waitoptions) 138 | + template3 = await waitnext 139 | + assert template3.to_dict() == {} 140 | + # Destroy template objects 141 | + template.result.destroy(ctx) 142 | + template2.result.destroy(ctx)
ismaelsadeeq commented at 3:39 PM on August 18, 2025:In "ci: add functional test for IPC interface" https://github.com/bitcoin/bitcoin/commit/3f7ff4849e6165a6bcc4d5977c35a467fd7fc232
Also add a branch for when we have a better template based on increase in transaction fees in the mempool above the fee threshold.
assert_equal(len(block2.vtx), 1) # Wait for another, get one after increase in fees in the mempool. waitnext = template2.result.waitNext(ctx, waitoptions) self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), 1, fee_rate=10) template3 = await waitnext assert template3 block3 = await self.parse_and_deserialize_block(template3, ctx) assert_equal(len(block3.vtx), 2) # Wait for another, but timeout. waitnext = template3.result.waitNext(ctx, waitoptions) template4 = await waitnext assert template4.to_dict() == {} # Destroy template objects template.result.destroy(ctx) template2.result.destroy(ctx) template3.result.destroy(ctx)
sipa commented at 5:17 PM on August 18, 2025:Thanks, added!
in test/functional/interface_ipc.py:135 in 3f7ff4849e outdated
130 | + template2 = await waitnext 131 | + assert template2 132 | + block2_data = BytesIO((await template2.result.getBlock(ctx)).result) 133 | + block2 = CBlock() 134 | + block2.deserialize(block2_data) 135 | + assert int(new_tip[0], 16) == block2.hashPrevBlock
ismaelsadeeq commented at 3:39 PM on August 18, 2025:In "ci: add functional test for IPC interface" https://github.com/bitcoin/bitcoin/commit/3f7ff4849e6165a6bcc4d5977c35a467fd7fc232
nit: use our internal assertion helpers for better errors?
in test/functional/interface_ipc.py:79 in 3f7ff4849e outdated
74 | + 75 | + def run_mining_test(self): 76 | + self.log.info("Running mining test") 77 | + modules = self.capnp_modules() 78 | + async def async_routine(): 79 | + init, ctx = await self.make_capnp_init_ctx()
ismaelsadeeq commented at 3:40 PM on August 18, 2025:In "ci: add functional test for IPC interface" https://github.com/bitcoin/bitcoin/commit/3f7ff4849e6165a6bcc4d5977c35a467fd7fc232
Is their benefit of having context for each test, else we can just have a global context?
sipa commented at 6:07 PM on September 2, 2025:I found it logistically simpler to give each their own, as otherwise all the test need to run within a single
asyncio.run(capnp.run(...)).ismaelsadeeq commented at 3:42 PM on August 18, 2025: memberApproach ACK
First pass initial comments
in test/README.md:45 in 3f7ff4849e outdated
40 | + 41 | +Depending on the system, it may be necessary to install and run in a venv: 42 | + 43 | +- `python -m venv venv` 44 | +- `venv/bin/pip3 install pycapnp` 45 | +- `venv/bin/python3 test/functional/interface_ipc.py`
josibake commented at 3:42 PM on August 18, 2025:These commands for installing via a venv dont work for me on macos or linux. I'm also not sure about installing via pip, since we need to be sure we are installing the package with capnproto bundled. Instead, I think
pycapnpshould be installed from source (venv or otherwise):git clone https://github.com/capnproto/pycapnp.git cd pycapnp pip install . -C force-bundled-libcapnp=TrueThis approach worked on macos and linux for me (inside a venv on both).
fanquake commented at 4:08 PM on August 18, 2025:venv/bin/python3 test/functional/interface_ipc.pyThis should be
build/test/functional/interface_ipc.py. Otherwise the test wont run.
ismaelsadeeq commented at 4:12 PM on August 18, 2025:When you build using depends with the autotools it will work I think? So both options maybe?
fanquake commented at 4:15 PM on August 18, 2025:Documentation being added to master shouldn't be for Autotools.
sipa commented at 5:18 PM on August 18, 2025:Cool, used this in the instructions.
sipa commented at 5:18 PM on August 18, 2025:Fixed.
in test/functional/interface_ipc.py:23 in 3f7ff4849e outdated
31 | + self.skip_if_no_py_capnp() 32 | + self.skip_if_no_bitcoind_ipc() 33 | + 34 | + @functools.cache 35 | + def capnp_modules(self): 36 | + cpp_capnp_dir = Path(capnp.__path__[0]).parent
josibake commented at 3:43 PM on August 18, 2025:AFAICT, this only works when capnproto is bundled inside
pycapnp.
ryanofsky commented at 5:11 PM on August 18, 2025:re: #33201 (review)
AFAICT, this only works when capnproto is bundled inside
pycapnp.Can confirm at least on nixos there are no .capnp files in this directory. Seems harmless to keep this if it helps other platforms, but would be good to document where it's necessary.
josibake commented at 6:25 PM on August 18, 2025:Sorry, didn’t mean to imply this breaks things if present, rather that adding this line works because it allows accessing the bundled version. This is why I recommended in a separate comment that our docs prompt the user to always install from source using the bundled version when installing pycapnp.
Longer term, I am confident we can drop all of this from the functional test in favour of having CMake pipe through the correct info to the config file.
ryanofsky commented at 7:54 PM on August 18, 2025:Thanks, when I saw this I wasn't aware what a "bundled" capnproto was but agree it makes sense to use it if the python extension includes it.
I guess I'd be curious what would happen if the docs were changed to use
force-system-libcapnp=Trueinstead offorce-bundled-libcapnp=True? It seems like since the c++ code already requires capnproto to be built, maybe it's wasteful for the instructions to recommend downloading it and rebuilding if it's not necessary?Also agree it could be good to get the location of the include directory from cmake. There's a
CAPNP_INCLUDE_DIRScmake variable that could work for this.I'm not sure it is necessarily better to use the cmake value because in theory there's no reason the python client needs to use the same library as the c++ server, and maybe it's even good to test with different versions. But getting a location directly from cmake is probably more reliable than having to derive one in python.
If we don't use the cmake path, the
shutil.whichapproach suggested #33201 (review) also could be reasonable and seems to match what python setup code does internally.Overall anything that works here seems fine to me.
josibake approvedjosibake commented at 3:51 PM on August 18, 2025: memberACK https://github.com/bitcoin/bitcoin/pull/33201/commits/3f7ff4849e6165a6bcc4d5977c35a467fd7fc232
Thanks for picking this up! I think the docs and multiplatform support still need a bit of work but that can be tackled in a follow-up(s), considering the main benefit I see here is getting a basic functional test in place that can be extended later.
DrahtBot requested review from Sjors on Aug 18, 2025DrahtBot requested review from ismaelsadeeq on Aug 18, 2025DrahtBot requested review from ryanofsky on Aug 18, 2025josibake commented at 3:53 PM on August 18, 2025: memberthe include paths used in the python test are a guess. It'd be nice to pass through the location of the capnp installation from the build system to the tests
I took a stab at this, ended up being harder than expected and I don't yet have something that works. But I think this is the correct longterm approach, instead of trying to guess in the functional test itself.
ismaelsadeeq approvedismaelsadeeq commented at 4:51 PM on August 18, 2025: memberACK 3f7ff4849e6165a6bcc4d5977c35a467fd7fc232
ryanofsky referenced this in commit 6d0103472f on Aug 18, 2025ryanofsky referenced this in commit c17b20d786 on Aug 18, 2025in test/functional/interface_ipc.py:28 in 3f7ff4849e outdated
23 | + def set_test_params(self): 24 | + self.num_nodes = 1 25 | + self.use_multiprocess_node = True 26 | + # The default socket path exceeds the maximum socket filename length in CI. 27 | + self.socket_path = Path(tempfile.mkdtemp()) / "ipc.sock" 28 | + self.extra_args = [[f"-ipcbind=unix:{self.socket_path}"]]
ryanofsky commented at 5:09 PM on August 18, 2025:In commit "ci: add functional test for IPC interface" (3f7ff4849e6165a6bcc4d5977c35a467fd7fc232)
This seems like it leaves behind an empty directory after the test runs. In any case it should be possible drop this code and simplify this commit by cherrypicking three commits from #30437:
- c3d82ef8fa947f108502ec80c522b4bd0084dd4c test: add is_ipc_enabled() and skip_if_no_ipc() functions
- 3af4d9a50ab008aa3974c580a831095e2ba17042 test: Provide path to
bitcoinbinary - 6d0103472f1d691e8e1a73049ba85163e103ae28 test: Add TestNode ipcbind option
I posted a branch doing this at https://github.com/ryanofsky/bitcoin/commits/pr/ipc-py.1
janb84 commented at 5:40 PM on August 18, 2025:This branch / patch works for me on Nix on macOS. Hint, do not forget to
import shutilwhen applying the patch below.
sipa commented at 5:41 PM on August 18, 2025:@ryanofsky Thanks! Will address later today.
sipa force-pushed on Aug 18, 2025sipa force-pushed on Aug 18, 2025DrahtBot added the label CI failed on Aug 18, 2025DrahtBot commented at 5:20 PM on August 18, 2025: contributor<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed. <sub>Task
lint: https://github.com/bitcoin/bitcoin/runs/48327091424</sub> <sub>LLM reason (✨ experimental): The CI failure is caused by errors detected in the lint test due to unused imports and variables in the Python code.</sub><details><summary>Hints</summary>
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
</details>
fanquake commented at 6:13 PM on August 18, 2025: memberhttps://github.com/bitcoin/bitcoin/actions/runs/17047602183/job/48327342413?pr=33201#step:8:3379:
stderr: Traceback (most recent call last): File "/Users/runner/work/bitcoin/bitcoin/ci/scratch/build-aarch64-apple-darwin23.6.0/test/functional/interface_ipc.py", line 161, in <module> IPCInterfaceTest(__file__).main() ~~~~~~~~~~~~~~~~^^^^^^^^^^ File "/Users/runner/work/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 186, in __init__ self.set_test_params() ~~~~~~~~~~~~~~~~~~~~^^ File "/Users/runner/work/bitcoin/bitcoin/ci/scratch/build-aarch64-apple-darwin23.6.0/test/functional/interface_ipc.py", line 40, in set_test_params self.capnp_modules = self.load_capnp_modules() ~~~~~~~~~~~~~~~~~~~~~~~^^ File "/Users/runner/work/bitcoin/bitcoin/ci/scratch/build-aarch64-apple-darwin23.6.0/test/functional/interface_ipc.py", line 23, in load_capnp_modules cpp_capnp_dir = Path(capnp.__path__[0]).parent ^^^^^ NameError: name 'capnp' is not definedsipa force-pushed on Aug 18, 2025sipa referenced this in commit f4ef6d2ce2 on Aug 18, 2025sipa force-pushed on Aug 18, 2025Sjors commented at 9:18 AM on August 19, 2025: memberIf this PR is to be based on #31802 or #33190 then it should be marked draft. Otherwise it could be made independent by dropping 7a7f11933b83d290129377523cc4a3cfc45fe9ef and instead enabling IPC for one or more CI machines that run functional tests.
It should be straightforward for me to rebase #31802 after this lands as long as you leave
ci/test/00_setup_env_i686_multiprocess.shalone (due to 789f7195b2acbc3e50ee75b2b5a88af058cf7b14).My preference would be to base it on #31802 because that gives the widest test coverage, including depends builds.
in test/README.md:40 in cd86ddaa1d outdated
33 | @@ -34,6 +34,26 @@ The ZMQ functional test requires a python ZMQ library. To install it: 34 | - on Unix, run `sudo apt-get install python3-zmq` 35 | - on mac OS, run `pip3 install pyzmq` 36 | 37 | +The IPC functional test requires a python IPC library. `pip3 install pycapnp` may work, but if not, install it from source: 38 | + 39 | +```sh 40 | +git clone https://github.com/capnproto/pycapnp.git`
maflcko commented at 9:19 AM on August 19, 2025:from the llm:
git clone https://github.com/capnproto/pycapnp.git
sipa commented at 7:28 AM on August 21, 2025:Done.
in test/functional/test_framework/test_framework.py:560 in cd86ddaa1d outdated
556 | @@ -550,15 +557,15 @@ def bin_dir_from_version(version): 557 | 558 | bin_dirs.append(bin_dir) 559 | 560 | + extra_init = [{}] * num_nodes if self.extra_init is None else self.extra_init
maflcko commented at 9:25 AM on August 19, 2025:Not sure why the lint task complains here:
[21:49:19.712] test/functional/test_framework/test_framework.py:560: error: Need type annotation for "extra_init" [var-annotated] [21:49:21.445] Found 1 error in 1 file (checked 301 source files) [21:49:21.473] ^---- ⚠️ Failure generated from lint-python.pyNo other variable in this function is required to be annotated and the lint should probably be fixed to not require it here either.
ryanofsky commented at 2:27 PM on August 19, 2025:If we wanted to annotate it, could write as
extra_init: list[dict[str, Any]] = ...(withfrom typing import Any) but would also seem reasonable to suppress this with# type: ignore[var-annotated]
sipa commented at 5:09 PM on August 19, 2025:Added the ignore annotation.
in test/functional/test_framework/test_framework.py:970 in cd86ddaa1d outdated
963 | @@ -964,6 +964,13 @@ def skip_if_no_py_sqlite3(self): 964 | except ImportError: 965 | raise SkipTest("sqlite3 module not available.") 966 | 967 | + def skip_if_no_py_capnp(self): 968 | + """Attempt to import the capnp package and skip the test if the import fails.""" 969 | + try: 970 | + import capnp # type: ignore[import] # noqa: F401
Sjors commented at 9:29 AM on August 19, 2025:In cd86ddaa1dac104415ab16cc8068fd50d068c12f ci: add functional test for IPC interface: is it somehow possible for
importto succeed but capnp to remain undefined?
sipa commented at 10:31 AM on August 19, 2025:This was with an earlier version of the PR which called
load_capnp_modulesfromset_test_params(which tried to do so before deciding whether pycapnp was even installed).
Sjors commented at 11:12 AM on August 19, 2025:Ah yes, now it's failing with a very cryptic "Tests successful"
sipa commented at 5:09 PM on August 19, 2025:Fixed, see elsewhere.
in test/functional/test_framework/test_framework.py:215 in 530ca0bc57 outdated
206 | @@ -207,6 +207,12 @@ def main(self): 207 | except BaseException: 208 | self.log.exception("Unexpected exception") 209 | self.success = TestStatus.FAILED 210 | + except SystemExit as e: 211 | + self.log.exception(f"System exit: {e.message}") 212 | + self.success = TestStatus.FAILED 213 | + except GeneratorExit as e: 214 | + self.log.exception(f"Generator exit: {e.message}") 215 | + self.success = TestStatus.FAILED
maflcko commented at 1:46 PM on August 19, 2025:this is a no-op, no? The
BaseExceptionabove callsself.log.exception, which fully logs the exception.Also, I don't think the logs contain "unexpected exception". In https://github.com/bitcoin/bitcoin/actions/runs/17053347607/job/48345965497#step:6:5037 the error is the stderr:
stderr: [node 0] Cleaning up ipc directory '/tmp/test-ipc-xj2xyof4'
sipa commented at 1:48 PM on August 19, 2025:See IRC:
09:15:59 < sipa> anyone have any clue what's going on here? https://github.com/bitcoin/bitcoin/actions/runs/17053347607/job/48345965497?pr=33201 09:16:13 < sipa> 240/272 - interface_ipc.py failed, Duration: 6 s 09:16:18 < sipa> 2025-08-18T22:28:30.854262Z TestFramework (INFO): Tests successful 09:28:06 < instagibbs> TIL SystemExit and GeneratorExit don't inherit Exception, so maybe SystemExit is triggering, causing the failure to not be set 09:46:23 < sipa> instagibbs: let's see!But it seems that these two do inherit from
BaseException, just not fromException, so nevermind.
ryanofsky commented at 2:21 PM on August 19, 2025:
janb84 commented at 2:39 PM on August 19, 2025:Maybe this line should be changed to use stdout instead of stderr?
(Sorry if this is the bug)
I Could reproduce the bug locally and this change solves it for me. (stderr -> stdout)
<details>
Without change :
Remaining jobs: [interface_ipc.py] 1/1 - interface_ipc.py failed, Duration: 4 s stdout: 2025-08-19T14:35:47.614991Z TestFramework (INFO): PRNG seed is: 1494337157176280909 2025-08-19T14:35:47.615519Z TestFramework (INFO): Initializing test directory /private/tmp/nix-shell-71975-4016936104/test_runner_₿_🏃_20250819_163547/interface_ipc_0 2025-08-19T14:35:49.166272Z TestFramework (INFO): Running echo test 2025-08-19T14:35:49.168265Z TestFramework (INFO): Running mining test 2025-08-19T14:35:51.247065Z TestFramework (INFO): Stopping nodes 2025-08-19T14:35:51.411476Z TestFramework (INFO): Cleaning up /private/tmp/nix-shell-71975-4016936104/test_runner_₿_🏃_20250819_163547/interface_ipc_0 on exit 2025-08-19T14:35:51.411598Z TestFramework (INFO): Tests successful stderr: [node 0] Cleaning up ipc directory '/private/tmp/nix-shell-71975-4016936104/test-ipc-qbv8akuk' TEST | STATUS | DURATION interface_ipc.py | ✖ Failed | 4 s ALL | ✖ Failed | 4 s (accumulated) Runtime: 4 swith change:
Temporary test directory at /private/tmp/nix-shell-71975-4016936104/test_runner_₿_🏃_20250819_163804 Remaining jobs: [interface_ipc.py] 1/1 - interface_ipc.py passed, Duration: 3 s TEST | STATUS | DURATION interface_ipc.py | ✓ Passed | 3 s ALL | ✓ Passed | 3 s (accumulated) Runtime: 3 s</details>
sipa commented at 5:08 PM on August 19, 2025:Changed to print to stdout, seems to work.
sipa referenced this in commit 0a5734e228 on Aug 19, 2025sipa force-pushed on Aug 19, 2025in test/functional/interface_ipc.py:56 in 088dc2c486 outdated
47 | + super().setup_nodes() 48 | + # Use this function to also load the capnp modules (we cannot use set_test_params for this, 49 | + # as it is being called before knowing whether capnp is available). 50 | + self.capnp_modules = self.load_capnp_modules() 51 | + 52 | + async def make_capnp_init_ctx(self):
Sjors commented at 5:00 PM on August 19, 2025:088dc2c486af0ad6803d919d8114ab29d3cd652f: eventually I'd like to move this and
load_capnp_modulesinto the test framework so that it's easier to expand e.g.mining_template_verification.pyto call bothgetblocktemplate proposalandcheckBlock().
sipa commented at 7:23 AM on August 21, 2025:Leaving this for a follow-up.
sipa commented at 10:45 PM on September 4, 2025:Actually, fixed.
in test/functional/interface_ipc.py:150 in 088dc2c486 outdated
145 | + self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), 1, fee_rate=10) 146 | + template3 = await waitnext 147 | + block3 = await self.parse_and_deserialize_block(template3, ctx) 148 | + assert_equal(len(block3.vtx), 2) 149 | + last_template = template3 150 | + # Wait for another, but timeout.
Sjors commented at 5:05 PM on August 19, 2025:088dc2c486af0ad6803d919d8114ab29d3cd652f: nit if you have to retouch: these comments are useful as
self.log.debug()
sipa commented at 7:22 AM on August 21, 2025:Done.
Sjors approvedSjors commented at 5:06 PM on August 19, 2025: memberACK 088dc2c486af0ad6803d919d8114ab29d3cd652f (after base PR lands)
DrahtBot requested review from ismaelsadeeq on Aug 19, 2025DrahtBot requested review from josibake on Aug 19, 2025DrahtBot requested review from ryanofsky on Aug 19, 2025sipa commented at 5:07 PM on August 19, 2025: memberAddressed a number of comments, and rebased on top of #31802.
I will however not have (much) time to push this further forward until the end of this month. @Sjors or @ryanofsky, or someone else, feel free to take this over and/or integrate into other PRs.
DrahtBot removed the label CI failed on Aug 19, 2025janb84 commented at 11:44 AM on August 20, 2025: contributorACK 088dc2c486af0ad6803d919d8114ab29d3cd652f (after base #31802)
This PR adds some functional tests for the IPC interface, setups the CI to use IPC and changes build documentation to match the changes in this PR.
Current state of the PR works on nix on macOS, builds & test run without trouble.
- build ✅
- tested ✅
- code review✅
small NIT if retouched, please see the remarks of DrahtBot's LLM linter.
in test/functional/test_framework/test_framework.py:1083 in eae32d1a9b outdated
1079 | @@ -1075,6 +1080,10 @@ def is_usdt_compiled(self): 1080 | """Checks whether the USDT tracepoints were compiled.""" 1081 | return self.config["components"].getboolean("ENABLE_USDT_TRACEPOINTS") 1082 | 1083 | + def is_ipc_enabled(self):
ryanofsky commented at 1:25 PM on August 20, 2025:In commit "test: add is_ipc_enabled() and skip_if_no_ipc() functions" (eae32d1a9b5c62707de6afd9832d40de859dd42d)
Not important, but might be better to rename enabled to compiled here to be more similar to surrounding functions which check ENABLE_XXX but are called is_xxx_compiled. (I think your PR actually did this originally but I didn't notice the pattern it was following)
sipa commented at 7:22 AM on August 21, 2025:Done.
in test/README.md:42 in 088dc2c486 outdated
33 | @@ -34,6 +34,26 @@ The ZMQ functional test requires a python ZMQ library. To install it: 34 | - on Unix, run `sudo apt-get install python3-zmq` 35 | - on mac OS, run `pip3 install pyzmq` 36 | 37 | +The IPC functional test requires a python IPC library. `pip3 install pycapnp` may work, but if not, install it from source: 38 | + 39 | +```sh 40 | +git clone https://github.com/capnproto/pycapnp.git` 41 | +cd pycapnp 42 | +pip install . -C force-bundled-libcapnp=True
ryanofsky commented at 1:34 PM on August 20, 2025:In commit "ci: add functional test for IPC interface" (088dc2c486af0ad6803d919d8114ab29d3cd652f)
As mentioned #33201 (review), would be curious to know if force-system-libcapnp=True works for people, since it seems wasteful to download and build a dependency that we already know must be installed (or the test would be skipped).
sipa commented at 7:22 AM on August 21, 2025:I opted to use system libcapnp in the description here because it's more likely to work. I've added a comment about it being optional now.
in ci/test/00_setup_env_native_tidy.sh:13 in 088dc2c486 outdated
9 | @@ -10,7 +10,8 @@ export CI_IMAGE_NAME_TAG="mirror.gcr.io/ubuntu:24.04" 10 | export CONTAINER_NAME=ci_native_tidy 11 | export TIDY_LLVM_V="20" 12 | export APT_LLVM_V="${TIDY_LLVM_V}" 13 | -export PACKAGES="clang-${TIDY_LLVM_V} libclang-${TIDY_LLVM_V}-dev llvm-${TIDY_LLVM_V}-dev libomp-${TIDY_LLVM_V}-dev clang-tidy-${TIDY_LLVM_V} jq libevent-dev libboost-dev libzmq3-dev systemtap-sdt-dev qt6-base-dev qt6-tools-dev qt6-l10n-tools libqrencode-dev libsqlite3-dev libcapnp-dev capnproto" 14 | +export PACKAGES="clang-${TIDY_LLVM_V} libclang-${TIDY_LLVM_V}-dev llvm-${TIDY_LLVM_V}-dev libomp-${TIDY_LLVM_V}-dev clang-tidy-${TIDY_LLVM_V} jq libevent-dev libboost-dev libzmq3-dev systemtap-sdt-dev qt6-base-dev qt6-tools-dev qt6-l10n-tools libqrencode-dev libsqlite3-dev libcapnp-dev capnproto python3-pip"
ryanofsky commented at 1:37 PM on August 20, 2025:In commit "ci: add functional test for IPC interface" (088dc2c486af0ad6803d919d8114ab29d3cd652f)
Not important, but for easier review and to avoid conflicts in the CI files, it may make sense to split this commit into two commits: one which adds the interface_ipc.py test and allows running it on systems with pycapnp installed, and another commit before or after that installs pycapnp in CI.
sipa commented at 7:22 AM on August 21, 2025:Done.
in test/functional/interface_ipc.py:31 in 088dc2c486 outdated
26 | + def load_capnp_modules(self): 27 | + # If pycapnp was installed with bundled capnp, capnp/c++.capnp can be found here. 28 | + bundled_capnp_dir = Path(capnp.__path__[0]).parent 29 | + # Otherwise guess the location of capnp/c++.capnp based on the installation path of the 30 | + # capnp binary itself. 31 | + bin_capnp_dir = Path(shutil.which("capnp")).parent.parent / "include"
ryanofsky commented at 1:45 PM on August 20, 2025:In commit "ci: add functional test for IPC interface" (088dc2c486af0ad6803d919d8114ab29d3cd652f)
This is probably fine in practice, but it does seem possible for this to fail if the
capnpbinary isn't on the path becauseshutil.whichwill return None. It might be better to write this as:if capnp_bin := shutil.which("capnp"): # Add the system cap'nproto path so include/capnp/c++.capnp can be found. capnp_dir = Path(shutil.which("capnp")).parent.parent / "include" else: # If there is system cap'nproto, the pycapnp module should have it's own "bundled" includes at this location capnp_dir = Path(capnp.__path__[0]).parent ```
sipa commented at 7:21 AM on August 21, 2025:Done.
in test/functional/interface_ipc.py:58 in 088dc2c486 outdated
53 | + node = self.nodes[0] 54 | + # Establish a connection, and create Init proxy object. 55 | + connection = await capnp.AsyncIoStream.create_unix_connection(node.ipc_socket_path) 56 | + client = capnp.TwoPartyClient(connection) 57 | + init = client.bootstrap().cast_as(self.capnp_modules['init'].Init) 58 | + # Create a thread, and a context object it is registered in.
ryanofsky commented at 1:47 PM on August 20, 2025:In commit "ci: add functional test for IPC interface" (088dc2c486af0ad6803d919d8114ab29d3cd652f)
I might describe this as # Create a remote thread on the server for the IPC calls to be executed in
sipa commented at 7:21 AM on August 21, 2025:Done.
ryanofsky approvedryanofsky commented at 1:57 PM on August 20, 2025: contributorCode review ACK 088dc2c486af0ad6803d919d8114ab29d3cd652f. Looks great! Left some comments that you should feel free to ignore. I am shocked and amazed about how much more friendly the python async API is compared to the c++ API, but I guess coroutines can bring similar benefits to c++. It is also nice how this tests covers different scenarios with waitNext()
DrahtBot added the label Needs rebase on Aug 20, 2025sipa referenced this in commit 61b154b1ec on Aug 21, 2025sipa force-pushed on Aug 21, 2025sipa force-pushed on Aug 21, 2025DrahtBot added the label CI failed on Aug 21, 2025DrahtBot commented at 7:28 AM on August 21, 2025: contributor<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed. <sub>Task
lint: https://github.com/bitcoin/bitcoin/runs/48558512670</sub> <sub>LLM reason (✨ experimental): Lint check failed due to unused variablecapnp_bin.</sub><details><summary>Hints</summary>
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
</details>
in test/functional/interface_ipc.py:31 in 03c66ee229 outdated
26 | + def load_capnp_modules(self): 27 | + if capnp_bin := shutil.which("capnp"): 28 | + # Add the system cap'nproto path so include/capnp/c++.capnp can be found. 29 | + capnp_dir = Path(capnp_bin).parent.parent / "include" 30 | + else: 31 | + # If there is system cap'nproto, the pycapnp module should have it's own "bundled" includes at this location
DrahtBot commented at 7:43 AM on August 21, 2025:in test/functional/interface_ipc.py comment: “have it’s own” -> “have its own” [possessive “its,” not contraction “it’s”]
sipa commented at 5:57 PM on September 2, 2025:Fixed.
DrahtBot removed the label CI failed on Aug 21, 2025janb84 commented at 8:59 AM on August 21, 2025: contributorre ACK 03c66ee229393335fabae1192b871f9ec68adf74
changes since last ACK:
- rebase
- addressed some NIT's
DrahtBot requested review from Sjors on Aug 21, 2025DrahtBot requested review from ryanofsky on Aug 21, 2025DrahtBot removed the label Needs rebase on Aug 21, 2025Sjors commented at 11:15 AM on August 21, 2025: memberFor some reason on the macOS native job (
00_setup_env_mac_native.sh) the interface test is skipped despitepycapnpbeing installed andbitcoin-nodebeing built: https://github.com/bitcoin/bitcoin/actions/runs/17120162263/job/48559027457?pr=33201#step:8:3718I checked that this test did run for native_asan.
I wrote a commit that enables it for additional jobs, which I'm testing in https://github.com/Sjors/bitcoin/pull/101:
- CentOS
- native man
- kernel
- native_tsan
- previous releases
- native valgrind
Update: 0c88edc2ab1eab711994523cf6654ce63e36af11 should be safe to cherry-pick, we can add the previous_release job later.
DrahtBot added the label Needs rebase on Aug 28, 2025sipa force-pushed on Sep 2, 2025sipa referenced this in commit df52d4588d on Sep 2, 2025sipa force-pushed on Sep 2, 2025DrahtBot removed the label Needs rebase on Sep 2, 2025Sjors commented at 3:24 PM on September 2, 2025: memberACK ebe87082cc482f0a0001e748e184b4e0db6ba996
I think you need to add
pycapnpto the macOS native job sointerface_ipc.pyisn't skipped there. I'm trying that now in https://github.com/Sjors/bitcoin/pull/103 (narrator: that's the wrong approach).DrahtBot requested review from janb84 on Sep 2, 2025in .github/workflows/ci.yml:143 in e043a279da outdated
135 | @@ -135,6 +136,13 @@ jobs: 136 | brew install --quiet python@3 || brew link --overwrite python@3 137 | brew install --quiet coreutils ninja pkgconf gnu-getopt ccache boost libevent zeromq qt@6 qrencode capnp 138 | 139 | + - name: Install Python packages 140 | + run: | 141 | + git clone https://github.com/capnproto/pycapnp 142 | + cd pycapnp 143 | + pip install . -C force-bundled-libcapnp=True --break-system-packages
Sjors commented at 4:33 PM on September 2, 2025:In e043a279da6ea901b5f6d6690f96ff0d419faf13 ci: enable IPC tests in CI:
pip3 installdid the trick for me, see https://github.com/Sjors/bitcoin/actions/runs/17409223017/job/49421870136?pr=103#step:8:3514Probably because
pippicks the system version whilepip3the Homebrew version.Using PyEnv, which I'm proposing for the linter in #33051, should prevent this kind of confusion.
sipa commented at 5:56 PM on September 2, 2025:Done.
sipa force-pushed on Sep 2, 2025in ci/test/00_setup_env_native_tidy.sh:14 in 7eb71775dc outdated
9 | @@ -10,7 +10,8 @@ export CI_IMAGE_NAME_TAG="mirror.gcr.io/ubuntu:24.04" 10 | export CONTAINER_NAME=ci_native_tidy 11 | export TIDY_LLVM_V="20" 12 | export APT_LLVM_V="${TIDY_LLVM_V}" 13 | -export PACKAGES="clang-${TIDY_LLVM_V} libclang-${TIDY_LLVM_V}-dev llvm-${TIDY_LLVM_V}-dev libomp-${TIDY_LLVM_V}-dev clang-tidy-${TIDY_LLVM_V} jq libevent-dev libboost-dev libzmq3-dev systemtap-sdt-dev qt6-base-dev qt6-tools-dev qt6-l10n-tools libqrencode-dev libsqlite3-dev libcapnp-dev capnproto" 14 | +export PACKAGES="clang-${TIDY_LLVM_V} libclang-${TIDY_LLVM_V}-dev llvm-${TIDY_LLVM_V}-dev libomp-${TIDY_LLVM_V}-dev clang-tidy-${TIDY_LLVM_V} jq libevent-dev libboost-dev libzmq3-dev systemtap-sdt-dev qt6-base-dev qt6-tools-dev qt6-l10n-tools libqrencode-dev libsqlite3-dev libcapnp-dev capnproto python3-pip" 15 | +export PIP_PACKAGES="pycapnp"
Sjors commented at 5:50 PM on September 2, 2025:7eb71775dce6f3f2fd0e4b62f439aab86d1b65a0: tidy doesn't run functional tests currently, but I guess it doesn't hurt either.
maflcko commented at 3:47 PM on September 3, 2025:currently
I don't think it will ever be possible to run them with tidy, also it doesn't make sense to do?
sipa commented at 6:35 PM on September 3, 2025:Fixed.
Sjors commented at 5:51 PM on September 2, 2025: memberACK 235016f5b78ba9f472b56df0825690307fffc7e6
janb84 commented at 6:05 PM on September 2, 2025: contributorre ACK 235016f5b78ba9f472b56df0825690307fffc7e6
changes since last ACK:
- Rebase
- Fixes for macOS native job (pip -> pip3)
- Added IPC interface tests to more hosts.
in .github/workflows/ci.yml:158 in 235016f5b7 outdated
135 | @@ -135,6 +136,13 @@ jobs: 136 | brew install --quiet python@3 || brew link --overwrite python@3 137 | brew install --quiet coreutils ninja pkgconf gnu-getopt ccache boost libevent zeromq qt@6 qrencode capnp 138 | 139 | + - name: Install Python packages 140 | + run: | 141 | + git clone https://github.com/capnproto/pycapnp
fanquake commented at 9:35 AM on September 3, 2025:Wanted to note that the maintainer of this repo has said they aren't actively working on it: https://github.com/capnproto/pycapnp/issues/372#issuecomment-3247204007.
Sjors commented at 4:59 PM on September 3, 2025:It looks like the maintainer wants to be replaced. Since people have been opening pull requests, hopefully they'll find a replacement.
So far
pycapnpseems to work fine up to Python 3.12 and we support a minimum of 3.10, so we have some time.in test/config.ini.in:29 in a5e470e134 outdated
25 | @@ -26,3 +26,4 @@ RPCAUTH=@abs_top_srcdir@/share/rpcauth/rpcauth.py 26 | @ENABLE_ZMQ_TRUE@ENABLE_ZMQ=true 27 | @ENABLE_EXTERNAL_SIGNER_TRUE@ENABLE_EXTERNAL_SIGNER=true 28 | @ENABLE_USDT_TRACEPOINTS_TRUE@ENABLE_USDT_TRACEPOINTS=true 29 | +@ENABLE_IPC_TRUE@ENABLE_IPC=true
ismaelsadeeq commented at 1:42 PM on September 3, 2025:In "test: add is_ipc_compiled() and skip_if_no_ipc() functions" a5e470e134b8fb60264c349d8580e6fb0175b7c2
nitty-nit feel free to ignore
In commit message body 's/ function tests /functional tests /g'
sipa commented at 6:33 PM on September 3, 2025:Done.
in test/functional/test_framework/test_framework.py:1102 in a5e470e134 outdated
1079 | @@ -1075,6 +1080,10 @@ def is_usdt_compiled(self): 1080 | """Checks whether the USDT tracepoints were compiled.""" 1081 | return self.config["components"].getboolean("ENABLE_USDT_TRACEPOINTS") 1082 | 1083 | + def is_ipc_compiled(self):
ismaelsadeeq commented at 2:51 PM on September 3, 2025:In "test: add is_ipc_compiled() and skip_if_no_ipc() functions" a5e470e134b8fb60264c349d8580e6fb0175b7c2
nit: 's/compiled/enabled/g'
maflcko commented at 3:46 PM on September 3, 2025:"enabled" feels a bit like it can change at runtime, but this is a check about whether it has been compiled in, so I think the name is correct
Sjors commented at 5:01 PM on September 3, 2025:Right, we use enable in cmake to decide if we want to compile. The test suite cares about whether it was compiled.
sipa commented at 6:33 PM on September 3, 2025:Leaving as-is.
in test/functional/interface_ipc.py:18 in 592e253f76 outdated
12 | + 13 | +# Test may be skipped and not have capnp installed 14 | +try: 15 | + import capnp # type: ignore[import] # noqa: F401 16 | +except ImportError: 17 | + pass
ismaelsadeeq commented at 2:56 PM on September 3, 2025:In "tests: add functional tests for IPC interface" 592e253f76764f108e9c34fa0da32451091b946a
This should be removed since we now have
skip_if_no_py_capnp
Sjors commented at 5:02 PM on September 3, 2025:I don't think we can remove this, because the
importhas to be at the top of the file and theskip_if_no_py_capnphappens later.
sipa commented at 6:33 PM on September 3, 2025:Yeah, the import is needed at the top level, I believe.
in test/functional/interface_ipc.py:101 in 592e253f76 outdated
96 | + assert (await mining.result.isTestChain(ctx)) 97 | + assert (await mining.result.isInitialBlockDownload(ctx)) 98 | + blockref = await mining.result.getTip(ctx) 99 | + assert blockref.hasResult 100 | + assert_equal(len(blockref.result.hash), 32) 101 | + assert blockref.result.height > 100
ismaelsadeeq commented at 3:24 PM on September 3, 2025:In "tests: add functional tests for IPC interface" 592e253f76764f108e9c34fa0da32451091b946a
It will be nice if we use defined constants or define new ones for the magic numbers, or use response from rpc. It will help in improving the readability of the test.
diff --git a/test/functional/interface_ipc.py b/test/functional/interface_ipc.py index 6ff11cbe0f8..912fa3a459b 100755 --- a/test/functional/interface_ipc.py +++ b/test/functional/interface_ipc.py @@ -97,18 +97,21 @@ class IPCInterfaceTest(BitcoinTestFramework): assert (await mining.result.isInitialBlockDownload(ctx)) blockref = await mining.result.getTip(ctx) assert blockref.hasResult - assert_equal(len(blockref.result.hash), 32) - assert blockref.result.height > 100 + block_hash_size = 32 + assert_equal(len(blockref.result.hash), block_hash_size) + current_block_height = self.nodes[0].getchaintips()[0]["height"] + assert blockref.result.height == current_block_height self.log.debug("Mine a block") - wait = mining.result.waitTipChanged(ctx, blockref.result.hash, 1000.0) + timeout = 1000.0 # 1000 milliseconds + wait = mining.result.waitTipChanged(ctx, blockref.result.hash, ) self.generate(self.nodes[0], 1) newblockref = await wait - assert_equal(len(newblockref.result.hash), 32) - assert_equal(newblockref.result.height, blockref.result.height + 1) - self.log.debug("Wait for timeout (1000 milliseconds)") - wait = mining.result.waitTipChanged(ctx, newblockref.result.hash, 1000.0) + assert_equal(len(newblockref.result.hash), block_hash_size) + assert_equal(newblockref.result.height, current_block_height + 1) + self.log.debug("Wait for timeout") + wait = mining.result.waitTipChanged(ctx, newblockref.result.hash, timeout) oldblockref = await wait - assert_equal(len(newblockref.result.hash), 32) + assert_equal(len(newblockref.result.hash), block_hash_size) assert_equal(oldblockref.result.hash, newblockref.result.hash) assert_equal(oldblockref.result.height, newblockref.result.height) @@ -120,7 +123,8 @@ class IPCInterfaceTest(BitcoinTestFramework): template = mining.result.createNewBlock(opts) self.log.debug("Test some inspectors of Template") header = await template.result.getBlockHeader(ctx) - assert_equal(len(header.result), 80) + block_header_size = 80 + assert_equal(len(header.result), block_header_size) block = await self.parse_and_deserialize_block(template, ctx) assert_equal(ser_uint256(block.hashPrevBlock), newblockref.result.hash) assert len(block.vtx) >= 1 @@ -134,7 +138,7 @@ class IPCInterfaceTest(BitcoinTestFramework): assert_equal(coinbase.vin[0].prevout.hash, 0) self.log.debug("Wait for a new template") waitoptions = self.capnp_modules['mining'].BlockWaitOptions() - waitoptions.timeout = 1000.0 + waitoptions.timeout = timeout waitnext = template.result.waitNext(ctx, waitoptions) self.generate(self.nodes[0], 1) template2 = await waitnext
sipa commented at 6:34 PM on September 3, 2025:Thanks, included.
ismaelsadeeq commented at 3:26 PM on September 3, 2025: memberCode review ACK 235016f5b78ba9f472b56df0825690307fffc7e6 🤖
Comments are suggestion for improvement and can come in a followup
in test/README.md:41 in 592e253f76 outdated
33 | @@ -34,6 +34,25 @@ The ZMQ functional test requires a python ZMQ library. To install it: 34 | - on Unix, run `sudo apt-get install python3-zmq` 35 | - on mac OS, run `pip3 install pyzmq` 36 | 37 | +The IPC functional test requires a python IPC library. `pip3 install pycapnp` may work, but if not, install it from source: 38 | + 39 | +```sh 40 | +git clone https://github.com/capnproto/pycapnp.git 41 | +pip install pycapnp
ryanofsky commented at 5:28 PM on September 3, 2025:In commit "tests: add functional tests for IPC interface" (592e253f76764f108e9c34fa0da32451091b946a)
Does this need to say
pip install ./pycapnp? It seems like on my system without./pip ignores the git clone and downloads a wheel instead. Same might apply to the venv instructions below.
sipa commented at 6:34 PM on September 3, 2025:Done.
in test/functional/interface_ipc.py:31 in 592e253f76 outdated
26 | + def load_capnp_modules(self): 27 | + if capnp_bin := shutil.which("capnp"): 28 | + # Add the system cap'nproto path so include/capnp/c++.capnp can be found. 29 | + capnp_dir = Path(capnp_bin).parent.parent / "include" 30 | + else: 31 | + # If there is system cap'nproto, the pycapnp module should have its own "bundled"
ryanofsky commented at 5:29 PM on September 3, 2025:In commit "tests: add functional tests for IPC interface" (592e253f76764f108e9c34fa0da32451091b946a)
s/there is/there is no/
sipa commented at 6:34 PM on September 3, 2025:Fixed.
in test/functional/interface_ipc.py:37 in 592e253f76 outdated
32 | + # includes at this location. If pycapnp was installed with bundled capnp, 33 | + # capnp/c++.capnp can be found here. 34 | + capnp_dir = Path(capnp.__path__[0]).parent 35 | + src_dir = Path(self.config['environment']['SRCDIR']) / "src" 36 | + mp_dir = src_dir / "ipc" / "libmultiprocess" / "include" 37 | + imports = [str(capnp_dir), str(capnp_dir), str(src_dir), str(mp_dir)]
ryanofsky commented at 5:30 PM on September 3, 2025:In commit "tests: add functional tests for IPC interface" (592e253f76764f108e9c34fa0da32451091b946a)
capnp_dir is listed twice here
sipa commented at 6:34 PM on September 3, 2025:Fixed.
ryanofsky approvedryanofsky commented at 6:00 PM on September 3, 2025: contributorCode review ACK 235016f5b78ba9f472b56df0825690307fffc7e6. Nice to have this ability to test the interfaces directly. Left a few comments but none are important
sipa referenced this in commit 59d842ff4c on Sep 3, 2025sipa force-pushed on Sep 3, 2025sipa force-pushed on Sep 3, 2025ryanofsky approvedryanofsky commented at 3:32 AM on September 4, 2025: contributorCode review ACK db52550507045402e89c6455bec680fcd61e26b6, just making small suggested changes since last review: fixing up pycapnp install instructions, using --break-system-packages only where needed, making small test cleanups
DrahtBot requested review from janb84 on Sep 4, 2025DrahtBot requested review from ismaelsadeeq on Sep 4, 2025DrahtBot requested review from Sjors on Sep 4, 2025in ci/test/00_setup_env_native_tidy.sh:13 in db52550507 outdated
9 | @@ -10,7 +10,7 @@ export CI_IMAGE_NAME_TAG="mirror.gcr.io/ubuntu:24.04" 10 | export CONTAINER_NAME=ci_native_tidy 11 | export TIDY_LLVM_V="20" 12 | export APT_LLVM_V="${TIDY_LLVM_V}" 13 | -export PACKAGES="clang-${TIDY_LLVM_V} libclang-${TIDY_LLVM_V}-dev llvm-${TIDY_LLVM_V}-dev libomp-${TIDY_LLVM_V}-dev clang-tidy-${TIDY_LLVM_V} jq libevent-dev libboost-dev libzmq3-dev systemtap-sdt-dev qt6-base-dev qt6-tools-dev qt6-l10n-tools libqrencode-dev libsqlite3-dev libcapnp-dev capnproto" 14 | +export PACKAGES="clang-${TIDY_LLVM_V} libclang-${TIDY_LLVM_V}-dev llvm-${TIDY_LLVM_V}-dev libomp-${TIDY_LLVM_V}-dev clang-tidy-${TIDY_LLVM_V} jq libevent-dev libboost-dev libzmq3-dev systemtap-sdt-dev qt6-base-dev qt6-tools-dev qt6-l10n-tools libqrencode-dev libsqlite3-dev libcapnp-dev capnproto python3-pip"
maflcko commented at 6:10 AM on September 4, 2025:nit: pip is now unused and can be removed
sipa commented at 6:29 PM on September 4, 2025:Fixed.
Sjors commented at 7:12 AM on September 4, 2025: memberre-ACK db52550507045402e89c6455bec680fcd61e26b6
I checked again that the test isn't skipped on the machines where we enable it. It's skipped in the test each commit job:
243/273 - interface_ipc.py skipped (capnp module not available.)That's for the last commit it runs, 1b69f79d138eaf099b108aea6476a0c790d49de7
ci: enable IPC tests in CI.I would be fine with leaving that for a followup.
in .github/workflows/ci.yml:158 in db52550507 outdated
152 | @@ -152,6 +153,13 @@ jobs: 153 | brew install --quiet python@3 || brew link --overwrite python@3 154 | brew install --quiet coreutils ninja pkgconf gnu-getopt ccache boost libevent zeromq qt@6 qrencode capnp 155 | 156 | + - name: Install Python packages 157 | + run: | 158 | + git clone https://github.com/capnproto/pycapnp
fanquake commented at 3:14 PM on September 4, 2025:Looks like there might actually be a new
2.1.0release shortly: https://github.com/capnproto/pycapnp/commit/3a3adfb5f1a8d1b52c98e4984f38ceb5b89a94a6. In which case, I think we should pin the checkout to that version, rather than cloning master (which may break at any point).achow101 added this to the milestone 30.0 on Sep 4, 2025sipa referenced this in commit 0d5f936c9d on Sep 4, 2025sipa force-pushed on Sep 4, 2025sipa force-pushed on Sep 4, 2025sipa force-pushed on Sep 4, 2025in .github/workflows/ci.yml:159 in 77ed42b1b2 outdated
152 | @@ -152,6 +153,12 @@ jobs: 153 | brew install --quiet python@3 || brew link --overwrite python@3 154 | brew install --quiet coreutils ninja pkgconf gnu-getopt ccache boost libevent zeromq qt@6 qrencode capnp 155 | 156 | + - name: Install Python packages 157 | + run: | 158 | + # Use commit with Python 3.13 support, which is not yet released as of writing. 159 | + git clone -b 3a3adfb5f1a8d1b52c98e4984f38ceb5b89a94a6 https://github.com/capnproto/pycapnp
maflcko commented at 6:19 PM on September 4, 2025:I don't think this is
-b. You'll have to use--revisionfor raw commit ids? See https://git-scm.com/docs/git-clone#Documentation/git-clone.txt---revisionrev
sipa commented at 6:29 PM on September 4, 2025:Thanks! It worked for a tag, but not for a commit.
sipa force-pushed on Sep 4, 2025sipa referenced this in commit 4ee395d8ef on Sep 4, 2025sipa force-pushed on Sep 4, 2025in test/functional/interface_ipc.py:150 in c2a47f57d7 outdated
145 | + template2 = await waitnext 146 | + block2 = await self.parse_and_deserialize_block(template2, ctx) 147 | + assert_equal(len(block2.vtx), 1) 148 | + template3 = None 149 | + last_template = template2 150 | + if self.uses_wallet:
TheCharlatan commented at 8:58 PM on September 4, 2025:How does this condition get fulfilled? I set
uses_walletmanually in theset_test_params, but then the assertion on line 161 fails, which is a bit puzzling, because that would mean something changed in the template in the meantime.
sipa commented at 9:07 PM on September 4, 2025:It's set by
skip_if_no_wallet.
TheCharlatan commented at 9:13 PM on September 4, 2025:Right, so we might want to call that here?
sipa commented at 9:18 PM on September 4, 2025:Oh, I see.
sipa commented at 10:44 PM on September 4, 2025:Ok, dropped this piece since this code basically never did anything at all, and adding a test that works with the wallet needs more work than I'm willing to put in right now, I'm going to leave that for all follow-up instead.
TheCharlatan commented at 6:28 AM on September 5, 2025:That seems fine, though it would be good to get the fee bumping functionality tested here (in a follow up).
sipa commented at 11:46 AM on September 5, 2025:Resolving, thanks to MiniWallet-based test.
sipa force-pushed on Sep 4, 2025in test/functional/interface_ipc.py:157 in 81971dcd77 outdated
152 | + template4 = await waitnext 153 | + assert_equal(template4.to_dict(), {}) 154 | + self.log.debug("Destroy template objects") 155 | + template.result.destroy(ctx) 156 | + template2.result.destroy(ctx) 157 | + if self.uses_wallet:
TheCharlatan commented at 7:52 AM on September 5, 2025:This (and the
template3declaration above) should go too, right?
TheCharlatan commented at 9:41 AM on September 5, 2025:Or it can be fixed with the following by using the MiniWallet:
diff --git a/test/functional/interface_ipc.py b/test/functional/interface_ipc.py index 03f0ed200c..c54e3f3a6b 100755 --- a/test/functional/interface_ipc.py +++ b/test/functional/interface_ipc.py @@ -11,0 +12 @@ from test_framework.test_framework import (BitcoinTestFramework, assert_equal) +from test_framework.wallet import MiniWallet @@ -94,0 +96,2 @@ class IPCInterfaceTest(BitcoinTestFramework): + miniwallet = MiniWallet(self.nodes[0]) + @@ -148,2 +151 @@ class IPCInterfaceTest(BitcoinTestFramework): - template3 = None - last_template = template2 + @@ -151 +153,5 @@ class IPCInterfaceTest(BitcoinTestFramework): - waitnext = last_template.result.waitNext(ctx, waitoptions) + template3 = await template2.result.waitNext(ctx, waitoptions) + assert_equal(template3.to_dict(), {}) + self.log.debug("Wait for another, get one after increase in fees in the mempool") + waitnext = template2.result.waitNext(ctx, waitoptions) + miniwallet.send_self_transfer(fee_rate=10, from_node=self.nodes[0]) @@ -153 +159,17 @@ class IPCInterfaceTest(BitcoinTestFramework): - assert_equal(template4.to_dict(), {}) + block3 = await self.parse_and_deserialize_block(template4, ctx) + assert_equal(len(block3.vtx), 2) + self.log.debug("Wait again, this should return the same template, since the fee threshold is zero") + template5 = await template4.result.waitNext(ctx, waitoptions) + block4 = await self.parse_and_deserialize_block(template5, ctx) + assert_equal(len(block4.vtx), 2) + waitoptions.feeThreshold = 1 + self.log.debug("Wait for another, get one after increase in fees in the mempool") + waitnext = template5.result.waitNext(ctx, waitoptions) + miniwallet.send_self_transfer(fee_rate=10, from_node=self.nodes[0]) + template6 = await waitnext + block4 = await self.parse_and_deserialize_block(template6, ctx) + assert_equal(len(block4.vtx), 3) + self.log.debug("Wait for another, but time out, since the fee threshold is set now") + template7 = await template6.result.waitNext(ctx, waitoptions) + assert_equal(template7.to_dict(), {}) + @@ -157,2 +179,5 @@ class IPCInterfaceTest(BitcoinTestFramework): - if self.uses_wallet: - template3.result.destroy(ctx) + template3.result.destroy(ctx) + template4.result.destroy(ctx) + template5.result.destroy(ctx) + template6.result.destroy(ctx) + template7.result.destroy(ctx)@Sjors it's a bit annoying that the options as initialized through capnp don't have our internal defaults, i.e. the default fee rate is zero instead of
MAX_MONEY. Should we add interface functions for constructing default objects for these cases?
Sjors commented at 9:53 AM on September 5, 2025:There was an earlier discussion about that. Apparently in c++ this isn't a problem, but for other language binding we should have defaults. So I'm inclined to agree that we should pick some.
sipa commented at 11:45 AM on September 5, 2025:@TheCharlatan Thanks, included the MiniWallet approach!
TheCharlatan commented at 12:37 PM on September 5, 2025:Sorry for the churn, but I think I made a mistake here:
diff --git a/test/functional/interface_ipc.py b/test/functional/interface_ipc.py index a87ada4ffa..8410fa24be 100755 --- a/test/functional/interface_ipc.py +++ b/test/functional/interface_ipc.py @@ -149,0 +150 @@ class IPCInterfaceTest(BitcoinTestFramework): + self.log.debug("Wait for another, but time out") @@ -155,2 +156 @@ class IPCInterfaceTest(BitcoinTestFramework): - self.log.debug("Wait for another, but time out") - template4 = await template2.result.waitNext(ctx, waitoptions) + template4 = await waitnext
sipa commented at 1:05 PM on September 5, 2025:Right. Fixed?
TheCharlatan commented at 1:10 PM on September 5, 2025:I think that should fix the problem, but the log line is still in the wrong place.
sipa commented at 1:24 PM on September 5, 2025:Right. More fixed?
8c7f005629test: add is_ipc_compiled() and skip_if_no_ipc() functions
Expose ENABLE_IPC build option to functional tests so new tests can test IPC-only features.
3cceb60a71test: Provide path to `bitcoin` binary
Set new `BitcoinTestFramework.binary_paths.bitcoin_bin` property with path to the `bitcoin` wrapper binary. This allows new tests for `bitcoin-mine` in #30437 and `bitcoin-cli` in #32297 to find the `bitcoin` binary and call `bitcoin -m` to start nodes with IPC support. This way the new tests can run whenever the ENABLE_IPC build option is enabled, instead of only running when the `BITCOIN_CMD` environment variable is set to `bitcoin -m`
3cc9a06c8dtest: Add TestNode ipcbind option
With this change, tests can specify `self.extra_init = [{ipcbind: True}]` to start a node listening on an IPC socket, instead of needing to choose which node binary to invoke and what `self.extra_args=[["-ipcbind=..."]]` value to pass to it. The eliminates boilerplate code #30437 (interface_ipc_mining.py), #32297 (interface_ipc_cli.py), and #33201 (interface_ipc.py) previously needed in their test setup.in .github/workflows/ci.yml:158 in 9baa7a1587 outdated
152 | @@ -152,6 +153,12 @@ jobs: 153 | brew install --quiet python@3 || brew link --overwrite python@3 154 | brew install --quiet coreutils ninja pkgconf gnu-getopt ccache boost libevent zeromq qt@6 qrencode capnp 155 | 156 | + - name: Install Python packages 157 | + run: | 158 | + # Use commit with Python 3.13 support, which is not yet released as of writing.
fanquake commented at 9:13 AM on September 5, 2025:v2.1.0 has been tagged: https://github.com/capnproto/pycapnp/releases/tag/v2.1.0.
sipa commented at 11:45 AM on September 5, 2025:Switched to it in docs and macOS CI.
sipa force-pushed on Sep 5, 2025sipa commented at 12:05 PM on September 5, 2025: memberStrange error, https://github.com/bitcoin/bitcoin/actions/runs/17491932432/job/49683489249?pr=33201:
node0 stderr bitcoin-node: ipc/libmultiprocess/include/mp/proxy-io.h:279: void mp::Waiter::post(Fn &&) [Fn = (lambda at ./ipc/libmultiprocess/include/mp/type-context.h:67:19)]: Assertion `!m_fn' failed.in line
template4 = await template2.result.waitNext(ctx, waitoptions)Any idea, @ryanofsky?
sipa force-pushed on Sep 5, 2025sipa commented at 12:23 PM on September 5, 2025: memberDebugged on IRC:
07:50:56 < sipa> ryanofsky: any idea about this error in https://github.com/bitcoin/bitcoin/actions/runs/17491932432/job/49683489249?pr=33201 : 07:51:08 < sipa> node0 stderr bitcoin-node: ipc/libmultiprocess/include/mp/proxy-io.h:279: void mp::Waiter::post(Fn &&) [Fn = (lambda at ./ipc/libmultiprocess/include/mp/type-context.h:67:19)]: Assertion `!m_fn' failed. 07:55:12 < kevkevin> looks like the test got hung up on this line in the ipc test: template4 = File "/home/admin/actions-runner/_work/_temp/build/test/functional/interface_ipc.py", line 156, in async_routine: await template2.result.waitNext(ctx, waitoptions) 07:56:46 < sipa> yeah, bitcoin-node crashed during that invocation 08:01:34 < sipa> i can reproduce it locally, though it's pretty rare (once in 60 runs, so far) 08:04:00 < kevkevin> hmm interesting, I'll try to reproduce it locally myself 08:05:03 < ryanofsky> that is interesting, i've never seen that assert hit before. i think it could happen if two ipc clients try to execute a different IPC calls on the same thread at the same time 08:06:01 < sipa> huh! 08:06:15 < ryanofsky> libmultiprocess assumes a 1:1 threading model where a there is one server thread for every client thread, emulating a traditional c++ call stack 08:06:26 < ryanofsky> allowing recursive mutexes, synchronous callbacks, etc 08:06:37 < sipa> should the python side do anything specific to arrange that? 08:07:01 < sipa> i would assume not, because there is only one thread on the python side that does all IPC calls 08:07:11 < ryanofsky> it should just make two IPC calls at the same time, and they should be calls that do not return instantly like waitNext 08:08:18 < ryanofsky> yeah, it is unexpected that current python test would cause that 08:08:35 < dodo> c 08:10:03 < ryanofsky> maybe there is a race condition where c++ code sends a response but the server thread doesn't return to being idle, and python code sends the next request quickly enough to hit the assert 08:11:00 < sipa> oh, i may have messed something up 08:11:09 < sipa> waitnext = template2.result.waitNext(ctx, waitoptions) 08:11:14 < sipa> template4 = await template2.result.waitNext(ctx, waitoptions) 08:11:31 < sipa> might create two asynchronous calls simultaneously? 08:12:27 < ryanofsky> it seems like that would just chain the calls so the server runs them back to back, and make the bug more likely to happen 08:13:09 < ryanofsky> but if you added an await on the first line it might prevent this 08:13:40 < sipa> the first one is just unnecessary - i inlined it into the second one, but forgot to remove the original 08:14:06 < sipa> you say "make the bug more likely", so you think that the bug isn't in this code itself, but elsewhere? 08:14:18 < ryanofsky> actually i misread. yeah i assumed you were using waitnext variable second line and the calls would be chained. but actually this does look like simultaneous calls 08:14:44 < sipa> alright, let me run it 1000 times to see if it reappears, but if not, i'm going to assume that was the culprit 08:14:48 < ryanofsky> no i think this code does explain the bug 08:15:15 < sipa> thanks! 08:15:23 < ryanofsky> i just first misread it as chained calls that would be pipelined when actually those are parallel calls 08:15:29 < sipa> right 08:16:14 < ryanofsky> thanks for the test, and finding this. server could definitely do better here, it could just refuse the request instead of asserting 08:18:03 < sipa> or at least a more helpful error message 08:18:23 < sipa> i could imagine third parties trying to use the interface might hit something like this too 08:20:08 < ryanofsky> yeah the error message is the most important thing in practiceSo the culprit was:
waitnext = template2.result.waitNext(ctx, waitoptions) template4 = await template2.result.waitNext(ctx, waitoptions)Which resulted in attempting two parallel IPC calls being executed in the same thread on the server.
sipa force-pushed on Sep 5, 20258d2ee88fa2tests: add functional tests for IPC interface
Co-Authored-By: ismaelsadeeq <ask4ismailsadiq@gmail.com> Co-Authored-By: ryanofsky <ryan@ofsky.org> Co-Authored-By: TheCharlatan <seb.kung@gmail.com> Co-Authored-By: Sjors Provoost <sjors@sprovoost.nl>
ci: enable IPC tests in CI 6aee573bfcin ci/test/00_setup_env_native_valgrind.sh:12 in d0b8af4985 outdated
8 | @@ -9,6 +9,7 @@ export LC_ALL=C.UTF-8 9 | export CI_IMAGE_NAME_TAG="mirror.gcr.io/ubuntu:24.04" 10 | export CONTAINER_NAME=ci_native_valgrind 11 | export PACKAGES="valgrind python3-zmq libevent-dev libboost-dev libzmq3-dev libsqlite3-dev libcapnp-dev capnproto" 12 | +export PIP_PACKAGES="pycapnp"
fanquake commented at 1:13 PM on September 5, 2025:This needs
python3-pipinPACKAGES.
fanquake commented at 1:30 PM on September 5, 2025:and
--break-system-packages. Probably time to roll this job back into the CI, now that we've swtiched over.
sipa commented at 1:32 PM on September 5, 2025:Done. I was going to say, this doesn't seem to be invoked anywhere?
sipa force-pushed on Sep 5, 2025a341e11ac9ci: test IPC on additional hosts
Install pycapnp on all (active) CI hosts which have IPC enabled and run the functional tests. Except for previous_releases, which uses an older version of pip that doesn't support --break-system-packages.
sipa force-pushed on Sep 5, 2025ryanofsky approvedryanofsky commented at 2:18 PM on September 5, 2025: contributorCode review ACK a341e11ac92b30aac856049c696a9ac39854569d. Changes since last review: rebasing, switching to miniwallet and expanding wallet test, improving pycapnp install steps in instructions and CI.
Looking at CI changes here, it's interesting that low-level way we configure CI jobs allows dependencies to be installed that might not be used, and things to be built that are not tested. I think a more ideal config / script boundary might make the config only responsible for specifying what should be tested, and let the script be responsible for figuring out what dependencies need to be installed. That's a wider issue though and CI changes here all look right from what I can tell.
re: libmultiprocess assert failure #33201 (comment), I opened https://github.com/bitcoin-core/libmultiprocess/issues/206 to improve this
TheCharlatan approvedTheCharlatan commented at 2:23 PM on September 5, 2025: contributorACK a341e11ac92b30aac856049c696a9ac39854569d
Sjors commented at 4:27 PM on September 5, 2025: memberACK a341e11ac92b30aac856049c696a9ac39854569d
As if by magic,
interface_ipc.pynow does run on the test each commit job.glozow merged this on Sep 5, 2025glozow closed this on Sep 5, 2025Sjors referenced this in commit 148ced15c3 on Sep 9, 2025alexanderwiederin referenced this in commit 49e068b15b on Sep 16, 2025alexanderwiederin referenced this in commit 4b0c2f2a8f on Sep 17, 2025alexanderwiederin referenced this in commit 2edb618ffe on Sep 17, 2025stringintech referenced this in commit fb8510ba20 on Sep 17, 2025bug-castercv502 referenced this in commit 9f5d12d027 on Sep 28, 2025yuvicc referenced this in commit d89c6d0002 on Sep 28, 2025
github-metadata-mirror
This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-02 12:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me
