Remove InputInfo from LocalCommit and compute localFundingPubKey from ChannelKeys#3102
Remove InputInfo from LocalCommit and compute localFundingPubKey from ChannelKeys#3102
InputInfo from LocalCommit and compute localFundingPubKey from ChannelKeys#3102Conversation
When a channel was force-closed, we previously stored partially created closing transactions for each output of the commitment transaction. In some cases those transactions didn't include any on-chain fee (HTLC txs with 0-fee anchors) and weren't signed. The fee is set in the publisher actors which then sign those transactions. This was an issue because we used those partial transactions as if they were the final transaction that would eventually confirm: this made it look like RBF wasn't an option to callers, and was thus easy to misuse. We now change our data model to only store the outputs of the commit tx that we may spend, without any actual spending transaction. We only store spending transactions once they are confirmed, at which point they can safely be used as closing transactions by callers such as our balance checks. This lets us get rid of some codec migration code related to closing transactions, and is more future-proof because we now don't need to encode closing transactions and can thus change their format easily. This also reduces the size of our channel state during force-close.
We previously stored the commitment transaction and HTLC transactions in our channel data along with the remote signatures, without our local signatures. It is unnecessary to store those transactions, as we have all the data we need to recompute them on-the-fly if we need to force close. Storing those transactions can easily use a few hundreds of kB per channel, which adds up to the post-restart latency when channels need to be read from the DB. By not storing those transactions in our commitments, we also remove the need to support encoding them, which gives us greater flexibility to change their format in the future. It also made it confusing in tests, where we sometimes use unsigned transactions as if they were signed.
We actually only add `localFundingPubKey` and a few other fields, and compute the `commitInput`. This saves 33 bytes because `remoteFundingPubKey` is now only persisted once.
It is now computed from `fundingTxIndex` + `ChannelKeys`.
|
|
||
| val commitmentsCodecWithoutFirstRemoteCommitIndex: Codec[Commitments] = ( | ||
| ("params" | paramsCodec) :: | ||
| ("params" | paramsCodec) >>:~ { params => |
There was a problem hiding this comment.
This is where we copy the commitmentFormat from ChannelParams to Commitment, which will be needed at some point I think.
| case class WaitingForSigs(fundingParams: InteractiveTxParams, | ||
| fundingTxIndex: Long, | ||
| fundingTx: PartiallySignedSharedTransaction, | ||
| commitInput: InputInfo, |
There was a problem hiding this comment.
This is wip. I think we should add CommitmentFormat to InteractiveTxParams at some point? Which will allow us to remove this commitInput.
There was a problem hiding this comment.
Agreed, this is going to be useful for migrating existing channels to taproot opportunistically using splicing, probably a good time to do it if the added codec migration isn't too painful? Otherwise this will be done when introducing codecs v5 for taproot.
t-bast
left a comment
t-bast
left a comment
There was a problem hiding this comment.
I think we should definitely make the changes you're proposing to Commitments.scala, but the codecs changes are a bit more involved than what I'd like...it may be a better idea to do this change when we introduce channel codecs v5 for taproot, where we know we have to do similar changes and will probably create a ChannelTypes4.Commitment class for the existing format, which will make the migration code easier?
|
|
||
| def validateSeed(channelKeys: ChannelKeys): Boolean = { | ||
| active.forall { commitment => | ||
| // TODO: the following is circular, need another method |
There was a problem hiding this comment.
I think we should use commitment.localFundingStatus.signedTx_opt: we'll skip the commitment we don't have it, but in most cases we will have it available. We can compare the txOut of the funding tx to what we recompute based on our seed?
| case class WaitingForSigs(fundingParams: InteractiveTxParams, | ||
| fundingTxIndex: Long, | ||
| fundingTx: PartiallySignedSharedTransaction, | ||
| commitInput: InputInfo, |
There was a problem hiding this comment.
Agreed, this is going to be useful for migrating existing channels to taproot opportunistically using splicing, probably a good time to do it if the added codec migration isn't too painful? Otherwise this will be done when introducing codecs v5 for taproot.
t-bast
force-pushed
the
remove-htlc-tx-with-remote-sig
branch
3 times, most recently
from
7d9a628 to
84317b5
Compare
|
Superseded by #3118 |
2 participants
This is a work-in-progress (
InteractiveTxBuildercould be improved, and tests aren't even compiling), but I'd like to get an early feedback before finishing.My starting point was to move
InputInfoout ofLocalCommitand intoCommitment. It seemed more logical because this is not exclusive to the local commit (indeed we use it to build remote txs).I actually didn't move
InputInfoitself, but only thelocalFundingPubKeyand funding outpoint. It saves a little bit of space, as we get rid of thepubkeyScript). To be able to compute theInputInfo, I had to also bring theCommitmentFormat, which I guess will be needed for taproot channels.Then I got rid of
localFundingPubKeyentirely by recomputing it fromChannelKeys. Since those are cached, I think performance shouldn't suffer, and it saves an additional 33B in persisted data.