Skip to content

Commit

Permalink
Make all internal signatures accept LowerBoundedFeeEstimator
Browse files Browse the repository at this point in the history
  • Loading branch information
dunxen committed Jun 29, 2022
1 parent fea9c4e commit 1956024
Show file tree
Hide file tree
Showing 9 changed files with 104 additions and 85 deletions.
4 changes: 2 additions & 2 deletions fuzz/src/chanmon_consistency.rs
Expand Up @@ -33,7 +33,7 @@ use lightning::chain;
use lightning::chain::{BestBlock, ChannelMonitorUpdateErr, chainmonitor, channelmonitor, Confirm, Watch};
use lightning::chain::channelmonitor::{ChannelMonitor, MonitorEvent};
use lightning::chain::transaction::OutPoint;
use lightning::chain::chaininterface::{BroadcasterInterface, ConfirmationTarget, FeeEstimator};
use lightning::chain::chaininterface::{BroadcasterInterface, ConfirmationTarget, FeeEstimator, LowerBoundedFeeEstimator};
use lightning::chain::keysinterface::{KeyMaterial, KeysInterface, InMemorySigner, Recipient};
use lightning::ln::{PaymentHash, PaymentPreimage, PaymentSecret};
use lightning::ln::channelmanager::{ChainParameters, ChannelManager, PaymentSendFailure, ChannelManagerReadArgs};
Expand Down Expand Up @@ -140,7 +140,7 @@ impl chain::Watch<EnforcingSigner> for TestChainMonitor {
};
let deserialized_monitor = <(BlockHash, channelmonitor::ChannelMonitor<EnforcingSigner>)>::
read(&mut Cursor::new(&map_entry.get().1), &*self.keys).unwrap().1;
deserialized_monitor.update_monitor(&update, &&TestBroadcaster{}, &&FuzzEstimator { ret_val: atomic::AtomicU32::new(253) }, &self.logger).unwrap();
deserialized_monitor.update_monitor(&update, &&TestBroadcaster{}, &LowerBoundedFeeEstimator::new(&FuzzEstimator { ret_val: atomic::AtomicU32::new(253) }), &self.logger).unwrap();
let mut ser = VecWriter(Vec::new());
deserialized_monitor.write(&mut ser).unwrap();
map_entry.insert((update.update_id, ser.0));
Expand Down
2 changes: 1 addition & 1 deletion lightning-block-sync/src/init.rs
Expand Up @@ -45,7 +45,7 @@ BlockSourceResult<ValidatedBlockHeader> where B::Target: BlockSource {
/// use lightning::chain::chainmonitor;
/// use lightning::chain::chainmonitor::ChainMonitor;
/// use lightning::chain::channelmonitor::ChannelMonitor;
/// use lightning::chain::chaininterface::BroadcasterInterface;
/// use lightning::chain::chaininterface::{BroadcasterInterface, LowerBoundedFeeEstimator};
/// use lightning::chain::chaininterface::FeeEstimator;
/// use lightning::chain::keysinterface;
/// use lightning::chain::keysinterface::KeysInterface;
Expand Down
16 changes: 8 additions & 8 deletions lightning/src/chain/chainmonitor.rs
Expand Up @@ -28,7 +28,7 @@ use bitcoin::hash_types::Txid;

use chain;
use chain::{ChannelMonitorUpdateErr, Filter, WatchedOutput};
use chain::chaininterface::{BroadcasterInterface, FeeEstimator};
use chain::chaininterface::{BroadcasterInterface, FeeEstimator, LowerBoundedFeeEstimator};
use chain::channelmonitor::{ChannelMonitor, ChannelMonitorUpdate, Balance, MonitorEvent, TransactionOutputs, LATENCY_GRACE_PERIOD_BLOCKS};
use chain::transaction::{OutPoint, TransactionData};
use chain::keysinterface::Sign;
Expand Down Expand Up @@ -231,7 +231,7 @@ pub struct ChainMonitor<ChannelSigner: Sign, C: Deref, T: Deref, F: Deref, L: De
chain_source: Option<C>,
broadcaster: T,
logger: L,
fee_estimator: F,
fee_estimator: LowerBoundedFeeEstimator<F>,
persister: P,
/// "User-provided" (ie persistence-completion/-failed) [`MonitorEvent`]s. These came directly
/// from the user and not from a [`ChannelMonitor`].
Expand Down Expand Up @@ -354,7 +354,7 @@ where C::Target: chain::Filter,
chain_source,
broadcaster,
logger,
fee_estimator: feeest,
fee_estimator: LowerBoundedFeeEstimator::new(feeest),
persister,
pending_monitor_events: Mutex::new(Vec::new()),
highest_chain_height: AtomicUsize::new(0),
Expand Down Expand Up @@ -505,7 +505,7 @@ where
log_debug!(self.logger, "New best block {} at height {} provided via block_connected", header.block_hash(), height);
self.process_chain_data(header, Some(height), &txdata, |monitor, txdata| {
monitor.block_connected(
header, txdata, height, &*self.broadcaster, &*self.fee_estimator, &*self.logger)
header, txdata, height, &*self.broadcaster, &self.fee_estimator, &*self.logger)
});
}

Expand All @@ -514,7 +514,7 @@ where
log_debug!(self.logger, "Latest block {} at height {} removed via block_disconnected", header.block_hash(), height);
for monitor_state in monitor_states.values() {
monitor_state.monitor.block_disconnected(
header, height, &*self.broadcaster, &*self.fee_estimator, &*self.logger);
header, height, &*self.broadcaster, &self.fee_estimator, &*self.logger);
}
}
}
Expand All @@ -532,15 +532,15 @@ where
log_debug!(self.logger, "{} provided transactions confirmed at height {} in block {}", txdata.len(), height, header.block_hash());
self.process_chain_data(header, None, txdata, |monitor, txdata| {
monitor.transactions_confirmed(
header, txdata, height, &*self.broadcaster, &*self.fee_estimator, &*self.logger)
header, txdata, height, &*self.broadcaster, &self.fee_estimator, &*self.logger)
});
}

fn transaction_unconfirmed(&self, txid: &Txid) {
log_debug!(self.logger, "Transaction {} reorganized out of chain", txid);
let monitor_states = self.monitors.read().unwrap();
for monitor_state in monitor_states.values() {
monitor_state.monitor.transaction_unconfirmed(txid, &*self.broadcaster, &*self.fee_estimator, &*self.logger);
monitor_state.monitor.transaction_unconfirmed(txid, &*self.broadcaster, &self.fee_estimator, &*self.logger);
}
}

Expand All @@ -551,7 +551,7 @@ where
// it's still possible if a chain::Filter implementation returns a transaction.
debug_assert!(txdata.is_empty());
monitor.best_block_updated(
header, height, &*self.broadcaster, &*self.fee_estimator, &*self.logger)
header, height, &*self.broadcaster, &self.fee_estimator, &*self.logger)
});
}

