-
Notifications
You must be signed in to change notification settings - Fork 76
[ETCM-927] enhance BlockchainTests/ValidBlocks/bcMultiChainTest/Chain… #1021
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
Changes from all commits
9246833
8eb7258
6a44fa5
92a9f30
cdbed23
e8a425f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -136,6 +136,10 @@ class TestService( | |
def setChainParams(request: SetChainParamsRequest): ServiceResponse[SetChainParamsResponse] = { | ||
currentConfig = buildNewConfig(request.chainParams.blockchainParams) | ||
|
||
// clear ledger's cache on test start | ||
// setChainParams is expected to be the first remote call for each test | ||
testModeComponentsProvider.clearState() | ||
|
||
val genesisData = GenesisData( | ||
nonce = request.chainParams.genesis.nonce, | ||
mixHash = Some(request.chainParams.genesis.mixHash), | ||
|
@@ -158,6 +162,9 @@ class TestService( | |
|
||
// remove current genesis (Try because it may not exist) | ||
Try(blockchain.removeBlock(blockchain.genesisHeader.hash, withState = false)) | ||
// TODO clear the storage ? When relaunching some tests on the same running test mantis client, | ||
// we end up with duplicate blocks because they are still present in the storage layer | ||
// for example: bcMultiChainTest/ChainAtoChainB_BlockHash_Istanbul | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about when There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do you mean put the cleaning into the setChainParams call, or into the setChainParams caller ? |
||
|
||
// load the new genesis | ||
val genesisDataLoader = new GenesisDataLoader(blockchain, blockchainReader, stateStorage, currentConfig) | ||
|
@@ -276,15 +283,20 @@ class TestService( | |
testModeComponentsProvider | ||
.blockImport(currentConfig, preimageCache, sealEngine) | ||
.importBlock(value) | ||
.flatMap(handleResult) | ||
.flatMap(handleResult(value)) | ||
} | ||
} | ||
|
||
private def handleResult(blockImportResult: BlockImportResult): ServiceResponse[ImportRawBlockResponse] = { | ||
private def handleResult( | ||
block: Block | ||
)(blockImportResult: BlockImportResult): ServiceResponse[ImportRawBlockResponse] = { | ||
blockImportResult match { | ||
case BlockImportedToTop(blockImportData) => | ||
val blockHash = s"0x${ByteStringUtils.hash2string(blockImportData.head.block.header.hash)}" | ||
ImportRawBlockResponse(blockHash).rightNow | ||
case BlockEnqueued | ChainReorganised(_, _, _) => | ||
val blockHash = s"0x${ByteStringUtils.hash2string(block.hash)}" | ||
ImportRawBlockResponse(blockHash).rightNow | ||
case e => | ||
log.warn("Block import failed with {}", e) | ||
Task.now(Left(JsonRpcError(-1, "block validation failed!", None))) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,18 +6,22 @@ import io.iohk.ethereum.jsonrpc.{ | |
BaseBlockResponse, | ||
BaseTransactionResponse, | ||
EthBlocksService, | ||
JsonRpcError, | ||
ServiceResponse, | ||
TransactionData | ||
} | ||
import io.iohk.ethereum.utils.Logger | ||
import io.iohk.ethereum.utils.ByteStringUtils._ | ||
import akka.util.ByteString | ||
import io.iohk.ethereum.consensus.Consensus | ||
import io.iohk.ethereum.ledger.BlockQueue | ||
|
||
class TestEthBlockServiceWrapper( | ||
blockchain: Blockchain, | ||
blockchainReader: BlockchainReader, | ||
consensus: Consensus | ||
) extends EthBlocksService(blockchain, blockchainReader, consensus) | ||
consensus: Consensus, | ||
blockQueue: BlockQueue | ||
) extends EthBlocksService(blockchain, blockchainReader, consensus, blockQueue) | ||
with Logger { | ||
|
||
/** | ||
|
@@ -31,10 +35,29 @@ class TestEthBlockServiceWrapper( | |
): ServiceResponse[EthBlocksService.BlockByBlockHashResponse] = super | ||
.getByBlockHash(request) | ||
.map( | ||
_.map(blockByBlockResponse => { | ||
val fullBlock = blockchainReader.getBlockByNumber(blockByBlockResponse.blockResponse.get.number).get | ||
BlockByBlockHashResponse(blockByBlockResponse.blockResponse.map(response => toEthResponse(fullBlock, response))) | ||
}) | ||
_.flatMap { | ||
|
||
case BlockByBlockHashResponse(None) => | ||
Left(JsonRpcError.LogicError(s"EthBlockService: unable to find block for hash ${request.blockHash.toHex}")) | ||
|
||
case BlockByBlockHashResponse(Some(baseBlockResponse)) if baseBlockResponse.hash.isEmpty => | ||
Left(JsonRpcError.LogicError(s"missing hash for block $baseBlockResponse")) | ||
|
||
case BlockByBlockHashResponse(Some(baseBlockResponse)) => | ||
val ethResponseOpt = for { | ||
hash <- baseBlockResponse.hash | ||
fullBlock <- blockchainReader.getBlockByHash(hash) orElse blockQueue.getBlockByHash(hash) | ||
} yield toEthResponse(fullBlock, baseBlockResponse) | ||
|
||
ethResponseOpt match { | ||
case None => | ||
Left( | ||
JsonRpcError.LogicError(s"Ledger: unable to find block for hash=${baseBlockResponse.hash.get.toHex}") | ||
) | ||
case Some(_) => | ||
Right(BlockByBlockHashResponse(ethResponseOpt)) | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a bit hard to follow, might be nicer as |
||
) | ||
|
||
/** | ||
|
@@ -122,9 +145,9 @@ final case class EthTransactionResponse( | |
gasPrice: BigInt, | ||
gas: BigInt, | ||
input: ByteString, | ||
r: ByteString, | ||
s: ByteString, | ||
v: ByteString | ||
r: BigInt, | ||
s: BigInt, | ||
v: BigInt | ||
) extends BaseTransactionResponse | ||
|
||
object EthTransactionResponse { | ||
|
@@ -149,8 +172,8 @@ object EthTransactionResponse { | |
gasPrice = stx.tx.gasPrice, | ||
gas = stx.tx.gasLimit, | ||
input = stx.tx.payload, | ||
r = UInt256(stx.signature.r).bytes, | ||
s = UInt256(stx.signature.s).bytes, | ||
v = ByteString(stx.signature.v) | ||
r = stx.signature.r, | ||
s = stx.signature.s, | ||
v = stx.signature.v | ||
) | ||
} |
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.
Are these two changes required by ETS? I wonder what would be a convenient way to refactor this with ledger gone...
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.
The underlying need was to be able to get a block by hash, no matter if it's on the canonical chain or an alternate one. The solution was to delegate the work to the ledger, which implements the getBlockByHash exactly as it (first look in the canonical blockchain, then into the BlockQueue). The getChainWeightByHash was added to match the getBlockByHash behaviour.
The feature (being able to query a block by its hash, no matter its location) is required by the ETS. After a successful insert, retesteth queries for the insert block by its hash to do some consistency check.
It is not restricted to the ledger, so if any other component provides this after the refactor, let's use the new one.