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

upgrade status-go with geth 1.9.5 #9056

Merged
merged 1 commit into from Oct 5, 2019
Merged

upgrade status-go with geth 1.9.5 #9056

merged 1 commit into from Oct 5, 2019

Conversation

@adambabik
Copy link
Member

adambabik commented Sep 27, 2019

No description provided.

@status-github-bot status-github-bot bot added this to REVIEW in Pipeline for QA Sep 27, 2019
@status-github-bot

This comment has been minimized.

Copy link

status-github-bot bot commented Sep 27, 2019

Pull Request Checklist

  • Docs: Updated the documentation, if affected
  • Docs: Added or updated inline comments explaining intention of the code
  • Tests: Ensured that all new UI elements have been assigned accessibility IDs
  • Tests: Signaled need for E2E tests with label, if applicable
  • Tests: Briefly described what was tested and what platforms were used
  • UI: In case of UI changes, ensured that UI matches Figma
  • UI: In case of UI changes, requested review from a Core UI designer
  • UI: In case of UI changes, included screenshots of implementation
@status-im-auto

This comment has been minimized.

Copy link
Member

status-im-auto commented Sep 27, 2019

Jenkins Builds

Click to see older builds (63)
Commit #️⃣ Finished (UTC) Duration Platform Result
06032ff #1 2019-09-27 08:52:03 ~1 min ios 📄log
06032ff #1 2019-09-27 08:52:08 ~1 min macos 📄log
06032ff #1 2019-09-27 08:52:34 ~2 min linux 📄log
06032ff #1 2019-09-27 08:54:46 ~4 min android 📄log
06032ff #1 2019-09-27 08:54:49 ~4 min android-e2e 📄log
06032ff #1 2019-09-27 09:02:13 ~11 min windows 📄log
129e1ab #2 2019-09-27 09:04:21 ~31 sec ios 📄log
129e1ab #2 2019-09-27 09:04:21 ~29 sec macos 📄log
129e1ab #2 2019-09-27 09:04:25 ~34 sec linux 📄log
129e1ab #2 2019-09-27 09:04:25 ~32 sec windows 📄log
129e1ab #2 2019-09-27 09:07:27 ~3 min android 📄log
129e1ab #2 2019-09-27 09:07:31 ~3 min android-e2e 📄log
20c033f #3 2019-09-27 09:43:28 ~39 sec linux 📄log
20c033f #3 2019-09-27 09:43:28 ~37 sec windows 📄log
20c033f #3 2019-09-27 09:43:31 ~45 sec android-e2e 📄log
20c033f #3 2019-09-27 09:43:37 ~50 sec android 📄log
20c033f #3 2019-09-27 09:43:37 ~47 sec macos 📄log
20c033f #3 2019-09-27 09:43:37 ~49 sec ios 📄log
a1cee11 #4 2019-09-27 09:51:18 ~25 sec macos 📄log
a1cee11 #4 2019-09-27 09:51:18 ~27 sec ios 📄log
a1cee11 #4 2019-09-27 09:51:20 ~26 sec windows 📄log
a1cee11 #4 2019-09-27 09:51:29 ~37 sec linux 📄log
529fcf9 #5 2019-09-27 09:53:19 ~22 sec linux 📄log
529fcf9 #5 2019-09-27 09:53:20 ~22 sec macos 📄log
529fcf9 #5 2019-09-27 09:53:20 ~21 sec windows 📄log
529fcf9 #5 2019-09-27 09:53:22 ~26 sec ios 📄log
529fcf9 #5 2019-09-27 09:55:52 ~2 min android 📄log
529fcf9 #5 2019-09-27 09:56:10 ~3 min android-e2e 📄log
bec6eae #6 2019-09-28 10:00:54 ~11 min ios 📄log
✔️ bec6eae #6 2019-09-28 10:05:03 ~15 min macos 📦dmg
bec6eae #6 2019-09-28 10:12:37 ~22 min windows 📄log
✔️ bec6eae #6 2019-09-28 10:19:47 ~30 min linux 📦App
bec6eae #6 2019-09-28 10:20:23 ~30 min android-e2e 📄log
bec6eae #6 2019-09-28 10:21:29 ~31 min android 📄log
e22ea21 #7 2019-09-30 10:40:40 ~8 min ios 📄log
✔️ e22ea21 #7 2019-09-30 10:50:38 ~18 min macos 📦dmg
e22ea21 #7 2019-09-30 10:53:45 ~21 min windows 📄log
✔️ e22ea21 #7 2019-09-30 10:56:12 ~23 min linux 📦App
e22ea21 #7 2019-09-30 11:00:59 ~28 min android-e2e 📄log
e22ea21 #7 2019-09-30 11:00:59 ~28 min android 📄log
✔️ 8f2e233 #8 2019-09-30 12:53:37 ~16 min ios 📦ipa 📲
✔️ 8f2e233 #8 2019-09-30 12:58:36 ~21 min macos 📦dmg
8f2e233 #8 2019-09-30 12:58:53 ~21 min windows 📄log
✔️ 8f2e233 #8 2019-09-30 13:04:08 ~27 min linux 📦App
8f2e233 #8 2019-09-30 13:08:38 ~31 min android 📄log
✔️ 490c8b1 #9 2019-09-30 13:31:04 ~11 min ios 📦ipa 📲
✔️ 490c8b1 #9 2019-09-30 13:33:12 ~13 min macos 📦dmg
490c8b1 #9 2019-09-30 13:33:29 ~13 min windows 📄log
✔️ 490c8b1 #9 2019-09-30 13:36:03 ~16 min android 📦apk 📲
✔️ 490c8b1 #9 2019-09-30 13:37:07 ~17 min android-e2e 📦apk 📲
✔️ 490c8b1 #9 2019-09-30 13:38:43 ~18 min linux 📦App
0441f16 #10 2019-10-01 15:21:39 ~3 min windows 📄log
0441f16 #10 2019-10-01 15:22:39 ~4 min macos 📄log
0441f16 #10 2019-10-01 15:24:10 ~5 min linux 📄log
✔️ 0441f16 #10 2019-10-01 15:29:56 ~11 min ios 📦ipa 📲
✔️ 0441f16 #10 2019-10-01 15:38:54 ~20 min android 📦apk 📲
✔️ 0441f16 #10 2019-10-01 15:39:08 ~20 min android-e2e 📦apk 📲
517aec3 #11 2019-10-01 19:31:54 ~2 min macos 📄log
517aec3 #11 2019-10-01 19:32:00 ~3 min windows 📄log
517aec3 #11 2019-10-01 19:32:53 ~3 min linux 📄log
✔️ 517aec3 #11 2019-10-01 19:39:06 ~10 min ios 📦ipa 📲
✔️ 517aec3 #11 2019-10-01 19:41:59 ~13 min android 📦apk 📲
✔️ 517aec3 #11 2019-10-01 19:42:21 ~13 min android-e2e 📦apk 📲
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 2a56386 #12 2019-10-02 16:56:25 ~10 min ios 📦ipa 📲
✔️ 2a56386 #12 2019-10-02 16:57:39 ~11 min macos 📦dmg
✔️ 2a56386 #12 2019-10-02 16:59:38 ~13 min windows 📦exe
✔️ 2a56386 #12 2019-10-02 17:00:52 ~14 min android-e2e 📦apk 📲
✔️ 2a56386 #12 2019-10-02 17:01:58 ~15 min android 📦apk 📲
✔️ 2a56386 #12 2019-10-02 17:02:30 ~16 min linux 📦App
✔️ 6e974bc #15 2019-10-05 09:40:40 ~12 min windows 📦exe
✔️ 6e974bc #15 2019-10-05 09:40:48 ~12 min ios 📦ipa 📲
✔️ 6e974bc #15 2019-10-05 09:41:38 ~13 min macos 📦dmg
✔️ 6e974bc #15 2019-10-05 09:44:22 ~16 min linux 📦App
✔️ 6e974bc #15 2019-10-05 09:47:57 ~19 min android 📦apk 📲
✔️ 6e974bc #15 2019-10-05 09:50:55 ~22 min android-e2e 📦apk 📲
@status-github-bot status-github-bot bot moved this from REVIEW to CONTRIBUTOR in Pipeline for QA Sep 27, 2019
@jakubgs jakubgs mentioned this pull request Sep 28, 2019
@jakubgs

This comment has been minimized.

Copy link
Member

jakubgs commented Sep 28, 2019

I did a separate PR just for Go upgrade to not compound the issues: #9065

@oskarth

This comment has been minimized.

Copy link
Member

oskarth commented Oct 1, 2019

@adambabik How will this play with the ongoing security audit? That's not quite clear to me. If this gets in later, how does that impact compatibility? Is there a related spec PR?

cc @rachelhamlin FYI

@oskarth

This comment has been minimized.

Copy link
Member

oskarth commented Oct 1, 2019

and @corpetty

@PombeirP

This comment has been minimized.

Copy link
Member

PombeirP commented Oct 1, 2019

@adambabik it looks like the build fails on Windows because of gosigar:

# github.com/status-im/status-go/vendor/github.com/elastic/gosigar/sys/windows
../vendor/github.com/elastic/gosigar/sys/windows/privileges.go:244:9: assignment mismatch: 2 variables but "github.com/status-im/status-go/vendor/golang.org/x/sys/windows".GetCurrentProcess returns 1 values

I already had issues with this library once and had to patch and make an upstream PR. Is it required for the upgrade?

@PombeirP

This comment has been minimized.

Copy link
Member

PombeirP commented Oct 1, 2019

@adambabik this commit seems relevant to the failing Windows build: golang/sys@855e68c

It looks like for a while, GetCurrentProcess was changed to return only one argument, before being reverted.

@PombeirP

This comment has been minimized.

Copy link
Member

PombeirP commented Oct 1, 2019

@PombeirP

This comment has been minimized.

Copy link
Member

PombeirP commented Oct 1, 2019

Fixed the Windows build by upgrading golang.org/x/sys and golang.org/x/crypto on status-go. Not sure if you agree with the approach for the short term @adambabik. I'm not sure if there's anything we can do in https://github.com/elastic/gosigar, because I don't see a go.mod file there.

@adambabik

This comment has been minimized.

Copy link
Member Author

adambabik commented Oct 1, 2019

@oskarth what kind of spec change do you have on mind? Does our audit verify go-ethereum and Whisper libraries as well? This is nothing more than just a library update.

@adambabik

This comment has been minimized.

Copy link
Member Author

adambabik commented Oct 1, 2019

Good catch @PombeirP! I merge the new changes from develop because the build failed due to some ClojureScript errors.

@adambabik

This comment has been minimized.

Copy link
Member Author

adambabik commented Oct 1, 2019

The same :/

[2019-10-01T19:30:19.636Z] Compiling ["index.desktop.js" "status-modules/cljs/i18n-raw.js" "status-modules/cljs/network-raw.js"] from ["components/src" "react-native/src/cljsjs" "react-native/src/desktop" "src" "env/prod" "prod"]...
[2019-10-01T19:31:15.861Z] WARNING: Use of undeclared Var status-im.ui.screens.desktop.main.chat.views/incoming-name at line 69 /home/jenkins/workspace/status-react_prs_linux_PR-9056/src/status_im/ui/screens/desktop/main/chat/views.cljs
[2019-10-01T19:31:15.861Z] Error encountered performing task 'cljsbuild' with profile(s): 'prod'
@jakubgs

This comment has been minimized.

Copy link
Member

jakubgs commented Oct 1, 2019

Yeah, this issue was introduced by 42961f3 and is fixed in #8630.

@jakubgs

This comment has been minimized.

Copy link
Member

jakubgs commented Oct 2, 2019

Since #8630 has been merged rebasing should fix the desktop build failures.

@adambabik adambabik marked this pull request as ready for review Oct 2, 2019
@adambabik adambabik requested a review from status-im/status-core as a code owner Oct 2, 2019
@auto-assign auto-assign bot removed the request for review from status-im/status-core Oct 2, 2019
@oskarth

This comment has been minimized.

Copy link
Member

oskarth commented Oct 3, 2019

@adambabik Perhaps this has been done already in a previous PR (says develop but not clear whether that is on 1.9.x to me), but for geth 1.9 there are major changes. See https://blog.ethereum.org/2019/07/10/geth-v1-9-0/ and specifically this:

