Refactor async signing test utils to toggle specific method availability#3115
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 #3115 +/- ##
==========================================
- Coverage 89.91% 89.79% -0.12%
==========================================
Files 121 119 -2
Lines 99172 97915 -1257
Branches 99172 97915 -1257
==========================================
- Hits 89167 87922 -1245
- Misses 7405 7428 +23
+ Partials 2600 2565 -35 ☔ View full report in Codecov by Sentry. |
alecchendev
force-pushed
the
2024-06-specific-async-sign
branch
2 times, most recently
from
ff75d8f to
3785375
Compare
alecchendev
left a comment
alecchendev
left a comment
There was a problem hiding this comment.
Notes for myself to fix soon
lightning/src/util/test_utils.rs
Outdated
| /// Holds a list of signer ops to disable for the next signer created by this interface. | ||
| pub disable_next_signer_ops: Mutex<HashSet<SignerOp>>, |
There was a problem hiding this comment.
remove next here since it just applies until you remove it?
There was a problem hiding this comment.
moving to separate PR
|
accidentally included previous commits, rebased to fix it. also have some nits/cleanups + a build issue i'll fix soon |
alecchendev
force-pushed
the
2024-06-specific-async-sign
branch
from
3785375 to
3ea35f8
Compare
|
squashed immediately since the changes were getting unreadable. Previously i included a change here to change how we disable a signer upon |
alecchendev
force-pushed
the
2024-06-specific-async-sign
branch
4 times, most recently
from
4d96e0c to
574bd6e
Compare
alecchendev
force-pushed
the
2024-06-specific-async-sign
branch
from
574bd6e to
21eeca4
Compare
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
Seems fine to me. We can always tweak test utils later anyway.
| SignChannelAnnouncementWithFundingKey, | ||
| } | ||
|
|
||
| impl fmt::Display for SignerOp { |
There was a problem hiding this comment.
nit: why bother? Debug should be fine?
| pub available: Arc<Mutex<bool>>, | ||
| /// Set of signer operations that are disabled. If an operation is disabled, | ||
| /// the signer will return `Err` when the corresponding method is called. | ||
| pub disabled_signer_ops: Arc<Mutex<HashSet<SignerOp>>>, |
There was a problem hiding this comment.
Not really specific to this PR, but we could move this into EnforcementState so that we get a common disable set across ChannelMonitor and ChannelManager for free.
| check_closed_event(&nodes[1], 1, ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(true) }, false, &[nodes[0].node.get_our_node_id()], 100_000); | ||
| } else { | ||
| nodes[0].disable_channel_signer_op(&nodes[1].node.get_our_node_id(), &chan_id, SignerOp::SignHolderCommitment); | ||
| nodes[0].disable_channel_signer_op(&nodes[1].node.get_our_node_id(), &chan_id, SignerOp::SignHolderHtlcTransaction); |
There was a problem hiding this comment.
For convenience we'll probably want some wrapper methods that set/clear everything, no matter the internal representation.
|
Its just test code, there's no reason to hold this up on another reviewer. |
3 participants
Another pre-requisite PR to help shrink async signing diffs. To test async signing, we want be able to control whether our test channel signer returns a valid signature versus no signature. Previously we just had a catch-all flag that would toggle the signer being available/not across all signing methods. In upcoming PRs we'll want to test individual methods becoming available at different times (e.g. we made multiple requests to a remote signer, and got responses back in various orders). This PR refactors our existing tests and test utils to implement this.