allow functional tests to be used externally with a dynamic signer factory #3016

devrandom · 2024-04-24T12:09:20Z

mut-global/src/lib.rs

codecov-commenter · 2024-04-24T13:31:37Z

Codecov Report

Attention: Patch coverage is 87.80037% with 66 lines in your changes missing coverage. Please review.

Project coverage is 88.30%. Comparing base (85d1e5f) to head (116379f).

Files with missing lines	Patch %	Lines
lightning-macros/src/lib.rs	71.42%	21 Missing and 1 partial ⚠️
lightning/src/util/dyn_signer.rs	86.90%	22 Missing ⚠️
lightning/src/util/test_channel_signer.rs	7.14%	13 Missing ⚠️
lightning/src/util/test_utils.rs	84.21%	3 Missing ⚠️
lightning/src/events/mod.rs	50.00%	2 Missing ⚠️
lightning/src/ln/onion_utils.rs	66.66%	2 Missing ⚠️
lightning/src/lib.rs	0.00%	1 Missing ⚠️
lightning/src/util/mut_global.rs	96.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3016      +/-   ##
==========================================
- Coverage   88.30%   88.30%   -0.01%     
==========================================
  Files         149      151       +2     
  Lines      112912   113204     +292     
  Branches   112912   113204     +292     
==========================================
+ Hits        99711    99964     +253     
- Misses      10716    10754      +38     
- Partials     2485     2486       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tnull · 2024-04-24T14:40:10Z

ext-test-macro/Cargo.toml

@@ -0,0 +1,14 @@
+[package]
+name = "ext-test-macro"


I wonder if we should call this more generally ldk-macros? Currently, we use bdk-macros to provide a dual blocking/async interface in lightning-transaction-sync. However, the bdk-macros crate is basically going away with BDK 1.0, so we might want to include the corresponding functionality here?

(cc @TheBlueMatt)

I recently opened #3330, IMO it might make sense to move the proc macros to lightning-macros eventually? Happy to rebase #3330 and move it there if this PR lands first though.

I'm fine with making this generic, I don't really have a strong opinion.

making it generic

devrandom · 2024-04-24T15:45:07Z

lightning/src/sign/mod.rs

@@ -727,6 +725,32 @@ impl HTLCDescriptor {
 /// A trait to handle Lightning channel key material without concretizing the channel type or
 /// the signature mechanism.
 pub trait ChannelSigner {
+	/// Returns the commitment seed for the channel.


maybe create a separate trait for these

Yea, sgtm, some kind of extension trait should do.

lightning/src/sign/mod.rs

lightning/src/util/mut_global.rs

devrandom · 2024-04-25T11:36:57Z

lightning/src/util/dyn_signer.rs

+        htlc: &HTLCOutputInCommitment,
+        secp_ctx: &Secp256k1<secp256k1::All>,
+    ) -> Result<Signature, ()>  {
+        EcdsaChannelSigner::sign_justice_revoked_htlc(&*self.inner, justice_tx, input, amount, per_commitment_key, htlc, secp_ctx)


name clashes are inconvenient, result in this need to disambiguate, and can't use delegate! macro

TheBlueMatt · 2024-05-08T14:50:35Z

lightning-invoice/src/utils.rs

-		let cross_node_seed = [44u8; 32];
-		chanmon_cfgs[1].keys_manager.backing = PhantomKeysManager::new(&seed_1, 43, 44, &cross_node_seed);
-		chanmon_cfgs[2].keys_manager.backing = PhantomKeysManager::new(&seed_2, 43, 44, &cross_node_seed);
+		chanmon_cfgs[1].keys_manager.backing = make_dyn_keys_interface(&seed_1);


I'm surprised these tests pass? The point is to have the same cross_node_seed across the nodes.

make_dyn_keys_interface does have a constant cross-node seed

TheBlueMatt · 2024-05-08T14:54:07Z

lightning/src/ln/channelmanager.rs

@@ -1981,9 +1978,6 @@ where
 	/// required to access the channel with the `counterparty_node_id`.
 	///
 	/// See `ChannelManager` struct-level documentation for lock order requirements.
-	#[cfg(not(test))]


In general I'd prefer to keep these kinds of things if possible. They're kinda verbose and dumb but I don't really want internal things leaking and someone starting to rely on them being usable across the crate.

ah, right. I put it back.

TheBlueMatt · 2024-05-08T14:57:24Z

lightning/src/sign/mod.rs

@@ -727,6 +725,32 @@ impl HTLCDescriptor {
 /// A trait to handle Lightning channel key material without concretizing the channel type or
 /// the signature mechanism.
 pub trait ChannelSigner {
+	/// Returns the commitment seed for the channel.


Yea, sgtm, some kind of extension trait should do.

TheBlueMatt · 2024-05-08T15:00:11Z

lightning/src/util/mod.rs

+pub mod dyn_signer;
+
+#[cfg(feature = "std")]
+pub mod mut_global;


Do these need to be visible outside of test builds?

TheBlueMatt · 2024-05-08T15:02:02Z

lightning/Cargo.toml

+delegate = "0.12.0"
+ext-test-macro = { path = "../ext-test-macro" }


Similar here, can we make these only a part of _test_utils builds only?

TheBlueMatt · 2024-05-08T15:02:55Z

lightning/src/util/dyn_signer.rs

+}
+
+impl NodeSigner for DynKeysInterface {
+	delegate! {


Honestly not convinced this actually saves enough typing to be worth it, but whatever.

more to reduce cognitive load when reading these things... but up to you.

How much work is it to just rebuild the macro? Seems like NIHing it would be pretty trivial too?

lightning/src/util/dyn_signer.rs

devrandom · 2024-09-24T21:32:31Z

one more idea. it would be nice to collect all the tests of a file into a static array. this would require one of the following to automatically collect the array:

putting the functional tests inside mod {}
or, using the inventory or linkme crates as a dev dependency
~~or, using lazy_static, but we'll need a second macro, e.g. xtest_collect and compile error if it's forgotten will be a bit confusing~~ this doesn't work because of constness requirements

devrandom · 2024-09-25T12:34:18Z

using the inventory or linkme crates as a dev dependency

I pushed a proposal

devrandom · 2024-09-25T14:30:45Z

lightning/src/ln/functional_tests.rs

@@ -94,8 +95,8 @@ fn test_channel_resumption_fail_post_funding() {
 	assert_eq!(nodes[0].node.get_and_clear_pending_msg_events(), Vec::new());
 }

-#[test]
-fn test_insane_channel_opens() {
+#[xtest(feature = "_test_utils")]


we could control the feature at the module level, reducing verbosity. the tradeoff is that we'll have to generate the inventory when testing locally, but maybe that's not significant.

I think its fine. It does read a bit funky, though - I'd read this as "this is a test only if _test_utils is set", not "this is a public test only if _test_utils is set". Should we just copy the gate verbatim and make things xtest(any(test, ...))?

so xtest(any(test, feature = "_test_utils")) ?

we would need to figure out semantics of #[xtest] without qualifiers - that would mean that it's always exposed, right?

Yea, I think so?

TheBlueMatt

Nice. Should be ready after the few comments here, I think. Mind cleaning up the commits to include more details in the commit message while you're at it?

lightning/src/util/dyn_signer.rs

TheBlueMatt · 2024-09-30T14:07:22Z

lightning/src/util/dyn_signer.rs

+}
+
+impl NodeSigner for DynKeysInterface {
+	delegate! {


How much work is it to just rebuild the macro? Seems like NIHing it would be pretty trivial too?

TheBlueMatt · 2024-09-30T14:09:36Z

lightning/Cargo.toml

@@ -49,10 +49,14 @@ regex = { version = "1.5.6", optional = true }
 backtrace = { version = "0.3", optional = true }

 libm = { version = "0.2", default-features = false }
+delegate = "0.12.0"
+ext-test-macro = { path = "../ext-test-macro", optional = true }
+inventory = { version = "0.3", optional = true  }


How much work would it be to just NIH this? Can we just have some global static Vec listing function pointers and names?

the issue is that you can't call non-const functions before main(), so it seems like you need to jump through the linker hacks inventory is doing. and those hacks are target specific, so it wouldn't make a lot of sense to maintain them ourselves.

a different approach would be to only inventory at the module level, requiring the mod to have an xtest macro on it. then the inventorying can happen inside the mod macro invocation.

Not quite sure I understand what the approach is that you're suggesting, but...sure?

since it's not possible to incrementally add to a global variable without inventory, I'm suggesting that tests are grouped into:

#[xtest] mod tests { ... }

and then the module level macro invocation will collect the tests inside the module in one swoop instead of incrementally.

This will require another level of mod inside the existing functional test mods. and it will also require adding a level of indent to a couple of thousand lines or so. Is that OK?

Ah, right, duh. I mean in principle we could even wrap the entire crate/top-level module in an #[xtest] no? We could also just do it on the funtional_tests/etc modules directly?

it has to be a single compilation unit. macros can't iterate on functions that are in another compilation unit, because they are not in the AST when the macro is invoked.

so what's the verdict? inventory or mod+indent everything?

I'm fine with the current inventory-based code, I think.

TheBlueMatt · 2024-09-30T14:14:57Z

lightning/src/sign/mod.rs

@@ -723,6 +722,35 @@ impl HTLCDescriptor {
 	}
 }

+/// Extension trait for [`ChannelSigner`] to add methods required for functional tests.


If its just for functional tests we should bound on _test_utils, but its also kinda nice to expose these things on our own InMemorySigner so maybe we should just say something about helpful utilities for InMemorySigner that provide more details which may be useful in tests?

see if you like the new wording

TheBlueMatt · 2024-09-30T14:15:37Z

lightning/src/util/dyn_signer.rs

@@ -0,0 +1,520 @@
+//! A dynamically dispatched signer


Care to break this out of the big xtest commit so that that commit is more mechanical?

devrandom · 2024-10-01T13:11:26Z

note that I didn't yet go over all the functional tests in the codebase and ensure they have the xtest macro. let me know if you want this right off.

TheBlueMatt · 2024-10-11T15:43:08Z

This basically LGTM I think.

Only outstanding items is #3016 (comment) (or some other way of removing the delegate dependency when not building with _test_utils) and #3016 (comment)

Either way feel free to clean up the git commits for merge.

note that I didn't yet go over all the functional tests in the codebase and ensure they have the xtest macro. let me know if you want this right off.

I don't really care, this is basically a feature for VLS so whatever y'all need.

devrandom · 2024-12-29T12:25:03Z

so I am seeing spurious BroadcastChannelAnnouncement messages causing functional tests failures when running the externalized tests. I think this might be an existing heisenbug, which is triggered here because of timing changes. see last commit.

never mind, was missing a cfg update

devrandom · 2024-12-29T12:37:49Z

I am making delegate optional for non-dev builds. is this what you wanted, or did you also want it to be optional for dev?

tnull · 2024-12-29T15:10:49Z

ldk-macros/Cargo.toml

@@ -0,0 +1,17 @@
+[package]
+name = "ldk-macros"


Note that we by now have the lightning-macros crate. IMO, you'll want to add the required macros there instead of creating yet another ldk-macros crate.

ah, OK, will do

devrandom · 2024-12-29T16:22:53Z

this is ready for final review

introduce TestSignerFactory

devrandom · 2024-12-29T17:47:56Z

getting some out-of-disk-space CI errors...

devrandom · 2024-12-29T21:07:33Z

should we run the xtest PoC only in one of the build variants, to reduce build time? or even make it a separate check?

~~I think it adds about 10 minutes to all the per-toolchain builds otherwise (66 minutes -> 76 minutes)~~

commit added for this, but it seems to run in only 3 minutes, so maybe it's not worth separating out...

devrandom · 2025-01-12T16:16:47Z

validated with VLS in https://gitlab.com/lightning-signer/validating-lightning-signer/-/merge_requests/731 and https://gitlab.com/lightning-signer/validating-lightning-signer/-/merge_requests/733

devrandom commented Apr 24, 2024

View reviewed changes

mut-global/src/lib.rs Outdated Show resolved Hide resolved

devrandom force-pushed the 2024-04-ext-test branch from 1a142e1 to 4077bc2 Compare April 24, 2024 14:39

tnull reviewed Apr 24, 2024

View reviewed changes

devrandom force-pushed the 2024-04-ext-test branch from 4077bc2 to 3a1986c Compare April 24, 2024 15:32

devrandom commented Apr 24, 2024

View reviewed changes

lightning/src/sign/mod.rs Show resolved Hide resolved

devrandom commented Apr 24, 2024

View reviewed changes

lightning/src/util/mut_global.rs Outdated Show resolved Hide resolved

devrandom force-pushed the 2024-04-ext-test branch 2 times, most recently from 9d4e233 to f341a0c Compare April 25, 2024 09:02

devrandom commented Apr 25, 2024

View reviewed changes

devrandom force-pushed the 2024-04-ext-test branch 6 times, most recently from 87aadef to 4efd374 Compare April 26, 2024 08:19

devrandom marked this pull request as ready for review April 26, 2024 08:20

devrandom force-pushed the 2024-04-ext-test branch 3 times, most recently from 3c454d0 to ef8f13d Compare April 27, 2024 11:39

TheBlueMatt reviewed May 8, 2024

View reviewed changes

devrandom force-pushed the 2024-04-ext-test branch 5 times, most recently from fd35254 to 2e2df96 Compare September 24, 2024 20:34

devrandom force-pushed the 2024-04-ext-test branch 2 times, most recently from 84abf70 to 0a29d64 Compare September 25, 2024 12:32

devrandom force-pushed the 2024-04-ext-test branch 2 times, most recently from 49a529e to 4a102fa Compare September 25, 2024 13:42

devrandom commented Sep 25, 2024

View reviewed changes

TheBlueMatt reviewed Sep 30, 2024

View reviewed changes

devrandom force-pushed the 2024-04-ext-test branch from bbfa2f0 to b1f0fc8 Compare October 1, 2024 13:00

devrandom force-pushed the 2024-04-ext-test branch from b1f0fc8 to 5c3be20 Compare December 6, 2024 17:09

devrandom force-pushed the 2024-04-ext-test branch 2 times, most recently from 82828d3 to 717e373 Compare December 29, 2024 12:23

devrandom force-pushed the 2024-04-ext-test branch 2 times, most recently from ec136e5 to e0a6864 Compare December 29, 2024 15:07

tnull reviewed Dec 29, 2024

View reviewed changes

devrandom force-pushed the 2024-04-ext-test branch from e0a6864 to a92c915 Compare December 29, 2024 16:22

devrandom added 6 commits December 29, 2024 17:25

macro for externally-visible tests

81c8f06

expose ser/de from lightning-invoice

1b1c694

mut-global utility

699125c

introduce DynSigner

55c2e6f

expose functional tests under _test_utils feature flag

f9ce0cc

introduce TestSignerFactory

run externalized tests in CI

5849f4b

devrandom force-pushed the 2024-04-ext-test branch from a92c915 to 5849f4b Compare December 29, 2024 17:48

f CI

116379f

devrandom force-pushed the 2024-04-ext-test branch from 52aeccc to 116379f Compare December 30, 2024 13:15

		delegate = "0.12.0"
		ext-test-macro = { path = "../ext-test-macro" }

allow functional tests to be used externally with a dynamic signer factory #3016

Are you sure you want to change the base?

allow functional tests to be used externally with a dynamic signer factory #3016

Conversation

devrandom commented Apr 24, 2024 • edited Loading

codecov-commenter commented Apr 24, 2024 • edited by codecov bot Loading

Codecov Report

tnull Apr 24, 2024 • edited Loading

Choose a reason for hiding this comment

tnull Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devrandom commented Sep 24, 2024 • edited Loading

devrandom commented Sep 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devrandom Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devrandom commented Oct 1, 2024

TheBlueMatt commented Oct 11, 2024

devrandom commented Dec 29, 2024 • edited Loading

devrandom commented Dec 29, 2024 • edited Loading

tnull Dec 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devrandom commented Dec 29, 2024

devrandom commented Dec 29, 2024 • edited Loading

devrandom commented Dec 29, 2024 • edited Loading

devrandom commented Jan 12, 2025

devrandom commented Apr 24, 2024 •

edited

Loading

codecov-commenter commented Apr 24, 2024 •

edited by codecov bot

Loading

tnull Apr 24, 2024 •

edited

Loading

tnull Oct 1, 2024 •

edited

Loading

devrandom commented Sep 24, 2024 •

edited

Loading

TheBlueMatt Oct 7, 2024 •

edited

Loading

devrandom Oct 10, 2024 •

edited

Loading

devrandom commented Dec 29, 2024 •

edited

Loading

devrandom commented Dec 29, 2024 •

edited

Loading

tnull Dec 29, 2024 •

edited

Loading

devrandom commented Dec 29, 2024 •

edited

Loading

devrandom commented Dec 29, 2024 •

edited

Loading