Skip to content

Refactor BestBlock to expose inner fields #2923

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 3 commits into from
Mar 7, 2024
Merged

Refactor BestBlock to expose inner fields #2923

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
57911dc
Refactor `BestBlock` to expose inner fields rather than accessors
tnull Mar 7, 2024
007a5db
Implement `Debug` for `BestBlock`
tnull Mar 7, 2024
edb22e1
Implement `Hash` for `BestBlock`
tnull Mar 7, 2024
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
4 changes: 2 additions & 2 deletions lightning-background-processor/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1342,8 +1342,8 @@ mod tests {

fn confirm_transaction_depth(node: &mut Node, tx: &Transaction, depth: u32) {
for i in 1..=depth {
let prev_blockhash = node.best_block.block_hash();
let height = node.best_block.height() + 1;
let prev_blockhash = node.best_block.block_hash;
let height = node.best_block.height + 1;
Comment on lines +1345 to +1346
Copy link

Choose a reason for hiding this comment

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

The code attempts to confirm a transaction by incrementing the block height and updating the best block. However, there's a potential issue with the way prev_blockhash is used to create a new block header. Specifically, the create_dummy_header function is not provided in the code snippet, making it unclear how the previous block hash is being used to generate a new block header. This could potentially lead to incorrect block chain simulation if not handled properly.

Consider ensuring that create_dummy_header correctly uses prev_blockhash to simulate a realistic blockchain environment. If create_dummy_header is part of external code not shown here, verify its implementation to ensure it aligns with blockchain principles.

The code attempts to confirm a transaction by incrementing the block height and updating the best block. However, there's a potential issue with the way prev_blockhash is used to create a new block header. Specifically, the create_dummy_header function is not provided in the code snippet, making it unclear how the previous block hash is being used to generate a new block header. This could potentially lead to incorrect block chain simulation if not handled properly.

Consider ensuring that create_dummy_header correctly uses prev_blockhash to simulate a realistic blockchain environment. If create_dummy_header is part of external code not shown here, verify its implementation to ensure it aligns with blockchain principles.

The code attempts to confirm a transaction by incrementing the block height and updating the best block. However, there's a potential issue with the way prev_blockhash is used to create a new block header. Specifically, the create_dummy_header function is not provided in the code snippet, making it unclear how the previous block hash is being used to generate a new block header. This could potentially lead to incorrect block chain simulation if not handled properly.

Consider ensuring that create_dummy_header correctly uses prev_blockhash to simulate a realistic blockchain environment. If create_dummy_header is part of external code not shown here, verify its implementation to ensure it aligns with blockchain principles.

The code attempts to confirm a transaction by incrementing the block height and updating the best block. However, there's a potential issue with the way prev_blockhash is used to create a new block header. Specifically, the create_dummy_header function is not provided in the code snippet, making it unclear how the previous block hash is being used to generate a new block header. This could potentially lead to incorrect block chain simulation if not handled properly.

Consider ensuring that create_dummy_header correctly uses prev_blockhash to simulate a realistic blockchain environment. If create_dummy_header is part of external code not shown here, verify its implementation to ensure it aligns with blockchain principles.

The code attempts to confirm a transaction by incrementing the block height and updating the best block. However, there's a potential issue with the way prev_blockhash is used to create a new block header. Specifically, the create_dummy_header function is not provided in the code snippet, making it unclear how the previous block hash is being used to generate a new block header. This could potentially lead to incorrect block chain simulation if not handled properly.

Consider ensuring that create_dummy_header correctly uses prev_blockhash to simulate a realistic blockchain environment. If create_dummy_header is part of external code not shown here, verify its implementation to ensure it aligns with blockchain principles.

let header = create_dummy_header(prev_blockhash, height);
let txdata = vec![(0, tx)];
node.best_block = BestBlock::new(header.block_hash(), height);
Expand Down
36 changes: 18 additions & 18 deletions lightning/src/chain/channelmonitor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,7 +387,7 @@ impl OnchainEventEntry {
}

fn has_reached_confirmation_threshold(&self, best_block: &BestBlock) -> bool {
best_block.height() >= self.confirmation_threshold()
best_block.height >= self.confirmation_threshold()
}
}

Expand Down Expand Up @@ -1077,8 +1077,8 @@ impl<Signer: WriteableEcdsaChannelSigner> Writeable for ChannelMonitorImpl<Signe
event.write(writer)?;
}

self.best_block.block_hash().write(writer)?;
writer.write_all(&self.best_block.height().to_be_bytes())?;
self.best_block.block_hash.write(writer)?;
writer.write_all(&self.best_block.height.to_be_bytes())?;

writer.write_all(&(self.onchain_events_awaiting_threshold_conf.len() as u64).to_be_bytes())?;
for ref entry in self.onchain_events_awaiting_threshold_conf.iter() {
Expand Down Expand Up @@ -2273,7 +2273,7 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitor<Signer> {
// before considering it "no longer pending" - this matches when we
// provide the ChannelManager an HTLC failure event.
Some(commitment_tx_output_idx) == htlc.transaction_output_index &&
us.best_block.height() >= event.height + ANTI_REORG_DELAY - 1
us.best_block.height >= event.height + ANTI_REORG_DELAY - 1
} else if let OnchainEvent::HTLCSpendConfirmation { commitment_tx_output_idx, .. } = event.event {
// If the HTLC was fulfilled with a preimage, we consider the HTLC
// immediately non-pending, matching when we provide ChannelManager
Expand Down Expand Up @@ -2674,7 +2674,7 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
macro_rules! claim_htlcs {
($commitment_number: expr, $txid: expr) => {
let (htlc_claim_reqs, _) = self.get_counterparty_output_claim_info($commitment_number, $txid, None);
self.onchain_tx_handler.update_claims_view_from_requests(htlc_claim_reqs, self.best_block.height(), self.best_block.height(), broadcaster, fee_estimator, logger);
self.onchain_tx_handler.update_claims_view_from_requests(htlc_claim_reqs, self.best_block.height, self.best_block.height, broadcaster, fee_estimator, logger);
}
}
if let Some(txid) = self.current_counterparty_commitment_txid {
Expand Down Expand Up @@ -2721,8 +2721,8 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
// Assume that the broadcasted commitment transaction confirmed in the current best
// block. Even if not, its a reasonable metric for the bump criteria on the HTLC
// transactions.
let (claim_reqs, _) = self.get_broadcasted_holder_claims(&holder_commitment_tx, self.best_block.height());
self.onchain_tx_handler.update_claims_view_from_requests(claim_reqs, self.best_block.height(), self.best_block.height(), broadcaster, fee_estimator, logger);
let (claim_reqs, _) = self.get_broadcasted_holder_claims(&holder_commitment_tx, self.best_block.height);
self.onchain_tx_handler.update_claims_view_from_requests(claim_reqs, self.best_block.height, self.best_block.height, broadcaster, fee_estimator, logger);
}
}
}
Expand All @@ -2736,7 +2736,7 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
let commitment_package = PackageTemplate::build_package(
self.funding_info.0.txid.clone(), self.funding_info.0.index as u32,
PackageSolvingData::HolderFundingOutput(funding_outp),
self.best_block.height(), self.best_block.height()
self.best_block.height, self.best_block.height
);
let mut claimable_outpoints = vec![commitment_package];
self.pending_monitor_events.push(MonitorEvent::HolderForceClosed(self.funding_info.0));
Expand All @@ -2753,7 +2753,7 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
// assuming it gets confirmed in the next block. Sadly, we have code which considers
// "not yet confirmed" things as discardable, so we cannot do that here.
let (mut new_outpoints, _) = self.get_broadcasted_holder_claims(
&self.current_holder_commitment_tx, self.best_block.height()
&self.current_holder_commitment_tx, self.best_block.height
);
let unsigned_commitment_tx = self.onchain_tx_handler.get_unsigned_holder_commitment_tx();
let new_outputs = self.get_broadcasted_holder_watch_outputs(
Expand All @@ -2777,7 +2777,7 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
{
let (claimable_outpoints, _) = self.generate_claimable_outpoints_and_watch_outputs();
self.onchain_tx_handler.update_claims_view_from_requests(
claimable_outpoints, self.best_block.height(), self.best_block.height(), broadcaster,
claimable_outpoints, self.best_block.height, self.best_block.height, broadcaster,
fee_estimator, logger
);
}
Expand Down Expand Up @@ -3593,11 +3593,11 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
{
let block_hash = header.block_hash();

if height > self.best_block.height() {
if height > self.best_block.height {
self.best_block = BestBlock::new(block_hash, height);
log_trace!(logger, "Connecting new block {} at height {}", block_hash, height);
self.block_confirmed(height, block_hash, vec![], vec![], vec![], &broadcaster, &fee_estimator, logger)
} else if block_hash != self.best_block.block_hash() {
} else if block_hash != self.best_block.block_hash {
self.best_block = BestBlock::new(block_hash, height);
log_trace!(logger, "Best block re-orged, replaced with new block {} at height {}", block_hash, height);
self.onchain_events_awaiting_threshold_conf.retain(|ref entry| entry.height <= height);
Expand Down Expand Up @@ -3742,7 +3742,7 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
}
}

if height > self.best_block.height() {
if height > self.best_block.height {
self.best_block = BestBlock::new(block_hash, height);
}

Expand Down Expand Up @@ -3774,7 +3774,7 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
L::Target: Logger,
{
log_trace!(logger, "Processing {} matched transactions for block at height {}.", txn_matched.len(), conf_height);
debug_assert!(self.best_block.height() >= conf_height);
debug_assert!(self.best_block.height >= conf_height);

let should_broadcast = self.should_broadcast_holder_commitment_txn(logger);
if should_broadcast {
Expand Down Expand Up @@ -3865,8 +3865,8 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
}
}

self.onchain_tx_handler.update_claims_view_from_requests(claimable_outpoints, conf_height, self.best_block.height(), broadcaster, fee_estimator, logger);
self.onchain_tx_handler.update_claims_view_from_matched_txn(&txn_matched, conf_height, conf_hash, self.best_block.height(), broadcaster, fee_estimator, logger);
self.onchain_tx_handler.update_claims_view_from_requests(claimable_outpoints, conf_height, self.best_block.height, broadcaster, fee_estimator, logger);
self.onchain_tx_handler.update_claims_view_from_matched_txn(&txn_matched, conf_height, conf_hash, self.best_block.height, broadcaster, fee_estimator, logger);

// Determine new outputs to watch by comparing against previously known outputs to watch,
// updating the latter in the process.
Expand Down Expand Up @@ -4017,7 +4017,7 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
// to the source, and if we don't fail the channel we will have to ensure that the next
// updates that peer sends us are update_fails, failing the channel if not. It's probably
// easier to just fail the channel as this case should be rare enough anyway.
let height = self.best_block.height();
let height = self.best_block.height;
macro_rules! scan_commitment {
($htlcs: expr, $holder_tx: expr) => {
for ref htlc in $htlcs {
Expand Down Expand Up @@ -4616,7 +4616,7 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP
chan_utils::get_to_countersignatory_with_anchors_redeemscript(&payment_point).to_v0_p2wsh();
}

Ok((best_block.block_hash(), ChannelMonitor::from_impl(ChannelMonitorImpl {
Ok((best_block.block_hash, ChannelMonitor::from_impl(ChannelMonitorImpl {
latest_update_id,
commitment_transaction_number_obscure_factor,

Expand Down
14 changes: 5 additions & 9 deletions lightning/src/chain/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,12 @@ pub(crate) mod onchaintx;
pub(crate) mod package;

/// The best known block as identified by its hash and height.
#[derive(Clone, Copy, PartialEq, Eq)]
#[derive(Clone, Copy, Debug, Hash, PartialEq, Eq)]
pub struct BestBlock {
block_hash: BlockHash,
height: u32,
/// The block's hash
pub block_hash: BlockHash,
/// The height at which the block was confirmed.
pub height: u32,
}

impl BestBlock {
Expand All @@ -51,12 +53,6 @@ impl BestBlock {
pub fn new(block_hash: BlockHash, height: u32) -> Self {
BestBlock { block_hash, height }
}

/// Returns the best block hash.
pub fn block_hash(&self) -> BlockHash { self.block_hash }

/// Returns the best block height.
pub fn height(&self) -> u32 { self.height }
}


Expand Down
4 changes: 2 additions & 2 deletions lightning/src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4086,7 +4086,7 @@ impl<SP: Deref> Channel<SP> where

log_info!(logger, "Received channel_ready from peer for channel {}", &self.context.channel_id());

Ok(self.get_announcement_sigs(node_signer, chain_hash, user_config, best_block.height(), logger))
Ok(self.get_announcement_sigs(node_signer, chain_hash, user_config, best_block.height, logger))
}

pub fn update_add_htlc<F, FE: Deref, L: Deref>(
Expand Down Expand Up @@ -5475,7 +5475,7 @@ impl<SP: Deref> Channel<SP> where

let shutdown_msg = self.get_outbound_shutdown();

let announcement_sigs = self.get_announcement_sigs(node_signer, chain_hash, user_config, best_block.height(), logger);
let announcement_sigs = self.get_announcement_sigs(node_signer, chain_hash, user_config, best_block.height, logger);

if matches!(self.context.channel_state, ChannelState::AwaitingChannelReady(_)) {
// If we're waiting on a monitor update, we shouldn't re-send any channel_ready's.
Expand Down
Loading