Expand Down
47 changes: 29 additions & 18 deletions lightning/src/chain/channelmonitor.rs
Expand Up @@ -40,7 +40,7 @@ use ln::chan_utils::{CounterpartyCommitmentSecrets, HTLCOutputInCommitment, HTLC
use ln::channelmanager::HTLCSource;
use chain;
use chain::{BestBlock, WatchedOutput};
use chain::chaininterface::{BroadcasterInterface, FeeEstimator};
use chain::chaininterface::{BroadcasterInterface, FeeEstimator, LowerBoundedFeeEstimator};
use chain::transaction::{OutPoint, TransactionData};
use chain::keysinterface::{SpendableOutputDescriptor, StaticPaymentOutputDescriptor, DelayedPaymentOutputDescriptor, Sign, KeysInterface};
use chain::onchaintx::OnchainTxHandler;
Expand Down Expand Up @@ -1099,7 +1099,7 @@ impl<Signer: Sign> ChannelMonitor<Signer> {
payment_hash: &PaymentHash,
payment_preimage: &PaymentPreimage,
broadcaster: &B,
fee_estimator: &F,
fee_estimator: &LowerBoundedFeeEstimator<F>,
logger: &L,
) where
B::Target: BroadcasterInterface,
Expand Down Expand Up @@ -1129,7 +1129,7 @@ impl<Signer: Sign> ChannelMonitor<Signer> {
&self,
updates: &ChannelMonitorUpdate,
broadcaster: &B,
fee_estimator: &F,
fee_estimator: &LowerBoundedFeeEstimator<F>,
logger: &L,
) -> Result<(), ()>
where
Expand Down Expand Up @@ -1295,8 +1295,9 @@ impl<Signer: Sign> ChannelMonitor<Signer> {
F::Target: FeeEstimator,
L::Target: Logger,
{
let bounded_fee_estimator = LowerBoundedFeeEstimator::new(fee_estimator);
self.inner.lock().unwrap().transactions_confirmed(
header, txdata, height, broadcaster, fee_estimator, logger)
header, txdata, height, broadcaster, &bounded_fee_estimator, logger)
}

/// Processes a transaction that was reorganized out of the chain.
Expand All @@ -1316,8 +1317,9 @@ impl<Signer: Sign> ChannelMonitor<Signer> {
F::Target: FeeEstimator,
L::Target: Logger,
{
let bounded_fee_estimator = LowerBoundedFeeEstimator::new(fee_estimator);
self.inner.lock().unwrap().transaction_unconfirmed(
txid, broadcaster, fee_estimator, logger);
txid, broadcaster, &bounded_fee_estimator, logger);
}

/// Updates the monitor with the current best chain tip, returning new outputs to watch. See
Expand All @@ -1340,8 +1342,9 @@ impl<Signer: Sign> ChannelMonitor<Signer> {
F::Target: FeeEstimator,
L::Target: Logger,
{
let bounded_fee_estimator = LowerBoundedFeeEstimator::new(fee_estimator);
self.inner.lock().unwrap().best_block_updated(
header, height, broadcaster, fee_estimator, logger)
header, height, broadcaster, &bounded_fee_estimator, logger)
}

/// Returns the set of txids that should be monitored for re-organization out of the chain.
Expand Down Expand Up @@ -1857,7 +1860,9 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {

/// Provides a payment_hash->payment_preimage mapping. Will be automatically pruned when all
/// commitment_tx_infos which contain the payment hash have been revoked.
fn provide_payment_preimage<B: Deref, F: Deref, L: Deref>(&mut self, payment_hash: &PaymentHash, payment_preimage: &PaymentPreimage, broadcaster: &B, fee_estimator: &F, logger: &L)
fn provide_payment_preimage<B: Deref, F: Deref, L: Deref>(
&mut self, payment_hash: &PaymentHash, payment_preimage: &PaymentPreimage, broadcaster: &B,
fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L)
where B::Target: BroadcasterInterface,
F::Target: FeeEstimator,
L::Target: Logger,
Expand Down Expand Up @@ -1914,7 +1919,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
self.pending_monitor_events.push(MonitorEvent::CommitmentTxConfirmed(self.funding_info.0));
}

pub fn update_monitor<B: Deref, F: Deref, L: Deref>(&mut self, updates: &ChannelMonitorUpdate, broadcaster: &B, fee_estimator: &F, logger: &L) -> Result<(), ()>
pub(crate) fn update_monitor<B: Deref, F: Deref, L: Deref>(&mut self, updates: &ChannelMonitorUpdate, broadcaster: &B, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L) -> Result<(), ()>
where B::Target: BroadcasterInterface,
F::Target: FeeEstimator,
L::Target: Logger,
Expand Down Expand Up @@ -1955,7 +1960,8 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
},
ChannelMonitorUpdateStep::PaymentPreimage { payment_preimage } => {
log_trace!(logger, "Updating ChannelMonitor with payment preimage");
self.provide_payment_preimage(&PaymentHash(Sha256::hash(&payment_preimage.0[..]).into_inner()), &payment_preimage, broadcaster, fee_estimator, logger)
let bounded_fee_estimator = LowerBoundedFeeEstimator::new(fee_estimator);
self.provide_payment_preimage(&PaymentHash(Sha256::hash(&payment_preimage.0[..]).into_inner()), &payment_preimage, broadcaster, &bounded_fee_estimator, logger)
},
ChannelMonitorUpdateStep::CommitmentSecret { idx, secret } => {
log_trace!(logger, "Updating ChannelMonitor with commitment secret");
Expand Down Expand Up @@ -2381,15 +2387,16 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
let block_hash = header.block_hash();
self.best_block = BestBlock::new(block_hash, height);

self.transactions_confirmed(header, txdata, height, broadcaster, fee_estimator, logger)
let bounded_fee_estimator = LowerBoundedFeeEstimator::new(fee_estimator);
self.transactions_confirmed(header, txdata, height, broadcaster, &bounded_fee_estimator, logger)
}

