-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HTLCs #27
base: master
Are you sure you want to change the base?
HTLCs #27
Conversation
- LICENSE: change from MIT to AGPL (.Kiss fork) - Change package name suffix from .Core to .Kiss (skipping native build)
According to lightning spec, if the receiver agrees with the fee, it should reply with a closing_signed with the same fee_satoshis value so to transport this message we need to pass it inside MutualClosePerformed event, to be transported to the other peer. Source: https://github.com/lightningnetwork/lightning-rfc/blob/master/02-peer-protocol.md#requirements-6 Upstream PR: joemphilips#168
Hmac should be written during the packet serialization and not during the payload serialization
All-zero initial packets should only be used for generating test vectors because of possible privacy leak. "Create packet (reference test vector)" testcase updated to new testcases provided by the RFC repo to meet this change of spec. See more: lightning/bolts@8dd0b75 https://github.com/lightningnetwork/lightning-rfc/blob/master/04-onion-routing.md#packet-construction
Secret field inside TLVPayload's PaymentData is PaymentSecret and not PaymentPreimage and It's used for mpp.
channelPrivKeys.SignHtlcTx htlc.Value localPerCommitmentPoint | ||
) | ||
localHtlcSigsAndHTLCTxs |> List.map(fst), localHtlcSigsAndHTLCTxs |> List.map(snd) |> Seq.cast<IHTLCTx> |> List.ofSeq | ||
let HTLCTxsAndSignatures = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
local vars should be camelCase, not PascalCase; now, I understand this var name starts with "HTLC" which is an acronym, but I like to do what the .NET BCL do wrt naming sometimes: HTTP -> Http, therefore HTLC -> Htlc, so here it can be htlcTxsAndSignatures
@@ -285,6 +286,7 @@ module KeyExtensions = | |||
: TransactionSignature * PSBT = | |||
let htlcPrivKey = perCommitmentPoint.DeriveHtlcPrivKey this.HtlcBasepointSecret | |||
let htlcPubKey = htlcPrivKey.HtlcPubKey() | |||
psbt.Settings.CustomBuilderExtensions <- ([new HTLCReceivedExtensions() :> BuilderExtension; new HTLCOfferedExtensions():> BuilderExtension] |> Seq.ofList) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nits:
- missing a space before
:>
- please split line in many, it's too long
- are HTLCReceivedExtensions and HTLCOfferedExtensions IDisposable? if not, then remove the
new
keyword
assert (stream.Length = padding1.Length) | ||
xor(padding1, stream) | ||
) [||] | ||
let rec internal generateFiller (keyType: string) (payloads: byte[] list) (sharedSecrets: Key list) = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use array<byte>
and list<Key>
pls
// set of filler bytes by using chacha20 with a key derived from the session | ||
// key. | ||
let DeterministicPacketFiller (sessionKey: Key) = | ||
generateStream(generateKey("pad",sessionKey.ToBytes()), 1300) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 1300: magic number? pls put in a constant
- space after comma
// instances that required deterministic packet generation. | ||
[<Obsolete("BlankPacketFiller is obsolete, see here: https://github.com/lightningnetwork/lightning-rfc/commit/8dd0b75809c9a7498bb9031a6674e5f58db509f4", false)>] | ||
let BlankPacketFiller _= | ||
Array.zeroCreate 1300 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use the constant for 1300 here
type PacketAndSecrets = { | ||
Packet: OnionPacket | ||
/// Shared secrets (one per node in the route). Known (and needed) only if you're creating the | ||
/// packet. Empty if you're just forwarding the packet to the next node | ||
SharedSecrets: (Key * PubKey) list | ||
} | ||
with | ||
static member Create (sessionKey: Key, pubKeys: PubKey list, payloads: byte[] list, ad: byte[]) = | ||
static member Create (sessionKey: Key, pubKeys: PubKey list, payloads: byte[] list, ad: byte[], initialPacketFiller: Key -> byte[]) = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use array<byte>
pls
@@ -1,4 +1,4 @@ | |||
<Project Sdk="Microsoft.NET.Sdk" xmlns="http://schemas.microsoft.com/developer/msbuild/2003"> | |||
<Project Sdk="Microsoft.NET.Sdk" xmlns="http://schemas.microsoft.com/developer/msbuild/2003"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: no need to change this line
UInt64.FromTruncatedBytes(tlv.Value) | ||
|> LNMoney.MilliSatoshis | ||
|> AmountToForward | ||
| 4UL -> | ||
NBitcoin.Utils.ToUInt32(tlv.Value, false) | ||
UInt32.FromTruncatedBytes(tlv.Value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for parens in the 2 lines of both hunks above
@@ -566,6 +567,7 @@ module Transactions = | |||
ScriptCoin(coin, redeem) | |||
let dest = Scripts.toLocalDelayed localRevocationPubKey toLocalDelay localDelayedPaymentPubKey | |||
// we have already done dust limit check above | |||
txb.Extensions.Add (new HTLCReceivedExtensions()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is HTLCReceivedExtensions IDisposable? if not, then remove the new
@@ -59,6 +59,47 @@ type System.UInt64 with | |||
buf.[8] <- (byte x) | |||
buf | |||
|
|||
member private x.GetLeadingZerosCount() = | |||
if (x = 0UL) then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for parenthesis
3 | ||
else if (x &&& 0xffff000000000000UL = 0UL) then | ||
2 | ||
else if (x &&& 0xff00000000000000UL = 0UL) then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please replace all else if
occurrences with elif
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(and no need for parenthesis in all lines above)
member x.GetTruncatedBytes() = | ||
let numZeros = | ||
x.GetLeadingZerosCount() | ||
x.GetBytesBigEndian() |> Array.skip(numZeros) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for parenthesis at the end
3 | ||
else if (x &&& 0xffff0000u = 0u) then | ||
2 | ||
else if (x &&& 0xff000000u = 0u) then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here about parenthesis and elif
member x.GetTruncatedBytes() = | ||
let numZeros = | ||
x.GetLeadingZerosCount() | ||
x.GetBytesBigEndian() |> Array.skip(numZeros) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for parenthesis here
| Ok data -> Some data | ||
| Error _consumeAllError -> None | ||
|
||
type internal HTLCOfferedExtensions() = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename to HtlcOfferedExtensions as per previous suggestion
@@ -345,23 +345,24 @@ module internal Commitments = | |||
Transactions.checkTxFinalized signedCommitTx localCommitTx.WhichInput sigPair | |||
|> expectTransactionError | |||
let! finalizedCommitTx = tmp | |||
let sortedHTLCTXs = Helpers.sortBothHTLCs htlcTimeoutTxs htlcSuccessTxs | |||
let sortedHTLCTXs = Helpers.sortBothHTLCs htlcTimeoutTxs htlcSuccessTxs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: remove trailing space pls
|> List.ofSeq | ||
|> Seq.ofList | ||
|
||
Console.WriteLine(JsonConvert.SerializeObject(ops)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove Console.WriteLine? if you leave it there, explain why, and use 2 parenthesis instead of 4
// should return null, indicating to NBitcoin that the sigScript | ||
// could not be generated. | ||
match pubKey with | ||
| null -> null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's use Option.ofObj instead of checking for null
let remoteHtlcSig = signer.Sign (parameters.RemoteHtlcPubKey.RawPubKey()) | ||
Script [ | ||
Op.GetPushOp (remoteHtlcSig.ToBytes()) | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's duplication in these 2 blocks, let's have just 1 block
|> List.ofSeq | ||
|> Seq.ofList | ||
|
||
Console.WriteLine(JsonConvert.SerializeObject(ops)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove CWL?
PSBT sign function needs transaction builder for any non-predefined script template
f817da9
to
0340b0b
Compare
0389f6a
to
60d2401
Compare
419cd8d
to
f0461b4
Compare
37fe5af
to
1f418fe
Compare
bd34034
to
b67e13f
Compare
Work in progress...