-
Notifications
You must be signed in to change notification settings - Fork 902
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
MOAR CHECK POWER! #7111
Conversation
4b081f6
to
265f607
Compare
265f607
to
672bc31
Compare
Here's what we have defined for
Hmm, maybe all of our I'll investigate what the TLV parsing will look like on our side ... |
66cc519
to
fda80d4
Compare
moving our new fields to TLVs sounds good to me, and I don't feel there's a need to maintain backwards compat. |
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. |
@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. |
@rustyrussell we've made strong progress on the TLV stuff for VLS: tl;dr - by connecting this branch
The reader is using a "combined" TLV definition w/ both your tags and our tags:
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>
fda80d4
to
2e0b58f
Compare
Any more feedback on the hsm preinit tlvs @ksedgwic? This works on your end? |
@endothermicdev Apologies for the delay ... hsm preinit tlvs are in good shape on the VLS side |
Ok, this lets plugins support
check
on their commands, and also letssetconfig
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!