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_: restore node config #6270

Open
wants to merge 1 commit into
base: fix/v1_upgrading
Choose a base branch
from

Conversation

qfrank
Copy link
Contributor

@qfrank qfrank commented Jan 20, 2025

Caution

This PR is not intended to be merged

This is a continue work based on PR, the fix could fix the error:

ERROR[12-23|23:54:56.136|github.com/status-im/status-go/mobile/status.go:474]     loginAccount failed                      error="failed to start node: Key: 'NodeConfig.NetworkID' Error:Field validation for 'NetworkID' failed on the 'required' tag\nKey: 'NodeConfig.LogLevel' Error:Field validation for 'LogLevel' failed on the 'eq=ERROR|eq=WARN|eq=INFO|eq=DEBUG|eq=TRACE' tag"

the solution is:

  • pass relate node config params that only mobile frontend configured/knows when invoke loginAccount
  • when loginAccount get invoked, check if there is one bad(e.g. networkid is 0 and networkid is not 0 in settings.node_config) node config record on status-go side, if yes, create a default node config for the user, we also need take care of existing node config values in settings.node_config when combining the default config and existing one

however, this solution will make loginAccount looks not beautiful anymore.

following is the opinion from @ilmotta

We could try to create a special build release build for this user without merging PRs, just having the status-go and mobile branches would be enough. That would make our lives easier because we'd be able to skip QA and we wouldn't need to worry too much about loginAccount getting ugly.
But if the problem is solved, then a second step could be to make the PRs proper and see if we can get them merged. Probably then, the adaptions/fixes you found to work with 3esmit would help other users who never reported anything to us.

Relate mobile PR
Relate comment

@status-im-auto
Copy link
Member

status-im-auto commented Jan 20, 2025

Jenkins Builds

Click to see older builds (11)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 77e75a4 #1 2025-01-20 01:16:51 ~3 min macos 📦zip
✔️ 77e75a4 #1 2025-01-20 01:17:42 ~4 min windows 📦zip
✔️ 77e75a4 #1 2025-01-20 01:17:55 ~4 min linux 📦zip
✔️ 77e75a4 #1 2025-01-20 01:18:45 ~5 min macos 📦zip
✖️ 77e75a4 #1 2025-01-20 06:51:26 ~5 hr 38 min tests 📄log
✔️ 77e75a4 #1 2025-01-20 06:52:47 ~5 hr 39 min android 📦aar
✔️ 77e75a4 #1 2025-01-20 06:53:37 ~5 hr 40 min tests-rpc 📄log
✔️ 8a439b4 #2 2025-01-20 14:24:19 ~2 min windows 📦zip
✔️ 8a439b4 #2 2025-01-20 14:24:47 ~3 min macos 📦zip
✔️ 8a439b4 #2 2025-01-20 14:25:54 ~4 min linux 📦zip
✔️ 8a439b4 #2 2025-01-20 14:26:53 ~5 min macos 📦zip
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ b0b61de #3 2025-01-21 11:06:20 ~3 min windows 📦zip
✔️ b0b61de #2 2025-01-21 11:06:52 ~3 min ios 📦zip
✔️ b0b61de #3 2025-01-21 11:08:23 ~5 min macos 📦zip
✔️ b0b61de #3 2025-01-21 11:08:30 ~5 min linux 📦zip
✔️ b0b61de #3 2025-01-21 11:08:39 ~5 min android 📦aar
✔️ b0b61de #3 2025-01-21 11:08:44 ~5 min macos 📦zip
✔️ b0b61de #3 2025-01-21 11:09:48 ~6 min tests-rpc 📄log
✔️ b0b61de #3 2025-01-21 11:34:18 ~31 min tests 📄log
✔️ d7f725f #4 2025-01-21 11:09:24 ~2 min windows 📦zip
✔️ d7f725f #3 2025-01-21 11:11:24 ~4 min ios 📦zip
✔️ d7f725f #4 2025-01-21 11:13:34 ~5 min macos 📦zip
✔️ d7f725f #4 2025-01-21 11:13:48 ~5 min linux 📦zip
✔️ d7f725f #4 2025-01-21 11:14:16 ~5 min macos 📦zip
✔️ d7f725f #4 2025-01-21 11:15:18 ~6 min android 📦aar
✔️ d7f725f #4 2025-01-21 11:16:15 ~6 min tests-rpc 📄log
✔️ d7f725f #4 2025-01-21 12:06:03 ~31 min tests 📄log

@qfrank qfrank force-pushed the fix/node_config_migration branch from 77e75a4 to 8a439b4 Compare January 20, 2025 14:21
@jakubgs
Copy link
Member

jakubgs commented Jan 21, 2025

You will need to rebase your branch on develop in order for the CI job to run using new Nix interpreter:

@qfrank qfrank force-pushed the fix/node_config_migration branch from 8a439b4 to b0b61de Compare January 21, 2025 11:02
@qfrank qfrank requested a review from a team as a code owner January 21, 2025 11:02
@qfrank qfrank force-pushed the fix/node_config_migration branch from b0b61de to d7f725f Compare January 21, 2025 11:06
Copy link

codecov bot commented Jan 21, 2025

Codecov Report

Attention: Patch coverage is 80.00000% with 13 lines in your changes missing coverage. Please review.

Project coverage is 61.77%. Comparing base (45d97be) to head (d7f725f).

Files with missing lines Patch % Lines
api/geth_backend.go 80.00% 7 Missing and 6 partials ⚠️
Additional details and impacted files
@@                 Coverage Diff                  @@
##           fix/v1_upgrading    #6270      +/-   ##
====================================================
- Coverage             61.79%   61.77%   -0.02%     
====================================================
  Files                   845      845              
  Lines                111336   111399      +63     
====================================================
+ Hits                  68797    68819      +22     
- Misses                34572    34601      +29     
- Partials               7967     7979      +12     
Flag Coverage Δ
functional 21.49% <0.00%> (+0.01%) ⬆️
unit 60.32% <80.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
api/defaults.go 81.40% <ø> (-0.16%) ⬇️
protocol/requests/login.go 57.14% <ø> (ø)
api/geth_backend.go 55.23% <80.00%> (+0.99%) ⬆️

... and 30 files with indirect coverage changes

@yakimant
Copy link
Member

hm, not sure I an help with review

@qfrank qfrank removed the request for review from a team January 22, 2025 09:02
Copy link
Collaborator

@igor-sirotin igor-sirotin left a comment

Choose a reason for hiding this comment

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

Since it's for a single user, and it won't be merged, I don't think we needs code review.
If it works for the user - great, if it doesn't - then there are issues in the fix 😄

I have added do not merge labels and a warning in description.
@qfrank, perhaps makes sense to convert this PR to draft as well?

--

But if the problem is solved, then a second step could be to make the PRs proper and see if we can get them merged. Probably then, the adaptions/fixes you found to work with 3esmit would help other users who never reported anything to us.

@ilmotta, do you think we need to persuade such little issues for few users?
This could even be done manually amending the DB 🤷‍♂️

Comment on lines +601 to +604
if currentConf.NetworkID == 0 &&
currentConf.KeyStoreDir == "" &&
currentConf.DataDir == "" &&
currentConf.NodeKey == "" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

So the problem is that the node_config is empty in the database, but... why is this a problem? Can't we just use the default values, without writing them to the database?

Perhaps this is also related (and could be mitigated by) #5597. It seems that loadNodeConfig actually gives priority to DB values rather than the values passed with CreateAccount/LoginAccount/.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants