Activate anchor output in channels #1491
Conversation
| * @return a signature generated with a private key generated from the input keys's matching | ||
| * private key and the remote point. | ||
| */ | ||
| def sign(tx: TransactionWithInputInfo, publicKey: ExtendedPublicKey, remotePoint: PublicKey, sighashType: Int): ByteVector64 |
There was a problem hiding this comment.
I'm not a huge fan of directly exposing the sighash here, I would have liked a slightly higher-level abstraction.
If you have a suggestion, I'd love to hear it! The gist of it is that we need to distinguish signing local vs remote txs: when signing local txs we will always use SIGHASH_ALL, but when signing remote txs we may use less restrictive sighash flags (in anchor outputs, we use SIGHASH_SINGLE | SIGHASH_ANYONECANPAY for htlc txs.
There was a problem hiding this comment.
I'd like the sighash flag to be somehow encapsulated in the TransactionWithInputInfo.
One thing I can try, is to enrich TransactionWithInputInfo with either Local or Remote -> this way our type system will let us know whether this is a local tx or a remote tx, which could be useful.
Then TransactionWithInputInfo can expose a method that returns the sighash that should be used depending on the commitment format. I'll experiment with that and will see if it's better.
2873839 does a better job at defining the sighash closer to the transactions and signing operations.
There was a problem hiding this comment.
The sighash handling feels a bit clunky, but it's not too bad either and I don't have a better proposition.
There was a problem hiding this comment.
Yeah, I think we can't fully hide it as the signing interface should allow setting it explicitly (it's part of a signature's "settings").
For now it's not used that much since only anchor output HTLCs use something different than sighash_all, but if a later commitment format makes more extensive use of different flags we may want to revisit that to ensure the code stays simple
1794df1 to
3b76820
Compare
Pick channel version based on local/remote feature support.
At this point all normal channel operations work (open, exchange htlcs, mutual close, etc). However force-closing isn't handled, so nodes should *not* activate the feature yet.
We add the concept of a TxOwner and encapsulate the choice of sighash flags to use when signing thanks for that abstraction.
8e460c2 to
2873839
Compare
| * @return a signature generated with the private key that matches the input | ||
| * extended public key | ||
| */ | ||
| def sign(tx: TransactionWithInputInfo, publicKey: ExtendedPublicKey, txOwner: TxOwner, commitmentFormat: CommitmentFormat): ByteVector64 |
There was a problem hiding this comment.
Wonder what @sstone thinks of the change in the interface.
eclair-core/src/main/scala/fr/acinq/eclair/channel/ChannelTypes.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/transactions/Transactions.scala
Outdated
Show resolved
Hide resolved
| * @return a signature generated with a private key generated from the input keys's matching | ||
| * private key and the remote point. | ||
| */ | ||
| def sign(tx: TransactionWithInputInfo, publicKey: ExtendedPublicKey, remotePoint: PublicKey, sighashType: Int): ByteVector64 |
There was a problem hiding this comment.
The sighash handling feels a bit clunky, but it's not too bad either and I don't have a better proposition.
With this PR, it's possible to activate anchor outputs and have fully operating channels that use the anchor commitment format.
Interop testing has been done with lnd, and there is only one pending issue during mutual close, where they incorrectly compute the closing amounts (we are discussing this on the spec PR, it should be resolved soon).
However, for the sake of simplicity, this PR doesn't handle unilateral close scenario. This will be done in another, separate PR.
We don't do any kind of automatic fee bumping either; this will be done later, once we have PSBT support and once
bitcoindoffers thepsbtbumpfeeRPC (see bitcoin/bitcoin#18654).