Uh oh!
There was an error while loading. Please reload this page.
Require static_remotekey - #539
Conversation
TheBlueMatt
commented
Mar 19, 2020
ariard
left a comment
There was a problem hiding this comment.
Far simpler than expected, would have my vote to go after ChanMonitor refactoring patchset
| // Thanks to data loss protection, we may be able to claim our non-htlc funds | ||
| // back, this is the script we have to spend from but we need to | ||
| // scan every commitment transaction for that | ||
| to_remote_rescue: Option<(Script, SecretKey)>, |
There was a problem hiding this comment.
Note: I'm already cutting out a lot of code based on to_remote_output after the #562 patchset, so rebasing this on top should even simplify change further.
961f225 to
524c324CompareTheBlueMatt
commented
Apr 28, 2020
Rebased on #590 (which was rebased on master). |
Codecov Report
@@ Coverage Diff @@## master #539 +/- ##
==========================================
+ Coverage 91.12% 91.13% +0.01%
==========================================
Files 34 34 Lines 20544 20504 -40 ==========================================
- Hits 18720 18686 -34 + Misses 1824 1818 -6
Continue to review full report at Codecov.
|
524c324 to
acdd6d8CompareThis makes it easier to amend the full_stack_target test_no_existing_test_breakage test by always providing the neccessary data in the log.
It appears the local signatures which are specified in the channel transaction-generation tests were never checked directly (though they were checked as a part of the overall fully-signed-transaction tests). Check them explicitly so that they can be updated for static remote key.
acdd6d8 to
b007a68CompareTheBlueMatt
commented
Apr 29, 2020
Rebased on master directly now and updated the fuzz tests for the new tx format. |
b007a68 to
0e42876CompareUh oh!
There was an error while loading. Please reload this page.
| DataLossProtect | InitialRoutingSync | UpfrontShutdownScript, | ||
| // Byte 1 | ||
| VariableLengthOnion | PaymentSecret, | ||
| VariableLengthOnion | StaticRemoteKey | PaymentSecret, |
There was a problem hiding this comment.
Please update commit message as anything listed here is considered known as of #590.
There was a problem hiding this comment.
Nah, thats a code bug, not a commit message bug :p.
| 5u8.write(w)?; | ||
| 4u8.write(w)?; |
There was a problem hiding this comment.
I assume there is no concern yet that this changes the serialization format. At what point will we need to be mindful about this?
There was a problem hiding this comment.
I don't think so yet, no, but its probably something worth discussing. My answer was always "0.1", ie "once we actually think its reasonable to start testing on mainnet", but that's probably now "once we support anchor outputs, or at least the insecure-HTLCs version that it looks like we'll get first".
Uh oh!
There was an error while loading. Please reload this page.
| @@ -3467,18 +3456,20 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> { | |||
| pub fn get_channel_reestablish(&self) -> msgs::ChannelReestablish { | |||
| assert_eq!(self.channel_state & ChannelState::PeerDisconnected as u32, ChannelState::PeerDisconnected as u32); | |||
| assert_ne!(self.cur_remote_commitment_transaction_number, INITIAL_COMMITMENT_NUMBER); | |||
| let mut pk = [2; 33]; pk[1] = 0xff; // Select a dummy pubkey which is valid in both "real" and fuzztarget modes | |||
There was a problem hiding this comment.
Not sure I follow what is meant by this comment. If it's valid in "real" mode why wouldn't it also be valid in fuzztarget mode?
There was a problem hiding this comment.
I updated the comment to be more verbose, but essentially fuzztarget mode has an arbitrary validity criteria, which we want to match, as well as actually being valid.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
0e42876 to
a37cd6dCompareariard
left a comment
There was a problem hiding this comment.
Few docs points, but otherwise ACK. I can take the TODO if this get first.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| return Err(ChannelError::CloseDelayBroadcast { | ||
| msg: "We have fallen behind - we have received proof that if we broadcast remote is going to claim our funds - we can't do any automated broadcasting", | ||
| update: monitor_update | ||
| }); |
There was a problem hiding this comment.
Doesn't matter anymore now but did we forget to implement "my_current_per_commitment_point" does not match the expected value"? I'm not even sure you can verify this given you don't have a comparison base if you're fallen behind.
There was a problem hiding this comment.
Hmm, yea, seems so. One of those awkward "checking this has no value except to enforce that people don't generally set it wrong".........
| self.their_pubkeys.as_ref().unwrap().payment_basepoint | ||
| } else { | ||
| self.local_keys.pubkeys().payment_basepoint | ||
| }.serialize()); |
There was a problem hiding this comment.
No? Its indented because its a parameter to a fn.
There was a problem hiding this comment.
My earlier comment was rather to make the expression inside WPubkeyHash::hash() (i.e. the payment basepoint) a local variable, which would be more readable IMHO.
There was a problem hiding this comment.
Ah, ok. Missed that. Also responded too quick to antoine's comment, the whole thing did have an extra indent, I'd intended that for these lines but not the top "let" line. Fixed.
a37cd6d to
e1e3090Compare
jkczyz
left a comment
There was a problem hiding this comment.
One small comment but otherwise looks good!
| self.their_pubkeys.as_ref().unwrap().payment_basepoint | ||
| } else { | ||
| self.local_keys.pubkeys().payment_basepoint | ||
| }.serialize()); |
There was a problem hiding this comment.
My earlier comment was rather to make the expression inside WPubkeyHash::hash() (i.e. the payment basepoint) a local variable, which would be more readable IMHO.
This adds the ability to check for static_remotekey in appropriate feature contexts and prints it at connect time. It is still considered unknown for the purposes of requires_unknown_bits() as we don't yet implement it.
This simplifies channelmonitor quite nicely (as expected) as we never have to be concerned with learning data in a DataLossProtect which is require for us to claim our funds from the latest remote commitment transaction.
We no longer derive any keys from the payment point, so they aren't a "base" but simply a point/key.
e1e3090 to
07db23dCompareTheBlueMatt
commented
May 6, 2020
ariard
commented
May 6, 2020
ACK 07db23d, just fixing my nit comments + Jeff one since last review. |
Based on #441 (tough I could maybe drop that), #472, and #537, this adds support for, requires, and drops code for pre-static_remotekey channels. Since this is gonna be required for simplified_commitment (which we're gonna want for 0.1 so that we don't have to rely on fee prediction) I don't see a reason to support non-static_remotekey channels.
[ ] I need to PR the changes to the test chases for channel transaction unit tests upstream to compare with other implementations.