Channel refactor #24
Channel refactor #24
Conversation
|
This is what was posted on the telegram channel w.r.t. getting these changes into a mergable state:
|
|
Please don't forget that this needs to be rebased against Joe's master, not DNL.Kiss' master. But don't propose PR there until I have re-reviewed myself. |
canndrew
force-pushed
the
channel-refactor
branch
from
3c408eb
to
ac975eb
Compare
|
I've made the requested changes with a couple of exceptions:
This didn't make a lot of sense to me. I'm not sure why it should be squashed into the two before it.
Maybe I shouldn't have change the formatting of I've also started rebasing on top of joe's commits. So this now almost up to date with joe's master, with the exception of the most recent commit ( |
Fair enough, but you can merge the 2 above into 1? (merge 2 into 1 instead of 3 into 1).
😬
But the real way to learn this properly is feeling the pain 😅 . |
FYI this should only take 5 minutes if you use |
Fix several build warnings.
canndrew
force-pushed
the
channel-refactor
branch
from
ac975eb
to
abd4c95
Compare
Done.
Done. That sort of thing doesn't take long :) |
|
I've rebased on top of the joe's master now. So we have the I've also fixed the warnings on joe's master and I'll rebase on top of that next and make sure none of my commits introduce any new warnings and that tests still pass after each one (this can be automated with |
- 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.
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
These tests are failing randomly and macaroons aren't used by geewallet anyway. So exclude them from CI.
Move/rename ChannelError.feeRateMismatch to FeeRatePerKw.MismatchRatio. This function is useful outside of DotNetLightning, so we now export it so that library consumers can use it.
Prior to this commit, apply an update_fee message would cause DNL to validate the message but not actually apply it to its commitments. It now updates its commitments as it should.
These states don't have commitments, only exist before the existence of a channel is confirmed, don't need to be saved in a wallet, and can only handle one specific ChannelCommand each. As such, it doesn't make much sense to have them be part of ChannelState. They can instead be seperate types which exist prior to the creation of a channel and which have specialized methods for performing the single operation which they can handle.
They don't need to anymore since all channel states have a channel id and commitments.
This field is unused.
These settings are only used for choosing whether to accept open/accept channel messages. They don't need to be stored in the channel.
Previously the local shutdown script was stored in two places in the channel and also provided as an argument when initiating a shutdown. This made it possible to send invalid shutdown messages if the shutdown scripts provided by the user at different times were different. Also, shutdown scripts have to be in certain forms, but the only place that they were validated was when initiating a shutdown. There's now a ShutdownScriptPubKey type which wraps a Script and enforces that the script is a valid shutdown script. This is used throughout the code for all scripts which are shutdown scripts, which forces us to check their validity at all required points. The DefaultFinalScriptPubKey field has been removed from LocalParams so that we only store the local shutdown script in (at most) one place in the channel datastructure. This shutdown script is an Option, and can be None in the case that we didn't specify a shutdown script to give to the remote peer when creating the channel. As such as we still pass a shutdown script as an argument when closing a channel but we also check that it matches the recorded shutdown script that we previously gave the remote peer in the case that it's Some.
The types WaitForOpenChannelData, WaitForFundingInternalData, and WaitForRemotePublishFutureCommitmentData were defined but not used at all. They have been removed.
Remove long lines and excessive rightward drift. Add types to function parameters and return types. This makes the code more readable and consistent.
Since all ChannelState variants have a Commitments field, this field can be factored out into the Channel object. Also, remove the ChannelId field from the ChannelState variants since it is duplicated inside Commitments.
Commitments already stores the funding tx outpoint, which is what the channel id is dervied from. So we can de-duplicate state by deriving the channel id when needed.
The types WaitForFundingSignedData, WaitForFundingCreatedData,
ChannelWaitingForAcceptChannel and InputInit{Funder,Fundee} are only
used in a single place each as part of larger structures that they can
be inlined into. This will make it easy to refactor the channel types in
a subsequent commit.
This module defined the following interfaces:
type IState = interface end
type IStateData = interface end
type ICommand = interface end
type IEvent = interface end
These were not used in the code anywhere
This is needed in geewallet for getting the spendable balance of channel when we only have the components of a Channel object but not the Channel object itself. This method can be moved onto SavedChannelState when the necessary fields remaining in Channel/Commitments are moved there.
When SpendableBalance is called when the funding is not yet locked, report the full channel balance rather than crashing.
Previously SignCommitment would return None instead of a CommitmentSignedMsg if either: (a) There were not updates to sign or (b) A revoke-and-ack for a previous commitment was not yet received Since these cases both indicate a misuse of the API they have been changed to errors.
canndrew
force-pushed
the
master
branch
4 times, most recently
from
f817da9
to
0340b0b
Compare
knocte
force-pushed
the
master
branch
3 times, most recently
from
0389f6a
to
60d2401
Compare
knocte
force-pushed
the
master
branch
4 times, most recently
from
419cd8d
to
f0461b4
Compare
knocte
force-pushed
the
master
branch
4 times, most recently
from
37fe5af
to
1f418fe
Compare
knocte
force-pushed
the
master
branch
5 times, most recently
from
bd34034
to
b67e13f
Compare
Refactor DNL's
Channeltype in order to (a) separate state which is saved to the wallet from transient state which is discarded on disconnection, (b) simplify the API to make it less confusing, less bug-prone and require less duplicated effort between DNL and geewallet.