search

celo-org / celo-blockchain

Official repository for the golang Celo Blockchain
https://celo.org
GNU Lesser General Public License v3.0
560 stars 198 forks source link

fix: nil curr var #2152

Closed kehiy closed 1 year ago

kehiy commented 1 year ago

Description

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

Related issues

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 75.00% and project coverage change: -0.02 :warning:

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

Additional details and impacted files ```diff @@ 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](https://app.codecov.io/gh/celo-org/celo-blockchain/pull/2152?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=celo-org) | Coverage Δ | | |---|---|---| | [miner/block.go](https://app.codecov.io/gh/celo-org/celo-blockchain/pull/2152?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=celo-org#diff-bWluZXIvYmxvY2suZ28=) | `52.52% <75.00%> (-0.02%)` | :arrow_down: | ... and [22 files with indirect coverage changes](https://app.codecov.io/gh/celo-org/celo-blockchain/pull/2152/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=celo-org)

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

karlb commented 1 year ago

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

kehiy commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.