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

fix: Use handshake version 2 #3157

Merged
merged 25 commits into from
Sep 4, 2020
Merged

Conversation

mfornet
Copy link
Member

@mfornet mfornet commented Aug 13, 2020

This change is only backward compatible with branch latest_supported_version,
We must merge and deploy that branch first.

Fixes #3136

Test plan

  1. Check latest_supported_version_2 branch is backward compatible with
    latest_supported_version branch.

    Run backward.py with main(near_root, "latest_supported_version", "latest_supported_version_2") and check it passes.

  2. handshake.py pass

@gitpod-io
Copy link

gitpod-io bot commented Aug 13, 2020

nearprotocol-bulldozer bot pushed a commit that referenced this pull request Aug 20, 2020
This will allow nodes that are compatible with each other to communicate disregarding who initiated the conversation.

This changes needs to be deployed in two step. This PR introduce HandshakeV2 and add ability to nodes to understand and process this types of messages. Later in PR #3157 it is started to use HandshakeV2 as default handshake.

Test plan
=========
Pass upgradability test
@mfornet mfornet changed the base branch from latest_supported_version to master August 21, 2020 19:41
@mfornet mfornet force-pushed the latest_supported_version_2 branch from b3d2afe to 7f8b199 Compare August 21, 2020 21:59
@codecov
Copy link

codecov bot commented Aug 21, 2020

Codecov Report

Merging #3157 into master will decrease coverage by 0.16%.
The diff coverage is 71.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3157      +/-   ##
==========================================
- Coverage   87.19%   87.03%   -0.17%     
==========================================
  Files         217      217              
  Lines       42397    42493      +96     
==========================================
+ Hits        36970    36984      +14     
- Misses       5427     5509      +82     
Impacted Files Coverage Δ
core/primitives/src/version.rs 100.00% <ø> (ø)
chain/network/src/peer.rs 77.73% <29.62%> (-1.70%) ⬇️
chain/network/src/types.rs 78.13% <80.00%> (+2.26%) ⬆️
chain/network/src/codec.rs 91.72% <88.88%> (-0.94%) ⬇️
chain/client/src/test_utils.rs 81.32% <0.00%> (-6.45%) ⬇️
chain/chain/src/error.rs 45.60% <0.00%> (-4.80%) ⬇️
chain/client/src/info.rs 72.93% <0.00%> (-3.76%) ⬇️
chain/client/src/types.rs 67.50% <0.00%> (-2.50%) ⬇️
chain/network/src/test_utils.rs 90.32% <0.00%> (-1.62%) ⬇️
core/primitives/src/types.rs 44.22% <0.00%> (-1.51%) ⬇️
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 51b2baa...f5aafd8. Read the comment docs.

Copy link
Collaborator

@SkidanovAlex SkidanovAlex left a comment

Choose a reason for hiding this comment

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

Please run a nayduck run and link here

version: handshake_old.oldest_supported_version,
// In previous version of handshake, nodes usually sent the oldest supported version instead of their current version.
// Computing the current version of the other as the oldest version plus 4, but not letting go bigger than 33.
version: std::cmp::min(PROTOCOL_VERSION - 1, handshake_old.version.saturating_add(4)),
Copy link
Collaborator

Choose a reason for hiding this comment

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

PROTOCOL_VERSION will be changing. This should be a separate named constant

Copy link
Member Author

Choose a reason for hiding this comment

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

The idea is that this changes with as the PROTOCOL_VERSION changes. This conversion will only be relevant during the transition from Handshake to HandshakeV2, since we were previously sending oldest supported version, we are predicting its version as oldest_supported_version+1, but we know for sure they should be at least one version lower than current version.

@mfornet mfornet force-pushed the latest_supported_version_2 branch from 7f8b199 to 23d746e Compare September 1, 2020 23:04
@mfornet
Copy link
Member Author

mfornet commented Sep 1, 2020

Please run a nayduck run and link here

http://nayduck.eastus.cloudapp.azure.com:3000/#/run/281

@mfornet mfornet force-pushed the latest_supported_version_2 branch from 3c4eaf4 to ca59279 Compare September 2, 2020 23:54
Copy link
Collaborator

@bowenwang1996 bowenwang1996 left a comment

Choose a reason for hiding this comment

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

Approving to unblock #3283

This will allow nodes that are compatible with each other to
communicate disregarding who initiated the conversation.

Test plan
=========
Pass upgradability test
**DO NOT MERGE**

This change is only backward compatible with branch
latest_supported_version. We must merge and deploy that branch first.

Test plan
=========

1. Check latest_supported_version_2 branch is backward compatible with
latest_supported_version branch.

Run backward.py with
main(near_root, "latest_supported_version", "latest_supported_version_2")

2. handshake.py pass
@mfornet mfornet force-pushed the latest_supported_version_2 branch from ca59279 to b153dc6 Compare September 4, 2020 02:15
chain/network/src/peer.rs Outdated Show resolved Hide resolved
chain/network/src/peer.rs Outdated Show resolved Hide resolved
@bowenwang1996
Copy link
Collaborator

@mfornet looks like upgradability test failed

@mfornet mfornet force-pushed the latest_supported_version_2 branch from deb2d2f to 002d8f0 Compare September 4, 2020 05:13
@mfornet
Copy link
Member Author

mfornet commented Sep 4, 2020

Both buildkite/nearcore/backward-compatible and buildkite/nearcore/upgradable are failing because, the stable_version that is being downloaded as beta is too old (version 31) vs expected beta (version 34).

It is downloading binary from: https://s3-us-west-1.amazonaws.com/build.nearprotocol.com/nearcore/

@bowenwang1996 bowenwang1996 merged commit d25f8c0 into master Sep 4, 2020
@bowenwang1996 bowenwang1996 deleted the latest_supported_version_2 branch September 4, 2020 23:51
mhalambek pushed a commit that referenced this pull request Sep 7, 2020
* fix: Use both version and oldest_supported version

This will allow nodes that are compatible with each other to
communicate disregarding who initiated the conversation.

Test plan
=========
Pass upgradability test

* Fix failing test

* fix: Use handshake version 2

**DO NOT MERGE**

This change is only backward compatible with branch
latest_supported_version. We must merge and deploy that branch first.

Test plan
=========

1. Check latest_supported_version_2 branch is backward compatible with
latest_supported_version branch.

Run backward.py with
main(near_root, "latest_supported_version", "latest_supported_version_2")

2. handshake.py pass

* Update handshake.py

* Use previous version as default version for incoming handshakes

* Use +1 predicted version

* Respond with current version to other nodes

* Reaction to ProtocolVersionMismatch and better API

* Update backward compatible test

* Remove Option

* Fix codec test

* Backward compatible with version bump

* hotfix upgradable tests

* Address bowen comments

* test fix

* test another attempt

* compile current

* add debug info

* try again

* remove debug info

* Remove unnecessary version checks

* Add responses to Handshake with future version

* Fix test

* Don't use match

Co-authored-by: Bowen Wang <[email protected]>
chefsale pushed a commit that referenced this pull request Sep 7, 2020
* fix: Use both version and oldest_supported version

This will allow nodes that are compatible with each other to
communicate disregarding who initiated the conversation.

Test plan
=========
Pass upgradability test

* Fix failing test

* fix: Use handshake version 2

**DO NOT MERGE**

This change is only backward compatible with branch
latest_supported_version. We must merge and deploy that branch first.

Test plan
=========

1. Check latest_supported_version_2 branch is backward compatible with
latest_supported_version branch.

Run backward.py with
main(near_root, "latest_supported_version", "latest_supported_version_2")

2. handshake.py pass

* Update handshake.py

* Use previous version as default version for incoming handshakes

* Use +1 predicted version

* Respond with current version to other nodes

* Reaction to ProtocolVersionMismatch and better API

* Update backward compatible test

* Remove Option

* Fix codec test

* Backward compatible with version bump

* hotfix upgradable tests

* Address bowen comments

* test fix

* test another attempt

* compile current

* add debug info

* try again

* remove debug info

* Remove unnecessary version checks

* Add responses to Handshake with future version

* Fix test

* Don't use match

Co-authored-by: Bowen Wang <[email protected]>
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.

handshake.py consistently fails
3 participants