Skip to content

walletkit+sweeper: add forgotten P2TR address type#6632

Merged
Roasbeef merged 3 commits into
masterfrom
addr-type-taproot
Jun 10, 2022
Merged

walletkit+sweeper: add forgotten P2TR address type#6632
Roasbeef merged 3 commits into
masterfrom
addr-type-taproot

Conversation

@guggero
Copy link
Copy Markdown
Collaborator

@guggero guggero commented Jun 10, 2022

Change Description

Fixed three more occurrences of address type enums that didn't yet support P2TR inputs.

NOTE: I didn't add anything to the release notes since this should've been added by any of the previous P2TR pull requests.

@guggero guggero added this to the v0.15.0 milestone Jun 10, 2022
@guggero guggero requested review from Roasbeef and bhandras June 10, 2022 12:07
@guggero guggero changed the title Addr type taproot walletkit+sweeper: add forgotten P2TR address type Jun 10, 2022
Copy link
Copy Markdown
Collaborator

@bhandras bhandras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🌴

Comment thread sweep/tx_input_set.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's also make the default addresses for the sweeper P2TR as well?

So we'd change this argument here: https://github.com/lightningnetwork/lnd/blob/master/server.go#L4422-L4424

That'll also indirectly cause all on chain sweeps (force closes, etc) to go into P2TR addresses.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also be able to change this as well:

lnd/peer/brontide.go

Lines 2162 to 2175 in de6fcf1

// genDeliveryScript returns a new script to be used to send our funds to in
// the case of a cooperative channel close negotiation.
func (p *Brontide) genDeliveryScript() ([]byte, error) {
deliveryAddr, err := p.cfg.Wallet.NewAddress(
lnwallet.WitnessPubKey, false, lnwallet.DefaultAccountName,
)
if err != nil {
return nil, err
}
peerLog.Infof("Delivery addr for channel close: %v",
deliveryAddr)
return txscript.PayToAddrScript(deliveryAddr)
}
.

In practice we should always be using the upfront shutdown addr, but this should be safe.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll also need to update this, so older nodes will accept a P2TR addr as an upfront shutdown script:

lnd/lnwallet/wallet.go

Lines 2247 to 2268 in dceb101

// validateUpfrontShutdown checks whether the provided upfront_shutdown_script
// is of a valid type that we accept.
func validateUpfrontShutdown(shutdown lnwire.DeliveryAddress,
params *chaincfg.Params) bool {
// We don't need to worry about a large UpfrontShutdownScript since it
// was already checked in lnwire when decoding from the wire.
scriptClass, _, _, _ := txscript.ExtractPkScriptAddrs(shutdown, params)
switch scriptClass {
case txscript.PubKeyHashTy,
txscript.WitnessV0PubKeyHashTy,
txscript.ScriptHashTy,
txscript.WitnessV0ScriptHashTy:
// The above four types are permitted according to BOLT#02.
// Everything else is disallowed.
return true
default:
return false
}
}

I think this means that we shouldn't yet switch over the default co-op close addr due to this stricter validation.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re the above, here's where we generate our upfront shutdown addr:

lnd/funding/manager.go

Lines 1363 to 1375 in 262591c

shutdown, err := getUpfrontShutdownScript(
f.cfg.EnableUpfrontShutdown, peer, acceptorResp.UpfrontShutdown,
func() (lnwire.DeliveryAddress, error) {
addr, err := f.cfg.Wallet.NewAddress(
lnwallet.WitnessPubKey, false,
lnwallet.DefaultAccountName,
)
if err != nil {
return nil, err
}
return txscript.PayToAddrScript(addr)
},
)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some other areas that are safe to change:

@guggero guggero force-pushed the addr-type-taproot branch from 0905844 to 427702d Compare June 10, 2022 20:04
Copy link
Copy Markdown
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🐡

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants