Remove exchange rate calculation for non CELO denominated txs in state_transition #2299
Conversation
|
Coverage from tests in coverage: 51.2% of statements across all listed packagescoverage: 63.6% of statements in consensus/istanbul coverage: 43.2% of statements in consensus/istanbul/announce coverage: 56.0% of statements in consensus/istanbul/backend coverage: 0.0% of statements in consensus/istanbul/backend/backendtest coverage: 24.3% of statements in consensus/istanbul/backend/internal/replica coverage: 65.9% of statements in consensus/istanbul/core coverage: 50.0% of statements in consensus/istanbul/db coverage: 0.0% of statements in consensus/istanbul/proxy coverage: 64.2% of statements in consensus/istanbul/uptime coverage: 51.8% of statements in consensus/istanbul/validator coverage: 79.2% of statements in consensus/istanbul/validator/random |
|
|
hbandura
force-pushed
the
hbandura/fix_st
branch
from
2f1e171
to
876d6c8
Compare
| var feeCurrency *common.Address = msg.FeeCurrency() | ||
| if msg.MaxFeeInFeeCurrency() != nil { | ||
| // Celo denominated tx | ||
| feeCurrency = nil | ||
| gasPriceMinimum = sysCtx.GetGasPriceMinimum(nil) | ||
| } else { | ||
| gasPriceMinimum = sysCtx.GetGasPriceMinimum(msg.FeeCurrency()) | ||
| } | ||
| gasPriceMinimum = sysCtx.GetGasPriceMinimum(feeCurrency) |
There was a problem hiding this comment.
Why is this necessary? This seems to be functionally unchanged by the change. I.E sysCtx.GetGasPriceMinimum is called once with either nil or a fee currency.
There was a problem hiding this comment.
Correct, it is unchanged. But since the bad block I wanted to make sure everything was like before, so while this is a diff here, it makes it closer than what we had before the cip66 change in state_transition. The line 224 is exactly what we had, I wanted to preserve that line, even if, functionally, this block is the same
| var currencyFee *big.Int | ||
| if st.msg.MaxFeeInFeeCurrency() == nil { | ||
| // Normal feeCurrency tx | ||
| currencyFee = effectiveFee | ||
| if st.msg.MaxFeeInFeeCurrency() != nil { | ||
| st.erc20FeeDebited = denominatedCurrencyRate.FromBase(effectiveFee) | ||
| } else { | ||
| // Celo denominated tx | ||
| currencyFee = feeCurrencyRate.FromBase(effectiveFee) | ||
| st.erc20FeeDebited = effectiveFee | ||
| } | ||
| st.erc20FeeDebited = currencyFee | ||
| return erc20gas.DebitFees(st.evm, from, currencyFee, feeCurrency) |
There was a problem hiding this comment.
This block also seems to be functionally unchanged
There was a problem hiding this comment.
Just removing an unnecessary local variable.
| denominatedCurrencyRate, err = currency.GetExchangeRate(st.vmRunner, st.msg.FeeCurrency()) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } |
There was a problem hiding this comment.
So this is the actual fix right? Only setting denominatedCurrencyRate for celo denominated transactions.
There was a problem hiding this comment.
Correct. The rest of the changes are: variable renames for (in my opinion) more explicit names, and changing a few ifblocks to be exactly like they were before the previous change (a mini revert if you will).
|
A full sync is currently running, may take a few days. When it finishes we can merge this |
hbandura
force-pushed
the
hbandura/fix_st
branch
from
876d6c8
to
5289d6d
Compare
Fix state_transition producing invalid gas used values for pre-H fork transactions by preventing from taking the exchange rate of fee currencies meant for only CELO denominated txs.
Rename some variables to be more specific about them applying only to this tx type.