Refactor async signing test utils to toggle specific method availability - #3115

Merged
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom
alecchendev:2024-06-specific-async-sign
Jun 18, 2024
Merged

Refactor async signing test utils to toggle specific method availability#3115
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom
alecchendev:2024-06-specific-async-sign

Conversation

@alecchendev

@alecchendevalecchendev commented Jun 11, 2024•
edited
Loading

Copy link
Copy Markdown
Contributor

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.

@codecov-commenter

codecov-commenter commented Jun 11, 2024•
edited
Loading

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 28.57143% with 40 lines in your changes missing coverage. Please review.

Project coverage is 89.79%. Comparing base (07f3380) to head (7e2789e).

❗ Current head 7e2789e differs from pull request most recent head 3785375

Please upload reports for the commit 3785375 to get more accurate results.

FilesPatch %Lines
lightning/src/util/test_channel_signer.rs30.43%32 Missing ⚠️
lightning/src/ln/functional_test_utils.rs0.00%6 Missing ⚠️
lightning/src/util/test_utils.rs50.00%0 Missing and 2 partials ⚠️

❗ 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.
📢 Have feedback on the report? Share it here.

@alecchendevalecchendev mentioned this pull request Jun 11, 2024
Merged
TheBlueMatt
TheBlueMatt reviewed Jun 11, 2024
View reviewed changes
Comment threadlightning/src/ln/functional_test_utils.rs Outdated
Comment threadlightning/src/ln/async_signer_tests.rs Outdated
Comment threadlightning/src/ln/async_signer_tests.rs Outdated
Comment threadlightning/src/ln/async_signer_tests.rs Outdated
Comment threadlightning/src/util/test_channel_signer.rs Outdated
@alecchendev
alecchendevforce-pushed the 2024-06-specific-async-sign branch 2 times, most recently from ff75d8f to 3785375CompareJune 15, 2024 00:49
alecchendev
alecchendev commented Jun 15, 2024
View reviewed changes

@alecchendevalecchendev left a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Notes for myself to fix soon

Comment threadlightning/src/chain/channelmonitor.rs Outdated
Comment threadlightning/src/ln/channel.rs
Comment threadlightning/src/ln/functional_test_utils.rs Outdated
Comment threadlightning/src/util/test_utils.rs Outdated
Comment on lines +1219 to +1220
/// Holds a list of signer ops to disable for the next signer created by this interface.
pub disable_next_signer_ops: Mutex<HashSet<SignerOp>>,

@alecchendevalecchendevJun 15, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

remove next here since it just applies until you remove it?

@alecchendevalecchendevJun 17, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

moving to separate PR

Comment threadlightning/src/util/test_channel_signer.rs
Comment threadlightning/src/ln/mod.rs Outdated
@alecchendev

alecchendev commented Jun 15, 2024

Copy link
Copy Markdown
ContributorAuthor

accidentally included previous commits, rebased to fix it. also have some nits/cleanups + a build issue i'll fix soon

@alecchendev
alecchendevforce-pushed the 2024-06-specific-async-sign branch from 3785375 to 3ea35f8CompareJune 17, 2024 21:30
@alecchendev

alecchendev commented Jun 17, 2024•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

squashed immediately since the changes were getting unreadable. Previously i included a change here to change how we disable a signer upon derive_channel_signer for channels that haven't be created yet, but I decided to move that to a separate PR, so this one is more focused. The other big change is instead of using a mask, I thought it was simpler to just use an enum for the signer ops

@alecchendev
alecchendevforce-pushed the 2024-06-specific-async-sign branch 4 times, most recently from 4d96e0c to 574bd6eCompareJune 17, 2024 22:57
@alecchendev
alecchendevforce-pushed the 2024-06-specific-async-sign branch from 574bd6e to 21eeca4CompareJune 17, 2024 23:36
TheBlueMatt
TheBlueMatt approved these changes Jun 18, 2024
View reviewed changes

@TheBlueMattTheBlueMatt left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Seems fine to me. We can always tweak test utils later anyway.

Comment threadlightning/src/util/test_channel_signer.rs
SignChannelAnnouncementWithFundingKey,
}

impl fmt::Display for SignerOp {

@TheBlueMattTheBlueMattJun 18, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: why bother? Debug should be fine?

Comment threadlightning/src/util/test_channel_signer.rs
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>>>,

@TheBlueMattTheBlueMattJun 18, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

Comment threadlightning/src/ln/async_signer_tests.rs
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);

@TheBlueMattTheBlueMattJun 18, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

For convenience we'll probably want some wrapper methods that set/clear everything, no matter the internal representation.

@TheBlueMatt

TheBlueMatt commented Jun 18, 2024

Copy link
Copy Markdown
Collaborator

Its just test code, there's no reason to hold this up on another reviewer.

@TheBlueMatt
TheBlueMatt merged commit c39ff87 into lightningdevkit:mainJun 18, 2024
@alecchendevalecchendev mentioned this pull request Jul 1, 2024
Merged
@TheBlueMattTheBlueMatt mentioned this pull request Jan 30, 2025
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TheBlueMattTheBlueMattTheBlueMatt approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@alecchendev@codecov-commenter@TheBlueMatt
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
Refactor async signing test utils to toggle specific method availability - #3115
Merged
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
alecchendev:2024-06-specific-async-sign
Jun 18, 2024
Merged
Refactor async signing test utils to toggle specific method availability#3115
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
alecchendev:2024-06-specific-async-sign
Conversation
@alecchendev
@alecchendevalecchendev
commented
Jun 11, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
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.
@codecov-commenter
codecov-commenter
commented
Jun 11, 2024•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Attention: Patch coverage is 28.57143% with 40 lines in your changes missing coverage. Please review.
Project coverage is 89.79%. Comparing base (07f3380) to head (7e2789e).
❗ Current head 7e2789e differs from pull request most recent head 3785375
Please upload reports for the commit 3785375 to get more accurate results.
FilesPatch %Lines
lightning/src/util/test_channel_signer.rs30.43%32 Missing ⚠️
lightning/src/ln/functional_test_utils.rs0.00%6 Missing ⚠️
lightning/src/util/test_utils.rs50.00%0 Missing and 2 partials ⚠️
❗ 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.

📢 Have feedback on the report? Share it here.
@alecchendevalecchendev
mentioned this pull request
Jun 11, 2024
 Merged

TheBlueMatt
TheBlueMatt
reviewed
Jun 11, 2024
View reviewed changes
Comment threadlightning/src/ln/functional_test_utils.rs
Outdated

Comment threadlightning/src/ln/async_signer_tests.rs
Outdated

Comment threadlightning/src/ln/async_signer_tests.rs
Outdated

Comment threadlightning/src/ln/async_signer_tests.rs
Outdated

Comment threadlightning/src/util/test_channel_signer.rs
Outdated

@alecchendev
alecchendevforce-pushed
the
2024-06-specific-async-sign
branch
2 times, most recently
from
ff75d8f to
3785375CompareJune 15, 2024 00:49
alecchendev
alecchendev
commented
Jun 15, 2024
View reviewed changes
@alecchendevalecchendev
left a comment


Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Notes for myself to fix soon
Comment threadlightning/src/chain/channelmonitor.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Comment threadlightning/src/ln/functional_test_utils.rs
Outdated

Comment threadlightning/src/util/test_utils.rs
Outdated


Comment on lines

+1219
 to 
+1220

	/// Holds a list of signer ops to disable for the next signer created by this interface.
	pub disable_next_signer_ops: Mutex<HashSet<SignerOp>>,
@alecchendevalecchendevJun 15, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

remove next here since it just applies until you remove it?
@alecchendevalecchendevJun 17, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

moving to separate PR
Comment threadlightning/src/util/test_channel_signer.rs
Comment threadlightning/src/ln/mod.rs
Outdated

@alecchendev
alecchendev
commented
Jun 15, 2024

Copy link


Copy Markdown

ContributorAuthor
accidentally included previous commits, rebased to fix it. also have some nits/cleanups + a build issue i'll fix soon
@alecchendev
alecchendevforce-pushed
the
2024-06-specific-async-sign
branch
from
3785375 to
3ea35f8CompareJune 17, 2024 21:30
@alecchendev
alecchendev
commented
Jun 17, 2024•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
squashed immediately since the changes were getting unreadable. Previously i included a change here to change how we disable a signer upon derive_channel_signer for channels that haven't be created yet, but I decided to move that to a separate PR, so this one is more focused. The other big change is instead of using a mask, I thought it was simpler to just use an enum for the signer ops
@alecchendev
alecchendevforce-pushed
the
2024-06-specific-async-sign
branch
4 times, most recently
from
4d96e0c to
574bd6eCompareJune 17, 2024 22:57
@alecchendev
alecchendevforce-pushed
the
2024-06-specific-async-sign
branch
from
574bd6e to
21eeca4CompareJune 17, 2024 23:36
TheBlueMatt
TheBlueMatt
approved these changes
Jun 18, 2024
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

Seems fine to me. We can always tweak test utils later anyway.
Comment threadlightning/src/util/test_channel_signer.rs
	SignChannelAnnouncementWithFundingKey,
}

impl fmt::Display for SignerOp {
@TheBlueMattTheBlueMattJun 18, 2024

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

nit: why bother? Debug should be fine?
Comment threadlightning/src/util/test_channel_signer.rs
	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>>>,
@TheBlueMattTheBlueMattJun 18, 2024

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

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.
Comment threadlightning/src/ln/async_signer_tests.rs
 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);
@TheBlueMattTheBlueMattJun 18, 2024

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

For convenience we'll probably want some wrapper methods that set/clear everything, no matter the internal representation.
@TheBlueMatt
TheBlueMatt
commented
Jun 18, 2024

Copy link


Copy Markdown

Collaborator
Its just test code, there's no reason to hold this up on another reviewer.
@TheBlueMatt
TheBlueMatt
merged commit c39ff87
into
lightningdevkit:mainJun 18, 2024
@alecchendevalecchendev
mentioned this pull request
Jul 1, 2024
 Merged

@TheBlueMattTheBlueMatt
mentioned this pull request
Jan 30, 2025
 Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@TheBlueMattTheBlueMattTheBlueMatt approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@alecchendev@codecov-commenter@TheBlueMatt

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Refactor async signing test utils to toggle specific method availability - #3115
Merged
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
alecchendev:2024-06-specific-async-sign
Jun 18, 2024
Merged
Refactor async signing test utils to toggle specific method availability#3115
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
alecchendev:2024-06-specific-async-sign
Conversation
@alecchendev
@alecchendevalecchendev
commented
Jun 11, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
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.
@codecov-commenter
codecov-commenter
commented
Jun 11, 2024•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Attention: Patch coverage is 28.57143% with 40 lines in your changes missing coverage. Please review.
Project coverage is 89.79%. Comparing base (07f3380) to head (7e2789e).
❗ Current head 7e2789e differs from pull request most recent head 3785375
Please upload reports for the commit 3785375 to get more accurate results.
FilesPatch %Lines
lightning/src/util/test_channel_signer.rs30.43%32 Missing ⚠️
lightning/src/ln/functional_test_utils.rs0.00%6 Missing ⚠️
lightning/src/util/test_utils.rs50.00%0 Missing and 2 partials ⚠️
❗ 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.

📢 Have feedback on the report? Share it here.
@alecchendevalecchendev
mentioned this pull request
Jun 11, 2024
 Merged

TheBlueMatt
TheBlueMatt
reviewed
Jun 11, 2024
View reviewed changes
Comment threadlightning/src/ln/functional_test_utils.rs
Outdated

Comment threadlightning/src/ln/async_signer_tests.rs
Outdated

Comment threadlightning/src/ln/async_signer_tests.rs
Outdated

Comment threadlightning/src/ln/async_signer_tests.rs
Outdated

Comment threadlightning/src/util/test_channel_signer.rs
Outdated

@alecchendev
alecchendevforce-pushed
the
2024-06-specific-async-sign
branch
2 times, most recently
from
ff75d8f to
3785375CompareJune 15, 2024 00:49
alecchendev
alecchendev
commented
Jun 15, 2024
View reviewed changes
@alecchendevalecchendev
left a comment


Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Notes for myself to fix soon
Comment threadlightning/src/chain/channelmonitor.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Comment threadlightning/src/ln/functional_test_utils.rs
Outdated

Comment threadlightning/src/util/test_utils.rs
Outdated


Comment on lines

+1219
 to 
+1220

	/// Holds a list of signer ops to disable for the next signer created by this interface.
	pub disable_next_signer_ops: Mutex<HashSet<SignerOp>>,
@alecchendevalecchendevJun 15, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

remove next here since it just applies until you remove it?
@alecchendevalecchendevJun 17, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

moving to separate PR
Comment threadlightning/src/util/test_channel_signer.rs
Comment threadlightning/src/ln/mod.rs
Outdated

@alecchendev
alecchendev
commented
Jun 15, 2024

Copy link


Copy Markdown

ContributorAuthor
accidentally included previous commits, rebased to fix it. also have some nits/cleanups + a build issue i'll fix soon
@alecchendev
alecchendevforce-pushed
the
2024-06-specific-async-sign
branch
from
3785375 to
3ea35f8CompareJune 17, 2024 21:30
@alecchendev
alecchendev
commented
Jun 17, 2024•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
squashed immediately since the changes were getting unreadable. Previously i included a change here to change how we disable a signer upon derive_channel_signer for channels that haven't be created yet, but I decided to move that to a separate PR, so this one is more focused. The other big change is instead of using a mask, I thought it was simpler to just use an enum for the signer ops
@alecchendev
alecchendevforce-pushed
the
2024-06-specific-async-sign
branch
4 times, most recently
from
4d96e0c to
574bd6eCompareJune 17, 2024 22:57
@alecchendev
alecchendevforce-pushed
the
2024-06-specific-async-sign
branch
from
574bd6e to
21eeca4CompareJune 17, 2024 23:36
TheBlueMatt
TheBlueMatt
approved these changes
Jun 18, 2024
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

Seems fine to me. We can always tweak test utils later anyway.
Comment threadlightning/src/util/test_channel_signer.rs
	SignChannelAnnouncementWithFundingKey,
}

impl fmt::Display for SignerOp {
@TheBlueMattTheBlueMattJun 18, 2024

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

nit: why bother? Debug should be fine?
Comment threadlightning/src/util/test_channel_signer.rs
	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>>>,
@TheBlueMattTheBlueMattJun 18, 2024

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

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.
Comment threadlightning/src/ln/async_signer_tests.rs
 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);
@TheBlueMattTheBlueMattJun 18, 2024

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

For convenience we'll probably want some wrapper methods that set/clear everything, no matter the internal representation.
@TheBlueMatt
TheBlueMatt
commented
Jun 18, 2024

Copy link


Copy Markdown

Collaborator
Its just test code, there's no reason to hold this up on another reviewer.
@TheBlueMatt
TheBlueMatt
merged commit c39ff87
into
lightningdevkit:mainJun 18, 2024
@alecchendevalecchendev
mentioned this pull request
Jul 1, 2024
 Merged

@TheBlueMattTheBlueMatt
mentioned this pull request
Jan 30, 2025
 Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@TheBlueMattTheBlueMattTheBlueMatt approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@alecchendev@codecov-commenter@TheBlueMatt

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Refactor async signing test utils to toggle specific method availability - #3115
Merged
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
alecchendev:2024-06-specific-async-sign
Jun 18, 2024
Merged
Refactor async signing test utils to toggle specific method availability#3115
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
alecchendev:2024-06-specific-async-sign
Conversation
@alecchendev
@alecchendevalecchendev
commented
Jun 11, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
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.
@codecov-commenter
codecov-commenter
commented
Jun 11, 2024•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Attention: Patch coverage is 28.57143% with 40 lines in your changes missing coverage. Please review.
Project coverage is 89.79%. Comparing base (07f3380) to head (7e2789e).
❗ Current head 7e2789e differs from pull request most recent head 3785375
Please upload reports for the commit 3785375 to get more accurate results.
FilesPatch %Lines
lightning/src/util/test_channel_signer.rs30.43%32 Missing ⚠️
lightning/src/ln/functional_test_utils.rs0.00%6 Missing ⚠️
lightning/src/util/test_utils.rs50.00%0 Missing and 2 partials ⚠️
❗ 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.

📢 Have feedback on the report? Share it here.
@alecchendevalecchendev
mentioned this pull request
Jun 11, 2024
 Merged

TheBlueMatt
TheBlueMatt
reviewed
Jun 11, 2024
View reviewed changes
Comment threadlightning/src/ln/functional_test_utils.rs
Outdated

Comment threadlightning/src/ln/async_signer_tests.rs
Outdated

Comment threadlightning/src/ln/async_signer_tests.rs
Outdated

Comment threadlightning/src/ln/async_signer_tests.rs
Outdated

Comment threadlightning/src/util/test_channel_signer.rs
Outdated

@alecchendev
alecchendevforce-pushed
the
2024-06-specific-async-sign
branch
2 times, most recently
from
ff75d8f to
3785375CompareJune 15, 2024 00:49
alecchendev
alecchendev
commented
Jun 15, 2024
View reviewed changes
@alecchendevalecchendev
left a comment


Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Notes for myself to fix soon
Comment threadlightning/src/chain/channelmonitor.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Comment threadlightning/src/ln/functional_test_utils.rs
Outdated

Comment threadlightning/src/util/test_utils.rs
Outdated


Comment on lines

+1219
 to 
+1220

	/// Holds a list of signer ops to disable for the next signer created by this interface.
	pub disable_next_signer_ops: Mutex<HashSet<SignerOp>>,
@alecchendevalecchendevJun 15, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

remove next here since it just applies until you remove it?
@alecchendevalecchendevJun 17, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

moving to separate PR
Comment threadlightning/src/util/test_channel_signer.rs
Comment threadlightning/src/ln/mod.rs
Outdated

@alecchendev
alecchendev
commented
Jun 15, 2024

Copy link


Copy Markdown

ContributorAuthor
accidentally included previous commits, rebased to fix it. also have some nits/cleanups + a build issue i'll fix soon
@alecchendev
alecchendevforce-pushed
the
2024-06-specific-async-sign
branch
from
3785375 to
3ea35f8CompareJune 17, 2024 21:30
@alecchendev
alecchendev
commented
Jun 17, 2024•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
squashed immediately since the changes were getting unreadable. Previously i included a change here to change how we disable a signer upon derive_channel_signer for channels that haven't be created yet, but I decided to move that to a separate PR, so this one is more focused. The other big change is instead of using a mask, I thought it was simpler to just use an enum for the signer ops
@alecchendev
alecchendevforce-pushed
the
2024-06-specific-async-sign
branch
4 times, most recently
from
4d96e0c to
574bd6eCompareJune 17, 2024 22:57
@alecchendev
alecchendevforce-pushed
the
2024-06-specific-async-sign
branch
from
574bd6e to
21eeca4CompareJune 17, 2024 23:36
TheBlueMatt
TheBlueMatt
approved these changes
Jun 18, 2024
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

Seems fine to me. We can always tweak test utils later anyway.
Comment threadlightning/src/util/test_channel_signer.rs
	SignChannelAnnouncementWithFundingKey,
}

impl fmt::Display for SignerOp {
@TheBlueMattTheBlueMattJun 18, 2024

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

nit: why bother? Debug should be fine?
Comment threadlightning/src/util/test_channel_signer.rs
	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>>>,
@TheBlueMattTheBlueMattJun 18, 2024

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

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.
Comment threadlightning/src/ln/async_signer_tests.rs
 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);
@TheBlueMattTheBlueMattJun 18, 2024

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

For convenience we'll probably want some wrapper methods that set/clear everything, no matter the internal representation.
@TheBlueMatt
TheBlueMatt
commented
Jun 18, 2024

Copy link


Copy Markdown

Collaborator
Its just test code, there's no reason to hold this up on another reviewer.
@TheBlueMatt
TheBlueMatt
merged commit c39ff87
into
lightningdevkit:mainJun 18, 2024
@alecchendevalecchendev
mentioned this pull request
Jul 1, 2024
 Merged

@TheBlueMattTheBlueMatt
mentioned this pull request
Jan 30, 2025
 Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@TheBlueMattTheBlueMattTheBlueMatt approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@alecchendev@codecov-commenter@TheBlueMatt

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
Refactor async signing test utils to toggle specific method availability - #3115
Merged
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
alecchendev:2024-06-specific-async-sign
Jun 18, 2024
Merged
Refactor async signing test utils to toggle specific method availability#3115
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
alecchendev:2024-06-specific-async-sign
Conversation
@alecchendev
@alecchendevalecchendev
commented
Jun 11, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
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.
@codecov-commenter
codecov-commenter
commented
Jun 11, 2024•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Attention: Patch coverage is 28.57143% with 40 lines in your changes missing coverage. Please review.
Project coverage is 89.79%. Comparing base (07f3380) to head (7e2789e).
❗ Current head 7e2789e differs from pull request most recent head 3785375
Please upload reports for the commit 3785375 to get more accurate results.
FilesPatch %Lines
lightning/src/util/test_channel_signer.rs30.43%32 Missing ⚠️
lightning/src/ln/functional_test_utils.rs0.00%6 Missing ⚠️
lightning/src/util/test_utils.rs50.00%0 Missing and 2 partials ⚠️
❗ 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.

📢 Have feedback on the report? Share it here.
@alecchendevalecchendev
mentioned this pull request
Jun 11, 2024
 Merged

TheBlueMatt
TheBlueMatt
reviewed
Jun 11, 2024
View reviewed changes
Comment threadlightning/src/ln/functional_test_utils.rs
Outdated

Comment threadlightning/src/ln/async_signer_tests.rs
Outdated

Comment threadlightning/src/ln/async_signer_tests.rs
Outdated

Comment threadlightning/src/ln/async_signer_tests.rs
Outdated

Comment threadlightning/src/util/test_channel_signer.rs
Outdated

@alecchendev
alecchendevforce-pushed
the
2024-06-specific-async-sign
branch
2 times, most recently
from
ff75d8f to
3785375CompareJune 15, 2024 00:49
alecchendev
alecchendev
commented
Jun 15, 2024
View reviewed changes
@alecchendevalecchendev
left a comment


Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Notes for myself to fix soon
Comment threadlightning/src/chain/channelmonitor.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Comment threadlightning/src/ln/functional_test_utils.rs
Outdated

Comment threadlightning/src/util/test_utils.rs
Outdated


Comment on lines

+1219
 to 
+1220

	/// Holds a list of signer ops to disable for the next signer created by this interface.
	pub disable_next_signer_ops: Mutex<HashSet<SignerOp>>,
@alecchendevalecchendevJun 15, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

remove next here since it just applies until you remove it?
@alecchendevalecchendevJun 17, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

moving to separate PR
Comment threadlightning/src/util/test_channel_signer.rs
Comment threadlightning/src/ln/mod.rs
Outdated

@alecchendev
alecchendev
commented
Jun 15, 2024

Copy link


Copy Markdown

ContributorAuthor
accidentally included previous commits, rebased to fix it. also have some nits/cleanups + a build issue i'll fix soon
@alecchendev
alecchendevforce-pushed
the
2024-06-specific-async-sign
branch
from
3785375 to
3ea35f8CompareJune 17, 2024 21:30
@alecchendev
alecchendev
commented
Jun 17, 2024•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
squashed immediately since the changes were getting unreadable. Previously i included a change here to change how we disable a signer upon derive_channel_signer for channels that haven't be created yet, but I decided to move that to a separate PR, so this one is more focused. The other big change is instead of using a mask, I thought it was simpler to just use an enum for the signer ops
@alecchendev
alecchendevforce-pushed
the
2024-06-specific-async-sign
branch
4 times, most recently
from
4d96e0c to
574bd6eCompareJune 17, 2024 22:57
@alecchendev
alecchendevforce-pushed
the
2024-06-specific-async-sign
branch
from
574bd6e to
21eeca4CompareJune 17, 2024 23:36
TheBlueMatt
TheBlueMatt
approved these changes
Jun 18, 2024
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

Seems fine to me. We can always tweak test utils later anyway.
Comment threadlightning/src/util/test_channel_signer.rs
	SignChannelAnnouncementWithFundingKey,
}

impl fmt::Display for SignerOp {
@TheBlueMattTheBlueMattJun 18, 2024

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

nit: why bother? Debug should be fine?
Comment threadlightning/src/util/test_channel_signer.rs
	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>>>,
@TheBlueMattTheBlueMattJun 18, 2024

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

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.
Comment threadlightning/src/ln/async_signer_tests.rs
 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);
@TheBlueMattTheBlueMattJun 18, 2024

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

For convenience we'll probably want some wrapper methods that set/clear everything, no matter the internal representation.
@TheBlueMatt
TheBlueMatt
commented
Jun 18, 2024

Copy link


Copy Markdown

Collaborator
Its just test code, there's no reason to hold this up on another reviewer.
@TheBlueMatt
TheBlueMatt
merged commit c39ff87
into
lightningdevkit:mainJun 18, 2024
@alecchendevalecchendev
mentioned this pull request
Jul 1, 2024
 Merged

@TheBlueMattTheBlueMatt
mentioned this pull request
Jan 30, 2025
 Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@TheBlueMattTheBlueMattTheBlueMatt approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@alecchendev@codecov-commenter@TheBlueMatt

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Refactor async signing test utils to toggle specific method availability - #3115
Merged
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
alecchendev:2024-06-specific-async-sign
Jun 18, 2024
Merged
Refactor async signing test utils to toggle specific method availability#3115
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
alecchendev:2024-06-specific-async-sign
Conversation
@alecchendev
@alecchendevalecchendev
commented
Jun 11, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
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.
@codecov-commenter
codecov-commenter
commented
Jun 11, 2024•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Attention: Patch coverage is 28.57143% with 40 lines in your changes missing coverage. Please review.
Project coverage is 89.79%. Comparing base (07f3380) to head (7e2789e).
❗ Current head 7e2789e differs from pull request most recent head 3785375
Please upload reports for the commit 3785375 to get more accurate results.
FilesPatch %Lines
lightning/src/util/test_channel_signer.rs30.43%32 Missing ⚠️
lightning/src/ln/functional_test_utils.rs0.00%6 Missing ⚠️
lightning/src/util/test_utils.rs50.00%0 Missing and 2 partials ⚠️
❗ 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.

📢 Have feedback on the report? Share it here.
@alecchendevalecchendev
mentioned this pull request
Jun 11, 2024
 Merged

TheBlueMatt
TheBlueMatt
reviewed
Jun 11, 2024
View reviewed changes
Comment threadlightning/src/ln/functional_test_utils.rs
Outdated

Comment threadlightning/src/ln/async_signer_tests.rs
Outdated

Comment threadlightning/src/ln/async_signer_tests.rs
Outdated

Comment threadlightning/src/ln/async_signer_tests.rs
Outdated

Comment threadlightning/src/util/test_channel_signer.rs
Outdated

@alecchendev
alecchendevforce-pushed
the
2024-06-specific-async-sign
branch
2 times, most recently
from
ff75d8f to
3785375CompareJune 15, 2024 00:49
alecchendev
alecchendev
commented
Jun 15, 2024
View reviewed changes
@alecchendevalecchendev
left a comment


Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Notes for myself to fix soon
Comment threadlightning/src/chain/channelmonitor.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Comment threadlightning/src/ln/functional_test_utils.rs
Outdated

Comment threadlightning/src/util/test_utils.rs
Outdated


Comment on lines

+1219
 to 
+1220

	/// Holds a list of signer ops to disable for the next signer created by this interface.
	pub disable_next_signer_ops: Mutex<HashSet<SignerOp>>,
@alecchendevalecchendevJun 15, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

remove next here since it just applies until you remove it?
@alecchendevalecchendevJun 17, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

moving to separate PR
Comment threadlightning/src/util/test_channel_signer.rs
Comment threadlightning/src/ln/mod.rs
Outdated

@alecchendev
alecchendev
commented
Jun 15, 2024

Copy link


Copy Markdown

ContributorAuthor
accidentally included previous commits, rebased to fix it. also have some nits/cleanups + a build issue i'll fix soon
@alecchendev
alecchendevforce-pushed
the
2024-06-specific-async-sign
branch
from
3785375 to
3ea35f8CompareJune 17, 2024 21:30
@alecchendev
alecchendev
commented
Jun 17, 2024•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
squashed immediately since the changes were getting unreadable. Previously i included a change here to change how we disable a signer upon derive_channel_signer for channels that haven't be created yet, but I decided to move that to a separate PR, so this one is more focused. The other big change is instead of using a mask, I thought it was simpler to just use an enum for the signer ops
@alecchendev
alecchendevforce-pushed
the
2024-06-specific-async-sign
branch
4 times, most recently
from
4d96e0c to
574bd6eCompareJune 17, 2024 22:57
@alecchendev
alecchendevforce-pushed
the
2024-06-specific-async-sign
branch
from
574bd6e to
21eeca4CompareJune 17, 2024 23:36
TheBlueMatt
TheBlueMatt
approved these changes
Jun 18, 2024
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

Seems fine to me. We can always tweak test utils later anyway.
Comment threadlightning/src/util/test_channel_signer.rs
	SignChannelAnnouncementWithFundingKey,
}

impl fmt::Display for SignerOp {
@TheBlueMattTheBlueMattJun 18, 2024

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

nit: why bother? Debug should be fine?
Comment threadlightning/src/util/test_channel_signer.rs
	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>>>,
@TheBlueMattTheBlueMattJun 18, 2024

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

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.
Comment threadlightning/src/ln/async_signer_tests.rs
 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);
@TheBlueMattTheBlueMattJun 18, 2024

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

For convenience we'll probably want some wrapper methods that set/clear everything, no matter the internal representation.
@TheBlueMatt
TheBlueMatt
commented
Jun 18, 2024

Copy link


Copy Markdown

Collaborator
Its just test code, there's no reason to hold this up on another reviewer.
@TheBlueMatt
TheBlueMatt
merged commit c39ff87
into
lightningdevkit:mainJun 18, 2024
@alecchendevalecchendev
mentioned this pull request
Jul 1, 2024
 Merged

@TheBlueMattTheBlueMatt
mentioned this pull request
Jan 30, 2025
 Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@TheBlueMattTheBlueMattTheBlueMatt approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@alecchendev@codecov-commenter@TheBlueMatt

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Refactor async signing test utils to toggle specific method availability - #3115
Merged
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
alecchendev:2024-06-specific-async-sign
Jun 18, 2024
Merged
Refactor async signing test utils to toggle specific method availability#3115
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
alecchendev:2024-06-specific-async-sign
Conversation
@alecchendev
@alecchendevalecchendev
commented
Jun 11, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
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.
@codecov-commenter
codecov-commenter
commented
Jun 11, 2024•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Attention: Patch coverage is 28.57143% with 40 lines in your changes missing coverage. Please review.
Project coverage is 89.79%. Comparing base (07f3380) to head (7e2789e).
❗ Current head 7e2789e differs from pull request most recent head 3785375
Please upload reports for the commit 3785375 to get more accurate results.
FilesPatch %Lines
lightning/src/util/test_channel_signer.rs30.43%32 Missing ⚠️
lightning/src/ln/functional_test_utils.rs0.00%6 Missing ⚠️
lightning/src/util/test_utils.rs50.00%0 Missing and 2 partials ⚠️
❗ 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.

📢 Have feedback on the report? Share it here.
@alecchendevalecchendev
mentioned this pull request
Jun 11, 2024
 Merged

TheBlueMatt
TheBlueMatt
reviewed
Jun 11, 2024
View reviewed changes
Comment threadlightning/src/ln/functional_test_utils.rs
Outdated

Comment threadlightning/src/ln/async_signer_tests.rs
Outdated

Comment threadlightning/src/ln/async_signer_tests.rs
Outdated

Comment threadlightning/src/ln/async_signer_tests.rs
Outdated

Comment threadlightning/src/util/test_channel_signer.rs
Outdated

@alecchendev
alecchendevforce-pushed
the
2024-06-specific-async-sign
branch
2 times, most recently
from
ff75d8f to
3785375CompareJune 15, 2024 00:49
alecchendev
alecchendev
commented
Jun 15, 2024
View reviewed changes
@alecchendevalecchendev
left a comment


Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Notes for myself to fix soon
Comment threadlightning/src/chain/channelmonitor.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Comment threadlightning/src/ln/functional_test_utils.rs
Outdated

Comment threadlightning/src/util/test_utils.rs
Outdated


Comment on lines

+1219
 to 
+1220

	/// Holds a list of signer ops to disable for the next signer created by this interface.
	pub disable_next_signer_ops: Mutex<HashSet<SignerOp>>,
@alecchendevalecchendevJun 15, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

remove next here since it just applies until you remove it?
@alecchendevalecchendevJun 17, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

moving to separate PR
Comment threadlightning/src/util/test_channel_signer.rs
Comment threadlightning/src/ln/mod.rs
Outdated

@alecchendev
alecchendev
commented
Jun 15, 2024

Copy link


Copy Markdown

ContributorAuthor
accidentally included previous commits, rebased to fix it. also have some nits/cleanups + a build issue i'll fix soon
@alecchendev
alecchendevforce-pushed
the
2024-06-specific-async-sign
branch
from
3785375 to
3ea35f8CompareJune 17, 2024 21:30
@alecchendev
alecchendev
commented
Jun 17, 2024•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
squashed immediately since the changes were getting unreadable. Previously i included a change here to change how we disable a signer upon derive_channel_signer for channels that haven't be created yet, but I decided to move that to a separate PR, so this one is more focused. The other big change is instead of using a mask, I thought it was simpler to just use an enum for the signer ops
@alecchendev
alecchendevforce-pushed
the
2024-06-specific-async-sign
branch
4 times, most recently
from
4d96e0c to
574bd6eCompareJune 17, 2024 22:57
@alecchendev
alecchendevforce-pushed
the
2024-06-specific-async-sign
branch
from
574bd6e to
21eeca4CompareJune 17, 2024 23:36
TheBlueMatt
TheBlueMatt
approved these changes
Jun 18, 2024
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

Seems fine to me. We can always tweak test utils later anyway.
Comment threadlightning/src/util/test_channel_signer.rs
	SignChannelAnnouncementWithFundingKey,
}

impl fmt::Display for SignerOp {
@TheBlueMattTheBlueMattJun 18, 2024

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

nit: why bother? Debug should be fine?
Comment threadlightning/src/util/test_channel_signer.rs
	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>>>,
@TheBlueMattTheBlueMattJun 18, 2024

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

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.
Comment threadlightning/src/ln/async_signer_tests.rs
 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);
@TheBlueMattTheBlueMattJun 18, 2024

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

For convenience we'll probably want some wrapper methods that set/clear everything, no matter the internal representation.
@TheBlueMatt
TheBlueMatt
commented
Jun 18, 2024

Copy link


Copy Markdown

Collaborator
Its just test code, there's no reason to hold this up on another reviewer.
@TheBlueMatt
TheBlueMatt
merged commit c39ff87
into
lightningdevkit:mainJun 18, 2024
@alecchendevalecchendev
mentioned this pull request
Jul 1, 2024
 Merged

@TheBlueMattTheBlueMatt
mentioned this pull request
Jan 30, 2025
 Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@TheBlueMattTheBlueMattTheBlueMatt approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@alecchendev@codecov-commenter@TheBlueMatt

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
Refactor async signing test utils to toggle specific method availability - #3115
Merged
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
alecchendev:2024-06-specific-async-sign
Jun 18, 2024
Merged
Refactor async signing test utils to toggle specific method availability#3115
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
alecchendev:2024-06-specific-async-sign
Conversation
@alecchendev
@alecchendevalecchendev
commented
Jun 11, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
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.
@codecov-commenter
codecov-commenter
commented
Jun 11, 2024•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Attention: Patch coverage is 28.57143% with 40 lines in your changes missing coverage. Please review.
Project coverage is 89.79%. Comparing base (07f3380) to head (7e2789e).
❗ Current head 7e2789e differs from pull request most recent head 3785375
Please upload reports for the commit 3785375 to get more accurate results.
FilesPatch %Lines
lightning/src/util/test_channel_signer.rs30.43%32 Missing ⚠️
lightning/src/ln/functional_test_utils.rs0.00%6 Missing ⚠️
lightning/src/util/test_utils.rs50.00%0 Missing and 2 partials ⚠️
❗ 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.

📢 Have feedback on the report? Share it here.
@alecchendevalecchendev
mentioned this pull request
Jun 11, 2024
 Merged

TheBlueMatt
TheBlueMatt
reviewed
Jun 11, 2024
View reviewed changes
Comment threadlightning/src/ln/functional_test_utils.rs
Outdated

Comment threadlightning/src/ln/async_signer_tests.rs
Outdated

Comment threadlightning/src/ln/async_signer_tests.rs
Outdated

Comment threadlightning/src/ln/async_signer_tests.rs
Outdated

Comment threadlightning/src/util/test_channel_signer.rs
Outdated

@alecchendev
alecchendevforce-pushed
the
2024-06-specific-async-sign
branch
2 times, most recently
from
ff75d8f to
3785375CompareJune 15, 2024 00:49
alecchendev
alecchendev
commented
Jun 15, 2024
View reviewed changes
@alecchendevalecchendev
left a comment


Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Notes for myself to fix soon
Comment threadlightning/src/chain/channelmonitor.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Comment threadlightning/src/ln/functional_test_utils.rs
Outdated

Comment threadlightning/src/util/test_utils.rs
Outdated


Comment on lines

+1219
 to 
+1220

	/// Holds a list of signer ops to disable for the next signer created by this interface.
	pub disable_next_signer_ops: Mutex<HashSet<SignerOp>>,
@alecchendevalecchendevJun 15, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

remove next here since it just applies until you remove it?
@alecchendevalecchendevJun 17, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

moving to separate PR
Comment threadlightning/src/util/test_channel_signer.rs
Comment threadlightning/src/ln/mod.rs
Outdated

@alecchendev
alecchendev
commented
Jun 15, 2024

Copy link


Copy Markdown

ContributorAuthor
accidentally included previous commits, rebased to fix it. also have some nits/cleanups + a build issue i'll fix soon
@alecchendev
alecchendevforce-pushed
the
2024-06-specific-async-sign
branch
from
3785375 to
3ea35f8CompareJune 17, 2024 21:30
@alecchendev
alecchendev
commented
Jun 17, 2024•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
squashed immediately since the changes were getting unreadable. Previously i included a change here to change how we disable a signer upon derive_channel_signer for channels that haven't be created yet, but I decided to move that to a separate PR, so this one is more focused. The other big change is instead of using a mask, I thought it was simpler to just use an enum for the signer ops
@alecchendev
alecchendevforce-pushed
the
2024-06-specific-async-sign
branch
4 times, most recently
from
4d96e0c to
574bd6eCompareJune 17, 2024 22:57
@alecchendev
alecchendevforce-pushed
the
2024-06-specific-async-sign
branch
from
574bd6e to
21eeca4CompareJune 17, 2024 23:36
TheBlueMatt
TheBlueMatt
approved these changes
Jun 18, 2024
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

Seems fine to me. We can always tweak test utils later anyway.
Comment threadlightning/src/util/test_channel_signer.rs
	SignChannelAnnouncementWithFundingKey,
}

impl fmt::Display for SignerOp {
@TheBlueMattTheBlueMattJun 18, 2024

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

nit: why bother? Debug should be fine?
Comment threadlightning/src/util/test_channel_signer.rs
	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>>>,
@TheBlueMattTheBlueMattJun 18, 2024

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

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.
Comment threadlightning/src/ln/async_signer_tests.rs
 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);
@TheBlueMattTheBlueMattJun 18, 2024

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

For convenience we'll probably want some wrapper methods that set/clear everything, no matter the internal representation.
@TheBlueMatt
TheBlueMatt
commented
Jun 18, 2024

Copy link


Copy Markdown

Collaborator
Its just test code, there's no reason to hold this up on another reviewer.
@TheBlueMatt
TheBlueMatt
merged commit c39ff87
into
lightningdevkit:mainJun 18, 2024
@alecchendevalecchendev
mentioned this pull request
Jul 1, 2024
 Merged

@TheBlueMattTheBlueMatt
mentioned this pull request
Jan 30, 2025
 Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@TheBlueMattTheBlueMattTheBlueMatt approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@alecchendev@codecov-commenter@TheBlueMatt