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

T1. Fix isolated connection bugs, improve tests, upgrade dependencies #3302

Merged
merged 16 commits into from
Jan 14, 2022

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Dec 28, 2021

Motivation

This PR contains some zebra-network changes that are required or useful for Tor support using arti. But it doesn't add any arti dependencies or Tor code yet. (That's in the next PR.)

We should merge this PR because these are all improvements, and we will have to do them eventually anyway.

It also makes testing easier, because it lets us write tests that don't depend on the network. And it improves test coverage.

Solution

Enhancements:

  • Make handshakes generic over AsyncRead + AsyncWrite
  • Add a wrapper function for isolated TCP connections to an IP address

Dependencies:

Fixes & Cleanups:

Testing:

  • Improve isolated TCP connection tests
  • Add an in-memory connection test that uses AsyncReadWrite

Review

This PR is ready for review, @dconnolly can review it.

It might have merge conflicts with future PRs, so we should get it in soon.

Reviewer Checklist

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

Follow Up Work

Actually add Tor support.

@teor2345 teor2345 added C-bug Category: This is a bug A-dependencies Area: Dependency file updates C-enhancement Category: This is an improvement C-cleanup Category: This is a cleanup P-Medium A-network Area: Network protocol updates or fixes C-testing Category: These are tests labels Dec 28, 2021
@teor2345 teor2345 self-assigned this Dec 28, 2021
@codecov
Copy link

codecov bot commented Dec 28, 2021

Codecov Report

Merging #3302 (29a8c62) into main (ee9f081) will increase coverage by 0.00%.
The diff coverage is 60.81%.

@@           Coverage Diff           @@
##             main    #3302   +/-   ##
=======================================
  Coverage   77.79%   77.80%           
=======================================
  Files         266      266           
  Lines       31449    31449           
=======================================
+ Hits        24466    24469    +3     
+ Misses       6983     6980    -3     

@teor2345 teor2345 requested review from oxarbitrage and removed request for oxarbitrage December 30, 2021 02:02
@teor2345 teor2345 marked this pull request as draft January 3, 2022 08:03
@teor2345

This comment has been minimized.

@teor2345 teor2345 marked this pull request as ready for review January 6, 2022 00:16
@teor2345 teor2345 enabled auto-merge (squash) January 6, 2022 00:16
@teor2345 teor2345 marked this pull request as draft January 6, 2022 00:18
auto-merge was automatically disabled January 6, 2022 00:18

Pull request was converted to draft

@teor2345 teor2345 requested a review from dconnolly January 6, 2022 00:18
@teor2345 teor2345 marked this pull request as ready for review January 7, 2022 07:00
@teor2345 teor2345 enabled auto-merge (squash) January 7, 2022 07:00
@conradoplg
Copy link
Collaborator

I've synced with main and solved conflicts

@mpguerra mpguerra requested a review from conradoplg January 14, 2022 14:26
@conradoplg conradoplg disabled auto-merge January 14, 2022 18:27
Copy link
Collaborator

@conradoplg conradoplg left a comment

Choose a reason for hiding this comment

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

Looks good!

@mergify mergify bot merged commit 6c787dd into main Jan 14, 2022
@mergify mergify bot deleted the isolated-generic branch January 14, 2022 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-dependencies Area: Dependency file updates A-network Area: Network protocol updates or fixes C-bug Category: This is a bug C-cleanup Category: This is a cleanup C-enhancement Category: This is an improvement C-testing Category: These are tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove outdated http dependencies connect_isolated only works on mainnet
4 participants