From 209f72d2297cfe833bc0060cd6447ffca67413c8 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Sat, 4 May 2024 01:28:40 +0930 Subject: [PATCH 1/6] pyln-client: allow dynamic=True add_option to actually allow updates. We didn't actually *change* the value you'd see, when we got a setconfig call! Changelog-Added: pyln-client: implement setconfig hook for plugins so you can see changes in `dynamic` options. Signed-off-by: Rusty Russell --- contrib/pyln-client/pyln/client/plugin.py | 5 ++--- tests/plugins/dynamic_option.py | 6 ++++++ tests/test_plugin.py | 2 ++ 3 files changed, 10 insertions(+), 3 deletions(-) diff --git a/contrib/pyln-client/pyln/client/plugin.py b/contrib/pyln-client/pyln/client/plugin.py index 6bdffa14b9a9..ab848e69abde 100644 --- a/contrib/pyln-client/pyln/client/plugin.py +++ b/contrib/pyln-client/pyln/client/plugin.py @@ -974,11 +974,10 @@ def verify_bool(d: Dict[str, JSONType], key: str) -> bool: return self._exec_func(self.child_init, request) return None - def _set_config(self, **_) -> None: + def _set_config(self, config: str, val: Optional[Any]) -> None: """Called when the value of a dynamic option is changed - For now we don't do anything. """ - pass + self.options[config]['value'] = val class PluginStream(object): diff --git a/tests/plugins/dynamic_option.py b/tests/plugins/dynamic_option.py index 181bc176bea1..5266dc01bab3 100755 --- a/tests/plugins/dynamic_option.py +++ b/tests/plugins/dynamic_option.py @@ -9,4 +9,10 @@ default="initial", dynamic=True) + +@plugin.method('dynamic-option-report') +def record_lookup(plugin): + return {'test-dynamic-config': plugin.get_option('test-dynamic-config')} + + plugin.run() diff --git a/tests/test_plugin.py b/tests/test_plugin.py index f9a6965ea9de..7b67426baf52 100644 --- a/tests/test_plugin.py +++ b/tests/test_plugin.py @@ -4314,8 +4314,10 @@ def test_dynamic_option_python_plugin(node_factory): assert result["configs"]["test-dynamic-config"]["value_str"] == "initial" + assert ln.rpc.dynamic_option_report() == {'test-dynamic-config': 'initial'} result = ln.rpc.setconfig("test-dynamic-config", "changed") assert result["config"]["value_str"] == "changed" + assert ln.rpc.dynamic_option_report() == {'test-dynamic-config': 'changed'} def test_renepay_not_important(node_factory): From 1b5c27df2161c8f946713aa6374019217162a2d5 Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Fri, 3 May 2024 18:27:17 +0200 Subject: [PATCH 2/6] pyln: Add and test a callback based setconfig listener I was looking into using the `threading.Condition` but since we're already rather heavily using callbacks, this allows us to stay single-threaded, and not having to completely hook the `setconfig` function. Changelog-Added: pyln-client: Added a notification mechanism for config changes --- contrib/pyln-client/pyln/client/plugin.py | 21 ++++++++++++++++++--- tests/plugins/dynamic_option.py | 22 ++++++++++++++++------ tests/test_plugin.py | 4 ++++ 3 files changed, 38 insertions(+), 9 deletions(-) diff --git a/contrib/pyln-client/pyln/client/plugin.py b/contrib/pyln-client/pyln/client/plugin.py index ab848e69abde..2d53e194c535 100644 --- a/contrib/pyln-client/pyln/client/plugin.py +++ b/contrib/pyln-client/pyln/client/plugin.py @@ -392,7 +392,9 @@ def add_option(self, name: str, default: Optional[str], description: Optional[str], opt_type: str = "string", deprecated: Union[bool, List[str]] = None, multi: bool = False, - dynamic=False) -> None: + dynamic=False, + on_change: Optional[Callable[["Plugin", str, Optional[Any]], None]] = None, + ) -> None: """Add an option that we'd like to register with lightningd. Needs to be called before `Plugin.run`, otherwise we might not @@ -417,7 +419,8 @@ def add_option(self, name: str, default: Optional[str], 'value': None, 'multi': multi, 'deprecated': deprecated, - "dynamic": dynamic + "dynamic": dynamic, + 'on_change': on_change, } def add_flag_option(self, name: str, description: str, @@ -977,7 +980,19 @@ def verify_bool(d: Dict[str, JSONType], key: str) -> bool: def _set_config(self, config: str, val: Optional[Any]) -> None: """Called when the value of a dynamic option is changed """ - self.options[config]['value'] = val + opt = self.options[config] + opt['value'] = val + + cb = opt['on_change'] + if cb is None: + return + + try: + opt['on_change'](self, config, val) + except Exception as e: + logging.getLogger('pyln.client.plugin').warn( + f"setconfig callback raised an exception: {e}" + ) class PluginStream(object): diff --git a/tests/plugins/dynamic_option.py b/tests/plugins/dynamic_option.py index 5266dc01bab3..2f6efc1493e8 100755 --- a/tests/plugins/dynamic_option.py +++ b/tests/plugins/dynamic_option.py @@ -1,18 +1,28 @@ #!/usr/bin/env python3 from pyln.client import Plugin +from typing import Any, Optional plugin = Plugin() -plugin.add_option( - name="test-dynamic-config", - description="A config option which can be changed at run-time", - default="initial", - dynamic=True) - @plugin.method('dynamic-option-report') def record_lookup(plugin): return {'test-dynamic-config': plugin.get_option('test-dynamic-config')} +def on_config_change(plugin, config: str, value: Optional[Any]) -> None: + """Callback method called when a config value is changed. + """ + plugin.log(f"Setting config {config} to {value}") + + +plugin.add_option( + name="test-dynamic-config", + description="A config option which can be changed at run-time", + default="initial", + dynamic=True, + on_change=on_config_change, +) + + plugin.run() diff --git a/tests/test_plugin.py b/tests/test_plugin.py index 7b67426baf52..902115eabdd7 100644 --- a/tests/test_plugin.py +++ b/tests/test_plugin.py @@ -4319,6 +4319,10 @@ def test_dynamic_option_python_plugin(node_factory): assert result["config"]["value_str"] == "changed" assert ln.rpc.dynamic_option_report() == {'test-dynamic-config': 'changed'} + ln.daemon.wait_for_log( + 'dynamic_option.py:.*Setting config test-dynamic-config to changed' + ) + def test_renepay_not_important(node_factory): # I mean, it's *important*, it's just not "mission-critical" just yet! From b2300d340ed87be2bca30628aecaf95a7e6fa6ff Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Fri, 3 May 2024 18:29:28 +0200 Subject: [PATCH 3/6] pyln: Drive-by type fixed --- contrib/pyln-client/pyln/client/plugin.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/contrib/pyln-client/pyln/client/plugin.py b/contrib/pyln-client/pyln/client/plugin.py index 2d53e194c535..be9dd42a71a0 100644 --- a/contrib/pyln-client/pyln/client/plugin.py +++ b/contrib/pyln-client/pyln/client/plugin.py @@ -390,7 +390,8 @@ def decorator(f: Callable[..., None]) -> Callable[..., None]: def add_option(self, name: str, default: Optional[str], description: Optional[str], - opt_type: str = "string", deprecated: Union[bool, List[str]] = None, + opt_type: str = "string", + deprecated: Optional[Union[bool, List[str]]] = None, multi: bool = False, dynamic=False, on_change: Optional[Callable[["Plugin", str, Optional[Any]], None]] = None, @@ -424,7 +425,7 @@ def add_option(self, name: str, default: Optional[str], } def add_flag_option(self, name: str, description: str, - deprecated: Union[bool, List[str]] = None, + deprecated: Optional[Union[bool, List[str]]] = None, dynamic: bool = False) -> None: """Add a flag option that we'd like to register with lightningd. From af78001dc822f124aafbd76e4037ce6d0da1a9f0 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Sat, 4 May 2024 07:48:34 +0930 Subject: [PATCH 4/6] pyln-client: allow dynamic option setter to throw exceptions And don't set the value unless it passes. Signed-off-by: Rusty Russell --- contrib/pyln-client/pyln/client/plugin.py | 23 +++++++++++------------ tests/plugins/dynamic_option.py | 4 +++- tests/test_plugin.py | 6 ++++++ 3 files changed, 20 insertions(+), 13 deletions(-) diff --git a/contrib/pyln-client/pyln/client/plugin.py b/contrib/pyln-client/pyln/client/plugin.py index be9dd42a71a0..446e15c84f2b 100644 --- a/contrib/pyln-client/pyln/client/plugin.py +++ b/contrib/pyln-client/pyln/client/plugin.py @@ -409,7 +409,12 @@ def add_option(self, name: str, default: Optional[str], if opt_type not in ["string", "int", "bool", "flag"]: raise ValueError( - '{} not in supported type set (string, int, bool, flag)' + '{} not in supported type set (string, int, bool, flag)'.format(opt_type) + ) + + if on_change is not None and not dynamic: + raise ValueError( + 'Option {} has on_change callback but is not dynamic'.format(name) ) self.options[name] = { @@ -981,19 +986,13 @@ def verify_bool(d: Dict[str, JSONType], key: str) -> bool: def _set_config(self, config: str, val: Optional[Any]) -> None: """Called when the value of a dynamic option is changed """ - opt = self.options[config] - opt['value'] = val - cb = opt['on_change'] - if cb is None: - return + if cb is not None: + # This may throw an exception: caller will turn into error msg for user. + cb(self, config, val) - try: - opt['on_change'](self, config, val) - except Exception as e: - logging.getLogger('pyln.client.plugin').warn( - f"setconfig callback raised an exception: {e}" - ) + opt = self.options[config] + opt['value'] = val class PluginStream(object): diff --git a/tests/plugins/dynamic_option.py b/tests/plugins/dynamic_option.py index 2f6efc1493e8..7ac3717ffe89 100755 --- a/tests/plugins/dynamic_option.py +++ b/tests/plugins/dynamic_option.py @@ -1,5 +1,5 @@ #!/usr/bin/env python3 -from pyln.client import Plugin +from pyln.client import Plugin, RpcException from typing import Any, Optional plugin = Plugin() @@ -14,6 +14,8 @@ def on_config_change(plugin, config: str, value: Optional[Any]) -> None: """Callback method called when a config value is changed. """ plugin.log(f"Setting config {config} to {value}") + if value == 'bad value': + raise RpcException("I don't like bad values!") plugin.add_option( diff --git a/tests/test_plugin.py b/tests/test_plugin.py index 902115eabdd7..9490a7e20f59 100644 --- a/tests/test_plugin.py +++ b/tests/test_plugin.py @@ -4323,6 +4323,12 @@ def test_dynamic_option_python_plugin(node_factory): 'dynamic_option.py:.*Setting config test-dynamic-config to changed' ) + with pytest.raises(RpcError, match="I don't like bad values!"): + ln.rpc.setconfig("test-dynamic-config", "bad value") + + # Does not alter value! + assert ln.rpc.dynamic_option_report() == {'test-dynamic-config': 'changed'} + def test_renepay_not_important(node_factory): # I mean, it's *important*, it's just not "mission-critical" just yet! From 2adcc676e53b6d5e00b104fe419045dad5ef8ff3 Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Sat, 4 May 2024 01:10:42 +0200 Subject: [PATCH 5/6] pyln: Turn the plugin options into a real Type (uppercase T) [ Fix not to include 'value' and 'default' (if None) in getmanifest response --RR ] [ Fix to support [] operator for existing plugins (including our test ones!) --RR ] --- contrib/pyln-client/pyln/client/plugin.py | 108 ++++++++++++++-------- 1 file changed, 72 insertions(+), 36 deletions(-) diff --git a/contrib/pyln-client/pyln/client/plugin.py b/contrib/pyln-client/pyln/client/plugin.py index 446e15c84f2b..1e7116b47677 100644 --- a/contrib/pyln-client/pyln/client/plugin.py +++ b/contrib/pyln-client/pyln/client/plugin.py @@ -1,10 +1,3 @@ -from .lightning import LightningRpc, Millisatoshi -from binascii import hexlify -from collections import OrderedDict -from enum import Enum -from threading import RLock -from typing import Any, Callable, Dict, List, Optional, Tuple, Union - import inspect import io import json @@ -14,6 +7,14 @@ import re import sys import traceback +from binascii import hexlify +from collections import OrderedDict +from dataclasses import dataclass +from enum import Enum +from threading import RLock +from typing import Any, Callable, Dict, List, Optional, Tuple, Union + +from .lightning import LightningRpc, Millisatoshi # Notice that this definition is incomplete as it only checks the # top-level. Arrays and Dicts could contain types that aren't encodeable. This @@ -183,6 +184,38 @@ def progress(self, self._notify("progress", d) +@dataclass +class Option: + name: str + default: Optional[Any] + description: Optional[str] + opt_type: str + value: Optional[Any] + multi: bool + deprecated: Optional[Union[bool, List[str]]] + dynamic: bool + on_change: Optional[Callable[["Plugin", str, Optional[Any]], None]] + + def __getitem__(self, key): + """Backwards compatibility for callers who directly asked for ['value']""" + if key == 'value': + return self.value + raise KeyError(f"Key {key} not supported, only 'value' is") + + def json(self) -> Dict[str, Any]: + ret = { + 'name': self.name, + 'description': self.description, + 'type': self.opt_type, + 'multi': self.multi, + 'deprecated': self.deprecated, + 'dynamic': self.dynamic, + } + if self.default is not None: + ret['default'] = self.default + return ret + + # If a hook call fails we need to coerce it into something the main daemon can # handle. Returning an error is not an option since we explicitly do not allow # those as a response to the calls, otherwise the only option we have is to @@ -224,7 +257,7 @@ def __init__(self, stdout: Optional[io.TextIOBase] = None, 'setconfig': Method('setconfig', self._set_config, MethodType.RPCMETHOD) } - self.options: Dict[str, Dict[str, Any]] = {} + self.options: Dict[str, Option] = {} self.notification_topics: List[str] = [] self.custom_msgs = custom_msgs @@ -297,7 +330,7 @@ def add_method(self, name: str, func: Callable[..., Any], category: Optional[str] = None, desc: Optional[str] = None, long_desc: Optional[str] = None, - deprecated: Union[bool, List[str]] = None) -> None: + deprecated: Optional[Union[bool, List[str]]] = None) -> None: """Add a plugin method to the dispatch table. The function will be expected at call time (see `_dispatch`) @@ -388,7 +421,7 @@ def decorator(f: Callable[..., None]) -> Callable[..., None]: return f return decorator - def add_option(self, name: str, default: Optional[str], + def add_option(self, name: str, default: Optional[Any], description: Optional[str], opt_type: str = "string", deprecated: Optional[Union[bool, List[str]]] = None, @@ -417,17 +450,17 @@ def add_option(self, name: str, default: Optional[str], 'Option {} has on_change callback but is not dynamic'.format(name) ) - self.options[name] = { - 'name': name, - 'default': default, - 'description': description, - 'type': opt_type, - 'value': None, - 'multi': multi, - 'deprecated': deprecated, - "dynamic": dynamic, - 'on_change': on_change, - } + self.options[name] = Option( + name=name, + default=default, + description=description, + opt_type=opt_type, + value=None, + dynamic=dynamic, + on_change=on_change, + multi=multi, + deprecated=deprecated if deprecated is not None else False, + ) def add_flag_option(self, name: str, description: str, deprecated: Optional[Union[bool, List[str]]] = None, @@ -446,19 +479,19 @@ def add_notification_topic(self, topic: str): """ self.notification_topics.append(topic) - def get_option(self, name: str) -> str: + def get_option(self, name: str) -> Optional[Any]: if name not in self.options: raise ValueError("No option with name {} registered".format(name)) - if self.options[name]['value'] is not None: - return self.options[name]['value'] + if self.options[name].value is not None: + return self.options[name].value else: - return self.options[name]['default'] + return self.options[name].default def async_method(self, method_name: str, category: Optional[str] = None, desc: Optional[str] = None, long_desc: Optional[str] = None, - deprecated: Union[bool, List[str]] = None) -> NoneDecoratorType: + deprecated: Optional[Union[bool, List[str]]] = None) -> NoneDecoratorType: """Decorator to add an async plugin method to the dispatch table. Internally uses add_method. @@ -835,16 +868,19 @@ def print_usage(self): parts.append(options_header) options_header = None - doc = textwrap.indent(opt['description'], prefix=" ") + if opt.description: + doc = textwrap.indent(opt.description, prefix=" ") + else: + doc = "" - if opt['multi']: + if opt.multi: doc += "\n\n This option can be specified multiple times" parts.append(option_tpl.format( - name=opt['name'], + name=opt.name, doc=doc, - default=opt['default'], - typ=opt['type'], + default=opt.default, + typ=opt.opt_type, )) sys.stdout.write("".join(parts)) @@ -930,7 +966,7 @@ def _getmanifest(self, **kwargs) -> JSONType: m["long_description"] = method.long_desc manifest = { - 'options': list({k: v for k, v in d.items() if v is not None} for d in self.options.values()), + 'options': list(d.json() for d in self.options.values()), 'rpcmethods': methods, 'subscriptions': list(self.subscriptions.keys()), 'hooks': hooks, @@ -976,7 +1012,7 @@ def verify_bool(d: Dict[str, JSONType], key: str) -> bool: self.rpc = LightningRpc(path) self.startup = verify_bool(configuration, 'startup') for name, value in options.items(): - self.options[name]['value'] = value + self.options[name].value = value # Dispatch the plugin's init handler if any if self.child_init: @@ -986,13 +1022,13 @@ def verify_bool(d: Dict[str, JSONType], key: str) -> bool: def _set_config(self, config: str, val: Optional[Any]) -> None: """Called when the value of a dynamic option is changed """ - cb = opt['on_change'] + opt = self.options[config] + cb = opt.on_change if cb is not None: # This may throw an exception: caller will turn into error msg for user. cb(self, config, val) - opt = self.options[config] - opt['value'] = val + opt.value = val class PluginStream(object): From fa8b6aa577e0d222ae7364dd3924ac275ad4c31e Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 20 Jun 2024 13:23:51 +0930 Subject: [PATCH 6/6] pytest: fix default/description order in zeroconf test plugin. Noticed as I was debugging. Signed-off-by: Rusty Russell --- tests/plugins/zeroconf-selective.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/plugins/zeroconf-selective.py b/tests/plugins/zeroconf-selective.py index 0ff72fd5a9c8..b359a206f568 100755 --- a/tests/plugins/zeroconf-selective.py +++ b/tests/plugins/zeroconf-selective.py @@ -21,8 +21,8 @@ def on_openchannel(openchannel, plugin, **kwargs): plugin.add_option( 'zeroconf-allow', + '03864ef025fde8fb587d989186ce6a4a186895ee44a926bfc370e2c366597a3f8f', 'A node_id to allow zeroconf channels from', - '03864ef025fde8fb587d989186ce6a4a186895ee44a926bfc370e2c366597a3f8f' ) plugin.add_option(