-
Notifications
You must be signed in to change notification settings - Fork 78
[ETCM-301] Fix block preparation #782
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
Conversation
| // // this seems to discard failures, for better errors messages we might want to implement a different method (simulateBlock?) | ||
| // val result = blockPreparator.prepareBlock(block) | ||
| // Right(result.blockResult.receipts) | ||
| // FIXME DO WE NEED THAT? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could create task to delete those snappy tests? (I highly doubt anybody will ever use it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. I just created https://jira.iohk.io/browse/ETCM-349
| * | ||
| * @param block the block with transactions to run | ||
| */ | ||
| private[ledger] def executeBlockTransactions(block: Block): Either[BlockExecutionError, BlockResult] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe it would be possible to pass parent header here as param , to avoid this blockchain.getBlockHeaderByHash(block.header.parentHash ?
ntallar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from the below comments, LGTM!
Do you think there's any test that could be added for any of the two issues?
src/main/scala/io/iohk/ethereum/consensus/ethash/MockedMiner.scala
Outdated
Show resolved
Hide resolved
|
@ntallar I don't think that we are able to add some reliable test to check that. I can't imagine any |
ntallar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
.\\ //.
. \ \ / /.
.\ ,\ /` /,.-
-. \ /'/ / .
` - `-' \ -
'. /.\`
- .-
:`//.'
.`.'
.' BP
Description
stateRootfromOptiontoByteStringinWorldStateProxyto avoid mistakesprepareBlockmethod.This change should also solve the issue described in ETCM-329