Integrating VLS option on tests #8917
Open
enaples wants to merge 5 commits intoElementsProject:masterfrom
Open
Integrating VLS option on tests #8917enaples wants to merge 5 commits intoElementsProject:masterfrom
enaples wants to merge 5 commits intoElementsProject:masterfrom
Conversation
Collaborator
Author
|
On because some error on vls-side. I report logs for convenience: |
Collaborator
Author
|
WHAT SHOULD BE TAKEN INTO ACCOUNTHaving |
Member
Good catch, that we can only run a single node using the vls signer. However if we do not set the envvar, and set it in the proc.env field we can give each node their own signer. So don't set the envvar, set the proc.env as you spawn the signer in start() and it should wire things up correctly. For reference: |
cdecker
reviewed
Apr 1, 2026
Member
cdecker
left a comment
cdecker
left a comment
There was a problem hiding this comment.
And it looks like I forgot to Submit my review once again. Apologies for the delay 🙏
| class ValidatingLightningSignerD(TailableProc): | ||
| def __init__(self, vlsd_dir, vlsd_port, vlsd_rpc_port, node_id, network): | ||
| TailableProc.__init__(self, vlsd_dir, verbose=True) | ||
| self.executable = env("REMOTE_SIGNER_CMD", 'vlsd2') |
Member
There was a problem hiding this comment.
These are commonly called something_PATH cmd implies a full command including arguments.
|
|
||
| @pytest.mark.openchannel('v1') | ||
| @pytest.mark.openchannel('v2') | ||
| def test_vls_simple(node_factory): |
Member
There was a problem hiding this comment.
We probably want a marker for opt-in here: @pytest.mark.vls
This allows us to pick only allowlisted tests in the VLS test, and gives us time to go through the tests and see if they are supposed to work with VLS.
| }[self.vls_mode] | ||
|
|
||
| self.use_vlsd = subdaemon is not None | ||
| self.use_vls = use_vls is not None |
Member
There was a problem hiding this comment.
Technically this maps use_vls=False to self.use_vls=True
| self.daemon.env["VLS_LSS"] = os.environ.get("LSS_URI", "") | ||
| self.daemon.env["VLS_NETWORK"] = self.network | ||
| self.daemon.env["BITCOIND_RPC_URL"] = env("BITCOIND_RPC_URL", "http://rpcuser:rpcpass@127.0.0.1:{}".format(self.bitcoin.rpcport)) | ||
| self.daemon.env["VLS_CLN_VERSION"] = env("VLS_CLN_VERSION", "v25.12-391-gc1dc506-modded") |
Member
There was a problem hiding this comment.
Don't set a default value, error out if it is required and not set.
Member
There was a problem hiding this comment.
The default value will always be wrong, except for the one day the snapshot was taken.
| self.daemon.env["VLS_CLN_VERSION"] = env("VLS_CLN_VERSION", "v25.12-391-gc1dc506-modded") | ||
| self.daemon.env["BITCOIND_RPC_URL"] = env("BITCOIND_RPC_URL", f"http://{BITCOIND_CONFIG['rpcuser']}:{BITCOIND_CONFIG['rpcpassword']}@127.0.0.1:{self.bitcoin.rpcport}") | ||
| cln_version_str = subprocess.check_output([self.daemon.executable, "--version"]).decode('ascii').strip() | ||
| self.daemon.env["VLS_CLN_VERSION"] = env("VLS_CLN_VERSION", cln_version_str) |
| self.daemon.env["VLS_LSS"] = os.environ.get("LSS_URI", "") | ||
| self.daemon.env["VLS_NETWORK"] = self.network | ||
| self.daemon.env["VLS_LSS"] = env("LSS_URI", "") | ||
| self.daemon.env["VLS_NETWORK"] = env("VLS_NETWORK", self.network) |
Member
There was a problem hiding this comment.
I think these get inherited automatically, but good habit to explicitly pass them in.
enaples
force-pushed
the
vls-hsmd-test
branch
3 times, most recently
from
b1ccf87 to
0e943c8
Compare
Member
|
Rebased on top of |
madelinevibes
added
PLEASE clear CI 🫠
Status::Ready for Review
cdecker
reviewed
May 5, 2026
Member
cdecker
left a comment
cdecker
left a comment
There was a problem hiding this comment.
Quite a nice change, thanks @enaples. I am not sure why the tests are failing, but it does seem like it is particularly dodgy when working with signer-specific things. Can you rebase and see why they fail? Otherwise I think this is all on the right track 👍
Comment on lines
+45
to
+50
| if "vls" in item.keywords: | ||
| if not os.environ.get('REMOTE_SIGNER_PATH') and not os.environ.get('VLS_AUTO_BUILD'): | ||
| pytest.skip( | ||
| 'VLS test skipped: set REMOTE_SIGNER_PATH (path to pre-built vlsd) ' | ||
| 'or VLS_AUTO_BUILD=1 to enable' | ||
| ) |
Member
There was a problem hiding this comment.
This effectively partitions the tests into VLS and non-VLS. Whereas we want to have all tests run without VLS, but some be marked as also supporting VLS. We then create a nightly run that runs the test suite with VLS as the signer backend.
Collaborator
Author
There was a problem hiding this comment.
Got it, loud and clear. We didn't notice the nuance but now we think we (me and @ScuttoZ) fixed it.
Basically, in fixtures.py we defined another fixture that returns a boolean by checking if "vls" is among the markers, and uses that value as the option use_vls to spawn normal or VLS node. In this way, if "vls" marker isn't provided, use_vls will be set to False and the node will spawn as default.
Comment on lines
+71
to
+76
| elif use_vls is False: | ||
| self.vls_mode = "cln:native" | ||
| else: | ||
| # use_vls=None (default) falls back to the VLS_MODE env var. | ||
| # Setting this env var causes all nodes use the same mode | ||
| self.vls_mode = env("VLS_MODE", "cln:native") |
Member
There was a problem hiding this comment.
Good point, though I doubt we will want that amount of flexibility. It's already rather confusing: either we have a VLS signer or we have a native one, now if we selectively use VLS on certain nodes we would also need to indicate which ones, and which other ones should be using the native one.
Let's keep it all or nothing for now, and rather mark a couple of tests and non-VLS only that may work with one side as VLS, but not others.
| # FIXME: VLS doesn't implement WIRE_HSMD_SIGN_SPLICE_TX, so lightningd | ||
| # would fatal() during hsm_init if OPT_SPLICE (bit 62) is offered. | ||
| # Strip the optional splice bit (63 = OPTIONAL_FEATURE(62)). | ||
| self.daemon.opts["dev-force-features"] = "-63" |
Member
There was a problem hiding this comment.
This should likely be removed pretty soon as we are working with VLS to add support. Then once that support lands, we can go back to marking entire tests as VLS-enabled or not.
| import bitstring | ||
| from pyln.client import Millisatoshi | ||
| from pyln.testing.utils import EXPERIMENTAL_DUAL_FUND | ||
| from pyln.testing.utils import EXPERIMENTAL_DUAL_FUND, BITCOIND_CONFIG # noqa: F401 |
Member
There was a problem hiding this comment.
What is this change for? Wildcard import?
Collaborator
Author
There was a problem hiding this comment.
Exactly, this is required for the BITCOIN_RPC_URL env variable used by VLS
self.daemon.env["BITCOIND_RPC_URL"] = env(
"BITCOIND_RPC_URL",
f"http://{BITCOIND_CONFIG['rpcuser']}:{BITCOIND_CONFIG['rpcpassword']}@127.0.0.1:{self.bitcoin.rpcport}",
)
| logging.info(f"Building vlsd in {vlsd_dir}") | ||
| run(["cargo", "build", "--features", "developer"], | ||
| cwd=vlsd_dir, check=True, timeout=600) | ||
| return (vlsd_dir / "target" / "debug" / "vlsd").resolve() |
Member
There was a problem hiding this comment.
Quietly assumes CARGO_TARGET_DIR is not set.
| self.datadir.mkdir(exist_ok=True, parents=True) | ||
|
|
||
| self.bin_dir = str(_resolve_executable(self.datadir)) | ||
| self.executable = self.bin_dir + "/vlsd" |
Member
There was a problem hiding this comment.
Suggested change
| self.executable = self.bin_dir + "/vlsd" | |
| self.executable = self.bin_dir / "vlsd" |
pathlib.Path works great for path manipulation.
| # state between them. | ||
| self.env["ALLOWLIST"] = env( | ||
| "REMOTE_SIGNER_ALLOWLIST", | ||
| "contrib/remote_hsmd/TESTING_ALLOWLIST", |
Member
There was a problem hiding this comment.
This is relative to the CLN repo, not the VLS repo, so if the default value is chosen it will most likely point to a non-existent location.
Comment on lines
+69
to
+70
| f"--rpc-user=bitcoind", | ||
| f"--rpc-pass=bitcoind" |
Member
There was a problem hiding this comment.
The linter will most likely not like the f-strings without placeholders.
Add a `use_vls` per-node option to `LightningNode` that spawns a `vlsd` signer process per node and routes hsmd through `remote_hsmd_socket`. Each node gets its own datadir, ports, and signer env so multiple signers can run in parallel. - New `ValidatingLightningSignerD` (tests/vls.py) wraps vlsd as a TailableProc; resolves the binary from `REMOTE_SIGNER_PATH` or builds it via `VLS_AUTO_BUILD=1`. - `NodeFactory` is extended so `use_vls` reaches the node ctor as a kwarg instead of being forwarded as a lightningd CLI flag. - `dev-force-features=-63` is set on VLS-backed nodes to strip the optional splice bit, since VLS does not implement WIRE_HSMD_SIGN_SPLICE_TX and lightningd would otherwise fatal() during hsm_init. - `VLS_AUTOAPPROVE=1` is set on the vlsd process env so invoice preapproval doesn't decline payments in tests. - Three smoke tests added: send/receive/route through a VLS-backed node.
By default the full test suite (including tests marked `vls`) now
runs with `use_vls=False`, so contributors without a VLS signer can run
everything end-to-end. The signer is exercised only when the session is
started with exactly `-m vls`; in that case pytest aborts up front
unless `REMOTE_SIGNER_PATH` or `VLS_AUTO_BUILD` is set (the two are
mutually exclusive).
- conftest.py: gate the env-var check in pytest_configure on the
`-m vls` markexpr and drop the per-test skip in pytest_runtest_setup.
- fixtures.py: expose a `use_vls` fixture that resolves to True only
when `-m vls` is active.
- test_pay.py: thread that fixture through the vls-marked tests so
`{'use_vls': use_vls}` collapses to False in default runs.
…-relative path The previous default `contrib/remote_hsmd/TESTING_ALLOWLIST` was resolved relative to vlsd's cwd (the VLS repo), not the CLN repo where the file historically lived, so it silently pointed at a non-existent location. Drop the fallback: if `REMOTE_SIGNER_ALLOWLIST` is unset, log a warning and let vlsd start without an allowlist, leaving it to the operator to provide an absolute path when one is needed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Important
26.04 FREEZE March 11th: Non-bugfix PRs not ready by this date will wait for 26.06.
RC1 is scheduled on March 23rd
The final release is scheduled for April 15th.
Checklist
Before submitting the PR, ensure the following tasks are completed. If an item is not applicable to your PR, please mark it as checked:
tools/lightning-downgradeChangelog-None
Refactoring fixtures to test VLS implementation
The goal is to integrate VLS testing into the CLN test suite in a way that is self-contained — no changes to the
pyln-testinglibrary — and flexible enough to test different VLS implementations at different stages (different repos or commits) against the current CLN version, taking inspiration fromtools/reckless.The core idea is that
ValidatingLightningSignerDshould be generic: driven by the repos variable and optionally a target commit, it clones, builds and manages any conforming signer implementation. The fixture layer then decides which nodes use it and which implementation to test (so far only this is supported).