Skip to content

Conversation

@sputn1ck
Copy link
Member

This PR adds a protocol version to the reservation state machine

@sputn1ck sputn1ck force-pushed the reservation_protocol_version branch 2 times, most recently from 43642ef to bfdecb3 Compare April 17, 2025 09:28
@sputn1ck sputn1ck requested review from bhandras and starius April 17, 2025 11:16
@sputn1ck sputn1ck force-pushed the reservation_protocol_version branch from bfdecb3 to 3eab968 Compare April 17, 2025 14:56
const (
// ProtocolVersionServerInitiated is the protocol version where the
// server initiates the reservation.
ProtocolVersionServerInitiated ProtocolVersion = 0
Copy link
Member

Choose a reason for hiding this comment

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

I think we could have a CurrentProtocolVersion function that returns the last one, so we don't need to pass it in when creating a new reservation.

Copy link
Member Author

Choose a reason for hiding this comment

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

There is no current as we will still use both simultaneously

Copy link
Member

Choose a reason for hiding this comment

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

Discussed offline, so the reason to not have is because there will be two current versions, one for the client requested one for the server requested.

}

var states fsm.States
switch reservation.ProtocolVersion {
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to log here which version of the state machine is running. Perhaps also consider adding a String() function to the ProtocolVersion type.

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1, will do that for the static state machine as well.

Copy link
Member

@bhandras bhandras left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

const (
// ProtocolVersionServerInitiated is the protocol version where the
// server initiates the reservation.
ProtocolVersionServerInitiated ProtocolVersion = 0
Copy link
Member

Choose a reason for hiding this comment

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

Discussed offline, so the reason to not have is because there will be two current versions, one for the client requested one for the server requested.

@sputn1ck sputn1ck force-pushed the reservation_protocol_version branch from 3eab968 to 8add1f3 Compare April 22, 2025 08:20
@lightninglabs-deploy
Copy link

@starius: review reminder

@sputn1ck sputn1ck requested a review from hieblmi April 30, 2025 07:48
Copy link
Collaborator

@hieblmi hieblmi left a comment

Choose a reason for hiding this comment

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

LGTM ✅

}

var states fsm.States
switch reservation.ProtocolVersion {
Copy link
Collaborator

Choose a reason for hiding this comment

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

+1, will do that for the static state machine as well.

@sputn1ck sputn1ck force-pushed the reservation_protocol_version branch from 8add1f3 to 02f9573 Compare April 30, 2025 12:23
@sputn1ck sputn1ck force-pushed the reservation_protocol_version branch from 02f9573 to 56848d0 Compare April 30, 2025 12:25
@sputn1ck sputn1ck merged commit cb18240 into lightninglabs:master Apr 30, 2025
4 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.

4 participants