autopilot: feature configuration #629
Conversation
bitromortac
force-pushed
the
persist-config
branch
from
30cefc3 to
6644b74
Compare
There was a problem hiding this comment.
First review looks good after Elle's comment has been addressed 🚀! Really nice work :)!!
Leaving two suggestions for improvements that are non blocking. I'd especially want to hear you're opinion on how the configuration should be specified in the autopilot a cmd.
bitromortac
force-pushed
the
persist-config
branch
from
6644b74 to
d1a83e8
Compare
|
requesting @guggero too for architecture ack |
There was a problem hiding this comment.
LGTM after fixing the issue mentioned in #629 🔥:rocket:, great work!!
bitromortac
force-pushed
the
persist-config
branch
from
d1a83e8 to
9145d3c
Compare
bitromortac
force-pushed
the
persist-config
branch
from
9145d3c to
9a88bbd
Compare
|
Rebased on #621 |
This commit adds a field to the autopilot server rpc that transports default configurations from autopilot to litd.
We pass on default configuration to litrpc in order to display them in the list features call.
We add command line flags to litcli in order to be able to submit feature configurations when registering a session.
bitromortac
force-pushed
the
persist-config
branch
from
9a88bbd to
bea1fc4
Compare
|
Rebased on #637 |
| privMapPairs := make(map[string]string) | ||
| newConfigMap := make(map[string]any) | ||
| for k, v := range configMap { | ||
| // We only substitute items in lists. |
There was a problem hiding this comment.
so far I didn't have any better applications than for lists of items, similar to the peer deny list and channel deny lists and didn't want to be too restrictive. a fallback for a single field could be to use a list with a single item (hacky)
| item, ok := item.(string) | ||
| allStrings = allStrings && ok | ||
| anyString = anyString || ok | ||
| stringList[i] = item |
There was a problem hiding this comment.
wont this panic if !ok since then we are assigning a non-string to a string list?
There was a problem hiding this comment.
ah ok it doesnt panic since item at this point is empty. But maybe just for clarity, let's do:
if !ok {
continue
}
stringList[i] = item
| newConfigMap := make(map[string]any) | ||
| for k, v := range configMap { | ||
| // We only substitute items in lists. | ||
| list, ok := v.([]any) |
There was a problem hiding this comment.
if we are only doing "lists of strings" then why not do v.([]string) here?
There was a problem hiding this comment.
I tried that, but it didn't work, see https://go.dev/play/p/iA-IUn5VHR4
| { | ||
| // A list of numbers is not obfuscated. Those can be | ||
| // useful to submit histograms for example. | ||
| name: "channel ids", |
There was a problem hiding this comment.
we would want to obfuscate channel IDs though. Even though they are numbers. This makes me think we should allow the user to specify specify somehow that a list/item in a config struct should be obfuscated. We need to think about how to do this though... We can do this in future though :) we just need to do it before ever asking for channel IDs in the config
There was a problem hiding this comment.
yes, agree that we need to think more about these cases and that such a signal could make sense, for now the signal for obfuscation is to have a list of strings, which includes the channel id case as well, just the numbers converted to strings
There was a problem hiding this comment.
which includes the channel id case as well, just the numbers converted to strings
Cool - as long as the firewalldb.Uint64ToStr method is being used for this conversion to strings. Since the converted string needs to match other places where the channel ID is being used .
We obfuscate pubkeys, channel points and ids entered in configurations. The channel id lengths for different block heights can be checked with: ```python len(str(1 << 40 | 2923 << 16 | 30)) len(str(10_000_000 << 40 | 2923 << 16 | 30)) ```
We also upate sample data to reflect the latest state of the api.
bitromortac
force-pushed
the
persist-config
branch
from
bea1fc4 to
00214c0
Compare
This commit returns an empty PrivacyMapReader if the privacy bucket for the group doesn't yet exist.
In this PR we add support for configuration of features within autopilot. Feature configuration can signal preferences to autopilot which are not enforceable by the litd firewall.