Skip to content
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

Vec Broadcaster #3448

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

yellowred
Copy link
Contributor

Implementation of BroacasterInterface that does not broadcast transactions immediately, but stores them internally to broadcast later with a call to release_transactions.

/// is ready.
pub struct VecBroadcaster {
channel_id: ChannelId,
transactions: Mutex<Vec<Transaction>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

As we'll still want to broadcast the individual packages, this might need to be a Vec<Vec<Transaction>>

Copy link
Contributor Author

@yellowred yellowred Dec 24, 2024

Choose a reason for hiding this comment

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

Sorry, trying to understand nomenclature, Package == Vec? If so, why do we need this? I.e. release_transactions broadcasts the whole vec of transactions, why some subsets of it need to be broadcasted separately?

}

/// Used to actually broadcast stored transactions to the network.
#[instrument(skip_all, fields(channel = %self.channel_id))]
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure we require the instrument statment here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh my bad, removed it and replaced tracing logging with ldk logger


impl BroadcasterInterface for VecBroadcaster {
fn broadcast_transactions(&self, txs: &[&Transaction]) {
let mut tx_storage = self.transactions.lock();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let mut tx_storage = self.transactions.lock();
let mut tx_storage = self.transactions.lock().unwrap();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

fn broadcast_transactions(&self, txs: &[&Transaction]) {
let mut tx_storage = self.transactions.lock();
for tx in txs {
tx_storage.push((*tx).to_owned())
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we might want to see if we can dedup the packages before pushing, i.e., possibly only keep the latest version of a transaction, for example if we'd bump over and over again.

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 envision this broadcaster to be used in channel monitor deserializer and I don't think it will create multiple versions of the same transaction. Do you see any other use cases?

Implementation of `BroacasterInterface` that does not broadcast transactions
immediately, but stores them internally to broadcast later with a call to
`release_transactions`.
@yellowred yellowred force-pushed the oleg_vec_broadcaster branch from 62671a2 to 8f3f61e Compare December 24, 2024 22:03
@yellowred yellowred requested a review from tnull December 24, 2024 22:09
@yellowred yellowred marked this pull request as ready for review December 24, 2024 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants