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

change(codec): Set relay to true if the last VersionMessage byte is omitted #5835

Merged
merged 5 commits into from Dec 13, 2022

Conversation

arya2
Copy link
Contributor

@arya2 arya2 commented Dec 9, 2022

Motivation

We want to make the relay field on VersionMessage optional.

Closes #5713.

Solution

  • Set relay field on VersionMessage to true when there's an UnexpectedEof error

Related changes: moves codec tests to their own module.

Review

Part of regular scheduled work.

Reviewer Checklist

  • Will the PR name make sense to users?
    • Does it need extra CHANGELOG info? (new features, breaking changes, large changes)
  • Are the PR labels correct?
  • Does the code do what the ticket and PR says?
    • Does it change concurrent code, unsafe code, or consensus rules?
  • How do you know it works? Does it have tests?

@arya2 arya2 added A-rust Area: Updates to Rust code E-help-wanted Call for participation: Help is requested to fix this issue. A-network Area: Network protocol updates or fixes labels Dec 9, 2022
@arya2 arya2 self-assigned this Dec 9, 2022
@github-actions github-actions bot added the C-enhancement Category: This is an improvement label Dec 9, 2022
@arya2 arya2 requested a review from teor2345 December 9, 2022 03:49
@arya2 arya2 added C-bug Category: This is a bug P-Medium ⚡ I-hang A Zebra component stops responding to requests do-not-merge Tells Mergify not to merge this PR labels Dec 9, 2022
@codecov
Copy link

codecov bot commented Dec 9, 2022

Codecov Report

Merging #5835 (b39b18c) into main (2041fda) will decrease coverage by 0.02%.
The diff coverage is 60.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5835      +/-   ##
==========================================
- Coverage   78.69%   78.66%   -0.03%     
==========================================
  Files         308      308              
  Lines       38781    38503     -278     
==========================================
- Hits        30517    30289     -228     
+ Misses       8264     8214      -50     

@teor2345
Copy link
Contributor

teor2345 commented Dec 9, 2022

I'm having difficulty writing the test for this, ideally it might read HEADER_LEN bytes first, decrement body_len by 1 via a test impl method, and then continue reading the frame as usual, but I'm not sure how to do that yet. I'm open to alternative designs.

One possible test:

  • parse a Version message with the relay field byte set to true/false, and check it is true/false
  • parse a Version message with no relay field byte, and check it is true

The second version message should be one byte shorter than the other one.

(There's no need to check for unread bytes, because the true/false test makes sure that the relay byte was read.)

@arya2 arya2 removed E-help-wanted Call for participation: Help is requested to fix this issue. do-not-merge Tells Mergify not to merge this PR labels Dec 9, 2022
@arya2
Copy link
Contributor Author

arya2 commented Dec 9, 2022

  • parse a Version message with the relay field byte set to true/false, and check it is true/false

There's a test for this part here https://github.com/ZcashFoundation/zebra/blob/main/zebra-network/src/isolated/tests/vectors.rs#L177

  • parse a Version message with no relay field byte, and check it is true

Ah, I was overcomplicating this part by trying to test it with a stream, I'll use Codec directly.

@arya2 arya2 marked this pull request as ready for review December 9, 2022 21:06
@arya2 arya2 requested a review from a team as a code owner December 9, 2022 21:06
moves codec tests to their own module
@arya2 arya2 changed the title change(codec): set relay to true if the last VersionMessage byte is omitted change(codec): Set relay to true if the last VersionMessage byte is omitted Dec 9, 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 looks good.

We could merge it as it is, but I'd like to improve test coverage, and document the specification we're implementing to.

zebra-network/src/protocol/external/codec.rs Outdated Show resolved Hide resolved
zebra-network/src/protocol/external/codec.rs Show resolved Hide resolved
zebra-network/src/protocol/external/codec/tests.rs Outdated Show resolved Hide resolved
zebra-network/src/protocol/external/codec/tests.rs Outdated Show resolved Hide resolved
adds doc comment to `read_version`

renames codec/tests.rs to codec/tests/vectors.rs
@arya2 arya2 requested a review from teor2345 December 12, 2022 20:20
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.

Thanks for the extra test and docs!

mergify bot added a commit that referenced this pull request Dec 12, 2022
@mergify mergify bot merged commit 07904d5 into main Dec 13, 2022
@mergify mergify bot deleted the opt-relay-field branch December 13, 2022 01:00
teor2345 pushed a commit that referenced this pull request Feb 6, 2023
…mitted (#5835) - manually delete moved tests

* sets relay = true when there's no relay byte

* adds test for missing relay field

moves codec tests to their own module

* simplifies version_message_omitted_relay test

* adds `version_message_with_relay` test

adds doc comment to `read_version`

renames codec/tests.rs to codec/tests/vectors.rs

* updates doc comment
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 A-rust Area: Updates to Rust code C-bug Category: This is a bug C-enhancement Category: This is an improvement I-hang A Zebra component stops responding to requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

relay (fRelayTxes) field when parse Message::Version from the remote node should be optional
2 participants