Skip to content

Remove generic Signer parameter where it can be inferred from KeysInterface #1806

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Nov 11, 2022
Merged

Remove generic Signer parameter where it can be inferred from KeysInterface #1806

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 11 additions & 13 deletions lightning-background-processor/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ extern crate lightning_rapid_gossip_sync;
use lightning::chain;
use lightning::chain::chaininterface::{BroadcasterInterface, FeeEstimator};
use lightning::chain::chainmonitor::{ChainMonitor, Persist};
use lightning::chain::keysinterface::{Sign, KeysInterface};
use lightning::chain::keysinterface::KeysInterface;
use lightning::ln::channelmanager::ChannelManager;
use lightning::ln::msgs::{ChannelMessageHandler, OnionMessageHandler, RoutingMessageHandler};
use lightning::ln::peer_handler::{CustomMessageHandler, PeerManager, SocketDescriptor};
Expand Down Expand Up @@ -365,7 +365,6 @@ macro_rules! define_run_body {
#[cfg(feature = "futures")]
pub async fn process_events_async<
'a,
Signer: 'static + Sign,
CA: 'static + Deref + Send + Sync,
CF: 'static + Deref + Send + Sync,
CW: 'static + Deref + Send + Sync,
Expand All @@ -381,7 +380,7 @@ pub async fn process_events_async<
OMH: 'static + Deref + Send + Sync,
EH: 'static + EventHandler + Send,
PS: 'static + Deref + Send,
M: 'static + Deref<Target = ChainMonitor<Signer, CF, T, F, L, P>> + Send + Sync,
M: 'static + Deref<Target = ChainMonitor<<K::Target as KeysInterface>::Signer, CF, T, F, L, P>> + Send + Sync,
Copy link
Collaborator

@TheBlueMatt TheBlueMatt Oct 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than moving to a signer here (and on chain::Watch and Persist and....) let's just parameterize those by KeysInterface directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should that be part of this PR or in a followup?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like you should just do that first - it would substantially reduce churn here, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually think there's a big difference between removing Signer from types that were parametrized by both Signer and KeysInterface, and reparametrizing types from Signer to KeysInterface. I'd much prefer to do the latter in a follow-up PR, if possible.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but it's about the same patch size and reduces churn - if we're gonna do it imminently anyway (which I thought was the point?) we should do it in one go because otherwise we're touching the same (sets of) line(s) of code in repeated PRs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's ok, I'd rather work a bit more on the Taproot refactors first because I'm starting to be a bit less sure if we wanna convert all the instances from using Signer to using KeysInterface. That's why I'd rather get this out of the way first, but not commit to the rest of the refactor yet.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, okay. This needs rebase, then.

CM: 'static + Deref<Target = ChannelManager<CW, T, K, F, L>> + Send + Sync,
PGS: 'static + Deref<Target = P2PGossipSync<G, CA, L>> + Send + Sync,
RGS: 'static + Deref<Target = RapidGossipSync<G, L>> + Send,
Expand All @@ -399,17 +398,17 @@ pub async fn process_events_async<
where
CA::Target: 'static + chain::Access,
CF::Target: 'static + chain::Filter,
CW::Target: 'static + chain::Watch<Signer>,
CW::Target: 'static + chain::Watch<<K::Target as KeysInterface>::Signer>,
T::Target: 'static + BroadcasterInterface,
K::Target: 'static + KeysInterface<Signer = Signer>,
K::Target: 'static + KeysInterface,
F::Target: 'static + FeeEstimator,
L::Target: 'static + Logger,
P::Target: 'static + Persist<Signer>,
P::Target: 'static + Persist<<K::Target as KeysInterface>::Signer>,
CMH::Target: 'static + ChannelMessageHandler,
OMH::Target: 'static + OnionMessageHandler,
RMH::Target: 'static + RoutingMessageHandler,
UMH::Target: 'static + CustomMessageHandler,
PS::Target: 'static + Persister<'a, Signer, CW, T, K, F, L, SC>,
PS::Target: 'static + Persister<'a, CW, T, K, F, L, SC>,
{
let mut should_continue = true;
define_run_body!(persister, event_handler, chain_monitor, channel_manager,
Expand Down Expand Up @@ -471,7 +470,6 @@ impl BackgroundProcessor {
/// [`NetworkGraph::write`]: lightning::routing::gossip::NetworkGraph#impl-Writeable
pub fn start<
'a,
Signer: 'static + Sign,
CA: 'static + Deref + Send + Sync,
CF: 'static + Deref + Send + Sync,
CW: 'static + Deref + Send + Sync,
Expand All @@ -487,7 +485,7 @@ impl BackgroundProcessor {
RMH: 'static + Deref + Send + Sync,
EH: 'static + EventHandler + Send,
PS: 'static + Deref + Send,
M: 'static + Deref<Target = ChainMonitor<Signer, CF, T, F, L, P>> + Send + Sync,
M: 'static + Deref<Target = ChainMonitor<<K::Target as KeysInterface>::Signer, CF, T, F, L, P>> + Send + Sync,
CM: 'static + Deref<Target = ChannelManager<CW, T, K, F, L>> + Send + Sync,
PGS: 'static + Deref<Target = P2PGossipSync<G, CA, L>> + Send + Sync,
RGS: 'static + Deref<Target = RapidGossipSync<G, L>> + Send,
Expand All @@ -502,17 +500,17 @@ impl BackgroundProcessor {
where
CA::Target: 'static + chain::Access,
CF::Target: 'static + chain::Filter,
CW::Target: 'static + chain::Watch<Signer>,
CW::Target: 'static + chain::Watch<<K::Target as KeysInterface>::Signer>,
T::Target: 'static + BroadcasterInterface,
K::Target: 'static + KeysInterface<Signer = Signer>,
K::Target: 'static + KeysInterface,
F::Target: 'static + FeeEstimator,
L::Target: 'static + Logger,
P::Target: 'static + Persist<Signer>,
P::Target: 'static + Persist<<K::Target as KeysInterface>::Signer>,
CMH::Target: 'static + ChannelMessageHandler,
OMH::Target: 'static + OnionMessageHandler,
RMH::Target: 'static + RoutingMessageHandler,
UMH::Target: 'static + CustomMessageHandler,
PS::Target: 'static + Persister<'a, Signer, CW, T, K, F, L, SC>,
PS::Target: 'static + Persister<'a, CW, T, K, F, L, SC>,
{
let stop_thread = Arc::new(AtomicBool::new(false));
let stop_thread_clone = stop_thread.clone();
Expand Down
11 changes: 5 additions & 6 deletions lightning-block-sync/src/init.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,16 +61,15 @@ BlockSourceResult<ValidatedBlockHeader> where B::Target: BlockSource {
///
/// async fn init_sync<
/// B: BlockSource,
/// K: KeysInterface<Signer = S>,
/// S: keysinterface::Sign,
/// K: KeysInterface,
/// T: BroadcasterInterface,
/// F: FeeEstimator,
/// L: Logger,
/// C: chain::Filter,
/// P: chainmonitor::Persist<S>,
/// P: chainmonitor::Persist<K::Signer>,
/// >(
/// block_source: &B,
/// chain_monitor: &ChainMonitor<S, &C, &T, &F, &L, &P>,
/// chain_monitor: &ChainMonitor<K::Signer, &C, &T, &F, &L, &P>,
/// config: UserConfig,
/// keys_manager: &K,
/// tx_broadcaster: &T,
Expand All @@ -80,7 +79,7 @@ BlockSourceResult<ValidatedBlockHeader> where B::Target: BlockSource {
/// ) {
/// // Read a serialized channel monitor paired with the block hash when it was persisted.
/// let serialized_monitor = "...";
/// let (monitor_block_hash, mut monitor) = <(BlockHash, ChannelMonitor<S>)>::read(
/// let (monitor_block_hash, mut monitor) = <(BlockHash, ChannelMonitor<K::Signer>)>::read(
/// &mut Cursor::new(&serialized_monitor), keys_manager).unwrap();
///
/// // Read the channel manager paired with the block hash when it was persisted.
Expand All @@ -95,7 +94,7 @@ BlockSourceResult<ValidatedBlockHeader> where B::Target: BlockSource {
/// config,
/// vec![&mut monitor],
/// );
/// <(BlockHash, ChannelManager<&ChainMonitor<S, &C, &T, &F, &L, &P>, &T, &K, &F, &L>)>::read(
/// <(BlockHash, ChannelManager<&ChainMonitor<K::Signer, &C, &T, &F, &L, &P>, &T, &K, &F, &L>)>::read(
/// &mut Cursor::new(&serialized_manager), read_args).unwrap()
/// };
///
Expand Down
21 changes: 10 additions & 11 deletions lightning-invoice/src/utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use bech32::ToBase32;
use bitcoin_hashes::{Hash, sha256};
use lightning::chain;
use lightning::chain::chaininterface::{BroadcasterInterface, FeeEstimator};
use lightning::chain::keysinterface::{Recipient, KeysInterface, Sign};
use lightning::chain::keysinterface::{Recipient, KeysInterface};
use lightning::ln::{PaymentHash, PaymentPreimage, PaymentSecret};
use lightning::ln::channelmanager::{ChannelDetails, ChannelManager, PaymentId, PaymentSendFailure, MIN_FINAL_CLTV_EXPIRY};
#[cfg(feature = "std")]
Expand Down Expand Up @@ -54,7 +54,7 @@ use crate::sync::Mutex;
/// [`ChannelManager::create_inbound_payment`]: lightning::ln::channelmanager::ChannelManager::create_inbound_payment
/// [`ChannelManager::create_inbound_payment_for_hash`]: lightning::ln::channelmanager::ChannelManager::create_inbound_payment_for_hash
/// [`PhantomRouteHints::channels`]: lightning::ln::channelmanager::PhantomRouteHints::channels
pub fn create_phantom_invoice<Signer: Sign, K: Deref, L: Deref>(
pub fn create_phantom_invoice<K: Deref, L: Deref>(
amt_msat: Option<u64>, payment_hash: Option<PaymentHash>, description: String,
invoice_expiry_delta_secs: u32, phantom_route_hints: Vec<PhantomRouteHints>, keys_manager: K,
logger: L, network: Currency,
Expand All @@ -65,7 +65,7 @@ where
{
let description = Description::new(description).map_err(SignOrCreationError::CreationError)?;
let description = InvoiceDescription::Direct(&description,);
_create_phantom_invoice::<Signer, K, L>(
_create_phantom_invoice::<K, L>(
amt_msat, payment_hash, description, invoice_expiry_delta_secs, phantom_route_hints,
keys_manager, logger, network,
)
Expand Down Expand Up @@ -103,7 +103,7 @@ where
/// [`ChannelManager::create_inbound_payment`]: lightning::ln::channelmanager::ChannelManager::create_inbound_payment
/// [`ChannelManager::create_inbound_payment_for_hash`]: lightning::ln::channelmanager::ChannelManager::create_inbound_payment_for_hash
/// [`PhantomRouteHints::channels`]: lightning::ln::channelmanager::PhantomRouteHints::channels
pub fn create_phantom_invoice_with_description_hash<Signer: Sign, K: Deref, L: Deref>(
pub fn create_phantom_invoice_with_description_hash<K: Deref, L: Deref>(
amt_msat: Option<u64>, payment_hash: Option<PaymentHash>, invoice_expiry_delta_secs: u32,
description_hash: Sha256, phantom_route_hints: Vec<PhantomRouteHints>, keys_manager: K,
logger: L, network: Currency
Expand All @@ -112,14 +112,14 @@ where
K::Target: KeysInterface,
L::Target: Logger,
{
_create_phantom_invoice::<Signer, K, L>(
_create_phantom_invoice::<K, L>(
amt_msat, payment_hash, InvoiceDescription::Hash(&description_hash),
invoice_expiry_delta_secs, phantom_route_hints, keys_manager, logger, network,
)
}

#[cfg(feature = "std")]
fn _create_phantom_invoice<Signer: Sign, K: Deref, L: Deref>(
fn _create_phantom_invoice<K: Deref, L: Deref>(
amt_msat: Option<u64>, payment_hash: Option<PaymentHash>, description: InvoiceDescription,
invoice_expiry_delta_secs: u32, phantom_route_hints: Vec<PhantomRouteHints>, keys_manager: K,
logger: L, network: Currency,
Expand Down Expand Up @@ -690,7 +690,6 @@ mod test {
use lightning::ln::functional_test_utils::*;
use lightning::ln::msgs::ChannelMessageHandler;
use lightning::routing::router::{PaymentParameters, RouteParameters, find_route};
use lightning::util::enforcing_trait_impls::EnforcingSigner;
use lightning::util::events::{MessageSendEvent, MessageSendEventsProvider, Event};
use lightning::util::test_utils;
use lightning::util::config::UserConfig;
Expand Down Expand Up @@ -1016,7 +1015,7 @@ mod test {
let non_default_invoice_expiry_secs = 4200;

let invoice =
crate::utils::create_phantom_invoice::<EnforcingSigner, &test_utils::TestKeysInterface, &test_utils::TestLogger>(
crate::utils::create_phantom_invoice::<&test_utils::TestKeysInterface, &test_utils::TestLogger>(
Some(payment_amt), payment_hash, "test".to_string(), non_default_invoice_expiry_secs,
route_hints, &nodes[1].keys_manager, &nodes[1].logger, Currency::BitcoinTestnet
).unwrap();
Expand Down Expand Up @@ -1125,7 +1124,7 @@ mod test {
nodes[2].node.get_phantom_route_hints(),
];

let invoice = crate::utils::create_phantom_invoice::<EnforcingSigner, &test_utils::TestKeysInterface, &test_utils::TestLogger>(Some(payment_amt), Some(payment_hash), "test".to_string(), 3600, route_hints, &nodes[1].keys_manager, &nodes[1].logger, Currency::BitcoinTestnet).unwrap();
let invoice = crate::utils::create_phantom_invoice::<&test_utils::TestKeysInterface, &test_utils::TestLogger>(Some(payment_amt), Some(payment_hash), "test".to_string(), 3600, route_hints, &nodes[1].keys_manager, &nodes[1].logger, Currency::BitcoinTestnet).unwrap();

let chan_0_1 = &nodes[1].node.list_usable_channels()[0];
assert_eq!(invoice.route_hints()[0].0[0].htlc_minimum_msat, chan_0_1.inbound_htlc_minimum_msat);
Expand Down Expand Up @@ -1153,7 +1152,7 @@ mod test {
let description_hash = crate::Sha256(Hash::hash("Description hash phantom invoice".as_bytes()));
let non_default_invoice_expiry_secs = 4200;
let invoice = crate::utils::create_phantom_invoice_with_description_hash::<
EnforcingSigner, &test_utils::TestKeysInterface, &test_utils::TestLogger,
&test_utils::TestKeysInterface, &test_utils::TestLogger,
>(
Some(payment_amt), None, non_default_invoice_expiry_secs, description_hash,
route_hints, &nodes[1].keys_manager, &nodes[1].logger, Currency::BitcoinTestnet
Expand Down Expand Up @@ -1470,7 +1469,7 @@ mod test {
.map(|route_hint| route_hint.phantom_scid)
.collect::<HashSet<u64>>();

let invoice = crate::utils::create_phantom_invoice::<EnforcingSigner, &test_utils::TestKeysInterface, &test_utils::TestLogger>(invoice_amt, None, "test".to_string(), 3600, phantom_route_hints, &invoice_node.keys_manager, &invoice_node.logger, Currency::BitcoinTestnet).unwrap();
let invoice = crate::utils::create_phantom_invoice::<&test_utils::TestKeysInterface, &test_utils::TestLogger>(invoice_amt, None, "test".to_string(), 3600, phantom_route_hints, &invoice_node.keys_manager, &invoice_node.logger, Currency::BitcoinTestnet).unwrap();

let invoice_hints = invoice.private_routes();

Expand Down
10 changes: 5 additions & 5 deletions lightning-persister/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ extern crate libc;
use bitcoin::hash_types::{BlockHash, Txid};
use bitcoin::hashes::hex::FromHex;
use lightning::chain::channelmonitor::ChannelMonitor;
use lightning::chain::keysinterface::{Sign, KeysInterface};
use lightning::chain::keysinterface::KeysInterface;
use lightning::util::ser::{ReadableArgs, Writeable};
use lightning::util::persist::KVStorePersister;
use std::fs;
Expand Down Expand Up @@ -59,10 +59,10 @@ impl FilesystemPersister {
}

/// Read `ChannelMonitor`s from disk.
pub fn read_channelmonitors<Signer: Sign, K: Deref> (
pub fn read_channelmonitors<K: Deref> (
&self, keys_manager: K
) -> Result<Vec<(BlockHash, ChannelMonitor<Signer>)>, std::io::Error>
where K::Target: KeysInterface<Signer=Signer> + Sized,
) -> Result<Vec<(BlockHash, ChannelMonitor<<K::Target as KeysInterface>::Signer>)>, std::io::Error>
where K::Target: KeysInterface + Sized,
{
let mut path = PathBuf::from(&self.path_to_channel_data);
path.push("monitors");
Expand Down Expand Up @@ -105,7 +105,7 @@ impl FilesystemPersister {

let contents = fs::read(&file.path())?;
let mut buffer = Cursor::new(&contents);
match <(BlockHash, ChannelMonitor<Signer>)>::read(&mut buffer, &*keys_manager) {
match <(BlockHash, ChannelMonitor<<K::Target as KeysInterface>::Signer>)>::read(&mut buffer, &*keys_manager) {
Ok((blockhash, channel_monitor)) => {
if channel_monitor.get_funding_txo().0.txid != txid.unwrap() || channel_monitor.get_funding_txo().0.index != index.unwrap() {
return Err(std::io::Error::new(std::io::ErrorKind::InvalidData, "ChannelMonitor was stored in the wrong file"));
Expand Down
6 changes: 3 additions & 3 deletions lightning/src/chain/channelmonitor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3551,8 +3551,8 @@ where

const MAX_ALLOC_SIZE: usize = 64*1024;

impl<'a, Signer: Sign, K: KeysInterface<Signer = Signer>> ReadableArgs<&'a K>
for (BlockHash, ChannelMonitor<Signer>) {
impl<'a, K: KeysInterface> ReadableArgs<&'a K>
for (BlockHash, ChannelMonitor<K::Signer>) {
fn read<R: io::Read>(reader: &mut R, keys_manager: &'a K) -> Result<Self, DecodeError> {
macro_rules! unwrap_obj {
($key: expr) => {
Expand Down Expand Up @@ -3736,7 +3736,7 @@ impl<'a, Signer: Sign, K: KeysInterface<Signer = Signer>> ReadableArgs<&'a K>
return Err(DecodeError::InvalidValue);
}
}
let onchain_tx_handler: OnchainTxHandler<Signer> = ReadableArgs::read(reader, keys_manager)?;
let onchain_tx_handler: OnchainTxHandler<K::Signer> = ReadableArgs::read(reader, keys_manager)?;

let lockdown_from_offchain = Readable::read(reader)?;
let holder_tx_signed = Readable::read(reader)?;
Expand Down
8 changes: 4 additions & 4 deletions lightning/src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4205,7 +4205,7 @@ impl<Signer: Sign> Channel<Signer> {
pub fn shutdown<K: Deref>(
&mut self, keys_provider: &K, their_features: &InitFeatures, msg: &msgs::Shutdown
) -> Result<(Option<msgs::Shutdown>, Option<ChannelMonitorUpdate>, Vec<(HTLCSource, PaymentHash)>), ChannelError>
where K::Target: KeysInterface<Signer = Signer>
where K::Target: KeysInterface
{
if self.channel_state & (ChannelState::PeerDisconnected as u32) == ChannelState::PeerDisconnected as u32 {
return Err(ChannelError::Close("Peer sent shutdown when we needed a channel_reestablish".to_owned()));
Expand Down Expand Up @@ -5825,7 +5825,7 @@ impl<Signer: Sign> Channel<Signer> {
/// holding cell HTLCs for payment failure.
pub fn get_shutdown<K: Deref>(&mut self, keys_provider: &K, their_features: &InitFeatures, target_feerate_sats_per_kw: Option<u32>)
-> Result<(msgs::Shutdown, Option<ChannelMonitorUpdate>, Vec<(HTLCSource, PaymentHash)>), APIError>
where K::Target: KeysInterface<Signer = Signer> {
where K::Target: KeysInterface {
for htlc in self.pending_outbound_htlcs.iter() {
if let OutboundHTLCState::LocalAnnounced(_) = htlc.state {
return Err(APIError::APIMisuseError{err: "Cannot begin shutdown with pending HTLCs. Process pending events first".to_owned()});
Expand Down Expand Up @@ -6284,8 +6284,8 @@ impl<Signer: Sign> Writeable for Channel<Signer> {
}

const MAX_ALLOC_SIZE: usize = 64*1024;
impl<'a, Signer: Sign, K: Deref> ReadableArgs<(&'a K, u32)> for Channel<Signer>
where K::Target: KeysInterface<Signer = Signer> {
impl<'a, K: Deref> ReadableArgs<(&'a K, u32)> for Channel<<K::Target as KeysInterface>::Signer>
where K::Target: KeysInterface {
fn read<R : io::Read>(reader: &mut R, args: (&'a K, u32)) -> Result<Self, DecodeError> {
let (keys_source, serialized_height) = args;
let ver = read_ver_prefix!(reader, SERIALIZATION_VERSION);
Expand Down
Loading