-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Payment session and error handling on onion packet forwarding #191
Conversation
8dbf8dd
to
3dcae70
Compare
0b18b5f
to
9292ffa
Compare
9292ffa
to
23c41c3
Compare
c71e4d8
to
a9f752b
Compare
f144887
to
abd1e46
Compare
17f6f2c
to
8cfb17f
Compare
9b595e3
to
118d9bc
Compare
.public_channel_info | ||
.as_ref() | ||
.map(|info| info.tlc_fee_proportional_millionths.unwrap_or_default()) | ||
.unwrap_or_default(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting a default fee value here effectively makes a non-public channel forwarding TLCs? Is my assumption correct? If it is, is this kind of behavior desirable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if this node broadcasting a fee rate 100, then a update on fee rate is done, but the latest fee rate haven't been received by the graph before a payment is sent.
now there are two scenarios:
- the new fee is lower than previous one, but we don't report error here [maybe need to discussed here]
- the new fee is higher than previous one, we need to report error here.
if there is no configuration for the fee(means the node removed fee rate), I think it's same with scenario #1.
How can we find out a route with this node if this node is non-public? At least our current code will not do this. maybe we should add a check that only public node will forward a TLC right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The public_channel_info
is set while creating the channel, we deem every channel with this information as public channel https://github.com/contrun/cfn-node/blob/4033caaae18933ed922fa05543b6d686407628c0/src/fiber/channel.rs#L2224 .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I resolve this issue with 9fc0a35
e372112
to
856a2ad
Compare
1734bfb
to
5ccb247
Compare
} | ||
} | ||
|
||
async fn try_payment_session( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggest to extract two helper functions build_route
and send_onion_packet
to simplifying the main function.
and setting final error to `None`` outside the loop and then assigning it within the loop lead to unnecessary state management and logic complexity.
async fn try_payment_session(
&self,
state: &mut NetworkActorState<S>,
mut payment_session: PaymentSession,
) -> Result<PaymentSession, Error> {
let payment_data = payment_session.request.clone();
let payment_hash = payment_data.payment_hash;
while payment_session.can_retry() {
payment_session.retried_times += 1;
let hops_infos = self.build_route(payment_data.clone()).await?;
let first_channel_outpoint = hops_infos[0]
.channel_outpoint
.clone()
.expect("first hop channel outpoint");
let tlc_id = self.send_onion_packet(state, hops_infos).await?;
payment_session.set_status(PaymentSessionStatus::Inflight);
payment_session.set_first_hop_info(first_channel_outpoint, tlc_id);
self.store.insert_payment_session(payment_session.clone());
return Ok(payment_session);
}
let error_msg = format!("Exhausted retries for payment: {:?}", payment_hash);
payment_session.set_status(PaymentSessionStatus::Failed);
payment_session.set_failed_status(&error_msg);
self.store.insert_payment_session(payment_session);
Err(Error::SendPaymentError(error_msg))
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When there is any error happened in build_route
or send_onion_packet
, instead of just throwing an exception to upper level, we need to set payment session status and store into db.
let me think how to make the code simpler.
Will refactor on PR #228, merge now. |
This PR contains these changes:
get_payment
rpc will query the payment status.