The PoW calculation in Whisper v6 was incompatible with Parity due to not fully adhering to the spec. This was fixed, but it also means that Whisped v6 shipped with Geth v1.9.0 is incompatible with previous versions.

@oskarth

This comment has been minimized.

Copy link
Member

oskarth commented Oct 3, 2019

Looking at develop commit hash 40e66e6186164a422e432fd754b8f86b958014f3 I see status-im/status-go@40e66e6#diff-37aff102a57d3d7b797f152915a6dc16 and https://github.com/status-im/status-go/blob/40e66e6186164a422e432fd754b8f86b958014f3/go.mod#L36 which seems to suggest the previous version was running under 1.8.x. Am I missing something here?

It'd be good to have clarity on this PoW change w.r.t v1 and the security audit, as this was initially raised a month ago (and before that as well), in e.g. protocol call.

@status-github-bot status-github-bot bot moved this from CONTRIBUTOR to E2E Tests in Pipeline for QA Oct 3, 2019
@statustestbot

This comment has been minimized.

Copy link

statustestbot commented Oct 3, 2019

98% of end-end tests have passed

Total executed tests: 47
Failed tests: 1
Passed tests: 46

Failed tests (1)

Click to expand
1. test_logcat_send_transaction_from_daap

Device 1: Type 'status-im.github.io/dapp' to EnterUrlEditbox
Device 1: Tap 'Confirm' on native keyboard

Device 1: 'AssetsButton' is not found on the screen

Device sessions

Passed tests (46)

Click to expand
1. test_block_user_from_public_chat
Device sessions

2. test_filters_from_daap
Device sessions

3. test_copy_and_paste_messages
Device sessions

4. test_send_transaction_from_daap
Device sessions

5. test_deploy_contract_from_daap
Device sessions

6. test_open_transaction_on_etherscan
Device sessions

7. test_public_chat_messaging
Device sessions

8. test_long_press_to_delete_1_1_chat
Device sessions

9. test_password_in_logcat_sign_in
Device sessions

10. test_text_message_1_1_chat
Device sessions

11. test_add_to_contacts
Device sessions

12. test_sign_typed_message
Device sessions

13. test_unread_messages_counter_1_1_chat
Device sessions

14. test_ens_in_public_chat
Device sessions

15. test_send_message_in_group_chat
Device sessions

16. test_logcat_send_transaction_from_wallet
Device sessions

17. test_send_token_with_7_decimals
Device sessions

18. test_offline_messaging_1_1_chat
Device sessions

19. test_modify_transaction_fee_values
Device sessions

20. test_send_eth_from_wallet_to_address
Device sessions

21. test_add_account_to_multiaccount_instance
Device sessions

22. test_manage_assets
Device sessions

23. test_long_press_to_delete_public_chat
Device sessions

24. test_send_emoji
Device sessions

25. test_search_chat_on_home
Device sessions

26. test_logcat_recovering_account
Device sessions

27. test_can_add_existing_ens
Device sessions

28. test_messaging_in_different_networks
Device sessions

29. test_logcat_backup_recovery_phrase
Device sessions

30. test_logcat_sign_message_from_daap
Device sessions

31. test_switch_users_and_add_new_account
Device sessions

32. test_send_stt_from_wallet
Device sessions

33. test_login_with_new_account
Device sessions

34. test_start_chat_with_ens
Device sessions

35. test_add_contact_from_public_chat
Device sessions

36. test_send_two_transactions_one_after_another_in_dapp
Device sessions

37. test_password_in_logcat_creating_account
Device sessions

38. test_backup_recovery_phrase
Device sessions

39. test_offline_status
Device sessions

40. test_open_google_com_via_open_dapp
Device sessions

41. test_unread_messages_counter_public_chat
Device sessions

42. test_sign_message_from_daap
Device sessions

43. test_user_can_remove_profile_picture
Device sessions

44. test_share_contact_code_and_wallet_address
Device sessions

45. test_refresh_button_browsing_app_webview
Device sessions

46. test_backup_recovery_phrase_warning_from_wallet
Device sessions

@PombeirP PombeirP self-requested a review Oct 3, 2019
@statustestbot

This comment has been minimized.

Copy link

statustestbot commented Oct 3, 2019

100% of end-end tests have passed

Total executed tests: 1
Failed tests: 0
Passed tests: 1

Passed tests (1)

Click to expand
1. test_logcat_send_transaction_from_daap
Device sessions

@corpetty

This comment has been minimized.

Copy link
Contributor

corpetty commented Oct 3, 2019

I would also like some clarity here w.r.t. PoW change and us addressing it. I was under the assumption that we fixed this months ago, is that right?

@adambabik adambabik moved this from E2E Tests to TO TEST in Pipeline for QA Oct 3, 2019
@adambabik

This comment has been minimized.

Copy link
Member Author

adambabik commented Oct 3, 2019

@corpetty no, it's only now with this PR. However, we are using extremely low PoW anyway so we actually do not use PoW protection at all.

@oskarth

This comment has been minimized.

Copy link
Member

oskarth commented Oct 3, 2019

@adambabik Regardless of our low PoW, that doesn't answer the underlying issue of compatibility. They call it out explicitly, and we should be explicit about which version of PoW we support and why. It doesn't make any sense to ship an old version in v1 and then do another release an incompatible version. From what I can tell here, this seems like a blocker for security audit and something we have to sort out asap, so there's clarity in (a) code (b) spec (c) commit hash for ToB

@oskarth

This comment has been minimized.

Copy link
Member

oskarth commented Oct 3, 2019

@oskarth

This comment has been minimized.

Copy link
Member

oskarth commented Oct 3, 2019

Also cc @kdeme as he identified the spec mismatch in the first place here ethereum/go-ethereum#18070 and fixed here ethereum/go-ethereum#19753

@kdeme

This comment has been minimized.

Copy link
Contributor

kdeme commented Oct 3, 2019

Be careful, even though you are using extremely low PoW, this can still cause a lot of messages to be dropped.

PoW calculation is: Pow = 2**leading_zeros(pow_hash) / (size * ttl)

So practically, you can do the worst case calculation easily. Due to a completely different pow_hash you will have at worst case 0 leading zeros.
So e.g. if you set ttl to 10 and message size is e.g. 256 (I have no idea how much overhead Status messages have), and you are already a factor lower than your PoW requirement.

So yeah, If you already have to break compatibility with v1, and it is still possible to do so, then I suggest you try to get this in there still.
Else the options are either PoW at practically 0 or implement both calculations at receiving end. Neither are nice.

@andremedeiros

This comment has been minimized.

Copy link
Member

andremedeiros commented Oct 3, 2019

@Serhy @churik could one of you do a sanity check on this build?

@adambabik

This comment has been minimized.

Copy link
Member Author

adambabik commented Oct 3, 2019

@oskarth

It doesn't make any sense to ship an old version in v1 and then do another release an incompatible version. From what I can tell here, this seems like a blocker for security audit and something we have to sort out asap, so there's clarity in (a) code (b) spec (c) commit hash for ToB

I agree 100%. We decided on breaking compatibility for V1 so this fix of PoW calculation for Whisper V6 should also be included in V1 release.

Regarding clarity:

  • code -- this PR is to test that status-go with geth 1.9.5 works fine with the Android, iOS and Desktop versions. We had some troubles with the CI but all checks are green, e2e tests pass so I guess the only thing left is some manual testing. In status-go, it's status-im/status-go#1617 where I bumped geth to 1.9.5.
  • spec -- yes, we need a PR saying that we support Whisper spec and use Whisper V6 from geth 1.9.5. Do we have any PR related to compatibility changes in V1?
  • Security Audit -- not sure how flexible they are and whether they will look into go-ethereum and Whisper codebases. status-im/status-go#1617 looks massive due to vendor/ but other changes are just in test files + geth and Whisper packages bumps.

@kdeme thanks for clarification on this. Yeah, in my opinion, we definitely should try to squeeze this in. We practically have PoW close to 0 (0.002 to be precise; we need to confirm whether it's both for incoming and outgoing message) but like you explained it might still lead to some messages being dropped. Implementation of any ugly solutions is not an option in my opinion because we haven't released V1 so there won't be a better chance to do it nice.

@kdeme

This comment has been minimized.

Copy link
Contributor

kdeme commented Oct 3, 2019

@adambabik re dropping messages based on PoW @ incoming and outgoing.

In the Nimbus implementation following drops at a node can occur:

  1. When an incoming message its PoW is lower than the PoW requirement of the node, the message will be dropped.
  2. When an outgoing message its PoW is lower than the PoW requirement of the node. I don't think this is a literal requirement of the EIP but we do it not to leak any information that a certain message originates from that node.
  3. When an outgoing message its PoW is lower than the PoW requirement of the peer it is being send to by the node, this message will not be send (it's not really dropped). A whisper node should know the PoW requirements of all its peers, this is passed at handshake. Practically, between Status nodes, this will be the same PoW requirement I guess.

On top of that you then have filters which can also filter (not drop) based on PoW and other parameters.

It's been a while that I looked at the geth implementation, but iirc it is the same situation there. After all, functionally wise, inspiration was drawn from that implementation.

@Serhy Serhy moved this from TO TEST to IN TESTING in Pipeline for QA Oct 4, 2019
@Serhy Serhy self-assigned this Oct 4, 2019
@Serhy

This comment has been minimized.

Copy link
Member

Serhy commented Oct 4, 2019

No regressions found here when tested PR build between iOS 12 and Android 8.1

  • Can create/recover account and attach userpic
  • Can send/receive (from offline) messages in 1-1/public/group chats in different networks (and same network as well)
  • Can send transactions from wallet
  • Can block/unblock user (and messaging stop receiving).
  • Can transact in Dapp (buy and gift cryptokitty)
  • Device syncing works fine between iOS and Android
    Ready for merge, @adambabik
@Serhy Serhy added the Tested - OK label Oct 4, 2019
@Serhy Serhy moved this from IN TESTING to MERGE in Pipeline for QA Oct 4, 2019
@jakubgs

This comment has been minimized.

Copy link
Member

jakubgs commented Oct 5, 2019

After status-go#1617 was merged the commit in develop that adds 1.9.5 is 26880b83 and not 40e66e61 used in this PR.

I'd recommend tagging the develop commit with a 0.34.0-beta.0 version tag, and using the tag with ./scripts/update-status-go.sh script to update status-go-version.json properly.

@adambabik

This comment has been minimized.

Copy link
Member Author

adambabik commented Oct 5, 2019

@adambabik adambabik force-pushed the upgrade-geth-1.9.5 branch from fbb9845 to 6e974bc Oct 5, 2019
@jakubgs
jakubgs approved these changes Oct 5, 2019
@adambabik

This comment has been minimized.

Copy link
Member Author

adambabik commented Oct 5, 2019

How can I merge it after all checks get green?

@jakubgs

This comment has been minimized.

Copy link
Member

jakubgs commented Oct 5, 2019

With the ./scripts/merge-pr.sh ${PR_ID} script.

Signed-off-by: Adam Babik <a.babik@designfortress.com>
@adambabik adambabik force-pushed the upgrade-geth-1.9.5 branch from 6e974bc to df87496 Oct 5, 2019
@adambabik adambabik merged commit df87496 into develop Oct 5, 2019
6 of 10 checks passed
6 of 10 checks passed
security/snyk - android/build.gradle (Status-im) Test in progress
security/snyk - desktop/js_files/package.json (Status-im) Test in progress
security/snyk - mobile/js_files/package.json (Status-im) Test in progress
security/snyk - test/appium/requirements.txt (Status-im) Test in progress
packages-check-bot No changes to dependencies
Details
GPG All commits have a verified GPG signature
WIP Ready for review
Details
security/snyk - fastlane/Gemfile.lock (Status-im) No manifest changes detected
security/snyk - modules/react-native-desktop-linking/package.json (Status-im) No manifest changes detected
security/snyk - modules/react-native-status/package.json (Status-im) No manifest changes detected
Pipeline for QA automation moved this from MERGE to DONE Oct 5, 2019
@delete-merged-branch delete-merged-branch bot deleted the upgrade-geth-1.9.5 branch Oct 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.