Skip to content

Revocation enforcement - #764

Merged
TheBlueMatt merged 4 commits into
lightningdevkit:mainfrom
lightning-signer:revoke-enforcement
Jan 25, 2021
Merged

Revocation enforcement#764
TheBlueMatt merged 4 commits into
lightningdevkit:mainfrom
lightning-signer:revoke-enforcement

Conversation

@devrandom

@devrandomdevrandom commented Dec 5, 2020

Copy link
Copy Markdown
Member

We want to make sure that we don't sign revoked transactions.

Given that ChannelKeys (actually EnforcingChannelKeys) are not singletons and revocation enforcement is stateful, we need to store the revocation state in KeysInterface.

This builds on #761. The first new commit "Use TestKeysInterface in functional tests" could be folded in to that PR.

@devrandomdevrandom mentioned this pull request Dec 5, 2020
Merged
@devrandom
devrandomforce-pushed the revoke-enforcement branch 2 times, most recently from 8bbb6d6 to 3e82940CompareDecember 5, 2020 16:58
@codecov

codecovBot commented Dec 5, 2020•
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #764 (142b0d6) into main (21a44da) will increase coverage by 0.04%.
The diff coverage is 96.26%.

Impacted file tree graph

@@ Coverage Diff @@## main #764 +/- ##
==========================================
+ Coverage 91.24% 91.28% +0.04% 
==========================================
Files 37 37 Lines 22846 22920 +74 ==========================================
+ Hits 20845 20922 +77 + Misses 2001 1998 -3
Impacted FilesCoverage Δ
lightning/src/util/enforcing_trait_impls.rs90.19% <80.76%> (-9.81%)⬇️
lightning/src/chain/keysinterface.rs93.47% <100.00%> (+0.17%)⬆️
lightning/src/ln/chanmon_update_fail_tests.rs97.56% <100.00%> (ø)
lightning/src/ln/functional_test_utils.rs95.13% <100.00%> (+<0.01%)⬆️
lightning/src/ln/functional_tests.rs97.18% <100.00%> (+0.23%)⬆️
lightning/src/ln/onchaintx.rs94.02% <100.00%> (ø)
lightning/src/util/test_utils.rs84.78% <100.00%> (+1.10%)⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 21a44da...142b0d6. Read the comment docs.

@devrandom

devrandom commented Dec 5, 2020

Copy link
Copy Markdown
MemberAuthor

I'm seeing a fuzzing problem, but after trying to fix it, looks like there's an actual out of order revocation.

The fuzz target is: TARGET=chanmon_consistency HEX=2d321d51ffff0b1030341d51ffff0b100a2d

@devrandom
devrandomforce-pushed the revoke-enforcement branch 2 times, most recently from 27b2eee to dff0448CompareDecember 7, 2020 12:32
@devrandomdevrandom changed the title Revoke enforcementRevocation enforcementDec 7, 2020
@devrandom

devrandom commented Dec 7, 2020

Copy link
Copy Markdown
MemberAuthor

Fuzzing issue fixed

arik-so
arik-so reviewed Dec 15, 2020
View reviewed changes
Comment threadlightning/src/util/enforcing_trait_impls.rs Outdated
Comment threadlightning/src/util/enforcing_trait_impls.rs Outdated
Comment threadlightning/src/util/enforcing_trait_impls.rs
@devrandom
devrandomforce-pushed the revoke-enforcement branch 3 times, most recently from 07e0a82 to 9b840d7CompareJanuary 9, 2021 02:09
TheBlueMatt
TheBlueMatt approved these changes Jan 11, 2021
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.

One nit.

Comment threadlightning/src/util/enforcing_trait_impls.rs Outdated
TheBlueMatt
TheBlueMatt reviewed Jan 11, 2021
View reviewed changes
Comment threadlightning/src/ln/functional_tests.rs Outdated
fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: 253 };
persister = test_utils::TestPersister::new();
monitor = test_utils::TestChainMonitor::new(Some(&chain_source), &tx_broadcaster, &logger, &fee_estimator, &persister);
monitor = test_utils::TestChainMonitor::new(Some(&chain_source), &tx_broadcaster, &logger, &fee_estimator, &persister, &keys_manager);

@TheBlueMattTheBlueMattJan 11, 2021

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.

Hmm, at least here, maybe elsewhere - dont we create the TestKeysInterface a few lines up, meaning we lose the state tracking?

@devrandomdevrandomJan 11, 2021

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

This is done, but together with the fix for the other nit, a significant issue was uncovered in data_loss_protect handling.

It looks like the NodeDrop implementation does a serialization round-trip, which ends up trying to broadcast a revoked commitment tx, since we restored an old state. This seems to be an actual issue with channel reestablish - not sure how you intended it to recover going forward since it's out of sync. It seems like if there's data-loss, we have to update our commitment number instead of just erroring in channel_reestablish?

@TheBlueMattTheBlueMattJan 11, 2021

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.

Hmm, can you push the branch that demonstrates that somewhere? I suppose the "right" answer could be to not panic, return an Err, make sure ChannelMonitor handles that fine (it should?) and then hope we can broadcast the current state when our counterparty broadcasts theirs. That's not quite enough because we should be able to broadcast the old state manually, but then it'd be up to the signer to accept a stale state if we give up waiting on our counterparty.

@TheBlueMattTheBlueMattJan 11, 2021

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.

I suppose we could tweak the signer to check a boolean whether we should panic or not and in a few tests (like this one), let it Err instead of panicing and then test that we can still claim a counterparty-broadcasted tx or retry broadcasting our state later.

@devrandomdevrandomJan 12, 2021

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Sorry, pushed now, forgot yesterday.

@devrandomdevrandomJan 12, 2021

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

It seems like full support for data_loss_protect would entail putting the system in a state where it doesn't try to go on-chain, because we don't have the needed data for a non-revoked tx. Punting on that for later, I've added a commit that marks the node as failed and doesn't try to execute Node::Drop for the node that suffered the data-loss in test_data_loss_protect.

@devrandomdevrandomJan 19, 2021

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

As discussed elsewhere, we now return Err when there is a revocation issue and err_on_sign_revoked_holder_tx is on in TestKeysInterface / EnforcingChannelKeys.

@devrandomdevrandomJan 19, 2021

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

... however, this workaround no longer works as a result of OnChainTx.holder_commitment no longer being optional. As part of that change, we panic if an Err is return when trying to sign the latest commitment. Also, this is now an issue also in tests that test justice reactions by simulating a bad actor - they panic during HTLC claims when they react to their own broadcast.

I see a couple of approaches:

  • propagate the Err up the call stack and don't panic
  • simulate bad actor / data loss more accurately by rolling back the signer or at least the revocation counter

The rebase that fails tests was pushed just now.

@devrandomdevrandomJan 20, 2021

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Actually, the simplest fix is to disable the revocation enforcement for these spots.

@devrandom
devrandomforce-pushed the revoke-enforcement branch 3 times, most recently from a1644a2 to c8bd159CompareJanuary 13, 2021 22:47
@devrandom

devrandom commented Jan 13, 2021

Copy link
Copy Markdown
MemberAuthor

Due to issues described in #775, test_data_loss_protect isn't quite right, but we're not going to fix this right now.

The panic has been changed to an Err.

@TheBlueMatt

TheBlueMatt commented Jan 13, 2021

Copy link
Copy Markdown
Collaborator

The panic has been changed to an Err.

Can we have the test enforcing signer have an internal setting to either panic or return an Err so we still ensure we hit panics in most tests, with a comment pointing to the new issue in test_data_loss_protect.

@devrandom

devrandom commented Jan 14, 2021

Copy link
Copy Markdown
MemberAuthor

Added err_on_sign_revoked_holder_tx. The issue is mentioned at the top of test_data_loss_protect

TheBlueMatt
TheBlueMatt approved these changes Jan 15, 2021
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.

This looks good mod the fuzzers failing to compile. One nit bug no need to fix it now.

Comment threadlightning/src/util/enforcing_trait_impls.rs Outdated
@devrandom
devrandomforce-pushed the revoke-enforcement branch 2 times, most recently from 10a87ba to ebc80c3CompareJanuary 15, 2021 02:36
@devrandom
Use TestKeysInterface in functional tests
2cbb835
This allows stateful validation in EnforcingChannelKeys
@devrandom
Revocation enforcement in signer
a5869b9
We want to make sure that we don't sign revoked transactions.
Given that ChannelKeys are not singletons and revocation enforcement is stateful,
we need to store the revocation state in KeysInterface.
@devrandom
Fix fuzzing issue with revocation
bd4345d
@devrandom

devrandom commented Jan 19, 2021

Copy link
Copy Markdown
MemberAuthor

Ran into an issue in #764 (comment) after rebase, will look into possible approaches.

@devrandom

devrandom commented Jan 20, 2021

Copy link
Copy Markdown
MemberAuthor

Ran into an issue in #764 (comment) after rebase, will look into possible approaches.

This has been corrected, and the PR is ready for final review.

@devrandom
devrandomforce-pushed the revoke-enforcement branch 2 times, most recently from f240e4a to 55f4ef8CompareJanuary 20, 2021 00:07
ariard
ariard reviewed Jan 20, 2021
View reviewed changes

@ariardariard left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Just minor comments otherwise SGTM.

Comment threadlightning/src/util/enforcing_trait_impls.rs Outdated
Comment threadlightning/src/util/enforcing_trait_impls.rs
Comment threadlightning/src/util/enforcing_trait_impls.rs
@devrandom

devrandom commented Jan 21, 2021

Copy link
Copy Markdown
MemberAuthor

Added some more docs, please take a look. I believe this addresses all outstanding comments.

@devrandom
Let some tests disable revocation policy check
142b0d6
When simulating a bad actor that broadcasts a revoked tx, the policy check would otherwise panic.
@TheBlueMatt
TheBlueMatt merged commit f151c02 into lightningdevkit:mainJan 25, 2021
@ariard

ariard commented Jan 25, 2021

Copy link
Copy Markdown

Post-Merge Code Review ACK 142b0d6

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

Reviewers

@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@arik-soarik-soarik-so left review comments
+1 more reviewer
@ariardariardariard left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@devrandom@TheBlueMatt@ariard@arik-so
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all 
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
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;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Revocation enforcement by devrandom · Pull Request #764 · lightningdevkit/rust-lightning · GitHub
Skip to content
Revocation enforcement - #764
Merged
TheBlueMatt merged 4 commits into
lightningdevkit:mainfrom 
lightning-signer:revoke-enforcement
Jan 25, 2021
Merged
Revocation enforcement#764
TheBlueMatt merged 4 commits into
lightningdevkit:mainfrom 
lightning-signer:revoke-enforcement
Conversation
@devrandom
@devrandomdevrandom
commented
Dec 5, 2020

Copy link


Copy Markdown

