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

Ignore non-verack and non-version messages in handshake #3522

Merged
merged 4 commits into from
Feb 16, 2022

Conversation

oxarbitrage
Copy link
Contributor

Motivation

We want to ignore some messages during handshake to make the conformance tests of ziggurat pass: https://github.com/eqlabs/ziggurat/blob/master/SPEC.md#zg-conformance-003

Close #3429

Solution

Ignore the verack can be done just by not closing the connection when receiving other messages at the end of the handshake. This is done in cce709a

The version is a bit trickier, we need to wait for another message when we receive a non version one at the beginning of the handshake.

I made a proof of concept of this at cce709a however the code can be improved as it is doing some duplicated work. I am looking for some suggestions there but before that i want to make sure that we will want to really do that as it is in this pull request.

Review

I will like @teor2345 and/or @jvff to take a look.

Reviewer Checklist

  • Code implements Specs and Designs
  • Tests for Expected Behaviour
  • Tests for Errors

@oxarbitrage
Copy link
Contributor Author

The ziggurat tests are a bit intermittent from here, sometimes stuff timeout, however with the code of this pull request the following tests that were always failing before now pass:

Verack:

$ cargo test ignore_message_inplace_of_verack -- --test-threads=1 
    Finished test [unoptimized + debuginfo] target(s) in 0.02s
     Running unittests (target/debug/deps/ziggurat-e77efbb62339a887)

running 22 tests
test tests::conformance::handshake::ignore_message_inplace_of_verack::when_node_initiates_connection::addr ... ok
test tests::conformance::handshake::ignore_message_inplace_of_verack::when_node_initiates_connection::get_addr ... ok
test tests::conformance::handshake::ignore_message_inplace_of_verack::when_node_initiates_connection::get_blocks ... ok
test tests::conformance::handshake::ignore_message_inplace_of_verack::when_node_initiates_connection::get_data_block ... ok
test tests::conformance::handshake::ignore_message_inplace_of_verack::when_node_initiates_connection::get_data_tx ... ok
test tests::conformance::handshake::ignore_message_inplace_of_verack::when_node_initiates_connection::get_headers ... ok
test tests::conformance::handshake::ignore_message_inplace_of_verack::when_node_initiates_connection::inv ... ok
test tests::conformance::handshake::ignore_message_inplace_of_verack::when_node_initiates_connection::mempool ... ok
test tests::conformance::handshake::ignore_message_inplace_of_verack::when_node_initiates_connection::not_found ... ok
test tests::conformance::handshake::ignore_message_inplace_of_verack::when_node_initiates_connection::ping ... ok
test tests::conformance::handshake::ignore_message_inplace_of_verack::when_node_initiates_connection::pong ... ok
test tests::conformance::handshake::ignore_message_inplace_of_verack::when_node_receives_connection::addr ... ok
test tests::conformance::handshake::ignore_message_inplace_of_verack::when_node_receives_connection::get_addr ... ok
test tests::conformance::handshake::ignore_message_inplace_of_verack::when_node_receives_connection::get_blocks ... ok
test tests::conformance::handshake::ignore_message_inplace_of_verack::when_node_receives_connection::get_data_block ... ok
test tests::conformance::handshake::ignore_message_inplace_of_verack::when_node_receives_connection::get_data_tx ... ok
test tests::conformance::handshake::ignore_message_inplace_of_verack::when_node_receives_connection::get_headers ... ok
test tests::conformance::handshake::ignore_message_inplace_of_verack::when_node_receives_connection::inv ... ok
test tests::conformance::handshake::ignore_message_inplace_of_verack::when_node_receives_connection::mempool ... ok
test tests::conformance::handshake::ignore_message_inplace_of_verack::when_node_receives_connection::not_found ... ok
test tests::conformance::handshake::ignore_message_inplace_of_verack::when_node_receives_connection::ping ... ok
test tests::conformance::handshake::ignore_message_inplace_of_verack::when_node_receives_connection::pong ... ok

Version

$ cargo test ignore_message_inplace_of_version -- --test-threads=1 
    Finished test [unoptimized + debuginfo] target(s) in 0.02s
     Running unittests (target/debug/deps/ziggurat-e77efbb62339a887)

running 24 tests
test tests::conformance::handshake::ignore_message_inplace_of_version::when_node_initiates_connection::addr ... ok
test tests::conformance::handshake::ignore_message_inplace_of_version::when_node_initiates_connection::get_addr ... ok
test tests::conformance::handshake::ignore_message_inplace_of_version::when_node_initiates_connection::get_blocks ... ok
test tests::conformance::handshake::ignore_message_inplace_of_version::when_node_initiates_connection::get_data_block ... ok
test tests::conformance::handshake::ignore_message_inplace_of_version::when_node_initiates_connection::get_data_tx ... ok
test tests::conformance::handshake::ignore_message_inplace_of_version::when_node_initiates_connection::get_headers ... ok
test tests::conformance::handshake::ignore_message_inplace_of_version::when_node_initiates_connection::inv ... ok
test tests::conformance::handshake::ignore_message_inplace_of_version::when_node_initiates_connection::mempool ... ok
test tests::conformance::handshake::ignore_message_inplace_of_version::when_node_initiates_connection::not_found ... ok
test tests::conformance::handshake::ignore_message_inplace_of_version::when_node_initiates_connection::ping ... ok
test tests::conformance::handshake::ignore_message_inplace_of_version::when_node_initiates_connection::pong ... ok
test tests::conformance::handshake::ignore_message_inplace_of_version::when_node_initiates_connection::verack ... ok
test tests::conformance::handshake::ignore_message_inplace_of_version::when_node_receives_connection::addr ... ok
test tests::conformance::handshake::ignore_message_inplace_of_version::when_node_receives_connection::get_addr ... ok
test tests::conformance::handshake::ignore_message_inplace_of_version::when_node_receives_connection::get_blocks ... ok
test tests::conformance::handshake::ignore_message_inplace_of_version::when_node_receives_connection::get_data_block ... ok
test tests::conformance::handshake::ignore_message_inplace_of_version::when_node_receives_connection::get_data_tx ... ok
test tests::conformance::handshake::ignore_message_inplace_of_version::when_node_receives_connection::get_headers ... ok
test tests::conformance::handshake::ignore_message_inplace_of_version::when_node_receives_connection::inv ... ok
test tests::conformance::handshake::ignore_message_inplace_of_version::when_node_receives_connection::mempool ... ok
test tests::conformance::handshake::ignore_message_inplace_of_version::when_node_receives_connection::not_found ... ok
test tests::conformance::handshake::ignore_message_inplace_of_version::when_node_receives_connection::ping ... ok
test tests::conformance::handshake::ignore_message_inplace_of_version::when_node_receives_connection::pong ... ok
test tests::conformance::handshake::ignore_message_inplace_of_version::when_node_receives_connection::verack ... ok

@oxarbitrage
Copy link
Contributor Author

By the way, it seems ziggurat uses the testnet protocol version so in order to make the tests run properly, the constant need to be updated. I sent a ticket to make this a bit easier as i had a hard time figuring it out by myself.

runziggurat/zcash#110

@codecov
Copy link

codecov bot commented Feb 12, 2022

Codecov Report

Merging #3522 (dc6679b) into main (499ae89) will increase coverage by 2.10%.
The diff coverage is 86.00%.

@@            Coverage Diff             @@
##             main    #3522      +/-   ##
==========================================
+ Coverage   78.34%   80.44%   +2.10%     
==========================================
  Files         267      274       +7     
  Lines       31526    32243     +717     
==========================================
+ Hits        24698    25939    +1241     
+ Misses       6828     6304     -524     

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Try using a loop to ignore messages that aren't the ones we're looking for.

zebra-network/src/peer/handshake.rs Outdated Show resolved Hide resolved
zebra-network/src/peer/handshake.rs Outdated Show resolved Hide resolved
teor2345
teor2345 previously approved these changes Feb 14, 2022
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

This is a nice compact fix for this issue, thanks!

I just want to tweak the debugging a bit. So if peer connections start behaving differently after this change, we'll be able to see if the skipped messages.

zebra-network/src/peer/handshake.rs Outdated Show resolved Hide resolved
zebra-network/src/peer/handshake.rs Outdated Show resolved Hide resolved
Co-authored-by: teor <[email protected]>
@oxarbitrage oxarbitrage marked this pull request as ready for review February 15, 2022 12:56
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Let's go!

@teor2345 teor2345 added A-network Area: Network protocol updates or fixes C-bug Category: This is a bug I-slow Problems with performance or responsiveness I-usability Zebra is hard to understand or use P-Medium ⚡ labels Feb 15, 2022
@teor2345
Copy link
Contributor

This change might fix some block explorers, so we should check them again when we have time:

mergify bot added a commit that referenced this pull request Feb 15, 2022
@mergify mergify bot merged commit 83827b9 into ZcashFoundation:main Feb 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-network Area: Network protocol updates or fixes C-bug Category: This is a bug I-slow Problems with performance or responsiveness I-usability Zebra is hard to understand or use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ZG-CONFORMANCE-003-006: ignore non-Version/Verack messages during the handshake
2 participants