lightning icon indicating copy to clipboard operation
lightning copied to clipboard

MOAR CHECK POWER!

Open rustyrussell opened this issue 1 year ago • 5 comments
trafficstars

Ok, this lets plugins support check on their commands, and also lets setconfig support check. Previously these would always trivially "pass", now they actually check.

autoclean is upgraded to improve setconfig, but the funder stuff would be a good candidate for dynamic settings too....

Someone should open some Good First Issue things to add setconfig to make more variables dynamic!

rustyrussell avatar Feb 27 '24 03:02 rustyrussell

Here's what we have defined for HsmdDevPreinit on the VLS side:

/// Developer setup for testing
/// Must preceed `HsmdInit{,2}` message
/// NOT FOR PRODUCTION USE
#[derive(SerBolt, Debug, Encodable, Decodable)]
#[message_id(90)]
pub struct HsmdDevPreinit {
    pub derivation_style: u8,
    pub network_name: WireString,
    pub seed: Option<DevSecret>,
    pub allowlist: Array<WireString>,
    // TLV collection needed here
}
  1. We included derivation_style and network_name which are needed to initialize a signer from scratch. CLN uses derivation_style=1, other values will be used by different node wallet layouts. The network_name values are "bitcoin", "testnet", "signet", "regtest", ...

  2. A forced seed is also optionally specified, if not present a random seed is generated.

  3. We also allow a testing allowlist to be specified. Currently in CLN integration testing we have a fairly large testing allowlist of target L1 address which the integration tests need.

  4. I picked message id 90, I don't think it is a problem switching to 99 as we are only beginning our transition to HsmdDevPreinit and it is only used by developers who generally are "in-sync" w/ both sides of the prtotocol.

  5. We'd love to eventually deprecate the other testing fields in the current HsmdInit message someday so it would only have production necessities. I think all of them can be moved to the TLV collection in HsmdDevPreinit at this point?

Hmm, maybe all of our HsmdDevPreinit fields should just be in the TLV collection as well? @devrandom WDYT?

I'll investigate what the TLV parsing will look like on our side ...

ksedgwic avatar Mar 21 '24 23:03 ksedgwic

Here's what we have defined for HsmdDevPreinit on the VLS side

moving our new fields to TLVs sounds good to me, and I don't feel there's a need to maintain backwards compat.

devrandom avatar Mar 22 '24 20:03 devrandom

BTW, I don't think there will be a need for VLS to synchronize the TLVs we understand with CLN, as long as we make sure types IDs are odd.

devrandom avatar Mar 22 '24 20:03 devrandom

@rustyrussell I think that puts the ball squarely in our court to figure out the TLV parsing, unless I'm missing something we can work with exactly what you have.

ksedgwic avatar Mar 22 '24 20:03 ksedgwic

@rustyrussell we've made strong progress on the TLV stuff for VLS:

  • https://gitlab.com/lightning-signer/validating-lightning-signer/-/merge_requests/644

tl;dr - by connecting this branch use-check-power to VLS code in !644 I'm able to see:

[2024-03-27 19:38:19.474 remote_hsmd_socket/vls_proxy::grpc::signer_loop DEBUG] HsmdDevPreinit2(HsmdDevPreinit2 { options: HsmdDevPreinit2Options { fail_preapprove: Some(true), no_preapprove_check: Some(false), derivation_style: None, network_name: None, seed: None, allowlist: None } })

The reader is using a "combined" TLV definition w/ both your tags and our tags:

/// TLV encoded options for HsmdDevPreinit2
#[derive(SerBoltTlvOptions, Default, Debug)]
pub struct HsmdDevPreinit2Options {
    // CLN: allocates from 1 ascending
    #[tlv_tag = 1]
    pub fail_preapprove: Option<bool>,
    #[tlv_tag = 3]
    pub no_preapprove_check: Option<bool>,

    // VLS: allocates from 252 descending (largest single byte tag value is 252)
    #[tlv_tag = 252]
    pub derivation_style: Option<u8>,
    #[tlv_tag = 251]
    pub network_name: Option<WireString>,
    #[tlv_tag = 250]
    pub seed: Option<DevSecret>,
    #[tlv_tag = 249]
    pub allowlist: Option<Array<WireString>>,
}

We're still polishing (mostly where things go) but I think we are out of the woods and this approach is going to work very nicely.

ksedgwic avatar Mar 27 '24 20:03 ksedgwic