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

[devrandom]

• [x] dynamic signer factory
• [x] DynSigner
• [x] apply xtest macros
• [x] PoC external usage

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 81.54206% with 79 lines in your changes missing coverage. Please review.

Project coverage is 88.56%. Comparing base (6cf270d) to head (21098e6).

Files with missing lines	Patch %	Lines
lightning-macros/src/lib.rs	45.45%	41 Missing and 1 partial :warning:
lightning/src/util/dyn_signer.rs	68.62%	16 Missing :warning:
lightning/src/util/test_channel_signer.rs	7.14%	13 Missing :warning:
lightning/src/util/test_utils.rs	85.71%	3 Missing :warning:
lightning/src/events/mod.rs	50.00%	2 Missing :warning:
lightning/src/ln/onion_utils.rs	66.66%	2 Missing :warning:
lightning/src/util/mut_global.rs	96.66%	1 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3016      +/-   ##
==========================================
- Coverage   88.61%   88.56%   -0.06%     
==========================================
  Files         149      152       +3     
  Lines      117091   117270     +179     
  Branches   117091   117270     +179     
==========================================
+ Hits       103765   103863      +98     
- Misses      10817    10889      +72     
- Partials     2509     2518       +9     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[devrandom]

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]

• using the inventory or linkme crates as a dev dependency

I pushed a proposal

[devrandom]

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]

This basically LGTM I think.

Only outstanding items is https://github.com/lightningdevkit/rust-lightning/pull/3016#discussion_r1781207030 (or some other way of removing the delegate dependency when not building with _test_utils) and https://github.com/lightningdevkit/rust-lightning/pull/3016#discussion_r1578011334

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]

~~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]

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?

[devrandom]

this is ready for final review

[devrandom]

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

[devrandom]

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]

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]

rebased

[TheBlueMatt]

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?

Ugh, sorry I missed this, I'm still not clear on how much it would take to NIH this, it looks pretty easy for the simple stuff we need - https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=9336573a1a8f019c83c31fe1a35d5bf7

[devrandom]

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?

Ugh, sorry I missed this, I'm still not clear on how much it would take to NIH this, it looks pretty easy for the simple stuff we need - https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=9336573a1a8f019c83c31fe1a35d5bf7

This seems close, but I don't see how to handle additional code in the impl - for example, impl SignerProvider for X requires:

	type EcdsaSigner = DynSigner;
	#[cfg(taproot)]
	type TaprootSigner = DynSigner;

and then there's also &mut self fns to handle.

[TheBlueMatt]

Hmm, harder to get it to read quite as great but its still quite doable - https://play.rust-lang.org/?version=stable&mode=release&edition=2021&gist=77fc7ed667016785cb760200f33585a1

[devrandom]

cool, didn't realize macro_rules had that flexibility

[devrandom]

done and rebased.

there was one instance that a fn had a body, but I just expanded it for now.

[TheBlueMatt]

Also CI is very sad.

[devrandom]

(used the UI to merge, but will rebase if/when everything else is done)

[wpaulino]

LGTM, the commit messages could be cleaned up though to follow our guidelines

[devrandom]

LGTM, the commit messages could be cleaned up though to follow our guidelines

done

[TheBlueMatt]

Ugh sorry this needs rebase after #3604. Should clean a few things up nicely though.

[devrandom]

no problem, rebase done