Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MOAR CHECK POWER! #7111

Merged

Conversation

rustyrussell
Copy link
Contributor

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 rustyrussell added this to the v24.05 milestone Feb 27, 2024
@cdecker cdecker marked this pull request as draft February 27, 2024 12:26
@rustyrussell rustyrussell force-pushed the guilt/use-check-power branch 2 times, most recently from 4b081f6 to 265f607 Compare March 21, 2024 06:30
@rustyrussell rustyrussell marked this pull request as ready for review March 21, 2024 06:30
@ksedgwic
Copy link
Collaborator

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 ...

@rustyrussell rustyrussell force-pushed the guilt/use-check-power branch 2 times, most recently from 66cc519 to fda80d4 Compare March 22, 2024 00:01
@devrandom
Copy link
Contributor

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
Copy link
Contributor

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.

@ksedgwic
Copy link
Collaborator

@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
Copy link
Collaborator

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

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.

This helps with -O3 warnings on these commits.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
gcc-12 (Ubuntu 12.3.0-9ubuntu2) 12.3.0 with -O3 gives:

```
In file included from plugins/renepay/test/../mcf.c:5,
                 from plugins/renepay/test/run-arc.c:13:
ccan/ccan/tal/str/str.h: In function ‘minflow’:
ccan/ccan/tal/str/str.h:43:9: error: ‘errmsg’ may be used uninitialized [-Werror=maybe-uninitialized]
   43 |         tal_fmt_(ctx, TAL_LABEL(char, "[]"), __VA_ARGS__)
      |         ^~~~~~~~
plugins/renepay/test/../mcf.c:1565:15: note: ‘errmsg’ was declared here
 1565 |         char *errmsg;
      |               ^~~~~~
cc1: all warnings being treated as errors
```

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We want to extend it to plugins, and we want it to be allowed to be async for more power,
so rather than not completing the cmd if we're checking, do it in command_check_done()
and call it.

This is cleaner than the special case we had before, and allows check to us all the
normal jsonrpc mechanisms, especially async requests (which we'll need if we want to
hand check requests to plugins!).

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
If this is set, instead of mangling the request in-place, we will hand
it through raw.  Plugins will use this.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
…commands through.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Added: Plugins: Can now opt in to handle `check` command on their commands, for more thorough checking.
We don't thoroughly handle `check setconfig`: it would be good to
allow this to do further checking!

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
For extra points, we use an async command which calls out to listdatastore
before returning.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We need to pass through setconfig in check mode, then we need to have libplugin
add (and use!) a `check_only` parameter to all option setting.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Added: JSON-RPC: `check` `setconfig` on plugin options can now check the config value would be accepted.
Now we can check that the setconfig would actually parse.  We do this
by handing NULL to the calback to indicate it's a test, which may not
work in general but works for now.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Added: `check` `setconfig` now checks that the new config setting would be valid.
This should make VLS's life easier: they can ignore dev flags they
don't understand, but we will know their capabilites after init and so
know what they didn't understand (if required).

The only flag for now is a flag to force failure for "preapprove" calls.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
…o anything.

Apparently VLS actually does something when we preapprove: if caller is just
checking we want to tell it not to do that!

I put in a flag so we can test both old and new APIs.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
…ith HSM.

If they support it, we can actually ask hsmd when we are called in
check mode.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We wire through --dev options into the hsmd, and test preapprove accept and deby
with both old and new protocols.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
… by `check`.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Added: JSON-RPC: `check` `keysend` now checks with HSM that it will approve it.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Added: JSON-RPC: `check` `keysend` now checks with HSM that it will approve it.
We add a dev flag so we can decline them.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@endothermicdev
Copy link
Collaborator

Any more feedback on the hsm preinit tlvs @ksedgwic? This works on your end?

@ksedgwic
Copy link
Collaborator

ksedgwic commented May 6, 2024

@endothermicdev Apologies for the delay ... hsm preinit tlvs are in good shape on the VLS side

@rustyrussell rustyrussell merged commit 153567d into ElementsProject:master May 7, 2024
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants