autopilot: generic rules per feature #634

bitromortac · 2023-08-29T08:34:45Z

We would like to be able to set different rules for different features. This is solved by introducing JSON-serialized (per-feature) rule flags where all the rules from litcli autopilot features can be set.

ViktorTigerstrom

First code review looks good 🚀fire:! Awaiting rebase before testing.

how do we signal empty restrictions for more than a single feature? currently we have to repeat the restrictions as often as the number of features, which requires to set several flags (a flag --feature-peer-restrict-list="" does not count)

Given that we opted for this version with feature configurations, I think it makes sense to do it like you've implemented it in this PR.

could we transition to json-style lists like "[item1,item1]" to signal empty restrictions with "[]", similarly to the feature configuration?

No strong opinion here, as long as we keep it consistent.

We change the flags to have a feature prefix, which is a breaking change.

Just a thought, would it make sense to keep the old flags as well for now, i.e. "channel-restrict-list" & "peer-restrict-list" but mark them as deprecated and remove them later to not make this a breaking change with this PR? And if the old flags are set, we keep the old behaviour, but also make sure that only one version of the flags are set and error if both versions are set. Do you think that would make sense?

ViktorTigerstrom · 2023-10-10T09:39:30Z

cmd/litcli/autopilot.go

+			Usage: `List of channel IDs that the ` +
+				`Autopilot server should not ` +
+				`perform actions on. In the ` +
+				`form of: [chanID1,chanID2,...]. ` +


nit:
Should be as below, correct?

Suggested change

`form of: [chanID1,chanID2,...]. ` +

`form of: ["chanID1","chanID2",...]. ` +

ah that was intentional, we would error to parse into []int64 otherwise

while we are here, let's clear up this confusion by specifying that this should be a list of numbers. Since even in LND, there is a ChannelID which is a uint64 and another which is a string ([32]byte)

a disadvantage of the generic flags is that it's not as well documented

bitromortac · 2023-10-16T15:02:41Z

Rebased the PR.

Just a thought, would it make sense to keep the old flags as well for now, i.e. "channel-restrict-list" & "peer-restrict-list" but mark them as deprecated and remove them later to not make this a breaking change with this PR? And if the old flags are set, we keep the old behaviour, but also make sure that only one version of the flags are set and error if both versions are set. Do you think that would make sense?

Am with you there, will check if it's worth to keep them backwards compatible.

I also found a small issue which is preexisting: ListChannels error: rpc error: code = Unknown desc = error parsing rules: invalid channel ID, which is related to the channel restrictions. It happens when an unknown channel id restriction, say channel id of 123456 is passed. Will also try to provide a fix.

ellemouton · 2023-11-20T07:55:30Z

Will also try to provide a fix.

@bitromortac - is this ready for review?

ellemouton

Looks good! will test on next round 👍

cmd/litcli/autopilot.go

ellemouton · 2023-11-20T08:03:36Z

cmd/litcli/autopilot.go

+			Usage: `List of channel IDs that the ` +
+				`Autopilot server should not ` +
+				`perform actions on. In the ` +
+				`form of: [chanID1,chanID2,...]. ` +


while we are here, let's clear up this confusion by specifying that this should be a list of numbers. Since even in LND, there is a ChannelID which is a uint64 and another which is a string ([32]byte)

ellemouton · 2023-11-20T08:06:41Z

cmd/litcli/autopilot.go

-				"perform actions on. In the " +
-				"form of: chanID1,chanID2,...",
+		cli.StringSliceFlag{
+			Name: "feature-channel-restrict-list",


technically this rename is a backwards incompatible change... imo it is fine since I dont think anyone is using this at the command line level yet. But perhaps something we should at least include in the release notes? (btw, I think we should have a running issue per release where we keep track of notes we want to make in the release so that we dont forget cc @ViktorTigerstrom )

can we move these to be next to feature-config just so that all the per-feature flags are together?

ah - I saw @ViktorTigerstrom's suggestion now re keeping the old flags for now.

We could do that and then add a waring and a todo that they will be removed in the release after the next one. That's definitely the more friendly way of doing things :)

cool, I changed it to remain backwards compatible

cmd/litcli/autopilot.go

bitromortac · 2024-01-26T15:27:33Z

I changed this PR to include generic feature rule setting via JSON flags. Legacy flags are preserved and marked for deprecation.

lightninglabs-deploy · 2024-02-02T16:17:32Z

@ViktorTigerstrom: review reminder
@ellemouton: review reminder

ViktorTigerstrom

tACK LGTM 🔥🚀!!

Great work!! Thanks for addressing the feedback about adding deprecating support 🚀!

Just adding some minor feedback in my comments below that's not blocking.

Also, I'd just like to show two examples of adding an autopilot session that intuitively probably shouldn't be accepted by the parsing, but that is. We probably don't really need to address these examples though, but feel free to address them if you want:

When using the legacy version of setting the peer-restrict-list arg with multiple features, if you add multiple peer-restrict-list args where the last arg is set to "", that is accepted:
litcli autopilot a --label="test" --feature=AutoFees --peer-restrict-list="peerID1" --feature=SampleFeature --peer-restrict-list=""
If the same feature is specified more than once when adding a autpilot session, that's accepted and only the last version is used:
litcli autopilot a --label="test" --feature=AutoFees --feature-rules="{\"rules\":{}}" --feature=AutoFees --feature-rules="{\"rules\":{\"channel-restriction\":{\"channel_restrict\":{\"channel_ids\":[chanID1]}}}}"

cmd/litcli/autopilot.go

ViktorTigerstrom · 2024-02-05T17:35:08Z

cmd/litcli/autopilot.go

+			"rate-limit": {
+				"rate_limit": {
+					"read_limit": {
+						"iterations": 10,
+						"num_hours": 1
+					},
+					"write_limit": {
+						"iterations": 1,
+						"num_hours": 1
+					}
+				}
+			},
+			"channel-restriction": {
+				"channel_restrict": {
+					"channel_ids": [
+						18283782789, 
+						22891928322, 
+						31878293727
+					]
+				}
+			}


nit:
Since this enables users to set any of the rules now, I do think that:

It makes sense to give an example for all types rules the user can set, i.e. also include examples of the channel-policy-bounds, history-limit & peer-restriction rule types here.

Clarify in the docs that if a specific rule type is not set, the default values will be used.

Good calls, concerning 1, I have added the most interesting rules for AutoFees, but left others away since I think it would add too much help output and it should be clear how to apply the pattern. I updated the help text a bit more to give more hints on how to correctly set rules. I've added that comment for 2.

but left others away since I think it would add too much help output and it should be clear how to apply the pattern

Ok thanks for the edits :)! Sure, I see your point, let's avoid adding too much output. In case we end up getting lot's of user questions of how to apply a specific rule in the future, we can always revisit this and add more info :)!

This commit adds generic feature rule flags. Legacy flags `channel-restrict-list` and `peer-restrict-list` are deprecated.

bitromortac · 2024-02-07T12:03:01Z

Also, I'd just like to show two examples of adding an autopilot session that intuitively probably shouldn't be accepted by the parsing, but that is. We probably don't really need to address these examples though, but feel free to address them if you want:

1. When using the legacy version of setting the `peer-restrict-list` arg with multiple features, if you add multiple `peer-restrict-list` args where the last arg is set to "", that is accepted:
   `litcli autopilot a --label="test" --feature=AutoFees --peer-restrict-list="peerID1" --feature=SampleFeature --peer-restrict-list=""`

2. If the same feature is specified more than once when adding a autpilot session, that's accepted and only the last version is used:
   `litcli autopilot a --label="test" --feature=AutoFees --feature-rules="{\"rules\":{}}" --feature=AutoFees --feature-rules="{\"rules\":{\"channel-restriction\":{\"channel_restrict\":{\"channel_ids\":[chanID1]}}}}"`

Thanks for the review and testing 🙏 @ViktorTigerstrom, those issues should be fixed in the latest version, great calls!

ellemouton

LGTM! thanks for this 🔥

@ViktorTigerstrom - will leave it to you to merge just incase you had anything to add re the latest updates to your feedback :)

ViktorTigerstrom · 2024-02-12T14:50:51Z

Also, I'd just like to show two examples of adding an autopilot session that intuitively probably shouldn't be accepted by the parsing, but that is. We probably don't really need to address these examples though, but feel free to address them if you want:
1. When using the legacy version of setting the `peer-restrict-list` arg with multiple features, if you add multiple `peer-restrict-list` args where the last arg is set to "", that is accepted:
   `litcli autopilot a --label="test" --feature=AutoFees --peer-restrict-list="peerID1" --feature=SampleFeature --peer-restrict-list=""`

2. If the same feature is specified more than once when adding a autpilot session, that's accepted and only the last version is used:
   `litcli autopilot a --label="test" --feature=AutoFees --feature-rules="{\"rules\":{}}" --feature=AutoFees --feature-rules="{\"rules\":{\"channel-restriction\":{\"channel_restrict\":{\"channel_ids\":[chanID1]}}}}"`
Thanks for the review and testing 🙏 @ViktorTigerstrom, those issues should be fixed in the latest version, great calls!

Thanks for the fixes, they look great 🙏!

bitromortac mentioned this pull request Aug 29, 2023

autopilot: feature configuration #629

Merged

bitromortac force-pushed the per-feature-restrictions branch from ebac616 to c861768 Compare September 1, 2023 12:38

bitromortac requested review from ellemouton and ViktorTigerstrom September 1, 2023 12:39

ViktorTigerstrom reviewed Oct 10, 2023

View reviewed changes

bitromortac force-pushed the per-feature-restrictions branch from c861768 to 8a4eeb6 Compare October 16, 2023 14:52

ellemouton reviewed Nov 20, 2023

View reviewed changes

levmi requested a review from ViktorTigerstrom January 9, 2024 16:52

levmi assigned bitromortac Jan 18, 2024

bitromortac force-pushed the per-feature-restrictions branch from 8a4eeb6 to cfa19f8 Compare January 26, 2024 15:24

bitromortac requested a review from ellemouton January 26, 2024 15:30

bitromortac changed the title ~~autopilot: per feature peer and channel restrictions~~ autopilot: generic rules per feature Jan 26, 2024

bitromortac force-pushed the per-feature-restrictions branch from cfa19f8 to fcac5f0 Compare January 26, 2024 15:49

ViktorTigerstrom approved these changes Feb 5, 2024

View reviewed changes

litcli: session registration with generic rules

34eeb2a

This commit adds generic feature rule flags. Legacy flags `channel-restrict-list` and `peer-restrict-list` are deprecated.

bitromortac force-pushed the per-feature-restrictions branch from fcac5f0 to 34eeb2a Compare February 7, 2024 11:56

ellemouton approved these changes Feb 12, 2024

View reviewed changes

ViktorTigerstrom merged commit 6bc86e5 into lightninglabs:master Feb 12, 2024
12 checks passed

bitromortac deleted the per-feature-restrictions branch April 17, 2024 12:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

autopilot: generic rules per feature #634

autopilot: generic rules per feature #634

bitromortac commented Aug 29, 2023 •

edited

Loading

ViktorTigerstrom left a comment

ViktorTigerstrom Oct 10, 2023

bitromortac Oct 16, 2023

ellemouton Nov 20, 2023

bitromortac Jan 26, 2024

bitromortac commented Oct 16, 2023

ellemouton commented Nov 20, 2023

ellemouton left a comment

ellemouton Nov 20, 2023

ellemouton Nov 20, 2023

ellemouton Nov 20, 2023

ellemouton Nov 20, 2023

bitromortac Jan 26, 2024

bitromortac commented Jan 26, 2024

lightninglabs-deploy commented Feb 2, 2024

ViktorTigerstrom left a comment •

edited

Loading

ViktorTigerstrom Feb 5, 2024

bitromortac Feb 7, 2024

ViktorTigerstrom Feb 12, 2024

bitromortac commented Feb 7, 2024

ellemouton left a comment •

edited

Loading

ViktorTigerstrom commented Feb 12, 2024

	`form of: [chanID1,chanID2,...]. ` +
	`form of: ["chanID1","chanID2",...]. ` +

autopilot: generic rules per feature #634

autopilot: generic rules per feature #634

Conversation

bitromortac commented Aug 29, 2023 • edited Loading

ViktorTigerstrom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bitromortac commented Oct 16, 2023

ellemouton commented Nov 20, 2023

ellemouton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bitromortac commented Jan 26, 2024

lightninglabs-deploy commented Feb 2, 2024

ViktorTigerstrom left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bitromortac commented Feb 7, 2024

ellemouton left a comment • edited Loading

Choose a reason for hiding this comment

ViktorTigerstrom commented Feb 12, 2024

bitromortac commented Aug 29, 2023 •

edited

Loading

ViktorTigerstrom left a comment •

edited

Loading

ellemouton left a comment •

edited

Loading