We shouldn't use hardcoded values here #19
Conversation
- LICENSE: change from MIT to AGPL (.Kiss fork) - Change package name suffix from .Core to .Kiss (skipping native build)
This commit adds the following: * getFundsFromForceClosingTransaction This function takes an on-chain transaction which spends the channel funds and tries to extract spendable utxos out of it. If successful, it returns a TransactionBuilder with txins added for each spendable output of the closing transaction and with the necessary keys and BuilderExtension added. To recover funds from a broadcast commitment transaction you just need to call this function, add outputs to the returned TransactionBuilder to send the money where you want it to go, then broadcast the transaction. * CommitmentToLocalBuilderExtension This is an NBitcoin BuilderExtension that tells TransactionBuilder how to recognise and sign lightning commitment transaction to_local outputs. getFundsFromForceClosingTransaction adds this extension to the TransactionBuilder it returns if it's needed to sign the transaction. * SeqConsumer A computation expression which makes it easy to write code that consumes a sequence one element at a time. * OptionCE A computation expression for creation options. Similar to the result computation expression.
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
In this commit, we add a function to prepare a Punishment Tx for later broadcasting incase of illegal broadcasting of a revoked transaction.
|
@canndrew please review |
It's not wise to use the word |
Take in account at some point I'll be proposing our commits to upstream DNL, and this commit message seems to be implicitly referring to geewallet. I guess you mean |
|
I've just realised that m4 is revocation, not channel closing; so really this PR should be retargeted to m3 |
but the code that this bug got introduced is in m4 |
|
So if force-closing is implemented from tthe m3 branch of this repo, it wouldn't have this bug? |
what's the exact commit? (I ask because I want to check if it's in DNL upstream) |
the whole "spending the commitment transactions" code is in m4 you can't implement force close in m3 without backporting that commit |
|
That commit is implementing revocation. Force-closing is a different concept that doesn't need revocation for it to work. If there's any subset part of that commit, though, that needs to be there for force-closing to work, then it can be implemented by Andrew in the m3 branch (and later I will rebase that m4 commit on top of that). |
|
This looks good to me. Thanks for picking up on it. |
This code assumes that the person who broadcasted the commitment transaction is always the funder and the person who is trying to spend the broadcasted remote commitment is always the fundee. This passed geewallet CI tests because that's the only case we test in geewallet end-to-end tests.
aarani
force-pushed
the
fix-hardcoded-value
branch
from
73c6e9f
to
e8f530d
Compare
knocte
force-pushed
the
geewalletLightningMilestone4
branch
from
f0a3778
to
bfd5ccc
Compare
This code assumes that the person who broadcasted the commitment transaction is always the funder and the person who is trying to spend the broadcasted remote commitment is always the fundee. This passed geewallet CI tests because that's the only case we test in geewallet end-to-end tests.