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.

Sign up for GitHub

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

Jump to bottom

Go1.22 #4722

Closed
wants to merge 17 commits into from
Closed

Go1.22 #4722

wants to merge 17 commits into from

Conversation

Frozen
Copy link
Contributor

@Frozen Frozen commented Jul 29, 2024

Golang version 1.22

@Frozen Frozen self-assigned this Jul 31, 2024
@mur-me mur-me requested review from sophoah and GheisMohammadi July 31, 2024 08:37
@GheisMohammadi
Copy link
Contributor

Thanks for the PR. It is a great work. There are a few points here to mention:

  • The PR is quite extensive, with over thousands lines of changes, but the description is only three words long. I see a lot more changes rather than updating go version. Could you please provide a more detailed description? This will help us understand the purpose and scope of the changes better.

  • It looks like you’re bringing in over 5000 lines of code from the harmony-one/harmony-test repository and placing them in a new folder called tests. We already have a test folder, and having both test and tests could be confusing. Additionally, duplicating code across repositories might make it very hard to maintain consistency. Could you explain the reasoning behind this approach? Perhaps we could consider using Docker to clone the harmony-test repository and use it directly, which might simplify maintenance.

  • The commit titles are quite brief and a bit unclear (e.g., "test", "tests", "1.21.11"). Could you please provide more descriptive commit messages? This would greatly help in understanding the changes at a glance and maintaining the project history.

  • There’s a commit titled "Fixed trailing zeroes" which seems to actually update dependencies instead. Clear and accurate commit messages are important to avoid confusion, so it would be helpful to correct this.

  • I see that the tests for this PR are still ongoing. It might be best to wait until all tests are complete and verified to ensure there are no unexpected issues. We have to have this PR on test-net for awhile to ensure consistency and stability.

GheisMohammadi
GheisMohammadi reviewed Aug 1, 2024
View reviewed changes
cmd/bootnode/main.go
@@ -108,7 +108,7 @@ func main() {
logConn := flag.Bool("log_conn", false, "log incoming/outgoing connections")
maxConnPerIP := flag.Int("max_conn_per_ip", 10, "max connections number for same ip")
forceReachabilityPublic := flag.Bool("force_public", false, "forcing the local node to believe it is reachable externally")
noTransportSecurity := flag.Bool("no_transport_security", false, "disable TLS encrypted transport")
noTransportSecurity := flag.Bool("no_transport_security", true, "disable TLS encrypted transport")
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems it disables transport security without proper discussion and a finalized decision from the team. Have we checked the compatibility of nodes with disabled security with the current mainnet nodes? We have to ensure they can communicate properly to avoid any network issues. Also it would be good to provide us with a few references that shows this change helps.

Copy link
Collaborator

@mur-me mur-me Aug 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Our inputs:

  • mainnet network
  • Default value for the release 2024.1.0 is "NoTransportSecurity": false, thus "NoTransportSecurity": true cases is out of scope
  • If mixing all possible combinations together we will end up in 16(!) possible cases on the communication between bootnode and Harmony node, but if we remove out of scope cases the number is decreasing to 9.
  • the next step will be to try enable security via "NoTransportSecurity": false on the bootnode 1.22 and test 3 cases
  Bootnode1.22 Node "NoTransportSecurity": true Bootnode1.22 Node "NoTransportSecurity": false Bootnode1.19 Node "NoTransportSecurity": true Bootnode1.19 Node "NoTransportSecurity": false
Harmony 1.19 Node "NoTransportSecurity": true Out of scope Out of scope Out of scope Out of scope
Harmony 1.19 Node "NoTransportSecurity": false OK OK Out of scope OK
Harmony 1.22 Node "NoTransportSecurity": true FAILED FAILED Out of scope FAILED
Harmony 1.22 Node "NoTransportSecurity": false OK OK Out of scope OK

Copy link
Collaborator

Choose a reason for hiding this comment

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

the same test on the devnet have given me inverted results:

  Bootnode1.22 Node "NoTransportSecurity": true Bootnode1.22 Node "NoTransportSecurity": false Bootnode1.19 Node "NoTransportSecurity": true Bootnode1.19 Node "NoTransportSecurity": false
Harmony 1.22 Node "NoTransportSecurity": true OK OK Out of scope OK
Harmony 1.22 Node "NoTransportSecurity": false FAILED FAILED Out of scope FAILED

What can it be? Some OS diff? OpenSSL versions diff(both envs are using openssl(3.0.1 and 1.0.1) with TLS 1.3 enabled)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Great tests, thanks

Copy link
Contributor

@sophoah sophoah Aug 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

the same test on the devnet have given me inverted results:

just check two devnet normal node and they had "NoTransportSecurity": true, Node might have received the list of peers in the network but couldn't connect to any of the node

based on test above:

  1. NoTransportSecurity is not preventing bootnode and normal node to exchange the peers
  2. the non bootnode node in the network need to match their settings NoTransportSecurity

Copy link
Collaborator

Choose a reason for hiding this comment

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

Results of canary tests all nodes have NoTransportSecurity: false:

  • RPC node 1.22 + Bootnode 1.22 ✅
  • RPC node 1.22 + Bootnode 1.19 ✅
  • RPC 1.19 + Bootnode 1.22 ✅
  • RPC node 1.22(95.217.207.240 aka mhe-explorers1-02) + traffic ✅
  • s1 validator 1.22 ✅
  • s0 validator 1.22 ✅

@mur-me mur-me mentioned this pull request Aug 13, 2024
Merged
@Frozen
Copy link
Contributor Author

Frozen commented Aug 19, 2024

check this one #4736

@Frozen Frozen closed this Aug 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mur-me mur-me mur-me left review comments

@GheisMohammadi GheisMohammadi GheisMohammadi left review comments

@ONECasey ONECasey ONECasey approved these changes

@sophoah sophoah Awaiting requested review from sophoah

Assignees

@Frozen Frozen

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Frozen @GheisMohammadi @mur-me @sophoah @ONECasey