[evm] fix GetHash function #2998
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2998 +/- ##
=======================================
Coverage 61.54% 61.54%
=======================================
Files 192 192
Lines 21680 21688 +8
=======================================
+ Hits 13342 13347 +5
- Misses 6661 6665 +4
+ Partials 1677 1676 -1
Continue to review full report at Codecov.
|
dustinxie
force-pushed
the
check
branch
2 times, most recently
from
92ce94e
to
854fa62
Compare
| require.NoError(err) | ||
| acHash7, err := addOneBlock(contract, 7, zero, gasLimit, gasPrice, getBlockHash(6)) // equal to genesis block | ||
| require.NoError(err) | ||
|
|
There was a problem hiding this comment.
move into for loop to make the code more clean
|
Could we test |
yes, the test deploys a contract which end up calling |
|
yes, that's EVM internal implementation, it checks if the requested number is within last 256 blocks, if yes, call |
|
Could you check why is time fixed in L647 |
| hash, err := getBlockHash(stateDB.blockHeight - n) | ||
| if err != nil { | ||
| hash, err := getBlockHash(n) | ||
| if err == nil { |
There was a problem hiding this comment.
for clarity would rather keep it same as current code
There was a problem hiding this comment.
ok. Could you file a new issue to correct the style here?
There was a problem hiding this comment.
i don't think it's necessary? we have lots of if err == nil in the codebase
| return common.BytesToHash(hash[:]) | ||
| } | ||
| return common.Hash{} | ||
| } | ||
| } else { | ||
| case featureCtx.FixGetHashFnHeight: | ||
| getHashFn = func(n uint64) common.Hash { | ||
| hash, err := getBlockHash(stateDB.blockHeight - (n + 1)) | ||
| if err == nil { |
| } | ||
| for _, test := range tests { | ||
| r, err := dao.GetReceiptByActionHash(test.acHash, test.commitHeight) | ||
| h, err := addOneBlock(contract, nonce, zero, gasLimit, gasPrice, getBlockHash(int64(test.getHashHeight))) |
There was a problem hiding this comment.
the name getBlockHash is confusing. It is actually the data of the tx
There was a problem hiding this comment.
yes, it is the data of tx, but what the tx does is to getBlockHash, so i think it's fine?
There was a problem hiding this comment.
No. the name getBlockHash is expected to return a hash. But it returns []byte.
I'd prefer calldata
| {11, 1}, | ||
| {12, 4}, | ||
| {13, 0}, | ||
| {14, 100}, |
No description provided.