Fetch KeysInterface's get_shutdown_pubkey at close-time with !commit_upfront_shutdown_pubkey

[TheBlueMatt]

Config allows users to select if they want to commit to the shutdown public key or not, but either way we pull the shutdown public key form users at channel-open (storing it in Channel::shutdown_pubkey). We should instead actually pull the public key at shutdown-time based on the config bool. https://docs.rs/lightning/0.0.99/lightning/util/config/struct.ChannelConfig.html#structfield.commit_upfront_shutdown_pubkey

[jkczyz]

To make Channel::shutdown_pubkey an Option, how should the (de)serialization change? Seems like there are checks on whether a PublicKey is valid when parsed, so using a fake one may not work.

[TheBlueMatt]

Right, I think we can just throw our hands up and say "if you use a non-p2wpkh (where we'd have to get the public key) shutdown script or do not commit to it upfront, then you cannot open your serialization on old versions". No reason to default to not committing upfront so at least the default stuff would still work.

[jkczyz]

Are you saying if self.config.commit_upfront_shutdown_pubkey is:

• true, then read/write shutdown_pubkey as a PublicKey
• false, then read/write shutdown_pubkey as an Option<PublicKey>

[jkczyz]

Ah, actually I didn't realize Channel has it's own SERIALIZATION_VERSION and TLVs, so the Option could go there.

[TheBlueMatt]

true, then read/write shutdown_pubkey as a PublicKey

It may make sense to merge this with #1006 somehow. Maybe an enum WitnessType { P2WPKH(PublicKey), P2WSH([u8; 32]), Other((NonZeroU8, Bytes)) } or something like that. In that case, for P2WPKH we'd write it as a PublicKey as you suggest here.

false, then read/write shutdown_pubkey as an Option

Yea.

[jkczyz]

It may make sense to merge this with #1006 somehow. Maybe an enum WitnessType { P2WPKH(PublicKey), P2WSH([u8; 32]), Other((NonZeroU8, Bytes)) } or something like that. In that case, for P2WPKH we'd write it as a PublicKey as you suggest here.

In conjunction with merging KeysInterface methods as mentioned in #1006, would you be opposed to having the interface return bitcoin::util::address::Payload? This seems to align with what was is permissible by BOLT 2:

https://github.com/lightningnetwork/lightning-rfc/blob/master/02-peer-protocol.md#closing-initiation-shutdown

Could also keep it as Script but parse as Payload in Channel as presumably get_destination_script could be used in contexts outside of BOLT 2's shutdown sciptpubkey. Or alternatively keep as two methods, making get_shutdown_pubkey return Payload.

Thoughts?

[TheBlueMatt]

In conjunction with merging KeysInterface methods as mentioned in #1006, would you be opposed to having the interface return bitcoin::util::address::Payload?

In theory I think we should do that, but (a) Payload allows any length version-0 witness script, whereas standardness on the network and BOLTS only allows 20 bytes and 32 bytes and (b) if we want to support writing content that old versions can read we have to support providing the full public key, not the public key hash.

I'm open to breaking (b) at this stage, but (a) is a bit more tricky. I guess we could return a Result to avoid (a), but it's a bit nicer to have it not be possible by API limits.

[jkczyz]

I'm open to breaking (b) at this stage, but (a) is a bit more tricky. I guess we could return a Result to avoid (a), but it's a bit nicer to have it not be possible by API limits.

I suppose we could wrap Payload in our own type, which could perform the additional checks during construction, and use that type in KeysInterface without Result.

Do we want to impose the BOLT restrictions on contexts outside of BOLT 2 usage? That is, should we merge the two KeysInterface API methods?

[TheBlueMatt]

I suppose we could wrap Payload in our own type, which could perform the additional checks during construction, and use that type in KeysInterface without Result.

I guess, but what does that get us vs just making our own enum? In most contexts users are creating addresses from public keys anyway, so the extra step is just an unwrap for its own sake. Plus, it'd be nice to avoid the backwards-incompatibility, even though I admit it's not really a big deal.

Do we want to impose the BOLT restrictions on contexts outside of BOLT 2 usage? That is, should we merge the two KeysInterface API methods?

I feel like it would be really nice to have a single interface for "send on-chain funds here", so to do that we'd need to, but if you feel that they should remain separate APIs no reason to require it.

[jkczyz]

I guess, but what does that get us vs just making our own enum? In most contexts users are creating addresses from public keys anyway, so the extra step is just an unwrap for its own sake. Plus, it'd be nice to avoid the backwards-incompatibility, even though I admit it's not really a big deal.

Thought about this more and realized we really don't need Payload. We just want to have a type that can be either a Script conforming to BOLT 2 requirements or our legacy PublicKey for backwards compatibility. Internally, it could use an enum to represent this which would allow us to vary the implementation in the future with additional variants if needed.

[TheBlueMatt]

Thought about this more and realized we really don't need Payload. We just want to have a type that can be either a Script conforming to BOLT 2 requirements or our legacy PublicKey for backwards compatibility.

Sounds good, I assume it'll look pretty similar, though, basically

enum Thing {
   P2WPKH(pubkey),
   P2WSH(scripthash),
   OtherWitness((NonZerou8, bytes)),
}