Skip to content
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

Draft
wants to merge 15 commits into
base: master
Choose a base branch
from
Draft

HTLCs #27

wants to merge 15 commits into from

Conversation

aarani
Copy link

@aarani aarani commented Jun 11, 2021

Work in progress...

knocte and others added 6 commits May 31, 2021 13:52
- 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
channelPrivKeys.SignHtlcTx htlc.Value localPerCommitmentPoint
)
localHtlcSigsAndHTLCTxs |> List.map(fst), localHtlcSigsAndHTLCTxs |> List.map(snd) |> Seq.cast<IHTLCTx> |> List.ofSeq
let HTLCTxsAndSignatures =
Copy link
Member

@knocte knocte Jun 14, 2021

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)
Copy link
Member

@knocte knocte Jun 14, 2021

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) =
Copy link
Member

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)
Copy link
Member

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
Copy link
Member

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[]) =
Copy link
Member

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">
Copy link
Member

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

Comment on lines 111 to 115
UInt64.FromTruncatedBytes(tlv.Value)
|> LNMoney.MilliSatoshis
|> AmountToForward
| 4UL ->
NBitcoin.Utils.ToUInt32(tlv.Value, false)
UInt32.FromTruncatedBytes(tlv.Value)
Copy link
Member

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())
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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

Copy link
Member

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)
Copy link
Member

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
Copy link
Member

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)
Copy link
Member

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() =
Copy link
Member

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
Copy link
Member

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))
Copy link
Member

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
Copy link
Member

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())
]
Copy link
Member

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))
Copy link
Member

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
@canndrew canndrew force-pushed the master branch 4 times, most recently from f817da9 to 0340b0b Compare June 18, 2021 07:38
@aarani aarani changed the base branch from master to upstream June 18, 2021 09:37
@aarani aarani changed the base branch from upstream to old/wip/monohop June 18, 2021 09:38
@aarani aarani changed the base branch from old/wip/monohop to wip/upstream June 18, 2021 09:38
@aarani aarani changed the base branch from wip/upstream to master June 18, 2021 09:38
@knocte knocte force-pushed the master branch 3 times, most recently from 0389f6a to 60d2401 Compare June 22, 2021 13:06
@knocte knocte force-pushed the master branch 4 times, most recently from 419cd8d to f0461b4 Compare November 17, 2021 06:50
@knocte knocte force-pushed the master branch 4 times, most recently from 37fe5af to 1f418fe Compare December 2, 2021 21:08
@knocte knocte force-pushed the master branch 5 times, most recently from bd34034 to b67e13f Compare August 23, 2022 04:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants