funding: use p2tr by default for batch_open_channel#7603
funding: use p2tr by default for batch_open_channel#7603Roasbeef merged 2 commits intolightningnetwork:masterfrom
Conversation
ziggie1984
left a comment
There was a problem hiding this comment.
Thanks for this quick PR, left some comments :)
itest/lnd_funding_test.go
Outdated
There was a problem hiding this comment.
I am not sure if the change-output is always the last output though, maybe just find the change by excluding the amounts? Its maybe constant for this scenario but not generally speaking because its sorted I think in the btcwallet package because there the funding takes place.
Could you explain your thought behind your formula ?
There was a problem hiding this comment.
This formula doesn't take the last output. Here is the explanation!
If you have M channels points and 1 change-output, then the indexes go from 0 to M.
We know the OutputIndex of every channel point, so we just need to find the missing int between 0 and M.
To do that, we know how to calculate the sum of the M consecutive int (aka if there is no missing int):
Ideal_Sum= M*(M+1)/2
In our case, we know the sum of all the channel output indexes, so, to find out the missing int, we just need to take the difference:
Ideal_Sum- Sum(Channels outpoint indexes)
In our case M=3, so you have the formula in the code (Ideal_Sum=6)
With an example: if channel indexes are 0,1,3 then change output index is 6-(0+1+3)=2
There was a problem hiding this comment.
Thanks for your explanation, nice again something learned :). Not sure if this ideal sum thing is common knowledge but I would suggest a nice comment maybe ?
There was a problem hiding this comment.
nit: your comment describes the code I would have prefered something like this:
Suggestion:
//For calculating the change output index we use the formula for consecutive sum of integers n(n+1)/2. In our case n=3. All the channel point indices are known so we just calculate the difference to get the change output index`
41663f1 to
978d4e5
Compare
ziggie1984
left a comment
There was a problem hiding this comment.
Thanks for the PR, LGTM
itest/lnd_funding_test.go
Outdated
There was a problem hiding this comment.
nit: your comment describes the code I would have prefered something like this:
Suggestion:
//For calculating the change output index we use the formula for consecutive sum of integers n(n+1)/2. In our case n=3. All the channel point indices are known so we just calculate the difference to get the change output index`
978d4e5 to
baf09db
Compare
guggero
left a comment
There was a problem hiding this comment.
Thanks for the fix, LGTM 🎉
funding/batch.go
Outdated
There was a problem hiding this comment.
nit: move to above the struct initialization to avoid line too long.
firstReq := b.channels[0].fundingReq
feeRateSatPerKVByte := firstReq.FundingFeePerKw.FeePerKVByte()
changeType := walletrpc.ChangeAddressType_CHANGE_ADDRESS_TYPE_P2TR
fundPsbtReq := &walletrpc.FundPsbtRequest{
Template: &walletrpc.FundPsbtRequest_Raw{
Raw: txTemplate,
},
Fees: &walletrpc.FundPsbtRequest_SatPerVbyte{
SatPerVbyte: uint64(feeRateSatPerKVByte) / 1000,
},
MinConfs: firstReq.MinConfs,
SpendUnconfirmed: firstReq.MinConfs == 0,
ChangeType: changeType,
}
lntest/harness_assertion.go
Outdated
There was a problem hiding this comment.
Can use txscript.ParsePkScript instead which returns a PkScript that has a Class() method.
itest/lnd_funding_test.go
Outdated
In LND v0.15.1, batch_open_channel used p2tr by default for change output. However, in a later version, a breaking change has been fixed in FundPSBT to make the change type configurable (default to p2wkh). As batch_open_channel uses FundPsbt, we need to put back p2tr as default for change output
baf09db to
894fc9f
Compare
fixes #7601
In LND v0.15.1, batch_open_channel used p2tr by default for change output. However, in a later version, a breaking change has been fixed in FundPSBT to make the change type configurable (default to p2wkh). As batch_open_channel uses FundPsbt, we need to put back p2tr as default for change output for this specific case