Commando: use inbuilt runes #6431
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustyrussell
force-pushed
the
guilt/commando-use-inbuilt-runes
branch
3 times, most recently
from
1a01c9a
to
9b31a3c
Compare
ShahanaFarooqui
approved these changes
Jul 24, 2023
|
ACK 9b31a3c |
rustyrussell
force-pushed
the
guilt/commando-use-inbuilt-runes
branch
from
9b31a3c
to
8b86aec
Compare
|
Rebased onto #6437, and I added @ShahanaFarooqui 's suggested cleanup at the end. |
rustyrussell
force-pushed
the
guilt/commando-use-inbuilt-runes
branch
from
8b86aec
to
83480a0
Compare
The uid isn't enough: it could be someone else's rune. This is tested in the command rune list tests. Signed-off-by: Rusty Russell <[email protected]>
And do them on the first run (where we check parameters), instead of every time. Might as well do them in non-developer mode too, since they're simply programmer correctness. Signed-off-by: Rusty Russell <[email protected]>
We're going to remove the ability to override the master secret, so fix up our tests. Signed-off-by: Rusty Russell <[email protected]>
Signed-off-by: Rusty Russell <[email protected]>
We used to activate on the first rune creation, but we're no longer in charge of runes, so we can't make that call. Signed-off-by: Rusty Russell <[email protected]>
We allocate one in three places, so at least a partial constructor is a nice pattern to have. Signed-off-by: Rusty Russell <[email protected]>
We would create a `struct commando` to marshal our incoming messages, then try_command would create a *new* one. We can simply reuse, but when I did I noticed a trick: the new one was not in the `incomings` array, so didn't work towards the ratelimit. So we need to remove it from `incomings` in `try_command`, but at least it's now explicit. Signed-off-by: Rusty Russell <[email protected]>
…mand() In preparation for going async: 1. Split try_command's tail into a new function called execute_command() after the rune checks have succeeded. 2. Put all the info execute_command() needs into struct cond_info, to make it a simple callback style. So we create new_cond_info() which dynamically allocates `struct cond_info` and sets the destructor. Signed-off-by: Rusty Russell <[email protected]>
…ation. This means (temporarily) that blacklisting won't work (fix later), and means that old-style (commando.py) master-secret-override doesn't work. Signed-off-by: Rusty Russell <[email protected]> Changelog-Removed: Plugins: `commando` no longer allows datastore ['commando', 'secret'] to override master secret (re-issue runes if you were using that!).
Separate patch to make the previous diff smaller. Signed-off-by: Rusty Russell <[email protected]>
The in-core commands are much more helpful with details on why runes failed, too! Signed-off-by: Rusty Russell <[email protected]>
We want to access this in db migrations, which happen very early, but runes_init needs the db, creating a circular dependency which must be split. Signed-off-by: Rusty Russell <[email protected]>
During migrations, wallet doesn't exist yet, so we use raw db. Split functions into lower-level ones and make public API a simple wrapper. Unfortunately, this means db_datastore_next needs to proceed db_datastore_first since they're now static (and first calls next), plus, fix some weird indents, so diff is bigger than expected. Signed-off-by: Rusty Russell <[email protected]>
The wallet_datastore_first() SELECT statement only iterates from the given key (if any), relying on the caller to notice when the key no longer applies. (e.g. startkey = ["foo", "bar"] will return key ["foo", "bar"] then ["foo", "bar", "child" ], then ["foo", "baz"]). The only caller (listdatastore) would notice the keychange and stop looping, but reallly wallet_datastore_next() should do this. When I tried to use it for migrations, I got very confused! Also, several places want a simple "wallet_datastore_get()" function, so provide that. Signed-off-by: Rusty Russell <[email protected]>
rustyrussell
force-pushed
the
guilt/commando-use-inbuilt-runes
branch
from
83480a0
to
54bd9ec
Compare
|
Now rebased on master, and disabled migration tests on elements (regtest chainhash is in the canned db, so it complains!) |
If they have invalid runes, we bail, but if they have runes which used a different master secret (old commando.py allowed you to override secret), we just complain and delete them. Note that this requires more mocks in wallet/test/run-db.c... Signed-off-by: Rusty Russell <[email protected]>
Signed-off-by: Rusty Russell <[email protected]>
Pointed out by @ShahanaFarooqui, we leave a single unused entry in the datastore, so we should clean that up too. Signed-off-by: Rusty Russell <[email protected]>
rustyrussell
force-pushed
the
guilt/commando-use-inbuilt-runes
branch
from
54bd9ec
to
7aafdea
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This not only is simpler and reduces code-reuse, it means we correctly share ratelimits with any other rune usage.