Member
We want to make sure that we don't sign revoked transactions.
Given that ChannelKeys (actually EnforcingChannelKeys) are not singletons and revocation enforcement is stateful, we need to store the revocation state in KeysInterface.
This builds on #761. The first new commit "Use TestKeysInterface in functional tests" could be folded in to that PR.
@devrandomdevrandom
mentioned this pull request
Dec 5, 2020
 Merged

@devrandom
devrandomforce-pushed
the
revoke-enforcement
branch
2 times, most recently
from
8bbb6d6 to
3e82940CompareDecember 5, 2020 16:58
@codecov
codecovBot
commented
Dec 5, 2020•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #764 (142b0d6) into main (21a44da) will increase coverage by 0.04%.

The diff coverage is 96.26%.
Impacted file tree graph
@@ Coverage Diff @@## main #764 +/- ##
==========================================
+ Coverage 91.24% 91.28% +0.04% 
==========================================
Files 37 37 Lines 22846 22920 +74 ==========================================
+ Hits 20845 20922 +77 + Misses 2001 1998 -3 
Impacted FilesCoverage Δ
lightning/src/util/enforcing_trait_impls.rs90.19% <80.76%> (-9.81%)⬇️
lightning/src/chain/keysinterface.rs93.47% <100.00%> (+0.17%)⬆️
lightning/src/ln/chanmon_update_fail_tests.rs97.56% <100.00%> (ø)
lightning/src/ln/functional_test_utils.rs95.13% <100.00%> (+<0.01%)⬆️
lightning/src/ln/functional_tests.rs97.18% <100.00%> (+0.23%)⬆️
lightning/src/ln/onchaintx.rs94.02% <100.00%> (ø)
lightning/src/util/test_utils.rs84.78% <100.00%> (+1.10%)⬆️
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data

Powered by Codecov. Last update 21a44da...142b0d6. Read the comment docs.
@devrandom
devrandom
commented
Dec 5, 2020

Copy link


Copy Markdown

MemberAuthor
I'm seeing a fuzzing problem, but after trying to fix it, looks like there's an actual out of order revocation.
The fuzz target is: TARGET=chanmon_consistency HEX=2d321d51ffff0b1030341d51ffff0b100a2d
@devrandom
devrandomforce-pushed
the
revoke-enforcement
branch
2 times, most recently
from
27b2eee to
dff0448CompareDecember 7, 2020 12:32
@devrandomdevrandom
changed the title
Revoke enforcementRevocation enforcementDec 7, 2020
@devrandom
devrandom
commented
Dec 7, 2020

Copy link


Copy Markdown

MemberAuthor
Fuzzing issue fixed
arik-so
arik-so
reviewed
Dec 15, 2020
View reviewed changes
Comment threadlightning/src/util/enforcing_trait_impls.rs
Outdated

Comment threadlightning/src/util/enforcing_trait_impls.rs
Outdated

Comment threadlightning/src/util/enforcing_trait_impls.rs
@devrandom
devrandomforce-pushed
the
revoke-enforcement
branch
3 times, most recently
from
07e0a82 to
9b840d7CompareJanuary 9, 2021 02:09
TheBlueMatt
TheBlueMatt
approved these changes
Jan 11, 2021
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.

One nit.
Comment threadlightning/src/util/enforcing_trait_impls.rs
Outdated

TheBlueMatt
TheBlueMatt
reviewed
Jan 11, 2021
View reviewed changes
Comment threadlightning/src/ln/functional_tests.rs
Outdated

	fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: 253 };
	persister = test_utils::TestPersister::new();
	monitor = test_utils::TestChainMonitor::new(Some(&chain_source), &tx_broadcaster, &logger, &fee_estimator, &persister);
	monitor = test_utils::TestChainMonitor::new(Some(&chain_source), &tx_broadcaster, &logger, &fee_estimator, &persister, &keys_manager);
@TheBlueMattTheBlueMattJan 11, 2021

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.

Hmm, at least here, maybe elsewhere - dont we create the TestKeysInterface a few lines up, meaning we lose the state tracking?
@devrandomdevrandomJan 11, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

This is done, but together with the fix for the other nit, a significant issue was uncovered in data_loss_protect handling.
It looks like the NodeDrop implementation does a serialization round-trip, which ends up trying to broadcast a revoked commitment tx, since we restored an old state. This seems to be an actual issue with channel reestablish - not sure how you intended it to recover going forward since it's out of sync. It seems like if there's data-loss, we have to update our commitment number instead of just erroring in channel_reestablish?
@TheBlueMattTheBlueMattJan 11, 2021

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.

Hmm, can you push the branch that demonstrates that somewhere? I suppose the "right" answer could be to not panic, return an Err, make sure ChannelMonitor handles that fine (it should?) and then hope we can broadcast the current state when our counterparty broadcasts theirs. That's not quite enough because we should be able to broadcast the old state manually, but then it'd be up to the signer to accept a stale state if we give up waiting on our counterparty.
@TheBlueMattTheBlueMattJan 11, 2021

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.

I suppose we could tweak the signer to check a boolean whether we should panic or not and in a few tests (like this one), let it Err instead of panicing and then test that we can still claim a counterparty-broadcasted tx or retry broadcasting our state later.
@devrandomdevrandomJan 12, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Sorry, pushed now, forgot yesterday.
@devrandomdevrandomJan 12, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

It seems like full support for data_loss_protect would entail putting the system in a state where it doesn't try to go on-chain, because we don't have the needed data for a non-revoked tx. Punting on that for later, I've added a commit that marks the node as failed and doesn't try to execute Node::Drop for the node that suffered the data-loss in test_data_loss_protect.
@devrandomdevrandomJan 19, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

As discussed elsewhere, we now return Err when there is a revocation issue and err_on_sign_revoked_holder_tx is on in TestKeysInterface / EnforcingChannelKeys.
@devrandomdevrandomJan 19, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

... however, this workaround no longer works as a result of OnChainTx.holder_commitment no longer being optional. As part of that change, we panic if an Err is return when trying to sign the latest commitment. Also, this is now an issue also in tests that test justice reactions by simulating a bad actor - they panic during HTLC claims when they react to their own broadcast.
I see a couple of approaches:
  • propagate the Err up the call stack and don't panic
  • simulate bad actor / data loss more accurately by rolling back the signer or at least the revocation counter
The rebase that fails tests was pushed just now.
@devrandomdevrandomJan 20, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Actually, the simplest fix is to disable the revocation enforcement for these spots.
@devrandom
devrandomforce-pushed
the
revoke-enforcement
branch
3 times, most recently
from
a1644a2 to
c8bd159CompareJanuary 13, 2021 22:47
@devrandom
devrandom
commented
Jan 13, 2021

Copy link


Copy Markdown

MemberAuthor
Due to issues described in #775, test_data_loss_protect isn't quite right, but we're not going to fix this right now.
The panic has been changed to an Err.
@TheBlueMatt
TheBlueMatt
commented
Jan 13, 2021

Copy link


Copy Markdown

Collaborator
The panic has been changed to an Err.
Can we have the test enforcing signer have an internal setting to either panic or return an Err so we still ensure we hit panics in most tests, with a comment pointing to the new issue in test_data_loss_protect.
@devrandom
devrandom
commented
Jan 14, 2021

Copy link


Copy Markdown

MemberAuthor
Added err_on_sign_revoked_holder_tx. The issue is mentioned at the top of test_data_loss_protect
TheBlueMatt
TheBlueMatt
approved these changes
Jan 15, 2021
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.

This looks good mod the fuzzers failing to compile. One nit bug no need to fix it now.
Comment threadlightning/src/util/enforcing_trait_impls.rs
Outdated

@devrandom
devrandomforce-pushed
the
revoke-enforcement
branch
2 times, most recently
from
10a87ba to
ebc80c3CompareJanuary 15, 2021 02:36
@devrandom
Use TestKeysInterface in functional tests
2cbb835
This allows stateful validation in EnforcingChannelKeys
@devrandom
Revocation enforcement in signer
a5869b9
We want to make sure that we don't sign revoked transactions.
Given that ChannelKeys are not singletons and revocation enforcement is stateful,
we need to store the revocation state in KeysInterface.
@devrandom
Fix fuzzing issue with revocation
bd4345d
@devrandom
devrandom
commented
Jan 19, 2021

Copy link


Copy Markdown

MemberAuthor
Ran into an issue in #764 (comment) after rebase, will look into possible approaches.
@devrandom
devrandom
commented
Jan 20, 2021

Copy link


Copy Markdown

MemberAuthor
Ran into an issue in #764 (comment) after rebase, will look into possible approaches.
This has been corrected, and the PR is ready for final review.
@devrandom
devrandomforce-pushed
the
revoke-enforcement
branch
2 times, most recently
from
f240e4a to
55f4ef8CompareJanuary 20, 2021 00:07
ariard
ariard
reviewed
Jan 20, 2021
View reviewed changes
@ariardariard
left a comment


Copy link


Copy Markdown

Choose a reason for hiding this comment

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

Just minor comments otherwise SGTM.
Comment threadlightning/src/util/enforcing_trait_impls.rs
Outdated

Comment threadlightning/src/util/enforcing_trait_impls.rs
Comment threadlightning/src/util/enforcing_trait_impls.rs
@devrandom
devrandom
commented
Jan 21, 2021

Copy link


Copy Markdown

MemberAuthor
Added some more docs, please take a look. I believe this addresses all outstanding comments.
@devrandom
Let some tests disable revocation policy check
142b0d6
When simulating a bad actor that broadcasts a revoked tx, the policy check would otherwise panic.
@TheBlueMatt
TheBlueMatt
merged commit f151c02
into
lightningdevkit:mainJan 25, 2021
@ariard
ariard
commented
Jan 25, 2021

Copy link


Copy Markdown

Post-Merge Code Review ACK 142b0d6
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@arik-soarik-soarik-so left review comments

+1 more reviewer

@ariardariardariard left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@devrandom@TheBlueMatt@ariard@arik-so

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Force GitHub README to respect dark mode
(function() {
var style = document.createElement('style');
style.textContent = '
.markdown-body {
color-scheme: dark light;
}
.markdown-body pre { background: #161b22 !important; }
.markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }
.markdown-body table th, .markdown-body table td { border-color: #30363d !important; }
.markdown-body img { background: #0d1117; }
.markdown-body blockquote { border-left-color: #8b949e; }
.markdown-body hr { border-color: #30363d; }
';
document.head.appendChild(style);
})();
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Revocation enforcement by devrandom · Pull Request #764 · lightningdevkit/rust-lightning · GitHub
Skip to content
Revocation enforcement - #764
Merged
TheBlueMatt merged 4 commits into
lightningdevkit:mainfrom 
lightning-signer:revoke-enforcement
Jan 25, 2021
Merged
Revocation enforcement#764
TheBlueMatt merged 4 commits into
lightningdevkit:mainfrom 
lightning-signer:revoke-enforcement
Conversation
@devrandom
@devrandomdevrandom
commented
Dec 5, 2020

Copy link


Copy Markdown

Member
We want to make sure that we don't sign revoked transactions.
Given that ChannelKeys (actually EnforcingChannelKeys) are not singletons and revocation enforcement is stateful, we need to store the revocation state in KeysInterface.
This builds on #761. The first new commit "Use TestKeysInterface in functional tests" could be folded in to that PR.
@devrandomdevrandom
mentioned this pull request
Dec 5, 2020
 Merged

@devrandom
devrandomforce-pushed
the
revoke-enforcement
branch
2 times, most recently
from
8bbb6d6 to
3e82940CompareDecember 5, 2020 16:58
@codecov
codecovBot
commented
Dec 5, 2020•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #764 (142b0d6) into main (21a44da) will increase coverage by 0.04%.

The diff coverage is 96.26%.
Impacted file tree graph
@@ Coverage Diff @@## main #764 +/- ##
==========================================
+ Coverage 91.24% 91.28% +0.04% 
==========================================
Files 37 37 Lines 22846 22920 +74 ==========================================
+ Hits 20845 20922 +77 + Misses 2001 1998 -3 
Impacted FilesCoverage Δ
lightning/src/util/enforcing_trait_impls.rs90.19% <80.76%> (-9.81%)⬇️
lightning/src/chain/keysinterface.rs93.47% <100.00%> (+0.17%)⬆️
lightning/src/ln/chanmon_update_fail_tests.rs97.56% <100.00%> (ø)
lightning/src/ln/functional_test_utils.rs95.13% <100.00%> (+<0.01%)⬆️
lightning/src/ln/functional_tests.rs97.18% <100.00%> (+0.23%)⬆️
lightning/src/ln/onchaintx.rs94.02% <100.00%> (ø)
lightning/src/util/test_utils.rs84.78% <100.00%> (+1.10%)⬆️
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data

Powered by Codecov. Last update 21a44da...142b0d6. Read the comment docs.
@devrandom
devrandom
commented
Dec 5, 2020

Copy link


Copy Markdown

MemberAuthor
I'm seeing a fuzzing problem, but after trying to fix it, looks like there's an actual out of order revocation.
The fuzz target is: TARGET=chanmon_consistency HEX=2d321d51ffff0b1030341d51ffff0b100a2d
@devrandom
devrandomforce-pushed
the
revoke-enforcement
branch
2 times, most recently
from
27b2eee to
dff0448CompareDecember 7, 2020 12:32
@devrandomdevrandom
changed the title
Revoke enforcementRevocation enforcementDec 7, 2020
@devrandom
devrandom
commented
Dec 7, 2020

Copy link


Copy Markdown

MemberAuthor
Fuzzing issue fixed
arik-so
arik-so
reviewed
Dec 15, 2020
View reviewed changes
Comment threadlightning/src/util/enforcing_trait_impls.rs
Outdated

Comment threadlightning/src/util/enforcing_trait_impls.rs
Outdated

Comment threadlightning/src/util/enforcing_trait_impls.rs
@devrandom
devrandomforce-pushed
the
revoke-enforcement
branch
3 times, most recently
from
07e0a82 to
9b840d7CompareJanuary 9, 2021 02:09
TheBlueMatt
TheBlueMatt
approved these changes
Jan 11, 2021
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.

One nit.
Comment threadlightning/src/util/enforcing_trait_impls.rs
Outdated

TheBlueMatt
TheBlueMatt
reviewed
Jan 11, 2021
View reviewed changes
Comment threadlightning/src/ln/functional_tests.rs
Outdated

	fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: 253 };
	persister = test_utils::TestPersister::new();
	monitor = test_utils::TestChainMonitor::new(Some(&chain_source), &tx_broadcaster, &logger, &fee_estimator, &persister);
	monitor = test_utils::TestChainMonitor::new(Some(&chain_source), &tx_broadcaster, &logger, &fee_estimator, &persister, &keys_manager);
@TheBlueMattTheBlueMattJan 11, 2021

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.

Hmm, at least here, maybe elsewhere - dont we create the TestKeysInterface a few lines up, meaning we lose the state tracking?
@devrandomdevrandomJan 11, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

This is done, but together with the fix for the other nit, a significant issue was uncovered in data_loss_protect handling.
It looks like the NodeDrop implementation does a serialization round-trip, which ends up trying to broadcast a revoked commitment tx, since we restored an old state. This seems to be an actual issue with channel reestablish - not sure how you intended it to recover going forward since it's out of sync. It seems like if there's data-loss, we have to update our commitment number instead of just erroring in channel_reestablish?
@TheBlueMattTheBlueMattJan 11, 2021

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.

Hmm, can you push the branch that demonstrates that somewhere? I suppose the "right" answer could be to not panic, return an Err, make sure ChannelMonitor handles that fine (it should?) and then hope we can broadcast the current state when our counterparty broadcasts theirs. That's not quite enough because we should be able to broadcast the old state manually, but then it'd be up to the signer to accept a stale state if we give up waiting on our counterparty.
@TheBlueMattTheBlueMattJan 11, 2021

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.

I suppose we could tweak the signer to check a boolean whether we should panic or not and in a few tests (like this one), let it Err instead of panicing and then test that we can still claim a counterparty-broadcasted tx or retry broadcasting our state later.
@devrandomdevrandomJan 12, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Sorry, pushed now, forgot yesterday.
@devrandomdevrandomJan 12, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

It seems like full support for data_loss_protect would entail putting the system in a state where it doesn't try to go on-chain, because we don't have the needed data for a non-revoked tx. Punting on that for later, I've added a commit that marks the node as failed and doesn't try to execute Node::Drop for the node that suffered the data-loss in test_data_loss_protect.
@devrandomdevrandomJan 19, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

As discussed elsewhere, we now return Err when there is a revocation issue and err_on_sign_revoked_holder_tx is on in TestKeysInterface / EnforcingChannelKeys.
@devrandomdevrandomJan 19, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

... however, this workaround no longer works as a result of OnChainTx.holder_commitment no longer being optional. As part of that change, we panic if an Err is return when trying to sign the latest commitment. Also, this is now an issue also in tests that test justice reactions by simulating a bad actor - they panic during HTLC claims when they react to their own broadcast.
I see a couple of approaches:
  • propagate the Err up the call stack and don't panic
  • simulate bad actor / data loss more accurately by rolling back the signer or at least the revocation counter
The rebase that fails tests was pushed just now.
@devrandomdevrandomJan 20, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Actually, the simplest fix is to disable the revocation enforcement for these spots.
@devrandom
devrandomforce-pushed
the
revoke-enforcement
branch
3 times, most recently
from
a1644a2 to
c8bd159CompareJanuary 13, 2021 22:47
@devrandom
devrandom
commented
Jan 13, 2021

Copy link


Copy Markdown

MemberAuthor
Due to issues described in #775, test_data_loss_protect isn't quite right, but we're not going to fix this right now.
The panic has been changed to an Err.
@TheBlueMatt
TheBlueMatt
commented
Jan 13, 2021

Copy link


Copy Markdown

Collaborator
The panic has been changed to an Err.
Can we have the test enforcing signer have an internal setting to either panic or return an Err so we still ensure we hit panics in most tests, with a comment pointing to the new issue in test_data_loss_protect.
@devrandom
devrandom
commented
Jan 14, 2021

Copy link


Copy Markdown

MemberAuthor
Added err_on_sign_revoked_holder_tx. The issue is mentioned at the top of test_data_loss_protect
TheBlueMatt
TheBlueMatt
approved these changes
Jan 15, 2021
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.

This looks good mod the fuzzers failing to compile. One nit bug no need to fix it now.
Comment threadlightning/src/util/enforcing_trait_impls.rs
Outdated

@devrandom
devrandomforce-pushed
the
revoke-enforcement
branch
2 times, most recently
from
10a87ba to
ebc80c3CompareJanuary 15, 2021 02:36
@devrandom
Use TestKeysInterface in functional tests
2cbb835
This allows stateful validation in EnforcingChannelKeys
@devrandom
Revocation enforcement in signer
a5869b9
We want to make sure that we don't sign revoked transactions.
Given that ChannelKeys are not singletons and revocation enforcement is stateful,
we need to store the revocation state in KeysInterface.
@devrandom
Fix fuzzing issue with revocation
bd4345d
@devrandom
devrandom
commented
Jan 19, 2021

Copy link


Copy Markdown

MemberAuthor
Ran into an issue in #764 (comment) after rebase, will look into possible approaches.
@devrandom
devrandom
commented
Jan 20, 2021

Copy link


Copy Markdown

MemberAuthor
Ran into an issue in #764 (comment) after rebase, will look into possible approaches.
This has been corrected, and the PR is ready for final review.
@devrandom
devrandomforce-pushed
the
revoke-enforcement
branch
2 times, most recently
from
f240e4a to
55f4ef8CompareJanuary 20, 2021 00:07
ariard
ariard
reviewed
Jan 20, 2021
View reviewed changes
@ariardariard
left a comment


Copy link


Copy Markdown

Choose a reason for hiding this comment

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

Just minor comments otherwise SGTM.
Comment threadlightning/src/util/enforcing_trait_impls.rs
Outdated

Comment threadlightning/src/util/enforcing_trait_impls.rs
Comment threadlightning/src/util/enforcing_trait_impls.rs
@devrandom
devrandom
commented
Jan 21, 2021

Copy link


Copy Markdown

MemberAuthor
Added some more docs, please take a look. I believe this addresses all outstanding comments.
@devrandom
Let some tests disable revocation policy check
142b0d6
When simulating a bad actor that broadcasts a revoked tx, the policy check would otherwise panic.
@TheBlueMatt
TheBlueMatt
merged commit f151c02
into
lightningdevkit:mainJan 25, 2021
@ariard
ariard
commented
Jan 25, 2021

Copy link


Copy Markdown

Post-Merge Code Review ACK 142b0d6
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@arik-soarik-soarik-so left review comments

+1 more reviewer

@ariardariardariard left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@devrandom@TheBlueMatt@ariard@arik-so

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Highlight search terms from Google/DuckDuckGo/Bing referrer
(function() {
var ref = document.referrer;
var terms = [];
if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {
var url = new URL(ref);
var q = url.searchParams.get('q') || url.searchParams.get('p');
if (q) {
terms = q.split(/\s+/).filter(function(t) { return t.length > 2; });
}
}
if (terms.length === 0) return;
var style = document.createElement('style');
style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';
document.head.appendChild(style);
function highlight(node) {
if (node.nodeType === 3) { // text node
var text = node.textContent;
var found = false;
terms.forEach(function(term) {
var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi');
if (regex.test(text)) {
found = true;
var frag = document.createDocumentFragment();
var parts = text.split(regex);
parts.forEach(function(part, i) {
if (i % 2 === 0) {
frag.appendChild(document.createTextNode(part));
} else {
var span = document.createElement('span');
span.className = 'userscript-highlight';
span.textContent = part;
frag.appendChild(span);
}
});
node.parentNode.replaceChild(frag, node);
}
});
} else if (node.nodeType === 1 && node.childNodes) { // element
var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];
if (!skipTags.includes(node.tagName)) {
Array.from(node.childNodes).forEach(highlight);
}
}
}
highlight(document.body);
// Re-highlight on dynamic content
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1 || node.nodeType === 3) highlight(node);
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Revocation enforcement by devrandom · Pull Request #764 · lightningdevkit/rust-lightning · GitHub
Skip to content
Revocation enforcement - #764
Merged
TheBlueMatt merged 4 commits into
lightningdevkit:mainfrom 
lightning-signer:revoke-enforcement
Jan 25, 2021
Merged
Revocation enforcement#764
TheBlueMatt merged 4 commits into
lightningdevkit:mainfrom 
lightning-signer:revoke-enforcement
Conversation
@devrandom
@devrandomdevrandom
commented
Dec 5, 2020

Copy link


Copy Markdown

Member
We want to make sure that we don't sign revoked transactions.
Given that ChannelKeys (actually EnforcingChannelKeys) are not singletons and revocation enforcement is stateful, we need to store the revocation state in KeysInterface.
This builds on #761. The first new commit "Use TestKeysInterface in functional tests" could be folded in to that PR.
@devrandomdevrandom
mentioned this pull request
Dec 5, 2020
 Merged

@devrandom
devrandomforce-pushed
the
revoke-enforcement
branch
2 times, most recently
from
8bbb6d6 to
3e82940CompareDecember 5, 2020 16:58
@codecov
codecovBot
commented
Dec 5, 2020•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #764 (142b0d6) into main (21a44da) will increase coverage by 0.04%.

The diff coverage is 96.26%.
Impacted file tree graph
@@ Coverage Diff @@## main #764 +/- ##
==========================================
+ Coverage 91.24% 91.28% +0.04% 
==========================================
Files 37 37 Lines 22846 22920 +74 ==========================================
+ Hits 20845 20922 +77 + Misses 2001 1998 -3 
Impacted FilesCoverage Δ
lightning/src/util/enforcing_trait_impls.rs90.19% <80.76%> (-9.81%)⬇️
lightning/src/chain/keysinterface.rs93.47% <100.00%> (+0.17%)⬆️
lightning/src/ln/chanmon_update_fail_tests.rs97.56% <100.00%> (ø)
lightning/src/ln/functional_test_utils.rs95.13% <100.00%> (+<0.01%)⬆️
lightning/src/ln/functional_tests.rs97.18% <100.00%> (+0.23%)⬆️
lightning/src/ln/onchaintx.rs94.02% <100.00%> (ø)
lightning/src/util/test_utils.rs84.78% <100.00%> (+1.10%)⬆️
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data

Powered by Codecov. Last update 21a44da...142b0d6. Read the comment docs.
@devrandom
devrandom
commented
Dec 5, 2020

Copy link


Copy Markdown

MemberAuthor
I'm seeing a fuzzing problem, but after trying to fix it, looks like there's an actual out of order revocation.
The fuzz target is: TARGET=chanmon_consistency HEX=2d321d51ffff0b1030341d51ffff0b100a2d
@devrandom
devrandomforce-pushed
the
revoke-enforcement
branch
2 times, most recently
from
27b2eee to
dff0448CompareDecember 7, 2020 12:32
@devrandomdevrandom
changed the title
Revoke enforcementRevocation enforcementDec 7, 2020
@devrandom
devrandom
commented
Dec 7, 2020

Copy link


Copy Markdown

MemberAuthor
Fuzzing issue fixed
arik-so
arik-so
reviewed
Dec 15, 2020
View reviewed changes
Comment threadlightning/src/util/enforcing_trait_impls.rs
Outdated

Comment threadlightning/src/util/enforcing_trait_impls.rs
Outdated

Comment threadlightning/src/util/enforcing_trait_impls.rs
@devrandom
devrandomforce-pushed
the
revoke-enforcement
branch
3 times, most recently
from
07e0a82 to
9b840d7CompareJanuary 9, 2021 02:09
TheBlueMatt
TheBlueMatt
approved these changes
Jan 11, 2021
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.

One nit.
Comment threadlightning/src/util/enforcing_trait_impls.rs
Outdated

TheBlueMatt
TheBlueMatt
reviewed
Jan 11, 2021
View reviewed changes
Comment threadlightning/src/ln/functional_tests.rs
Outdated

	fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: 253 };
	persister = test_utils::TestPersister::new();
	monitor = test_utils::TestChainMonitor::new(Some(&chain_source), &tx_broadcaster, &logger, &fee_estimator, &persister);
	monitor = test_utils::TestChainMonitor::new(Some(&chain_source), &tx_broadcaster, &logger, &fee_estimator, &persister, &keys_manager);
@TheBlueMattTheBlueMattJan 11, 2021

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.

Hmm, at least here, maybe elsewhere - dont we create the TestKeysInterface a few lines up, meaning we lose the state tracking?
@devrandomdevrandomJan 11, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

This is done, but together with the fix for the other nit, a significant issue was uncovered in data_loss_protect handling.
It looks like the NodeDrop implementation does a serialization round-trip, which ends up trying to broadcast a revoked commitment tx, since we restored an old state. This seems to be an actual issue with channel reestablish - not sure how you intended it to recover going forward since it's out of sync. It seems like if there's data-loss, we have to update our commitment number instead of just erroring in channel_reestablish?
@TheBlueMattTheBlueMattJan 11, 2021

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.

Hmm, can you push the branch that demonstrates that somewhere? I suppose the "right" answer could be to not panic, return an Err, make sure ChannelMonitor handles that fine (it should?) and then hope we can broadcast the current state when our counterparty broadcasts theirs. That's not quite enough because we should be able to broadcast the old state manually, but then it'd be up to the signer to accept a stale state if we give up waiting on our counterparty.
@TheBlueMattTheBlueMattJan 11, 2021

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.

I suppose we could tweak the signer to check a boolean whether we should panic or not and in a few tests (like this one), let it Err instead of panicing and then test that we can still claim a counterparty-broadcasted tx or retry broadcasting our state later.
@devrandomdevrandomJan 12, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Sorry, pushed now, forgot yesterday.
@devrandomdevrandomJan 12, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

It seems like full support for data_loss_protect would entail putting the system in a state where it doesn't try to go on-chain, because we don't have the needed data for a non-revoked tx. Punting on that for later, I've added a commit that marks the node as failed and doesn't try to execute Node::Drop for the node that suffered the data-loss in test_data_loss_protect.
@devrandomdevrandomJan 19, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

As discussed elsewhere, we now return Err when there is a revocation issue and err_on_sign_revoked_holder_tx is on in TestKeysInterface / EnforcingChannelKeys.
@devrandomdevrandomJan 19, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

... however, this workaround no longer works as a result of OnChainTx.holder_commitment no longer being optional. As part of that change, we panic if an Err is return when trying to sign the latest commitment. Also, this is now an issue also in tests that test justice reactions by simulating a bad actor - they panic during HTLC claims when they react to their own broadcast.
I see a couple of approaches:
  • propagate the Err up the call stack and don't panic
  • simulate bad actor / data loss more accurately by rolling back the signer or at least the revocation counter
The rebase that fails tests was pushed just now.
@devrandomdevrandomJan 20, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Actually, the simplest fix is to disable the revocation enforcement for these spots.
@devrandom
devrandomforce-pushed
the
revoke-enforcement
branch
3 times, most recently
from
a1644a2 to
c8bd159CompareJanuary 13, 2021 22:47
@devrandom
devrandom
commented
Jan 13, 2021

Copy link


Copy Markdown

MemberAuthor
Due to issues described in #775, test_data_loss_protect isn't quite right, but we're not going to fix this right now.
The panic has been changed to an Err.
@TheBlueMatt
TheBlueMatt
commented
Jan 13, 2021

Copy link


Copy Markdown

Collaborator
The panic has been changed to an Err.
Can we have the test enforcing signer have an internal setting to either panic or return an Err so we still ensure we hit panics in most tests, with a comment pointing to the new issue in test_data_loss_protect.
@devrandom
devrandom
commented
Jan 14, 2021

Copy link


Copy Markdown

MemberAuthor
Added err_on_sign_revoked_holder_tx. The issue is mentioned at the top of test_data_loss_protect
TheBlueMatt
TheBlueMatt
approved these changes
Jan 15, 2021
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.

This looks good mod the fuzzers failing to compile. One nit bug no need to fix it now.
Comment threadlightning/src/util/enforcing_trait_impls.rs
Outdated

@devrandom
devrandomforce-pushed
the
revoke-enforcement
branch
2 times, most recently
from
10a87ba to
ebc80c3CompareJanuary 15, 2021 02:36
@devrandom
Use TestKeysInterface in functional tests
2cbb835
This allows stateful validation in EnforcingChannelKeys
@devrandom
Revocation enforcement in signer
a5869b9
We want to make sure that we don't sign revoked transactions.
Given that ChannelKeys are not singletons and revocation enforcement is stateful,
we need to store the revocation state in KeysInterface.
@devrandom
Fix fuzzing issue with revocation
bd4345d
@devrandom
devrandom
commented
Jan 19, 2021

Copy link


Copy Markdown

MemberAuthor
Ran into an issue in #764 (comment) after rebase, will look into possible approaches.
@devrandom
devrandom
commented
Jan 20, 2021

Copy link


Copy Markdown

MemberAuthor
Ran into an issue in #764 (comment) after rebase, will look into possible approaches.
This has been corrected, and the PR is ready for final review.
@devrandom
devrandomforce-pushed
the
revoke-enforcement
branch
2 times, most recently
from
f240e4a to
55f4ef8CompareJanuary 20, 2021 00:07
ariard
ariard
reviewed
Jan 20, 2021
View reviewed changes
@ariardariard
left a comment


Copy link


Copy Markdown

Choose a reason for hiding this comment

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

Just minor comments otherwise SGTM.
Comment threadlightning/src/util/enforcing_trait_impls.rs
Outdated

Comment threadlightning/src/util/enforcing_trait_impls.rs
Comment threadlightning/src/util/enforcing_trait_impls.rs
@devrandom
devrandom
commented
Jan 21, 2021

Copy link


Copy Markdown

MemberAuthor
Added some more docs, please take a look. I believe this addresses all outstanding comments.
@devrandom
Let some tests disable revocation policy check
142b0d6
When simulating a bad actor that broadcasts a revoked tx, the policy check would otherwise panic.
@TheBlueMatt
TheBlueMatt
merged commit f151c02
into
lightningdevkit:mainJan 25, 2021
@ariard
ariard
commented
Jan 25, 2021

Copy link


Copy Markdown

Post-Merge Code Review ACK 142b0d6
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@arik-soarik-soarik-so left review comments

+1 more reviewer

@ariardariardariard left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@devrandom@TheBlueMatt@ariard@arik-so

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Strip utm_, fbclid, gclid, etc. from all links on page
(function() {
var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',
'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',
'ref', 'ref_src', 'source', 'medium', 'campaign'];
function cleanUrl(url) {
try {
var u = new URL(url, window.location.origin);
var changed = false;
trackingParams.forEach(function(p) {
if (u.searchParams.has(p)) {
u.searchParams.delete(p);
changed = true;
}
});
return changed ? u.toString() : url;
} catch (e) {
return url;
}
}
function cleanLinks() {
document.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
cleanLinks();
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1) {
if (node.tagName === 'A') cleanLinks();
node.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '
Revocation enforcement by devrandom · Pull Request #764 · lightningdevkit/rust-lightning · GitHub
Skip to content
Revocation enforcement - #764
Merged
TheBlueMatt merged 4 commits into
lightningdevkit:mainfrom 
lightning-signer:revoke-enforcement
Jan 25, 2021
Merged
Revocation enforcement#764
TheBlueMatt merged 4 commits into
lightningdevkit:mainfrom 
lightning-signer:revoke-enforcement
Conversation
@devrandom
@devrandomdevrandom
commented
Dec 5, 2020

Copy link


Copy Markdown

Member
We want to make sure that we don't sign revoked transactions.
Given that ChannelKeys (actually EnforcingChannelKeys) are not singletons and revocation enforcement is stateful, we need to store the revocation state in KeysInterface.
This builds on #761. The first new commit "Use TestKeysInterface in functional tests" could be folded in to that PR.
@devrandomdevrandom
mentioned this pull request
Dec 5, 2020
 Merged

@devrandom
devrandomforce-pushed
the
revoke-enforcement
branch
2 times, most recently
from
8bbb6d6 to
3e82940CompareDecember 5, 2020 16:58
@codecov
codecovBot
commented
Dec 5, 2020•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #764 (142b0d6) into main (21a44da) will increase coverage by 0.04%.

The diff coverage is 96.26%.
Impacted file tree graph
@@ Coverage Diff @@## main #764 +/- ##
==========================================
+ Coverage 91.24% 91.28% +0.04% 
==========================================
Files 37 37 Lines 22846 22920 +74 ==========================================
+ Hits 20845 20922 +77 + Misses 2001 1998 -3 
Impacted FilesCoverage Δ
lightning/src/util/enforcing_trait_impls.rs90.19% <80.76%> (-9.81%)⬇️
lightning/src/chain/keysinterface.rs93.47% <100.00%> (+0.17%)⬆️
lightning/src/ln/chanmon_update_fail_tests.rs97.56% <100.00%> (ø)
lightning/src/ln/functional_test_utils.rs95.13% <100.00%> (+<0.01%)⬆️
lightning/src/ln/functional_tests.rs97.18% <100.00%> (+0.23%)⬆️
lightning/src/ln/onchaintx.rs94.02% <100.00%> (ø)
lightning/src/util/test_utils.rs84.78% <100.00%> (+1.10%)⬆️
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data

Powered by Codecov. Last update 21a44da...142b0d6. Read the comment docs.
@devrandom
devrandom
commented
Dec 5, 2020

Copy link


Copy Markdown

MemberAuthor
I'm seeing a fuzzing problem, but after trying to fix it, looks like there's an actual out of order revocation.
The fuzz target is: TARGET=chanmon_consistency HEX=2d321d51ffff0b1030341d51ffff0b100a2d
@devrandom
devrandomforce-pushed
the
revoke-enforcement
branch
2 times, most recently
from
27b2eee to
dff0448CompareDecember 7, 2020 12:32
@devrandomdevrandom
changed the title
Revoke enforcementRevocation enforcementDec 7, 2020
@devrandom
devrandom
commented
Dec 7, 2020

Copy link


Copy Markdown

MemberAuthor
Fuzzing issue fixed
arik-so
arik-so
reviewed
Dec 15, 2020
View reviewed changes
Comment threadlightning/src/util/enforcing_trait_impls.rs
Outdated

Comment threadlightning/src/util/enforcing_trait_impls.rs
Outdated

Comment threadlightning/src/util/enforcing_trait_impls.rs
@devrandom
devrandomforce-pushed
the
revoke-enforcement
branch
3 times, most recently
from
07e0a82 to
9b840d7CompareJanuary 9, 2021 02:09
TheBlueMatt
TheBlueMatt
approved these changes
Jan 11, 2021
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.

One nit.
Comment threadlightning/src/util/enforcing_trait_impls.rs
Outdated

TheBlueMatt
TheBlueMatt
reviewed
Jan 11, 2021
View reviewed changes
Comment threadlightning/src/ln/functional_tests.rs
Outdated

	fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: 253 };
	persister = test_utils::TestPersister::new();
	monitor = test_utils::TestChainMonitor::new(Some(&chain_source), &tx_broadcaster, &logger, &fee_estimator, &persister);
	monitor = test_utils::TestChainMonitor::new(Some(&chain_source), &tx_broadcaster, &logger, &fee_estimator, &persister, &keys_manager);
@TheBlueMattTheBlueMattJan 11, 2021

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.

Hmm, at least here, maybe elsewhere - dont we create the TestKeysInterface a few lines up, meaning we lose the state tracking?
@devrandomdevrandomJan 11, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

This is done, but together with the fix for the other nit, a significant issue was uncovered in data_loss_protect handling.
It looks like the NodeDrop implementation does a serialization round-trip, which ends up trying to broadcast a revoked commitment tx, since we restored an old state. This seems to be an actual issue with channel reestablish - not sure how you intended it to recover going forward since it's out of sync. It seems like if there's data-loss, we have to update our commitment number instead of just erroring in channel_reestablish?
@TheBlueMattTheBlueMattJan 11, 2021

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.

Hmm, can you push the branch that demonstrates that somewhere? I suppose the "right" answer could be to not panic, return an Err, make sure ChannelMonitor handles that fine (it should?) and then hope we can broadcast the current state when our counterparty broadcasts theirs. That's not quite enough because we should be able to broadcast the old state manually, but then it'd be up to the signer to accept a stale state if we give up waiting on our counterparty.
@TheBlueMattTheBlueMattJan 11, 2021

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.

I suppose we could tweak the signer to check a boolean whether we should panic or not and in a few tests (like this one), let it Err instead of panicing and then test that we can still claim a counterparty-broadcasted tx or retry broadcasting our state later.
@devrandomdevrandomJan 12, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Sorry, pushed now, forgot yesterday.
@devrandomdevrandomJan 12, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

It seems like full support for data_loss_protect would entail putting the system in a state where it doesn't try to go on-chain, because we don't have the needed data for a non-revoked tx. Punting on that for later, I've added a commit that marks the node as failed and doesn't try to execute Node::Drop for the node that suffered the data-loss in test_data_loss_protect.
@devrandomdevrandomJan 19, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

As discussed elsewhere, we now return Err when there is a revocation issue and err_on_sign_revoked_holder_tx is on in TestKeysInterface / EnforcingChannelKeys.
@devrandomdevrandomJan 19, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

... however, this workaround no longer works as a result of OnChainTx.holder_commitment no longer being optional. As part of that change, we panic if an Err is return when trying to sign the latest commitment. Also, this is now an issue also in tests that test justice reactions by simulating a bad actor - they panic during HTLC claims when they react to their own broadcast.
I see a couple of approaches:
  • propagate the Err up the call stack and don't panic
  • simulate bad actor / data loss more accurately by rolling back the signer or at least the revocation counter
The rebase that fails tests was pushed just now.
@devrandomdevrandomJan 20, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Actually, the simplest fix is to disable the revocation enforcement for these spots.
@devrandom
devrandomforce-pushed
the
revoke-enforcement
branch
3 times, most recently
from
a1644a2 to
c8bd159CompareJanuary 13, 2021 22:47
@devrandom
devrandom
commented
Jan 13, 2021

Copy link


Copy Markdown

MemberAuthor
Due to issues described in #775, test_data_loss_protect isn't quite right, but we're not going to fix this right now.
The panic has been changed to an Err.
@TheBlueMatt
TheBlueMatt
commented
Jan 13, 2021

Copy link


Copy Markdown

Collaborator
The panic has been changed to an Err.
Can we have the test enforcing signer have an internal setting to either panic or return an Err so we still ensure we hit panics in most tests, with a comment pointing to the new issue in test_data_loss_protect.
@devrandom
devrandom
commented
Jan 14, 2021

Copy link


Copy Markdown

MemberAuthor
Added err_on_sign_revoked_holder_tx. The issue is mentioned at the top of test_data_loss_protect
TheBlueMatt
TheBlueMatt
approved these changes
Jan 15, 2021
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.

This looks good mod the fuzzers failing to compile. One nit bug no need to fix it now.
Comment threadlightning/src/util/enforcing_trait_impls.rs
Outdated

@devrandom
devrandomforce-pushed
the
revoke-enforcement
branch
2 times, most recently
from
10a87ba to
ebc80c3CompareJanuary 15, 2021 02:36
@devrandom
Use TestKeysInterface in functional tests
2cbb835
This allows stateful validation in EnforcingChannelKeys
@devrandom
Revocation enforcement in signer
a5869b9
We want to make sure that we don't sign revoked transactions.
Given that ChannelKeys are not singletons and revocation enforcement is stateful,
we need to store the revocation state in KeysInterface.
@devrandom
Fix fuzzing issue with revocation
bd4345d
@devrandom
devrandom
commented
Jan 19, 2021

Copy link


Copy Markdown

MemberAuthor
Ran into an issue in #764 (comment) after rebase, will look into possible approaches.
@devrandom
devrandom
commented
Jan 20, 2021

Copy link


Copy Markdown

MemberAuthor
Ran into an issue in #764 (comment) after rebase, will look into possible approaches.
This has been corrected, and the PR is ready for final review.
@devrandom
devrandomforce-pushed
the
revoke-enforcement
branch
2 times, most recently
from
f240e4a to
55f4ef8CompareJanuary 20, 2021 00:07
ariard
ariard
reviewed
Jan 20, 2021
View reviewed changes
@ariardariard
left a comment


Copy link


Copy Markdown

Choose a reason for hiding this comment

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

Just minor comments otherwise SGTM.
Comment threadlightning/src/util/enforcing_trait_impls.rs
Outdated

Comment threadlightning/src/util/enforcing_trait_impls.rs
Comment threadlightning/src/util/enforcing_trait_impls.rs
@devrandom
devrandom
commented
Jan 21, 2021

Copy link


Copy Markdown

MemberAuthor
Added some more docs, please take a look. I believe this addresses all outstanding comments.
@devrandom
Let some tests disable revocation policy check
142b0d6
When simulating a bad actor that broadcasts a revoked tx, the policy check would otherwise panic.
@TheBlueMatt
TheBlueMatt
merged commit f151c02
into
lightningdevkit:mainJan 25, 2021
@ariard
ariard
commented
Jan 25, 2021

Copy link


Copy Markdown

Post-Merge Code Review ACK 142b0d6
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@arik-soarik-soarik-so left review comments

+1 more reviewer

@ariardariardariard left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@devrandom@TheBlueMatt@ariard@arik-so

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Auto-enable theater mode on YouTube
(function() {
function tryTheater() {
var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]');
if (btn && !btn.classList.contains('activated')) {
btn.click();
}
}
// Try immediately
tryTheater();
// Try after navigation (SPA)
var lastUrl = location.href;
setInterval(function() {
if (location.href !== lastUrl) {
lastUrl = location.href;
setTimeout(tryTheater, 500);
}
}, 1000);
// Also try on player load
var observer = new MutationObserver(tryTheater);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Revocation enforcement by devrandom · Pull Request #764 · lightningdevkit/rust-lightning · GitHub
Skip to content
Revocation enforcement - #764
Merged
TheBlueMatt merged 4 commits into
lightningdevkit:mainfrom 
lightning-signer:revoke-enforcement
Jan 25, 2021
Merged
Revocation enforcement#764
TheBlueMatt merged 4 commits into
lightningdevkit:mainfrom 
lightning-signer:revoke-enforcement
Conversation
@devrandom
@devrandomdevrandom
commented
Dec 5, 2020

Copy link


Copy Markdown

Member
We want to make sure that we don't sign revoked transactions.
Given that ChannelKeys (actually EnforcingChannelKeys) are not singletons and revocation enforcement is stateful, we need to store the revocation state in KeysInterface.
This builds on #761. The first new commit "Use TestKeysInterface in functional tests" could be folded in to that PR.
@devrandomdevrandom
mentioned this pull request
Dec 5, 2020
 Merged

@devrandom
devrandomforce-pushed
the
revoke-enforcement
branch
2 times, most recently
from
8bbb6d6 to
3e82940CompareDecember 5, 2020 16:58
@codecov
codecovBot
commented
Dec 5, 2020•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #764 (142b0d6) into main (21a44da) will increase coverage by 0.04%.

The diff coverage is 96.26%.
Impacted file tree graph
@@ Coverage Diff @@## main #764 +/- ##
==========================================
+ Coverage 91.24% 91.28% +0.04% 
==========================================
Files 37 37 Lines 22846 22920 +74 ==========================================
+ Hits 20845 20922 +77 + Misses 2001 1998 -3 
Impacted FilesCoverage Δ
lightning/src/util/enforcing_trait_impls.rs90.19% <80.76%> (-9.81%)⬇️
lightning/src/chain/keysinterface.rs93.47% <100.00%> (+0.17%)⬆️
lightning/src/ln/chanmon_update_fail_tests.rs97.56% <100.00%> (ø)
lightning/src/ln/functional_test_utils.rs95.13% <100.00%> (+<0.01%)⬆️
lightning/src/ln/functional_tests.rs97.18% <100.00%> (+0.23%)⬆️
lightning/src/ln/onchaintx.rs94.02% <100.00%> (ø)
lightning/src/util/test_utils.rs84.78% <100.00%> (+1.10%)⬆️
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data

Powered by Codecov. Last update 21a44da...142b0d6. Read the comment docs.
@devrandom
devrandom
commented
Dec 5, 2020

Copy link


Copy Markdown

MemberAuthor
I'm seeing a fuzzing problem, but after trying to fix it, looks like there's an actual out of order revocation.
The fuzz target is: TARGET=chanmon_consistency HEX=2d321d51ffff0b1030341d51ffff0b100a2d
@devrandom
devrandomforce-pushed
the
revoke-enforcement
branch
2 times, most recently
from
27b2eee to
dff0448CompareDecember 7, 2020 12:32
@devrandomdevrandom
changed the title
Revoke enforcementRevocation enforcementDec 7, 2020
@devrandom
devrandom
commented
Dec 7, 2020

Copy link


Copy Markdown

MemberAuthor
Fuzzing issue fixed
arik-so
arik-so
reviewed
Dec 15, 2020
View reviewed changes
Comment threadlightning/src/util/enforcing_trait_impls.rs
Outdated

Comment threadlightning/src/util/enforcing_trait_impls.rs
Outdated

Comment threadlightning/src/util/enforcing_trait_impls.rs
@devrandom
devrandomforce-pushed
the
revoke-enforcement
branch
3 times, most recently
from
07e0a82 to
9b840d7CompareJanuary 9, 2021 02:09
TheBlueMatt
TheBlueMatt
approved these changes
Jan 11, 2021
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.

One nit.
Comment threadlightning/src/util/enforcing_trait_impls.rs
Outdated

TheBlueMatt
TheBlueMatt
reviewed
Jan 11, 2021
View reviewed changes
Comment threadlightning/src/ln/functional_tests.rs
Outdated

	fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: 253 };
	persister = test_utils::TestPersister::new();
	monitor = test_utils::TestChainMonitor::new(Some(&chain_source), &tx_broadcaster, &logger, &fee_estimator, &persister);
	monitor = test_utils::TestChainMonitor::new(Some(&chain_source), &tx_broadcaster, &logger, &fee_estimator, &persister, &keys_manager);
@TheBlueMattTheBlueMattJan 11, 2021

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.

Hmm, at least here, maybe elsewhere - dont we create the TestKeysInterface a few lines up, meaning we lose the state tracking?
@devrandomdevrandomJan 11, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

This is done, but together with the fix for the other nit, a significant issue was uncovered in data_loss_protect handling.
It looks like the NodeDrop implementation does a serialization round-trip, which ends up trying to broadcast a revoked commitment tx, since we restored an old state. This seems to be an actual issue with channel reestablish - not sure how you intended it to recover going forward since it's out of sync. It seems like if there's data-loss, we have to update our commitment number instead of just erroring in channel_reestablish?
@TheBlueMattTheBlueMattJan 11, 2021

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.

Hmm, can you push the branch that demonstrates that somewhere? I suppose the "right" answer could be to not panic, return an Err, make sure ChannelMonitor handles that fine (it should?) and then hope we can broadcast the current state when our counterparty broadcasts theirs. That's not quite enough because we should be able to broadcast the old state manually, but then it'd be up to the signer to accept a stale state if we give up waiting on our counterparty.
@TheBlueMattTheBlueMattJan 11, 2021

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.

I suppose we could tweak the signer to check a boolean whether we should panic or not and in a few tests (like this one), let it Err instead of panicing and then test that we can still claim a counterparty-broadcasted tx or retry broadcasting our state later.
@devrandomdevrandomJan 12, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Sorry, pushed now, forgot yesterday.
@devrandomdevrandomJan 12, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

It seems like full support for data_loss_protect would entail putting the system in a state where it doesn't try to go on-chain, because we don't have the needed data for a non-revoked tx. Punting on that for later, I've added a commit that marks the node as failed and doesn't try to execute Node::Drop for the node that suffered the data-loss in test_data_loss_protect.
@devrandomdevrandomJan 19, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

As discussed elsewhere, we now return Err when there is a revocation issue and err_on_sign_revoked_holder_tx is on in TestKeysInterface / EnforcingChannelKeys.
@devrandomdevrandomJan 19, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

... however, this workaround no longer works as a result of OnChainTx.holder_commitment no longer being optional. As part of that change, we panic if an Err is return when trying to sign the latest commitment. Also, this is now an issue also in tests that test justice reactions by simulating a bad actor - they panic during HTLC claims when they react to their own broadcast.
I see a couple of approaches:
  • propagate the Err up the call stack and don't panic
  • simulate bad actor / data loss more accurately by rolling back the signer or at least the revocation counter
The rebase that fails tests was pushed just now.
@devrandomdevrandomJan 20, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Actually, the simplest fix is to disable the revocation enforcement for these spots.
@devrandom
devrandomforce-pushed
the
revoke-enforcement
branch
3 times, most recently
from
a1644a2 to
c8bd159CompareJanuary 13, 2021 22:47
@devrandom
devrandom
commented
Jan 13, 2021

Copy link


Copy Markdown

MemberAuthor
Due to issues described in #775, test_data_loss_protect isn't quite right, but we're not going to fix this right now.
The panic has been changed to an Err.
@TheBlueMatt
TheBlueMatt
commented
Jan 13, 2021

Copy link


Copy Markdown

Collaborator
The panic has been changed to an Err.
Can we have the test enforcing signer have an internal setting to either panic or return an Err so we still ensure we hit panics in most tests, with a comment pointing to the new issue in test_data_loss_protect.
@devrandom
devrandom
commented
Jan 14, 2021

Copy link


Copy Markdown

MemberAuthor
Added err_on_sign_revoked_holder_tx. The issue is mentioned at the top of test_data_loss_protect
TheBlueMatt
TheBlueMatt
approved these changes
Jan 15, 2021
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.

This looks good mod the fuzzers failing to compile. One nit bug no need to fix it now.
Comment threadlightning/src/util/enforcing_trait_impls.rs
Outdated

@devrandom
devrandomforce-pushed
the
revoke-enforcement
branch
2 times, most recently
from
10a87ba to
ebc80c3CompareJanuary 15, 2021 02:36
@devrandom
Use TestKeysInterface in functional tests
2cbb835
This allows stateful validation in EnforcingChannelKeys
@devrandom
Revocation enforcement in signer
a5869b9
We want to make sure that we don't sign revoked transactions.
Given that ChannelKeys are not singletons and revocation enforcement is stateful,
we need to store the revocation state in KeysInterface.
@devrandom
Fix fuzzing issue with revocation
bd4345d
@devrandom
devrandom
commented
Jan 19, 2021

Copy link


Copy Markdown

MemberAuthor
Ran into an issue in #764 (comment) after rebase, will look into possible approaches.
@devrandom
devrandom
commented
Jan 20, 2021

Copy link


Copy Markdown

MemberAuthor
Ran into an issue in #764 (comment) after rebase, will look into possible approaches.
This has been corrected, and the PR is ready for final review.
@devrandom
devrandomforce-pushed
the
revoke-enforcement
branch
2 times, most recently
from
f240e4a to
55f4ef8CompareJanuary 20, 2021 00:07
ariard
ariard
reviewed
Jan 20, 2021
View reviewed changes
@ariardariard
left a comment


Copy link


Copy Markdown

Choose a reason for hiding this comment

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

Just minor comments otherwise SGTM.
Comment threadlightning/src/util/enforcing_trait_impls.rs
Outdated

Comment threadlightning/src/util/enforcing_trait_impls.rs
Comment threadlightning/src/util/enforcing_trait_impls.rs
@devrandom
devrandom
commented
Jan 21, 2021

Copy link


Copy Markdown

MemberAuthor
Added some more docs, please take a look. I believe this addresses all outstanding comments.
@devrandom
Let some tests disable revocation policy check
142b0d6
When simulating a bad actor that broadcasts a revoked tx, the policy check would otherwise panic.
@TheBlueMatt
TheBlueMatt
merged commit f151c02
into
lightningdevkit:mainJan 25, 2021
@ariard
ariard
commented
Jan 25, 2021

Copy link


Copy Markdown

Post-Merge Code Review ACK 142b0d6
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@arik-soarik-soarik-so left review comments

+1 more reviewer

@ariardariardariard left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@devrandom@TheBlueMatt@ariard@arik-so

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Remove or un-stick sticky/fixed headers that block content
(function() {
function unstick() {
document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) {
if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {
el.style.position = 'static';
el.style.top = 'auto';
el.style.zIndex = 'auto';
}
});
}
unstick();
var observer = new MutationObserver(unstick);
observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });
})();
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Revocation enforcement by devrandom · Pull Request #764 · lightningdevkit/rust-lightning · GitHub
Skip to content
Revocation enforcement - #764
Merged
TheBlueMatt merged 4 commits into
lightningdevkit:mainfrom 
lightning-signer:revoke-enforcement
Jan 25, 2021
Merged
Revocation enforcement#764
TheBlueMatt merged 4 commits into
lightningdevkit:mainfrom 
lightning-signer:revoke-enforcement
Conversation
@devrandom
@devrandomdevrandom
commented
Dec 5, 2020

Copy link


Copy Markdown

Member
We want to make sure that we don't sign revoked transactions.
Given that ChannelKeys (actually EnforcingChannelKeys) are not singletons and revocation enforcement is stateful, we need to store the revocation state in KeysInterface.
This builds on #761. The first new commit "Use TestKeysInterface in functional tests" could be folded in to that PR.
@devrandomdevrandom
mentioned this pull request
Dec 5, 2020
 Merged

@devrandom
devrandomforce-pushed
the
revoke-enforcement
branch
2 times, most recently
from
8bbb6d6 to
3e82940CompareDecember 5, 2020 16:58
@codecov
codecovBot
commented
Dec 5, 2020•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #764 (142b0d6) into main (21a44da) will increase coverage by 0.04%.

The diff coverage is 96.26%.
Impacted file tree graph
@@ Coverage Diff @@## main #764 +/- ##
==========================================
+ Coverage 91.24% 91.28% +0.04% 
==========================================
Files 37 37 Lines 22846 22920 +74 ==========================================
+ Hits 20845 20922 +77 + Misses 2001 1998 -3 
Impacted FilesCoverage Δ
lightning/src/util/enforcing_trait_impls.rs90.19% <80.76%> (-9.81%)⬇️
lightning/src/chain/keysinterface.rs93.47% <100.00%> (+0.17%)⬆️
lightning/src/ln/chanmon_update_fail_tests.rs97.56% <100.00%> (ø)
lightning/src/ln/functional_test_utils.rs95.13% <100.00%> (+<0.01%)⬆️
lightning/src/ln/functional_tests.rs97.18% <100.00%> (+0.23%)⬆️
lightning/src/ln/onchaintx.rs94.02% <100.00%> (ø)
lightning/src/util/test_utils.rs84.78% <100.00%> (+1.10%)⬆️
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data

