-
Notifications
You must be signed in to change notification settings - Fork 363
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
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #3016 +/- ##
==========================================
+ Coverage 89.15% 89.83% +0.68%
==========================================
Files 118 121 +3
Lines 97678 102001 +4323
Branches 97678 102001 +4323
==========================================
+ Hits 87080 91637 +4557
+ Misses 8357 8166 -191
+ Partials 2241 2198 -43 ☔ View full report in Codecov by Sentry. |
1a142e1
to
4077bc2
Compare
@@ -0,0 +1,14 @@ | |||
[package] | |||
name = "ext-test-macro" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
4077bc2
to
3a1986c
Compare
@@ -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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe create a separate trait for these
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, sgtm, some kind of extension trait should do.
9d4e233
to
f341a0c
Compare
lightning/src/util/dyn_signer.rs
Outdated
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
name clashes are inconvenient, result in this need to disambiguate, and can't use delegate!
macro
87aadef
to
4efd374
Compare
3c454d0
to
ef8f13d
Compare
lightning-invoice/src/utils.rs
Outdated
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised these tests pass? The point is to have the same cross_node_seed across the nodes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make_dyn_keys_interface
does have a constant cross-node seed
@@ -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))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, right. I put it back.
@@ -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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, sgtm, some kind of extension trait should do.
lightning/src/util/mod.rs
Outdated
pub mod dyn_signer; | ||
|
||
#[cfg(feature = "std")] | ||
pub mod mut_global; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do these need to be visible outside of test builds?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
lightning/Cargo.toml
Outdated
delegate = "0.12.0" | ||
ext-test-macro = { path = "../ext-test-macro" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar here, can we make these only a part of _test_utils
builds only?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
} | ||
|
||
impl NodeSigner for DynKeysInterface { | ||
delegate! { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly not convinced this actually saves enough typing to be worth it, but whatever.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more to reduce cognitive load when reading these things... but up to you.
lightning/src/util/dyn_signer.rs
Outdated
|
||
#[cfg(not(taproot))] | ||
/// A super-trait for all the traits that a dyn signer backing implements | ||
pub trait DynSignerTrait: EcdsaChannelSigner + Send + Sync {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this all because we need Writeable
for the channelsigner? I think we can remove the Writeable
bound entirely now! We started ignoring the bytes read in OnchainTx
in 0.0.113, and according to our release notes LDK objects written by 0.0.119 are no longer expected to be readable in 113 anyway, so we can just wholesale remove the Writeable
bound and move on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the connection you are making to Writeable
? it's needed by InnerSign
, which is used by DynSigner
fd35254
to
2e2df96
Compare
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:
|
84abf70
to
0a29d64
Compare
I pushed a proposal |
introduce DynSigner and TestSignerFactory
0a29d64
to
49a529e
Compare
49a529e
to
4a102fa
Compare
@@ -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")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
DynSigner
xtest
macros