-
Notifications
You must be signed in to change notification settings - Fork 406
Expose counterparty forwarding info in ChannelDetails #841
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -281,6 +281,19 @@ impl HTLCCandidate { | |
} | ||
} | ||
|
||
/// Information needed for constructing an invoice route hint for this channel. | ||
#[derive(Clone)] | ||
pub struct CounterpartyForwardingInfo { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had a look on #207, effectively this make sense to learn counterparty forwarding policy on this link to communicate a routing hint to a payee. I think this struct should document this usage, as otherwise it's a bit confusing why we need to observe peer's Should we also add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Documented!
Hm, but with MPP, the total invoice amount may violate an individual channel's |
||
/// Base routing fee in millisatoshis. | ||
pub fee_base_msat: u32, | ||
/// Amount in millionths of a satoshi the channel will charge per transferred satoshi. | ||
pub fee_proportional_millionths: u32, | ||
/// The minimum difference in cltv_expiry between an ingoing HTLC and its outgoing counterpart, | ||
/// such that the outgoing HTLC is forwardable to this counterparty. See `msgs::ChannelUpdate`'s | ||
/// `cltv_expiry_delta` for more details. | ||
pub cltv_expiry_delta: u16, | ||
} | ||
|
||
// TODO: We should refactor this to be an Inbound/OutboundChannel until initial setup handshaking | ||
// has been completed, and then turn into a Channel to get compiler-time enforcement of things like | ||
// calling channel_id() before we're set up or things like get_outbound_funding_signed on an | ||
|
@@ -391,6 +404,8 @@ pub(super) struct Channel<Signer: Sign> { | |
//implied by OUR_MAX_HTLCS: max_accepted_htlcs: u16, | ||
minimum_depth: u32, | ||
|
||
counterparty_forwarding_info: Option<CounterpartyForwardingInfo>, | ||
|
||
pub(crate) channel_transaction_parameters: ChannelTransactionParameters, | ||
|
||
counterparty_cur_commitment_point: Option<PublicKey>, | ||
|
@@ -577,6 +592,8 @@ impl<Signer: Sign> Channel<Signer> { | |
counterparty_max_accepted_htlcs: 0, | ||
minimum_depth: 0, // Filled in in accept_channel | ||
|
||
counterparty_forwarding_info: None, | ||
|
||
channel_transaction_parameters: ChannelTransactionParameters { | ||
holder_pubkeys: pubkeys, | ||
holder_selected_contest_delay: config.own_channel_config.our_to_self_delay, | ||
|
@@ -813,6 +830,8 @@ impl<Signer: Sign> Channel<Signer> { | |
counterparty_max_accepted_htlcs: msg.max_accepted_htlcs, | ||
minimum_depth: config.own_channel_config.minimum_depth, | ||
|
||
counterparty_forwarding_info: None, | ||
|
||
channel_transaction_parameters: ChannelTransactionParameters { | ||
holder_pubkeys: pubkeys, | ||
holder_selected_contest_delay: config.own_channel_config.our_to_self_delay, | ||
|
@@ -4114,6 +4133,25 @@ impl<Signer: Sign> Channel<Signer> { | |
} | ||
} | ||
|
||
/// Get forwarding information for the counterparty. | ||
pub fn counterparty_forwarding_info(&self) -> Option<CounterpartyForwardingInfo> { | ||
self.counterparty_forwarding_info.clone() | ||
} | ||
|
||
pub fn channel_update(&mut self, msg: &msgs::ChannelUpdate) -> Result<(), ChannelError> { | ||
let usable_channel_value_msat = (self.channel_value_satoshis - self.counterparty_selected_channel_reserve_satoshis) * 1000; | ||
if msg.contents.htlc_minimum_msat >= usable_channel_value_msat { | ||
return Err(ChannelError::Close("Minimum htlc value is greater than channel value".to_string())); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this check is purposeful to make sure when we initiate channel with respect our own config or that other counterparty don't waste time/fees with an unroutable channel, but better to move it in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The check is present in |
||
} | ||
self.counterparty_forwarding_info = Some(CounterpartyForwardingInfo { | ||
fee_base_msat: msg.contents.fee_base_msat, | ||
fee_proportional_millionths: msg.contents.fee_proportional_millionths, | ||
cltv_expiry_delta: msg.contents.cltv_expiry_delta | ||
}); | ||
|
||
Ok(()) | ||
} | ||
|
||
/// Begins the shutdown process, getting a message for the remote peer and returning all | ||
/// holding cell HTLCs for payment failure. | ||
pub fn get_shutdown(&mut self) -> Result<(msgs::Shutdown, Vec<(HTLCSource, PaymentHash)>), APIError> { | ||
|
@@ -4437,6 +4475,16 @@ impl<Signer: Sign> Writeable for Channel<Signer> { | |
self.counterparty_max_accepted_htlcs.write(writer)?; | ||
self.minimum_depth.write(writer)?; | ||
|
||
match &self.counterparty_forwarding_info { | ||
Some(info) => { | ||
1u8.write(writer)?; | ||
info.fee_base_msat.write(writer)?; | ||
info.fee_proportional_millionths.write(writer)?; | ||
info.cltv_expiry_delta.write(writer)?; | ||
}, | ||
None => 0u8.write(writer)? | ||
} | ||
|
||
self.channel_transaction_parameters.write(writer)?; | ||
self.counterparty_cur_commitment_point.write(writer)?; | ||
|
||
|
@@ -4597,6 +4645,16 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel<Signer> | |
let counterparty_max_accepted_htlcs = Readable::read(reader)?; | ||
let minimum_depth = Readable::read(reader)?; | ||
|
||
let counterparty_forwarding_info = match <u8 as Readable>::read(reader)? { | ||
0 => None, | ||
1 => Some(CounterpartyForwardingInfo { | ||
fee_base_msat: Readable::read(reader)?, | ||
fee_proportional_millionths: Readable::read(reader)?, | ||
cltv_expiry_delta: Readable::read(reader)?, | ||
}), | ||
_ => return Err(DecodeError::InvalidValue), | ||
}; | ||
|
||
let channel_parameters = Readable::read(reader)?; | ||
let counterparty_cur_commitment_point = Readable::read(reader)?; | ||
|
||
|
@@ -4667,6 +4725,8 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel<Signer> | |
counterparty_max_accepted_htlcs, | ||
minimum_depth, | ||
|
||
counterparty_forwarding_info, | ||
|
||
channel_transaction_parameters: channel_parameters, | ||
counterparty_cur_commitment_point, | ||
|
||
|
@@ -4702,7 +4762,7 @@ mod tests { | |
use ln::channel::{Channel,Sign,InboundHTLCOutput,OutboundHTLCOutput,InboundHTLCState,OutboundHTLCState,HTLCOutputInCommitment,HTLCCandidate,HTLCInitiator,TxCreationKeys}; | ||
use ln::channel::MAX_FUNDING_SATOSHIS; | ||
use ln::features::InitFeatures; | ||
use ln::msgs::{OptionalField, DataLossProtect, DecodeError}; | ||
use ln::msgs::{ChannelUpdate, DataLossProtect, DecodeError, OptionalField, UnsignedChannelUpdate}; | ||
use ln::chan_utils; | ||
use ln::chan_utils::{ChannelPublicKeys, HolderCommitmentTransaction, CounterpartyChannelTransactionParameters, HTLC_SUCCESS_TX_WEIGHT, HTLC_TIMEOUT_TX_WEIGHT}; | ||
use chain::chaininterface::{FeeEstimator,ConfirmationTarget}; | ||
|
@@ -4713,6 +4773,7 @@ mod tests { | |
use util::test_utils; | ||
use util::logger::Logger; | ||
use bitcoin::secp256k1::{Secp256k1, Message, Signature, All}; | ||
use bitcoin::secp256k1::ffi::Signature as FFISignature; | ||
use bitcoin::secp256k1::key::{SecretKey,PublicKey}; | ||
use bitcoin::hashes::sha256::Hash as Sha256; | ||
use bitcoin::hashes::Hash; | ||
|
@@ -4969,6 +5030,54 @@ mod tests { | |
} | ||
} | ||
|
||
#[test] | ||
fn channel_update() { | ||
let feeest = TestFeeEstimator{fee_est: 15000}; | ||
let secp_ctx = Secp256k1::new(); | ||
let seed = [42; 32]; | ||
let network = Network::Testnet; | ||
let chain_hash = genesis_block(network).header.block_hash(); | ||
let keys_provider = test_utils::TestKeysInterface::new(&seed, network); | ||
|
||
// Create a channel. | ||
let node_b_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap()); | ||
let config = UserConfig::default(); | ||
let mut node_a_chan = Channel::<EnforcingSigner>::new_outbound(&&feeest, &&keys_provider, node_b_node_id, 10000000, 100000, 42, &config).unwrap(); | ||
assert!(node_a_chan.counterparty_forwarding_info.is_none()); | ||
assert_eq!(node_a_chan.holder_htlc_minimum_msat, 1); // the default | ||
assert!(node_a_chan.counterparty_forwarding_info().is_none()); | ||
|
||
// Make sure that receiving a channel update will update the Channel as expected. | ||
let update = ChannelUpdate { | ||
contents: UnsignedChannelUpdate { | ||
chain_hash, | ||
short_channel_id: 0, | ||
timestamp: 0, | ||
flags: 0, | ||
cltv_expiry_delta: 100, | ||
htlc_minimum_msat: 5, | ||
htlc_maximum_msat: OptionalField::Absent, | ||
fee_base_msat: 110, | ||
fee_proportional_millionths: 11, | ||
excess_data: Vec::new(), | ||
}, | ||
signature: Signature::from(unsafe { FFISignature::new() }) | ||
}; | ||
node_a_chan.channel_update(&update).unwrap(); | ||
|
||
// The counterparty can send an update with a higher minimum HTLC, but that shouldn't | ||
// change our official htlc_minimum_msat. | ||
assert_eq!(node_a_chan.holder_htlc_minimum_msat, 1); | ||
match node_a_chan.counterparty_forwarding_info() { | ||
Some(info) => { | ||
assert_eq!(info.cltv_expiry_delta, 100); | ||
assert_eq!(info.fee_base_msat, 110); | ||
assert_eq!(info.fee_proportional_millionths, 11); | ||
}, | ||
None => panic!("expected counterparty forwarding info to be Some") | ||
} | ||
} | ||
|
||
#[test] | ||
fn outbound_commitment_test() { | ||
// Test vectors from BOLT 3 Appendix C: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -142,6 +142,8 @@ impl ChannelMessageHandler for ErroringMessageHandler { | |
fn handle_channel_reestablish(&self, their_node_id: &PublicKey, msg: &msgs::ChannelReestablish) { | ||
ErroringMessageHandler::push_error(self, their_node_id, msg.channel_id); | ||
} | ||
// msgs::ChannelUpdate does not contain the channel_id field, so we just drop them. | ||
fn handle_channel_update(&self, _their_node_id: &PublicKey, _msg: &msgs::ChannelUpdate) {} | ||
fn peer_disconnected(&self, _their_node_id: &PublicKey, _no_connection_possible: bool) {} | ||
fn peer_connected(&self, _their_node_id: &PublicKey, _msg: &msgs::Init) {} | ||
fn handle_error(&self, _their_node_id: &PublicKey, _msg: &msgs::ErrorMessage) {} | ||
|
@@ -970,6 +972,7 @@ impl<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, L: Deref> PeerManager<D | |
} | ||
}, | ||
wire::Message::ChannelUpdate(msg) => { | ||
self.message_handler.chan_handler.handle_channel_update(&peer.their_node_id.unwrap(), &msg); | ||
let should_forward = match self.message_handler.route_handler.handle_channel_update(&msg) { | ||
Comment on lines
+975
to
976
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If checks in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We'd need some way to differentiate "we haven't seen a channel_announcement/the channel is private" as a failure from "invalid signature" or other failures. We /could/ check the signature in ChannelManager, but we know it came directly from our peer, so not sure if we need to. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see what you are saying for this particular message. Though I do wonder more broadly if we are expecting custom handler implementations to re-implement all BOLT checks or if there is some subset we should check regardless of implementation. Definitely outside the scope of this PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yea, thats a good question. I kinda figure there's relatively limited uses for implementing a full custom message handler, and its more about the message handlers being optional/dummy in cases where, eg, you don't want to have a network graph so I was never too worried about it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suppose the wrapper approach in a similar vein as |
||
Ok(v) => v, | ||
Err(e) => { return Err(e.into()); }, | ||
|
Uh oh!
There was an error while loading. Please reload this page.