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: nil curr var #2152

Closed
wants to merge 1 commit into from
Closed

fix: nil curr var #2152

wants to merge 1 commit into from

Conversation

kehiy
Copy link
Contributor

@kehiy kehiy commented Jul 5, 2023

Description

I added an if statement to check the value of curr to make sure it doesn't panic.

Related issues

@codecov
Copy link

codecov bot commented Jul 5, 2023

Codecov Report

Patch coverage: 75.00% and project coverage change: -0.02 ⚠️

Comparison is base (a72fd91) 55.23% compared to head (986c11f) 55.22%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2152      +/-   ##
==========================================
- Coverage   55.23%   55.22%   -0.02%     
==========================================
  Files         675      675              
  Lines      113729   113731       +2     
==========================================
- Hits        62819    62808      -11     
- Misses      47066    47078      +12     
- Partials     3844     3845       +1     
Impacted Files Coverage Δ
miner/block.go 52.52% <75.00%> (-0.02%) ⬇️

... and 22 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@karlb
Copy link
Contributor

karlb commented Jul 5, 2023

The situation is still to unclear for me to trust that this change fixes the situation:

  • I still don't know how the panic in Seg fault / panic syncing mainnet on v1.7.4 #2134 really happened
  • We don't have a test to reproduce the issue or to test the nil case that is handled in this PR
  • I suspect that returning nil from toCeloFn will cause problems for the caller, as no nil was returned before. We also still lose the error returned from GetCurrency.

@kehiy
Copy link
Contributor Author

kehiy commented Jul 5, 2023

The situation is still too unclear for me to trust that this change fixes the situation:

  • I still don't know how the panic in Seg fault / panic syncing mainnet on v1.7.4 #2134 really happened
  • We don't have a test to reproduce the issue or to test the nil case that is handled in this PR
  • I suspect that returning nil from toCeloFn will cause problems for the caller, as no nil was returned before. We also still lose the error returned from GetCurrency.

when we return a nil, we can make sure that the full node was not panicking.

@karlb
Copy link
Contributor

karlb commented Jul 5, 2023

when we return a nil, we can make sure that the full node was not panicking.

Have you tested what happens when nil gets returned? Maybe it just panics higher up the stack?

If it does not panic, how does returning nil affect the transactions getting executed? Will transactions with a certain feeCurrency be free, will they revert, or does something else happen?

@kehiy
Copy link
Contributor Author

kehiy commented Jul 5, 2023

Have you tested what happens when nil gets returned? Maybe it just panics higher up the stack?

If it does not panic, how does returning nil affect the transactions getting executed? Will transactions with a certain fee currency be free, will they revert, or does something else happen?

when we return nil, if we have an error handling in higher layers it don,t panic or something else
but without error handling in higher layers we going to panic again or face with new issues.
this change just helps us to make sure the createConversionFunctions function will not panic and simply assign the issue to higher layers with error handling.

@kehiy kehiy closed this by deleting the head repository Jul 5, 2023
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.

Seg fault / panic syncing mainnet on v1.7.4
2 participants