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

Always use HTTP handshaking in overlay: #885

Closed
wants to merge 1 commit into from

Conversation

vinniefalco
Copy link
Contributor

Inbound and outbound peer connections always use HTTP handshakes to negotiate connections, instead of the deprecated TMHello protocol message.

rippled versions 0.27.0 and later support both optional HTTP handshakes and legacy TMHello messages, so always using HTTP handshakes should not cause disruption. However, versions before 0.27.0 will no longer be able to participate in the overlay network - support for handshaking via the TMHello message is removed.

@nbougalis
Copy link
Contributor

This looks good. 👍 If this goes into .28 we should be sure to include a note in the release notes that versions of rippled prior to .27 won't be able to participate in the overlay network.

Inbound and outbound peer connections always use HTTP handshakes to
negotiate connections, instead of the deprecated TMHello protocol
message.

rippled versions 0.27.0 and later support both optional HTTP handshakes
and legacy TMHello messages, so always using HTTP handshakes should not
cause disruption. However, versions before 0.27.0 will no longer be
able to participate in the overlay network - support for handshaking
via the TMHello message is removed.
#
# When set, outgoing peer connections will handshaking using a HTTP
# request instead of the legacy TMHello protocol buffers message.
# Incoming peer connections have their handshakes detected automatically.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we know if anybody was using http_handshake = 1 in the wild?

Per the "Tx Changes" label, I forgot this setting was available. Perhaps this isn't breaking, but would require anybody who wants to stay on 0.27.0 to set this flag.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I answered my own question: Set http_handshake = 1 on my dogfood rippled, started it up, and it had 15 outbound peer connections within moments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whether they are using http_handshake = 1 or not, with this change everyone will be using it ^_^

Copy link
Collaborator

Choose a reason for hiding this comment

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

Exactly. I was thinking of forward compatibility if we wanted to roll this out before a Tx Change release.

@ximinez
Copy link
Collaborator

ximinez commented Feb 24, 2015

This looks good. Changes are a lot easier to review when they remove so much more code than they add. :)

👍

@ximinez
Copy link
Collaborator

ximinez commented Feb 24, 2015

PS. Tests pass in Windows for release and debug, and running rippled is able to connect to the network. I don't have the patience to wait for it to sync.

@vinniefalco
Copy link
Contributor Author

@ximinez It should sync in less than a minute... do you have nudb configured?

@ximinez
Copy link
Collaborator

ximinez commented Feb 24, 2015

I do, but it's still slow to sync. I've still got a spinning disk, and VirtualBox taking up resources.

@vinniefalco
Copy link
Contributor Author

nudb only runs correctly on SSDs

@ximinez
Copy link
Collaborator

ximinez commented Feb 24, 2015

I didn't know that. Still, given that fact, it will usually sync eventually.

@ximinez ximinez added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Feb 24, 2015
@vinniefalco
Copy link
Contributor Author

This is something of a Tx change but we should still merge it.

@ximinez
Copy link
Collaborator

ximinez commented Feb 26, 2015

I have no objection to removing the "Tx Change" label.

@vinniefalco
Copy link
Contributor Author

I posted an announcement to architects regarding the change

@rec rec closed this Feb 27, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants