Make onion fields optional on lsp client hook handler #544
Conversation
nepet
changed the title
libs/gl-plugin/src/lsp.rs
Outdated
| #[serde(deserialize_with = "from_hex_opt")] | ||
| shared_secret: Option<Vec<u8>>, | ||
| #[serde(deserialize_with = "from_hex_opt")] | ||
| next_onion: Option<Vec<u8>>, |
There was a problem hiding this comment.
next_onion and shared_secret are not optional, everything else is: https://github.com/ElementsProject/lightning/blob/4186591a7074c45031f4ec221b925264ad78e923/lightningd/peer_htlcs.c#L1110-L1140
libs/gl-plugin/src/lsp.rs
Outdated
| // An onion payload without an `amt_to_forward` is unorthodox and | ||
| // can not be processed by this plugin. Skip it. | ||
| log::debug!( | ||
| "lsp-plugin: got an onion payload={} without an amt forward_msat.", |
There was a problem hiding this comment.
Add the outcome continuing so we don't spook people reading the logs, thinking that it is not being handled.
| @@ -22,8 +22,7 @@ def test_node_start(scheduler, clients): | |||
|
|
|||
|
|
|||
| def test_node_connect(scheduler, clients, bitcoind): | |||
| """Register and schedule a node, then connect to it. | |||
There was a problem hiding this comment.
Feels like the changes here should have been a separate format commit. Are there any logic changes in here?
| /// signal to core-lightning. This is to be used when we don't know how to | ||
| /// handle a given payload or as a shortcut in case we could identify that the | ||
| /// incoming htlc is not part of a LSP jit channel opening. | ||
| macro_rules! unwrap_or_continue { |
There was a problem hiding this comment.
I wonder if this couldn't have been a trait on the Result type instead? That would retain the operation.unwrap_or_continue() order and allow better chaining, rather than having to wrap like this unwrap_or_continue!(operation).
There was a problem hiding this comment.
This is what i considered to do first, but I fail to see how this is possible. How can we return an outer function from an inner function without calling panic (like unwrap does)? I also explored the possibility of using a ControlFlow or a custom implementation of the Try trait, using the ?-operator, but that also seemed a bit overly complicated given that we only use this in one place for now (the try trait is experimental as well).
I would suggest to leave it as is for now and If we feel that we could profit from this pattern at other places as well, we can move it upstream to the cln-plugin repo and may come up with a solution that is a bit more aligned with the chaining approach that rust follows.
As reported by JssDWt in #541, the fields of the onion passed to the htlc_accepted_hook might not be set. To avoid any unnecessary panics we make them optional and handle an unset field in the lsp plugin. Signed-off-by: Peter Neuroth <[email protected]>
Adds a check that we return "continue" on the htlc_accepted_hook if we don't understand an onion payload in the lsp client plugin because of missing fields. Signed-off-by: Peter Neuroth <[email protected]>
Adds a macro that allows us to replace `unwrap`s in order to prevent panics. The macro shortcuts further execution and returns from the htlc_accepted_hook. Signed-off-by: Peter Neuroth <[email protected]>
Remove clippy errors on lsp.rs: `the borrowed expression implements the required traits` Signed-off-by: Peter Neuroth <[email protected]>
nepet
force-pushed
the
2447-make-onion-fields-optional
branch
from
74b1a0e to
be47278
Compare
|
Made |
This PR makes all of the onion payload fields for the lsp clients
htlc_acceptedhook handler optional. We can not assume that all fields are set and we need to act to what we get. If the handler processes an onion payload that is missing essential fields it will stop executing and return thecontinuesignal to core-lightning to pass the htlc to the next processor/handler in line.Resolves #541