Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions .github/workflows/build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,20 @@ concurrency:
cancel-in-progress: true

jobs:
ext-test:
runs-on: ubuntu-latest
steps:
- name: Checkout source code
uses: actions/checkout@v4
- name: Install Rust stable toolchain
run: |
curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y --profile=minimal --default-toolchain stable
rustup override set stable
- name: Run externalized tests
run: |
cd ext-functional-test-demo
cargo test --verbose --color always
cargo test --verbose --color always --features test-broken
build:
strategy:
fail-fast: false
Expand Down
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -13,5 +13,6 @@ lightning-rapid-gossip-sync/res/full_graph.lngossip
lightning-custom-message/target
lightning-transaction-sync/target
lightning-dns-resolver/target
ext-functional-test-demo/target
no-std-check/target
msrv-no-dev-deps-check/target
1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ members = [

exclude = [
"lightning-transaction-sync",
"ext-functional-test-demo",
"no-std-check",
"msrv-no-dev-deps-check",
"bench",
Expand Down
10 changes: 10 additions & 0 deletions ext-functional-test-demo/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
[package]
name = "ext-functional-tester"
version = "0.1.0"
edition = "2021"

[features]
test-broken = []

[dependencies]
lightning = { path = "../lightning", features = ["_test_utils"] }
61 changes: 61 additions & 0 deletions ext-functional-test-demo/src/main.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
fn main() {
println!("{} tests were exported", lightning::get_xtests().len());
}

#[cfg(test)]
#[allow(unused)]
mod tests {
use lightning::ln::functional_tests::*;
use lightning::util::dyn_signer::{DynKeysInterfaceTrait, DynSigner};
use lightning::util::test_utils::{TestSignerFactory, SIGNER_FACTORY};
use std::panic::catch_unwind;
use std::sync::Arc;
use std::time::Duration;

struct BrokenSignerFactory();

impl TestSignerFactory for BrokenSignerFactory {
fn make_signer(
&self, _seed: &[u8; 32], _now: Duration,
) -> Box<dyn DynKeysInterfaceTrait<EcdsaSigner = DynSigner>> {
panic!()
}
}

#[cfg(feature = "test-broken")]
#[test]
fn test_broken() {
SIGNER_FACTORY.set(Arc::new(BrokenSignerFactory()));
catch_unwind(|| fake_network_test()).unwrap_err();
}

#[cfg(not(feature = "test-broken"))]
#[test]
fn test_default_one() {
test_htlc_on_chain_success();
}

#[cfg(not(feature = "test-broken"))]
#[test]
fn test_default_all() {
let mut failed_tests = Vec::new();
for test in lightning::get_xtests() {
print!("Running test: {}", test.test_name);
let mut pass = catch_unwind(|| (test.test_fn)()).is_ok();
if test.should_panic {
pass = !pass;
}
if !pass {
failed_tests.push(test.test_name);
}
}
if !failed_tests.is_empty() {
println!("Failed tests:");
for test in failed_tests.iter() {
println!("- {}", test);
}
}
println!("Done with {} failures", failed_tests.len());
assert!(failed_tests.is_empty());
}
}
3 changes: 3 additions & 0 deletions fuzz/src/chanmon_consistency.rs
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ use bitcoin::secp256k1::schnorr;
use bitcoin::secp256k1::{self, Message, PublicKey, Scalar, Secp256k1, SecretKey};

use lightning::io::Cursor;
use lightning::util::dyn_signer::DynSigner;
use std::cmp::{self, Ordering};
use std::collections::VecDeque;
use std::mem;
Expand Down Expand Up @@ -391,6 +392,7 @@ impl SignerProvider for KeyProvider {
channel_keys_id,
);
let revoked_commitment = self.make_enforcement_state_cell(keys.commitment_seed);
let keys = DynSigner::new(keys);
TestChannelSigner::new_with_revoked(keys, revoked_commitment, false)
}

Expand All @@ -399,6 +401,7 @@ impl SignerProvider for KeyProvider {

let inner: InMemorySigner = ReadableArgs::read(&mut reader, self)?;
let state = self.make_enforcement_state_cell(inner.commitment_seed);
let inner = DynSigner::new(inner);

Ok(TestChannelSigner::new_with_revoked(inner, state, false))
}
Expand Down
6 changes: 4 additions & 2 deletions fuzz/src/full_stack.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ use bitcoin::secp256k1::ecdsa::{RecoverableSignature, Signature};
use bitcoin::secp256k1::schnorr;
use bitcoin::secp256k1::{self, Message, PublicKey, Scalar, Secp256k1, SecretKey};

use lightning::util::dyn_signer::DynSigner;
use std::cell::RefCell;
use std::cmp;
use std::sync::atomic::{AtomicBool, AtomicU64, AtomicUsize, Ordering};
Expand Down Expand Up @@ -447,7 +448,7 @@ impl SignerProvider for KeyProvider {
let ctr = channel_keys_id[0];
let (inbound, state) = self.signer_state.borrow().get(&ctr).unwrap().clone();
TestChannelSigner::new_with_revoked(
if inbound {
DynSigner::new(if inbound {
InMemorySigner::new(
&secp_ctx,
SecretKey::from_slice(&[
Expand Down Expand Up @@ -519,7 +520,7 @@ impl SignerProvider for KeyProvider {
channel_keys_id,
channel_keys_id,
)
},
}),
state,
false,
)
Expand All @@ -528,6 +529,7 @@ impl SignerProvider for KeyProvider {
fn read_chan_signer(&self, mut data: &[u8]) -> Result<TestChannelSigner, DecodeError> {
let inner: InMemorySigner = ReadableArgs::read(&mut data, self)?;
let state = Arc::new(Mutex::new(EnforcementState::new()));
let inner = DynSigner::new(inner);

Ok(TestChannelSigner::new_with_revoked(inner, state, false))
}
Expand Down
4 changes: 4 additions & 0 deletions lightning-background-processor/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2550,6 +2550,8 @@ mod tests {
failure: PathFailure::OnPath { network_update: None },
path: path.clone(),
short_channel_id: Some(scored_scid),
error_code: None,
error_data: None,
});
let event = $receive.expect("PaymentPathFailed not handled within deadline");
match event {
Expand All @@ -2567,6 +2569,8 @@ mod tests {
failure: PathFailure::OnPath { network_update: None },
path: path.clone(),
short_channel_id: None,
error_code: None,
error_data: None,
});
let event = $receive.expect("PaymentPathFailed not handled within deadline");
match event {
Expand Down
7 changes: 5 additions & 2 deletions lightning-macros/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,12 @@ proc-macro = true
[features]

[dependencies]
syn = { version = "2.0.77", default-features = false, features = ["parsing", "printing", "proc-macro", "full"] }
proc-macro2 = { version = "1.0.86", default-features = false, features = ["proc-macro"] }
syn = { version = "2.0", default-features = false, features = ["parsing", "printing", "proc-macro", "full"] }
proc-macro2 = { version = "1.0", default-features = false, features = ["proc-macro"] }
quote = { version = "1.0", default-features = false, features = ["proc-macro"] }

[dev-dependencies]
inventory = "0.3"

[lints]
workspace = true
138 changes: 138 additions & 0 deletions lightning-macros/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,15 @@
#![deny(rustdoc::private_intra_doc_links)]
#![cfg_attr(docsrs, feature(doc_auto_cfg))]

extern crate alloc;

use alloc::string::ToString;
use proc_macro::TokenStream;
use proc_macro2::TokenStream as TokenStream2;
use quote::quote;
use syn::spanned::Spanned;
use syn::{parse, ImplItemFn, Token};
use syn::{parse_macro_input, Item};

fn add_async_method(mut parsed: ImplItemFn) -> TokenStream {
let output = quote! {
Expand Down Expand Up @@ -74,3 +79,136 @@ pub fn maybe_await(expr: TokenStream) -> TokenStream {

quoted.into()
}

/// An exposed test. This is a test that will run locally and also be
/// made available to other crates that want to run it in their own context.
///
/// For example:
/// ```rust
/// use lightning_macros::xtest;
///
/// fn f1() {}
///
/// #[xtest(feature = "_test_utils")]
/// pub fn test_f1() {
/// f1();
/// }
/// ```
///
/// May also be applied to modules, like so:
///
/// ```rust
/// use lightning_macros::xtest;
///
/// #[xtest(feature = "_test_utils")]
/// pub mod tests {
/// use super::*;
///
/// fn f1() {}
///
/// #[xtest]
/// pub fn test_f1() {
/// f1();
/// }
/// }
/// ```
///
/// Which will include the module if we are testing or the `_test_utils` feature
/// is on.
#[proc_macro_attribute]
pub fn xtest(attrs: TokenStream, item: TokenStream) -> TokenStream {
let attrs = parse_macro_input!(attrs as TokenStream2);
let input = parse_macro_input!(item as Item);

let expanded = match input {
Item::Mod(item_mod) => {
let cfg = if attrs.is_empty() {
quote! { #[cfg_attr(test, test)] }
} else {
quote! { #[cfg_attr(test, test)] #[cfg(any(test, #attrs))] }
};
quote! {
#cfg
#item_mod
}
},
Item::Fn(item_fn) => {
let (cfg_attr, submit_attr) = if attrs.is_empty() {
(quote! { #[cfg_attr(test, test)] }, quote! { #[cfg(not(test))] })
} else {
(
quote! { #[cfg_attr(test, test)] #[cfg(any(test, #attrs))] },
quote! { #[cfg(all(not(test), #attrs))] },
)
};

// Check that the function doesn't take args and returns nothing
if !item_fn.sig.inputs.is_empty()
|| !matches!(item_fn.sig.output, syn::ReturnType::Default)
{
return syn::Error::new_spanned(
item_fn.sig,
"xtest functions must not take arguments and must return nothing",
)
.to_compile_error()
.into();
}

// Check for #[should_panic] attribute
let should_panic =
item_fn.attrs.iter().any(|attr| attr.path().is_ident("should_panic"));

let fn_name = &item_fn.sig.ident;
let fn_name_str = fn_name.to_string();
quote! {
#cfg_attr
#item_fn

// We submit the test to the inventory only if we're not actually testing
#submit_attr
inventory::submit! {
crate::XTestItem {
test_fn: #fn_name,
test_name: #fn_name_str,
should_panic: #should_panic,
}
}
}
},
_ => {
return syn::Error::new_spanned(
input,
"xtest can only be applied to functions or modules",
)
.to_compile_error()
.into();
},
};

TokenStream::from(expanded)
}

/// Collects all externalized tests marked with `#[xtest]`
/// into a vector of `XTestItem`s. This vector can be
/// retrieved by calling `get_xtests()`.
#[proc_macro]
pub fn xtest_inventory(_input: TokenStream) -> TokenStream {
let expanded = quote! {
/// An externalized test item, including the test function, name, and whether it is marked with `#[should_panic]`.
pub struct XTestItem {
pub test_fn: fn(),
pub test_name: &'static str,
pub should_panic: bool,
}

inventory::collect!(XTestItem);

pub fn get_xtests() -> Vec<&'static XTestItem> {
inventory::iter::<XTestItem>
.into_iter()
.collect()
}
};

TokenStream::from(expanded)
}
8 changes: 6 additions & 2 deletions lightning/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,7 @@ rustdoc-args = ["--cfg", "docsrs"]

[features]
# Internal test utilities exposed to other repo crates
_test_utils = ["regex", "bitcoin/bitcoinconsensus", "lightning-types/_test_utils"]

_test_utils = ["regex", "bitcoin/bitcoinconsensus", "lightning-macros", "lightning-types/_test_utils", "inventory", "delegate"]
# Allow signing of local transactions that may have been revoked or will be revoked, for functional testing (e.g. justice tx handling).
# This is unsafe to use in production because it may result in the counterparty publishing taking our funds.
unsafe_revoked_tx_signing = []
Expand Down Expand Up @@ -47,10 +46,15 @@ regex = { version = "1.5.6", optional = true }
backtrace = { version = "0.3", optional = true }

libm = { version = "0.2", default-features = false }
delegate = { version = "0.13.1", optional = true }
lightning-macros = { path = "../lightning-macros", optional = true }
inventory = { version = "0.3", optional = true }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@devrandom devrandom Oct 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, damn.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


[dev-dependencies]
regex = "1.5.6"
lightning-types = { version = "0.1.0", path = "../lightning-types", features = ["_test_utils"] }
lightning-macros = { path = "../lightning-macros" }
delegate = { version = "0.13.1" }

[dev-dependencies.bitcoin]
version = "0.32.2"
Expand Down
2 changes: 1 addition & 1 deletion lightning/src/chain/chainmonitor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -471,7 +471,7 @@ where C::Target: chain::Filter,
}


#[cfg(test)]
#[cfg(any(test, feature = "_test_utils"))]
pub fn remove_monitor(&self, funding_txo: &OutPoint) -> ChannelMonitor<ChannelSigner> {
self.monitors.write().unwrap().remove(funding_txo).unwrap().monitor
}
Expand Down
Loading
Loading