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

LSPS1- Channel request #8

Merged
merged 4 commits into from Nov 17, 2023
Merged

LSPS1- Channel request #8

merged 4 commits into from Nov 17, 2023

Conversation

Srg213
Copy link
Contributor

@Srg213 Srg213 commented Jun 20, 2023

No description provided.

@Srg213 Srg213 changed the title msgs for LSPS1 LSPS1- Channel request Jun 20, 2023
@Srg213 Srg213 marked this pull request as draft June 20, 2023 06:31
Copy link
Collaborator

@tnull tnull left a comment

Choose a reason for hiding this comment

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

This needs a rebase after #7 has been merged. Also, compilation currently fails in multiple places.

src/channel_request/channel_manager.rs Outdated Show resolved Hide resolved
src/channel_request/utils.rs Outdated Show resolved Hide resolved
src/channel_request/channel_manager.rs Outdated Show resolved Hide resolved
let channel_id = self.request_to_cid.remove(request_id)?;

if let Some(channel) = self.channels_by_id.get_mut(&channel_id) {
if channel.state == state {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Mh, IIUC this means we would just remove the channel and fail silently if the channel is in a wrong state? We should probably return an error and at least log this, even though it should never happen.

*self.peer_manager.lock().unwrap() = Some(peer_manager);
}

fn connect_to_counterparty(&self, counterparty_node_id: PublicKey) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm confused: what is the purpose of this method and why are we generating a random channel id?

src/events.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@tnull tnull left a comment

Choose a reason for hiding this comment

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

Did a high-level pass over the channel_manager.rs structures and found them a bit confusing tbh. Would be great to get some clarity here and as part of the docs.

src/channel_request/channel_manager.rs Outdated Show resolved Hide resolved
src/channel_request/channel_manager.rs Outdated Show resolved Hide resolved
src/channel_request/channel_manager.rs Outdated Show resolved Hide resolved
src/channel_request/channel_manager.rs Outdated Show resolved Hide resolved
// confirms_within_blocks: Option<u32>,
// channel_expiry_blocks: Option<u32>,
announce_channel: bool,
order_id: Option<OrderId>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

See above, if we store the Order object that lead to the channel, this field would be superfluous.

src/channel_request/channel_manager.rs Outdated Show resolved Hide resolved
src/channel_request/channel_manager.rs Outdated Show resolved Hide resolved
src/channel_request/channel_manager.rs Outdated Show resolved Hide resolved
Ok(())
}

/* No errors for getinfo method */
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Please remove.

src/channel_request/channel_manager.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@tnull tnull left a comment

Choose a reason for hiding this comment

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

Excuse the delay here! I think now that #11 is getting refactored it would make sense to adjust this PR to follow the same model w.r.t. terminology and state (transition) handling.

Btw. seems to need a rebase (either on main or said #11) to get rid of the conflicts.

src/channel_request/channel_manager.rs Outdated Show resolved Hide resolved
src/channel_request/channel_manager.rs Outdated Show resolved Hide resolved
src/channel_request/channel_manager.rs Outdated Show resolved Hide resolved
@tnull
Copy link
Collaborator

tnull commented Nov 9, 2023

Would love to land this first work-in-progress draft so that the code can be included in the upcoming project restructuring. For this, could you resolve any conflicts with current main and make sure it compiles and passes tests?

Copy link
Collaborator

@tnull tnull left a comment

Choose a reason for hiding this comment

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

Thanks!

Did a quick high-level pass. Will look more closely when this is rebased on main and the conflicts have been resolved. As stated above, let's do this soon so that all this code can be part of the restructuring PR coming up.

.github/workflows/build.yml Outdated Show resolved Hide resolved
src/channel_request/msgs.rs Outdated Show resolved Hide resolved
src/channel_request/msgs.rs Outdated Show resolved Hide resolved
src/channel_request/msgs.rs Outdated Show resolved Hide resolved
src/channel_request/event.rs Outdated Show resolved Hide resolved
{
entropy_source: ES,
peer_manager: Mutex<Option<Arc<PeerManager<Descriptor, CM, RM, OM, L, CMH, NS>>>>,
channel_manager: Arc<ChannelManager<M, T, ES, NS, SP, F, R, L>>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can use the AChannelManager trait to at least make this a bit easier now.


impl InboundRequestState {
fn info_received(&self, versions: Vec<u16>, options: OptionsSupported) -> Result<Self, ChannelStateError> {
let max_shared_version = versions
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note that we're in the process of dropping versioning support in all specs, so all the corresponding code can be removed: BitcoinAndLightningLayerSpecs/lsp#63

src/channel_request/channel_manager.rs Outdated Show resolved Hide resolved
src/channel_request/channel_manager.rs Outdated Show resolved Hide resolved
src/transport/message_handler.rs Outdated Show resolved Hide resolved
@Srg213 Srg213 force-pushed the lsps1 branch 2 times, most recently from b37b754 to f1e259e Compare November 16, 2023 14:43
@tnull
Copy link
Collaborator

tnull commented Nov 16, 2023

@Srg213 Thank you for the rebase and addressing most of the comments! If you don't mind, could you undraft this PR? I'd then land it and start the restructuring work early next week. Any further changes to make this work could then happen as follow-ups.

@Srg213 Srg213 marked this pull request as ready for review November 17, 2023 07:56
Copy link
Collaborator

@tnull tnull left a comment

Choose a reason for hiding this comment

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

Thanks! Going ahead and merge this draft so that we have a unified codebase. Any further changes can then be done in follow-up PRs.

@tnull tnull merged commit 81bbbea into lightningdevkit:main Nov 17, 2023
3 checks passed
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.

None yet

2 participants