fn best_block_updated<B: Deref, F: Deref, L: Deref>(
&mut self,
header: &BlockHeader,
height: u32,
broadcaster: B,
fee_estimator: F,
fee_estimator: &LowerBoundedFeeEstimator<F>,
logger: L,
) -> Vec<TransactionOutputs>
where
Expand All @@ -2416,7 +2423,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
txdata: &TransactionData,
height: u32,
broadcaster: B,
fee_estimator: F,
fee_estimator: &LowerBoundedFeeEstimator<F>,
logger: L,
) -> Vec<TransactionOutputs>
where
Expand Down Expand Up @@ -2513,7 +2520,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
mut watch_outputs: Vec<TransactionOutputs>,
mut claimable_outpoints: Vec<PackageTemplate>,
broadcaster: &B,
fee_estimator: &F,
fee_estimator: &LowerBoundedFeeEstimator<F>,
logger: &L,
) -> Vec<TransactionOutputs>
where
Expand Down Expand Up @@ -2651,7 +2658,8 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
//- maturing spendable output has transaction paying us has been disconnected
self.onchain_events_awaiting_threshold_conf.retain(|ref entry| entry.height < height);

self.onchain_tx_handler.block_disconnected(height, broadcaster, fee_estimator, logger);
let bounded_fee_estimator = LowerBoundedFeeEstimator::new(fee_estimator);
self.onchain_tx_handler.block_disconnected(height, broadcaster, &bounded_fee_estimator, logger);

self.best_block = BestBlock::new(header.prev_blockhash, height - 1);
}
Expand All @@ -2660,7 +2668,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
&mut self,
txid: &Txid,
broadcaster: B,
fee_estimator: F,
fee_estimator: &LowerBoundedFeeEstimator<F>,
logger: L,
) where
B::Target: BroadcasterInterface,
Expand Down Expand Up @@ -3390,6 +3398,8 @@ mod tests {

use hex;

use crate::chain::chaininterface::LowerBoundedFeeEstimator;

use super::ChannelMonitorUpdateStep;
use ::{check_added_monitors, check_closed_broadcast, check_closed_event, check_spends, get_local_commitment_txn, get_monitor, get_route_and_payment_hash, unwrap_send_err};
use chain::{BestBlock, Confirm};
Expand Down Expand Up @@ -3486,7 +3496,7 @@ mod tests {

let broadcaster = TestBroadcaster::new(Arc::clone(&nodes[1].blocks));
assert!(
pre_update_monitor.update_monitor(&replay_update, &&broadcaster, &&chanmon_cfgs[1].fee_estimator, &nodes[1].logger)
pre_update_monitor.update_monitor(&replay_update, &&broadcaster, &LowerBoundedFeeEstimator::new(&chanmon_cfgs[1].fee_estimator), &nodes[1].logger)
.is_err());
// Even though we error'd on the first update, we should still have generated an HTLC claim
// transaction
Expand All @@ -3511,7 +3521,7 @@ mod tests {
let secp_ctx = Secp256k1::new();
let logger = Arc::new(TestLogger::new());
let broadcaster = Arc::new(TestBroadcaster{txn_broadcasted: Mutex::new(Vec::new()), blocks: Arc::new(Mutex::new(Vec::new()))});
let fee_estimator = Arc::new(TestFeeEstimator { sat_per_kw: Mutex::new(253) });
let fee_estimator = TestFeeEstimator { sat_per_kw: Mutex::new(253) };

let dummy_key = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap());
let dummy_tx = Transaction { version: 0, lock_time: 0, input: Vec::new(), output: Vec::new() };
Expand Down Expand Up @@ -3610,7 +3620,8 @@ mod tests {
monitor.provide_latest_counterparty_commitment_tx(dummy_txid, preimages_slice_to_htlc_outputs!(preimages[17..20]), 281474976710653, dummy_key, &logger);
monitor.provide_latest_counterparty_commitment_tx(dummy_txid, preimages_slice_to_htlc_outputs!(preimages[18..20]), 281474976710652, dummy_key, &logger);
for &(ref preimage, ref hash) in preimages.iter() {
monitor.provide_payment_preimage(hash, preimage, &broadcaster, &fee_estimator, &logger);
let bounded_fee_estimator = LowerBoundedFeeEstimator::new(&fee_estimator);
monitor.provide_payment_preimage(hash, preimage, &broadcaster, &bounded_fee_estimator, &logger);
}

// Now provide a secret, pruning preimages 10-15
Expand Down
12 changes: 6 additions & 6 deletions lightning/src/chain/onchaintx.rs
Expand Up @@ -24,7 +24,7 @@ use bitcoin::secp256k1;
use ln::msgs::DecodeError;
use ln::PaymentPreimage;
use ln::chan_utils::{ChannelTransactionParameters, HolderCommitmentTransaction};
use chain::chaininterface::{FeeEstimator, BroadcasterInterface};
use chain::chaininterface::{FeeEstimator, BroadcasterInterface, LowerBoundedFeeEstimator};
use chain::channelmonitor::{ANTI_REORG_DELAY, CLTV_SHARED_CLAIM_BUFFER};
use chain::keysinterface::{Sign, KeysInterface};
use chain::package::PackageTemplate;
Expand Down Expand Up @@ -377,7 +377,7 @@ impl<ChannelSigner: Sign> OnchainTxHandler<ChannelSigner> {
/// (CSV or CLTV following cases). In case of high-fee spikes, claim tx may stuck in the mempool, so you need to bump its feerate quickly using Replace-By-Fee or Child-Pay-For-Parent.
/// Panics if there are signing errors, because signing operations in reaction to on-chain events
/// are not expected to fail, and if they do, we may lose funds.
fn generate_claim_tx<F: Deref, L: Deref>(&mut self, cur_height: u32, cached_request: &PackageTemplate, fee_estimator: &F, logger: &L) -> Option<(Option<u32>, u64, Transaction)>
fn generate_claim_tx<F: Deref, L: Deref>(&mut self, cur_height: u32, cached_request: &PackageTemplate, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L) -> Option<(Option<u32>, u64, Transaction)>
where F::Target: FeeEstimator,
L::Target: Logger,
{
Expand Down Expand Up @@ -415,7 +415,7 @@ impl<ChannelSigner: Sign> OnchainTxHandler<ChannelSigner> {
/// `conf_height` represents the height at which the transactions in `txn_matched` were
/// confirmed. This does not need to equal the current blockchain tip height, which should be
/// provided via `cur_height`, however it must never be higher than `cur_height`.
pub(crate) fn update_claims_view<B: Deref, F: Deref, L: Deref>(&mut self, txn_matched: &[&Transaction], requests: Vec<PackageTemplate>, conf_height: u32, cur_height: u32, broadcaster: &B, fee_estimator: &F, logger: &L)
pub(crate) fn update_claims_view<B: Deref, F: Deref, L: Deref>(&mut self, txn_matched: &[&Transaction], requests: Vec<PackageTemplate>, conf_height: u32, cur_height: u32, broadcaster: &B, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L)
where B::Target: BroadcasterInterface,
F::Target: FeeEstimator,
L::Target: Logger,
Expand Down Expand Up @@ -617,7 +617,7 @@ impl<ChannelSigner: Sign> OnchainTxHandler<ChannelSigner> {
&mut self,
txid: &Txid,
broadcaster: B,
fee_estimator: F,
fee_estimator: &LowerBoundedFeeEstimator<F>,
logger: L,
) where
B::Target: BroadcasterInterface,
Expand All @@ -637,7 +637,7 @@ impl<ChannelSigner: Sign> OnchainTxHandler<ChannelSigner> {
}
}

pub(crate) fn block_disconnected<B: Deref, F: Deref, L: Deref>(&mut self, height: u32, broadcaster: B, fee_estimator: F, logger: L)
pub(crate) fn block_disconnected<B: Deref, F: Deref, L: Deref>(&mut self, height: u32, broadcaster: B, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: L)
where B::Target: BroadcasterInterface,
F::Target: FeeEstimator,
L::Target: Logger,
Expand Down Expand Up @@ -667,7 +667,7 @@ impl<ChannelSigner: Sign> OnchainTxHandler<ChannelSigner> {
}
}
for (_, request) in bump_candidates.iter_mut() {
if let Some((new_timer, new_feerate, bump_tx)) = self.generate_claim_tx(height, &request, &&*fee_estimator, &&*logger) {
if let Some((new_timer, new_feerate, bump_tx)) = self.generate_claim_tx(height, &request, &fee_estimator, &&*logger) {
request.set_timer(new_timer);
request.set_feerate(new_feerate);
log_info!(logger, "Broadcasting onchain {}", log_tx!(bump_tx));
Expand Down
8 changes: 5 additions & 3 deletions lightning/src/chain/package.rs
Expand Up @@ -38,6 +38,8 @@ use core::mem;
use core::ops::Deref;
use bitcoin::Witness;

use super::chaininterface::LowerBoundedFeeEstimator;

const MAX_ALLOC_SIZE: usize = 64*1024;


Expand Down Expand Up @@ -665,7 +667,7 @@ impl PackageTemplate {
/// Returns value in satoshis to be included as package outgoing output amount and feerate
/// which was used to generate the value. Will not return less than `dust_limit_sats` for the
/// value.
pub(crate) fn compute_package_output<F: Deref, L: Deref>(&self, predicted_weight: usize, dust_limit_sats: u64, fee_estimator: &F, logger: &L) -> Option<(u64, u64)>
pub(crate) fn compute_package_output<F: Deref, L: Deref>(&self, predicted_weight: usize, dust_limit_sats: u64, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L) -> Option<(u64, u64)>
where F::Target: FeeEstimator,
L::Target: Logger,
{
Expand Down Expand Up @@ -772,7 +774,7 @@ impl Readable for PackageTemplate {
/// If the proposed fee is less than the available spent output's values, we return the proposed
/// fee and the corresponding updated feerate. If the proposed fee is equal or more than the
/// available spent output's values, we return nothing
fn compute_fee_from_spent_amounts<F: Deref, L: Deref>(input_amounts: u64, predicted_weight: usize, fee_estimator: &F, logger: &L) -> Option<(u64, u64)>
fn compute_fee_from_spent_amounts<F: Deref, L: Deref>(input_amounts: u64, predicted_weight: usize, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L) -> Option<(u64, u64)>
where F::Target: FeeEstimator,
L::Target: Logger,
{
Expand Down Expand Up @@ -808,7 +810,7 @@ fn compute_fee_from_spent_amounts<F: Deref, L: Deref>(input_amounts: u64, predic
/// attempt, use them. Otherwise, blindly bump the feerate by 25% of the previous feerate. We also
/// verify that those bumping heuristics respect BIP125 rules 3) and 4) and if required adjust
/// the new fee to meet the RBF policy requirement.
fn feerate_bump<F: Deref, L: Deref>(predicted_weight: usize, input_amounts: u64, previous_feerate: u64, fee_estimator: &F, logger: &L) -> Option<(u64, u64)>
fn feerate_bump<F: Deref, L: Deref>(predicted_weight: usize, input_amounts: u64, previous_feerate: u64, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L) -> Option<(u64, u64)>
where F::Target: FeeEstimator,
L::Target: Logger,
{
Expand Down

0 comments on commit 1956024

Please sign in to comment.