WIP - track more information about commit history #22
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- LICENSE: change from MIT to AGPL (.Kiss fork) - Change package name suffix from .Core to .Kiss (skipping native build)
It seems that these attributes can cause this compilation error in F#4.0: error FS0927: The kind of the type specified by its attributes does not match the kind implied by its definition Newer versions of F# (like 4.5 or even newer, like the one being used by .NETCore to build the binary that is later published in nuget) allow compiling this code with no issues, but if you reference the generated assembly later from an old F# compiler, it could generate exceptions at runtime, e.g.: System.BadFormatImageException (or other types) whose inner exception could be the following: System.TypeLoadException : Could not load type of field 'GWallet.Backend.UtxoCoin.Lightning.SerializedChannel:MinSafeDepth@' (6) due to: Expected reference type but got type kind 17 FSharp.Core's Result type is also affected by this so in this commit we create a replacement for it that is only used in the BouncyCastle build (which we now rename as 'Portability' build). Forward-ported from a4a59d0 Co-authored-by: Andres G. Aragoneses <knocte@gmail.com> Co-authored-by: Andrew Cann <shum@canndrew.org>
This NBitcoin issue: MetacoSA/NBitcoin#931 somehow still hasn't been fixed properly (our geewallet CI still encounters it, surprisingly). So let's reapply the workaround[1] that we had removed[2]. [1] joemphilips@d813a97 [2] joemphilips@256893c
ResultUtils: new OptionCE, a computation expression for creating options, similar to the result computation expression which DNL already has. Core.Utils: new SeqConsumerCE, a computation expression which makes it easy to write code which consumes a sequence, one element at a time.
In order to support this, this commit adds two elements: 1) CommitmentToLocalExtension This is an NBitcoin BuilderExtension that tells NBitcoin.TransactionBuilder how to recognise and sign to_local txouts from lightning commitment transactions. This is needed for force-closing to spend to_local outputs. 2) tryGetFundsFromLocalCommitmentTx When force-closing a channel this function is used to recover funds from our commitment transaction. It generates a transaction which can be broadcast once the timelock on the to_local output has expired.
This function recovers funds from the to_remote output of a remote commitment tx which has been broadcast to the blockchain.
canndrew
force-pushed
the
new-lightning-milestone3-tx-identification
branch
from
d6d19f7
to
95e95b6
Compare
I see you force-pushed this PR, is it to address the above? if yes, state so here pls |
knocte
reviewed
Jan 19, 2021
| MaybeBestUnpublishedTx = None } | ||
| return [ AcceptedShutdownWhenNoPendingHTLCs(closingSignedMsg |> Some, nextState) ] | ||
| else | ||
| let nextState = { NegotiatingData.ChannelId = cm.ChannelId | ||
| Commitments = cm | ||
| LocalShutdown = localShutdown | ||
| RemoteShutdown = msg | ||
| ClosingTxProposed = [ [] ] | ||
| ClosingTxProposed = [ ] |
There was a problem hiding this comment.
let's rather use List.Empty, as per geewallet's CONTRIBUTING
knocte
reviewed
Jan 19, 2021
| Signed = cm.LocalChanges.Proposed | ||
| } | ||
| RemoteChanges = { | ||
| cm.RemoteChanges with |
There was a problem hiding this comment.
many identation warnings above ^
knocte
reviewed
Jan 19, 2021
| } | ||
| RemoteChanges = { | ||
| cm.RemoteChanges with | ||
| ACKed = [] |
knocte
reviewed
Jan 19, 2021
| LocalSignaturesOfRemoteCommitments = | ||
| List.append | ||
| cm.LocalSignaturesOfRemoteCommitments | ||
| [ !> signature.Signature ] |
There was a problem hiding this comment.
nit: I think foo::List.Empty is more readable [foo]
knocte
reviewed
Jan 19, 2021
| LocalChanges = ourChanges1 | ||
| RemoteChanges = theirChanges1 | ||
| OriginChannels = originChannels1 | ||
| } |
There was a problem hiding this comment.
you're not really changing the lines above, so it's not worth it to change the style
knocte
force-pushed
the
geewalletLightningMilestone3
branch
2 times, most recently
from
de70b20
to
85d6eb8
Compare
knocte
force-pushed
the
geewalletLightningMilestone3
branch
from
3ff7695
to
1d1de3f
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a WIP PR to track more information about commit history in DNL so that we can recover funds from remote commitment txs.