Powered by Codecov. Last update 21a44da...142b0d6. Read the comment docs.
@devrandom
devrandom
commented
Dec 5, 2020

Copy link


Copy Markdown

MemberAuthor
I'm seeing a fuzzing problem, but after trying to fix it, looks like there's an actual out of order revocation.
The fuzz target is: TARGET=chanmon_consistency HEX=2d321d51ffff0b1030341d51ffff0b100a2d
@devrandom
devrandomforce-pushed
the
revoke-enforcement
branch
2 times, most recently
from
27b2eee to
dff0448CompareDecember 7, 2020 12:32
@devrandomdevrandom
changed the title
Revoke enforcementRevocation enforcementDec 7, 2020
@devrandom
devrandom
commented
Dec 7, 2020

Copy link


Copy Markdown

MemberAuthor
Fuzzing issue fixed
arik-so
arik-so
reviewed
Dec 15, 2020
View reviewed changes
Comment threadlightning/src/util/enforcing_trait_impls.rs
Outdated

Comment threadlightning/src/util/enforcing_trait_impls.rs
Outdated

Comment threadlightning/src/util/enforcing_trait_impls.rs
@devrandom
devrandomforce-pushed
the
revoke-enforcement
branch
3 times, most recently
from
07e0a82 to
9b840d7CompareJanuary 9, 2021 02:09
TheBlueMatt
TheBlueMatt
approved these changes
Jan 11, 2021
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.

One nit.
Comment threadlightning/src/util/enforcing_trait_impls.rs
Outdated

TheBlueMatt
TheBlueMatt
reviewed
Jan 11, 2021
View reviewed changes
Comment threadlightning/src/ln/functional_tests.rs
Outdated

	fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: 253 };
	persister = test_utils::TestPersister::new();
	monitor = test_utils::TestChainMonitor::new(Some(&chain_source), &tx_broadcaster, &logger, &fee_estimator, &persister);
	monitor = test_utils::TestChainMonitor::new(Some(&chain_source), &tx_broadcaster, &logger, &fee_estimator, &persister, &keys_manager);
@TheBlueMattTheBlueMattJan 11, 2021

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.

Hmm, at least here, maybe elsewhere - dont we create the TestKeysInterface a few lines up, meaning we lose the state tracking?
@devrandomdevrandomJan 11, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

This is done, but together with the fix for the other nit, a significant issue was uncovered in data_loss_protect handling.
It looks like the NodeDrop implementation does a serialization round-trip, which ends up trying to broadcast a revoked commitment tx, since we restored an old state. This seems to be an actual issue with channel reestablish - not sure how you intended it to recover going forward since it's out of sync. It seems like if there's data-loss, we have to update our commitment number instead of just erroring in channel_reestablish?
@TheBlueMattTheBlueMattJan 11, 2021

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.

Hmm, can you push the branch that demonstrates that somewhere? I suppose the "right" answer could be to not panic, return an Err, make sure ChannelMonitor handles that fine (it should?) and then hope we can broadcast the current state when our counterparty broadcasts theirs. That's not quite enough because we should be able to broadcast the old state manually, but then it'd be up to the signer to accept a stale state if we give up waiting on our counterparty.
@TheBlueMattTheBlueMattJan 11, 2021

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.

I suppose we could tweak the signer to check a boolean whether we should panic or not and in a few tests (like this one), let it Err instead of panicing and then test that we can still claim a counterparty-broadcasted tx or retry broadcasting our state later.
@devrandomdevrandomJan 12, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Sorry, pushed now, forgot yesterday.
@devrandomdevrandomJan 12, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

It seems like full support for data_loss_protect would entail putting the system in a state where it doesn't try to go on-chain, because we don't have the needed data for a non-revoked tx. Punting on that for later, I've added a commit that marks the node as failed and doesn't try to execute Node::Drop for the node that suffered the data-loss in test_data_loss_protect.
@devrandomdevrandomJan 19, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

As discussed elsewhere, we now return Err when there is a revocation issue and err_on_sign_revoked_holder_tx is on in TestKeysInterface / EnforcingChannelKeys.
@devrandomdevrandomJan 19, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

... however, this workaround no longer works as a result of OnChainTx.holder_commitment no longer being optional. As part of that change, we panic if an Err is return when trying to sign the latest commitment. Also, this is now an issue also in tests that test justice reactions by simulating a bad actor - they panic during HTLC claims when they react to their own broadcast.
I see a couple of approaches:
  • propagate the Err up the call stack and don't panic
  • simulate bad actor / data loss more accurately by rolling back the signer or at least the revocation counter
The rebase that fails tests was pushed just now.
@devrandomdevrandomJan 20, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Actually, the simplest fix is to disable the revocation enforcement for these spots.
@devrandom
devrandomforce-pushed
the
revoke-enforcement
branch
3 times, most recently
from
a1644a2 to
c8bd159CompareJanuary 13, 2021 22:47
@devrandom
devrandom
commented
Jan 13, 2021

Copy link


Copy Markdown

MemberAuthor
Due to issues described in #775, test_data_loss_protect isn't quite right, but we're not going to fix this right now.
The panic has been changed to an Err.
@TheBlueMatt
TheBlueMatt
commented
Jan 13, 2021

Copy link


Copy Markdown

Collaborator
The panic has been changed to an Err.
Can we have the test enforcing signer have an internal setting to either panic or return an Err so we still ensure we hit panics in most tests, with a comment pointing to the new issue in test_data_loss_protect.
@devrandom
devrandom
commented
Jan 14, 2021

Copy link


Copy Markdown

MemberAuthor
Added err_on_sign_revoked_holder_tx. The issue is mentioned at the top of test_data_loss_protect
TheBlueMatt
TheBlueMatt
approved these changes
Jan 15, 2021
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.

This looks good mod the fuzzers failing to compile. One nit bug no need to fix it now.
Comment threadlightning/src/util/enforcing_trait_impls.rs
Outdated

@devrandom
devrandomforce-pushed
the
revoke-enforcement
branch
2 times, most recently
from
10a87ba to
ebc80c3CompareJanuary 15, 2021 02:36
@devrandom
Use TestKeysInterface in functional tests
2cbb835
This allows stateful validation in EnforcingChannelKeys
@devrandom
Revocation enforcement in signer
a5869b9
We want to make sure that we don't sign revoked transactions.
Given that ChannelKeys are not singletons and revocation enforcement is stateful,
we need to store the revocation state in KeysInterface.
@devrandom
Fix fuzzing issue with revocation
bd4345d
@devrandom
devrandom
commented
Jan 19, 2021

Copy link


Copy Markdown

MemberAuthor
Ran into an issue in #764 (comment) after rebase, will look into possible approaches.
@devrandom
devrandom
commented
Jan 20, 2021

Copy link


Copy Markdown

MemberAuthor
Ran into an issue in #764 (comment) after rebase, will look into possible approaches.
This has been corrected, and the PR is ready for final review.
@devrandom
devrandomforce-pushed
the
revoke-enforcement
branch
2 times, most recently
from
f240e4a to
55f4ef8CompareJanuary 20, 2021 00:07
ariard
ariard
reviewed
Jan 20, 2021
View reviewed changes
@ariardariard
left a comment


Copy link


Copy Markdown

Choose a reason for hiding this comment

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

Just minor comments otherwise SGTM.
Comment threadlightning/src/util/enforcing_trait_impls.rs
Outdated

Comment threadlightning/src/util/enforcing_trait_impls.rs
Comment threadlightning/src/util/enforcing_trait_impls.rs
@devrandom
devrandom
commented
Jan 21, 2021

Copy link


Copy Markdown

MemberAuthor
Added some more docs, please take a look. I believe this addresses all outstanding comments.
@devrandom
Let some tests disable revocation policy check
142b0d6
When simulating a bad actor that broadcasts a revoked tx, the policy check would otherwise panic.
@TheBlueMatt
TheBlueMatt
merged commit f151c02
into
lightningdevkit:mainJan 25, 2021
@ariard
ariard
commented
Jan 25, 2021

Copy link


Copy Markdown

Post-Merge Code Review ACK 142b0d6
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@arik-soarik-soarik-so left review comments

+1 more reviewer

@ariardariardariard left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@devrandom@TheBlueMatt@ariard@arik-so

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

Copy link


Copy Markdown

Member
We want to make sure that we don't sign revoked transactions.
Given that ChannelKeys (actually EnforcingChannelKeys) are not singletons and revocation enforcement is stateful, we need to store the revocation state in KeysInterface.
This builds on #761. The first new commit "Use TestKeysInterface in functional tests" could be folded in to that PR.
@devrandomdevrandom
mentioned this pull request
Dec 5, 2020
 Merged

@devrandom
devrandomforce-pushed
the
revoke-enforcement
branch
2 times, most recently
from
8bbb6d6 to
3e82940CompareDecember 5, 2020 16:58
@codecov
codecovBot
commented
Dec 5, 2020•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #764 (142b0d6) into main (21a44da) will increase coverage by 0.04%.

The diff coverage is 96.26%.
Impacted file tree graph
@@ Coverage Diff @@## main #764 +/- ##
==========================================
+ Coverage 91.24% 91.28% +0.04% 
==========================================
Files 37 37 Lines 22846 22920 +74 ==========================================
+ Hits 20845 20922 +77 + Misses 2001 1998 -3 
Impacted FilesCoverage Δ
lightning/src/util/enforcing_trait_impls.rs90.19% <80.76%> (-9.81%)⬇️
lightning/src/chain/keysinterface.rs93.47% <100.00%> (+0.17%)⬆️
lightning/src/ln/chanmon_update_fail_tests.rs97.56% <100.00%> (ø)
lightning/src/ln/functional_test_utils.rs95.13% <100.00%> (+<0.01%)⬆️
lightning/src/ln/functional_tests.rs97.18% <100.00%> (+0.23%)⬆️
lightning/src/ln/onchaintx.rs94.02% <100.00%> (ø)
lightning/src/util/test_utils.rs84.78% <100.00%> (+1.10%)⬆️
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data

Powered by Codecov. Last update 21a44da...142b0d6. Read the comment docs.
@devrandom
devrandom
commented
Dec 5, 2020

Copy link


Copy Markdown

MemberAuthor
I'm seeing a fuzzing problem, but after trying to fix it, looks like there's an actual out of order revocation.
The fuzz target is: TARGET=chanmon_consistency HEX=2d321d51ffff0b1030341d51ffff0b100a2d
@devrandom
devrandomforce-pushed
the
revoke-enforcement
branch
2 times, most recently
from
27b2eee to
dff0448CompareDecember 7, 2020 12:32
@devrandomdevrandom
changed the title
Revoke enforcementRevocation enforcementDec 7, 2020
@devrandom
devrandom
commented
Dec 7, 2020

Copy link


Copy Markdown

MemberAuthor
Fuzzing issue fixed
arik-so
arik-so
reviewed
Dec 15, 2020
View reviewed changes
Comment threadlightning/src/util/enforcing_trait_impls.rs
Outdated

Comment threadlightning/src/util/enforcing_trait_impls.rs
Outdated

Comment threadlightning/src/util/enforcing_trait_impls.rs
@devrandom
devrandomforce-pushed
the
revoke-enforcement
branch
3 times, most recently
from
07e0a82 to
9b840d7CompareJanuary 9, 2021 02:09
TheBlueMatt
TheBlueMatt
approved these changes
Jan 11, 2021
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.

One nit.
Comment threadlightning/src/util/enforcing_trait_impls.rs
Outdated

TheBlueMatt
TheBlueMatt
reviewed
Jan 11, 2021
View reviewed changes
Comment threadlightning/src/ln/functional_tests.rs
Outdated

	fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: 253 };
	persister = test_utils::TestPersister::new();
	monitor = test_utils::TestChainMonitor::new(Some(&chain_source), &tx_broadcaster, &logger, &fee_estimator, &persister);
	monitor = test_utils::TestChainMonitor::new(Some(&chain_source), &tx_broadcaster, &logger, &fee_estimator, &persister, &keys_manager);
@TheBlueMattTheBlueMattJan 11, 2021

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.

Hmm, at least here, maybe elsewhere - dont we create the TestKeysInterface a few lines up, meaning we lose the state tracking?
@devrandomdevrandomJan 11, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

This is done, but together with the fix for the other nit, a significant issue was uncovered in data_loss_protect handling.
It looks like the NodeDrop implementation does a serialization round-trip, which ends up trying to broadcast a revoked commitment tx, since we restored an old state. This seems to be an actual issue with channel reestablish - not sure how you intended it to recover going forward since it's out of sync. It seems like if there's data-loss, we have to update our commitment number instead of just erroring in channel_reestablish?
@TheBlueMattTheBlueMattJan 11, 2021

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.

Hmm, can you push the branch that demonstrates that somewhere? I suppose the "right" answer could be to not panic, return an Err, make sure ChannelMonitor handles that fine (it should?) and then hope we can broadcast the current state when our counterparty broadcasts theirs. That's not quite enough because we should be able to broadcast the old state manually, but then it'd be up to the signer to accept a stale state if we give up waiting on our counterparty.
@TheBlueMattTheBlueMattJan 11, 2021

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.

I suppose we could tweak the signer to check a boolean whether we should panic or not and in a few tests (like this one), let it Err instead of panicing and then test that we can still claim a counterparty-broadcasted tx or retry broadcasting our state later.
@devrandomdevrandomJan 12, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Sorry, pushed now, forgot yesterday.
@devrandomdevrandomJan 12, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

It seems like full support for data_loss_protect would entail putting the system in a state where it doesn't try to go on-chain, because we don't have the needed data for a non-revoked tx. Punting on that for later, I've added a commit that marks the node as failed and doesn't try to execute Node::Drop for the node that suffered the data-loss in test_data_loss_protect.
@devrandomdevrandomJan 19, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

As discussed elsewhere, we now return Err when there is a revocation issue and err_on_sign_revoked_holder_tx is on in TestKeysInterface / EnforcingChannelKeys.
@devrandomdevrandomJan 19, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

... however, this workaround no longer works as a result of OnChainTx.holder_commitment no longer being optional. As part of that change, we panic if an Err is return when trying to sign the latest commitment. Also, this is now an issue also in tests that test justice reactions by simulating a bad actor - they panic during HTLC claims when they react to their own broadcast.
I see a couple of approaches:
  • propagate the Err up the call stack and don't panic
  • simulate bad actor / data loss more accurately by rolling back the signer or at least the revocation counter
The rebase that fails tests was pushed just now.
@devrandomdevrandomJan 20, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Actually, the simplest fix is to disable the revocation enforcement for these spots.
@devrandom
devrandomforce-pushed
the
revoke-enforcement
branch
3 times, most recently
from
a1644a2 to
c8bd159CompareJanuary 13, 2021 22:47
@devrandom
devrandom
commented
Jan 13, 2021

Copy link


Copy Markdown

MemberAuthor
Due to issues described in #775, test_data_loss_protect isn't quite right, but we're not going to fix this right now.
The panic has been changed to an Err.
@TheBlueMatt
TheBlueMatt
commented
Jan 13, 2021

Copy link


Copy Markdown

Collaborator
The panic has been changed to an Err.
Can we have the test enforcing signer have an internal setting to either panic or return an Err so we still ensure we hit panics in most tests, with a comment pointing to the new issue in test_data_loss_protect.
@devrandom
devrandom
commented
Jan 14, 2021

Copy link


Copy Markdown

MemberAuthor
Added err_on_sign_revoked_holder_tx. The issue is mentioned at the top of test_data_loss_protect
TheBlueMatt
TheBlueMatt
approved these changes
Jan 15, 2021
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.

This looks good mod the fuzzers failing to compile. One nit bug no need to fix it now.
Comment threadlightning/src/util/enforcing_trait_impls.rs
Outdated

@devrandom
devrandomforce-pushed
the
revoke-enforcement
branch
2 times, most recently
from
10a87ba to
ebc80c3CompareJanuary 15, 2021 02:36
@devrandom
Use TestKeysInterface in functional tests
2cbb835
This allows stateful validation in EnforcingChannelKeys
@devrandom
Revocation enforcement in signer
a5869b9
We want to make sure that we don't sign revoked transactions.
Given that ChannelKeys are not singletons and revocation enforcement is stateful,
we need to store the revocation state in KeysInterface.
@devrandom
Fix fuzzing issue with revocation
bd4345d
@devrandom
devrandom
commented
Jan 19, 2021

Copy link


Copy Markdown

MemberAuthor
Ran into an issue in #764 (comment) after rebase, will look into possible approaches.
@devrandom
devrandom
commented
Jan 20, 2021

Copy link


Copy Markdown

MemberAuthor
Ran into an issue in #764 (comment) after rebase, will look into possible approaches.
This has been corrected, and the PR is ready for final review.
@devrandom
devrandomforce-pushed
the
revoke-enforcement
branch
2 times, most recently
from
f240e4a to
55f4ef8CompareJanuary 20, 2021 00:07
ariard
ariard
reviewed
Jan 20, 2021
View reviewed changes
@ariardariard
left a comment


Copy link


Copy Markdown

Choose a reason for hiding this comment

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

Just minor comments otherwise SGTM.
Comment threadlightning/src/util/enforcing_trait_impls.rs
Outdated

Comment threadlightning/src/util/enforcing_trait_impls.rs
Comment threadlightning/src/util/enforcing_trait_impls.rs
@devrandom
devrandom
commented
Jan 21, 2021

Copy link


Copy Markdown

MemberAuthor
Added some more docs, please take a look. I believe this addresses all outstanding comments.
@devrandom
Let some tests disable revocation policy check
142b0d6
When simulating a bad actor that broadcasts a revoked tx, the policy check would otherwise panic.
@TheBlueMatt
TheBlueMatt
merged commit f151c02
into
lightningdevkit:mainJan 25, 2021
@ariard
ariard
commented
Jan 25, 2021

Copy link


Copy Markdown

Post-Merge Code Review ACK 142b0d6
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@arik-soarik-soarik-so left review comments

+1 more reviewer

@ariardariardariard left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@devrandom@TheBlueMatt@ariard@arik-so