Eclair node support #225
Eclair node support #225
Conversation
|
Thanks for the PR, awesome stuff! On my list to review this week 🫡 Small CI nitpick - |
|
@carlaKC should be good now. Should I squash every time I push some fix? |
My personal preference is to use |
alexzaitsev
force-pushed
the
eclair-support
branch
from
0c9bff1 to
d9409b7
Compare
|
I squshed. I'm not aware about fixup commits, will study this. Thank you |
|
hi @alexzaitsev thanks for the PR. I am trying to run it with polar but I am getting this error: |
README.md
Outdated
| enabled in LND using `--accept-keysend` (for CLN node it is enabled by default). | ||
| Use `--features.keysend=optional` to enable keysend in Eclair. |
There was a problem hiding this comment.
nit: Now that we've got another node let's make this a list to improve readability, something like:
which must be enabed as follows:
* LND: `--accept-keysend`
* Eclair: ...
There was a problem hiding this comment.
Done: 185d020
I also moved this info to prerequisites section where it should be IMO. Let me know if you have any objections.
simln-lib/src/eclair.rs
Outdated
| } | ||
|
|
||
| impl EclairConnection { | ||
| fn construct_url(&self, endpoint: &str) -> anyhow::Result<Url> { |
There was a problem hiding this comment.
Let's use a regular Result in this module? We're only using anyhow in main at the moment and I'd actually like to get rid of it in future.
There was a problem hiding this comment.
Done: f0e1c2f
I used a trait object so I don't need to import url just to return its error from construct_url
simln-lib/src/eclair.rs
Outdated
| } | ||
|
|
||
| pub struct EclairNode { | ||
| connection: EclairConnection, |
There was a problem hiding this comment.
Thoughts on adding a constructor that accepts the fields from EclairConnection and storing the construct_url and basic_auth values in EclairNode instead of saving the connection details to DRY things up a bit?
That way we don't have to keep a duplicate NodeID (connection+ NodeInfo) and re-construct base_url/
There was a problem hiding this comment.
Addressed: 5f2457c
simln-lib/src/eclair.rs
Outdated
| // Task: https://github.com/bitcoin-dev-project/sim-ln/issues/26#issuecomment-1691780018 | ||
| PaymentStatus::Failed => PaymentOutcome::UnexpectedError, |
There was a problem hiding this comment.
Not following the comment linked to here with this line of code?
Does this status from eclair mean that the API call failed?
There was a problem hiding this comment.
I followed other implementations in this codebase. In this particular case here is respective CLN code. Basically, as whole range of errors is not well documented (comparing to LND GRPC, for example), I followed CLN node implementation approach and returning UnexpectedError here. For consistency, I copied this comment as well.
There was a problem hiding this comment.
Cool got it, could you please expand the comment to just briefly say that PaymentStatus::Failed means that the API call failed and then link to the issue for additional context?
simln-lib/src/eclair.rs
Outdated
| Ok(response) | ||
| } | ||
|
|
||
| async fn request<T: for<'de> Deserialize<'de>>( |
There was a problem hiding this comment.
The division in duties of this wrapping function and request_static weren't immediately obvious to be, could we either add a comment to explain the need for this or move the connection details into its own client struct and implement request on it?
Could work well with the request above, EclairNode then just holds a client / info / network - this is how we have it for LndNode, for example.
There was a problem hiding this comment.
request_static is used in the constructor, this is the only reason why it exists. Otherwise, it will be only request. After all the refactoring it looks a bit cleaner so I won't go deeper for now and leave it as it is. If after second review you still find this can be improved, please let me know and I'll handle it.
There was a problem hiding this comment.
I was thinking something like this patch to pull the request details out into their own struct, so that the EclairNode's single responsibility is implementing LightningNode trait.
There was a problem hiding this comment.
makes sense! Done 0157beb
There was a problem hiding this comment.
We can simplify this now that we've got a EclairClient type:
impl EclairClient {
async fn request<T: for<'de> Deserialize<'de>>(
&self,
endpoint: &str,
params: Option<HashMap<String, String>>,
) -> Result<T, Box<dyn Error>> {
let url = self.base_url.join(endpoint)?;
let mut request = self.http_client
.request(Method::POST, url)
.basic_auth(self.api_username.clone(), Some(self.api_password.clone()));
if let Some(params) = params {
let mut form = Form::new();
for (key, value) in params {
form = form.text(key, value);
}
request = request.multipart(form);
}
let response = request
.send()
.await?
.error_for_status()?
.json::<T>()
.await?;
Ok(response)
}
}
README.md
Outdated
| ``` | ||
| { | ||
| "id": <node_id>, | ||
| "base_url": <scheme://ip:port or scheme://domain:port>, |
There was a problem hiding this comment.
nit: remove scheme:// here and http: below so that the readme has the simplest example possible
There was a problem hiding this comment.
Addressed: be85f5b
I also did some light cleanup and moved list of implementations to be above the prerequisites (it's more important IMO). Let me know if any of these changes doesn't work for you.
simln-lib/src/eclair.rs
Outdated
| Ok(response) | ||
| } | ||
|
|
||
| async fn request<T: for<'de> Deserialize<'de>>( |
There was a problem hiding this comment.
I was thinking something like this patch to pull the request details out into their own struct, so that the EclairNode's single responsibility is implementing LightningNode trait.
simln-lib/src/eclair.rs
Outdated
| // Task: https://github.com/bitcoin-dev-project/sim-ln/issues/26#issuecomment-1691780018 | ||
| PaymentStatus::Failed => PaymentOutcome::UnexpectedError, |
There was a problem hiding this comment.
Cool got it, could you please expand the comment to just briefly say that PaymentStatus::Failed means that the API call failed and then link to the issue for additional context?
There was a problem hiding this comment.
I did more tests.
- A defined activity between two nodes where only one has all the liquidity. ✅
DEBUG [simln_lib] Send payment: bob(02c560...045557) -> alice(020717...b16e7c): (94c34420ce56e43289c4e761c30e76b5136b02da432d4f25efaf9725dd221bfb).
DEBUG [simln_lib] Tracking payment outcome for: 94c34420ce56e43289c4e761c30e76b5136b02da432d4f25efaf9725dd221bfb.
DEBUG [simln_lib] Track payment 94c34420ce56e43289c4e761c30e76b5136b02da432d4f25efaf9725dd221bfb result: UnexpectedError.
DEBUG [simln_lib] Generated payment: alice(020717...b16e7c) -> bob(02c560...045557): 1000 msat.
DEBUG [simln_lib] Next payment for alice(020717...b16e7c) in 3s.The failure case: when the node without liquidity sends a payment is handled correctly.
- Using the same two nodes I tried to run a random activity ❌
There is a problem here because it looks likeasync fn list_channelsdoes not return the defined channels between the nodes.
Node: 020717539a3ad606adb68107c7f9c5abf0fc91e37f13d461ca1949b71546b16e7c not eligible for activity generation: InsufficientCapacity: node needs at least 7600000 capacity (has: 0) to process expected payment amount: 3800000@alexzaitsev could you check this case from your side, please?
|
@f3r10 here is I'm getting next output: sim.json: Seems like I have another output? I'm confused. |
|
Ok, I see the issue, I can confirm |
|
@f3r10 I applied a few fixes and it should be good now, could you retest? I found out that Eclair channels are unstable, especially if you restart those Docker nodes (or whole network). Maybe it's related to Polar, I don't know. It can result in some unexpected behaviour, especially considering the channels look OK in Polar UI. For example, here is investigation of As you see, it shows Indeed shows channels are offline: This is just something to consider as this can be an obstacle in the testing process. For the best experience, I recommend spawning fresh nodes with the fresh channels each time. Another thing, not related to the current discussion. I noticed pretty often sim-ln tries to send zero amounts: Is it expected? |
This is okay! TL;DR on how we generate random activity:
You can run into this zero-amount behavior when you have channels that are relatively close to the expected payment amount, because to meet the expected amount you don't need to send that many payments. Eg, if the expected payment amount is 10k, your channel will only need to send 4 payments to hit the total it needs to send. The defaults are set based on mainnet values, so they can be a bit funky in test envs! |
simln-lib/src/eclair.rs
Outdated
| Ok(response) | ||
| } | ||
|
|
||
| async fn request<T: for<'de> Deserialize<'de>>( |
There was a problem hiding this comment.
We can simplify this now that we've got a EclairClient type:
impl EclairClient {
async fn request<T: for<'de> Deserialize<'de>>(
&self,
endpoint: &str,
params: Option<HashMap<String, String>>,
) -> Result<T, Box<dyn Error>> {
let url = self.base_url.join(endpoint)?;
let mut request = self.http_client
.request(Method::POST, url)
.basic_auth(self.api_username.clone(), Some(self.api_password.clone()));
if let Some(params) = params {
let mut form = Form::new();
for (key, value) in params {
form = form.text(key, value);
}
request = request.multipart(form);
}
let response = request
.send()
.await?
.error_for_status()?
.json::<T>()
.await?;
Ok(response)
}
}
alexzaitsev
force-pushed
the
eclair-support
branch
from
4c8ce4c to
1b29d37
Compare
|
@f3r10 any additional testing you'd like to do here or are we good to go? |
|
Ah sorry @alexzaitsev, gave you some minor conflicts merging #229 - could you rebase and then I'll merge? |
alexzaitsev
force-pushed
the
eclair-support
branch
from
1b29d37 to
b41ebca
Compare
Added Eclair node support. Tracking issue: #26
Tested on local Polar network with different use cases.