Merged
Conversation
kapke
reviewed
Nov 18, 2020
Contributor
kapke
left a comment
kapke
left a comment
There was a problem hiding this comment.
Mostly minor comments. Overall looks good!
| private def newConfig[C <: AnyRef](c: C): FullConsensusConfig[C] = | ||
| FullConsensusConfig(consensusConfig, c) | ||
|
|
||
| //TODO refactor configs to avoid possibility of running mocked or restricted-ethash consensus on real network like ETC or Mordor |
| blockchainConfig = blockchainConfig, | ||
| consensusConfig = config.generic, | ||
| blockPreparator = blockPreparator, | ||
| difficultyCalculator, |
Contributor
There was a problem hiding this comment.
minor - use named parameters all the way down?
| blockchainConfig = blockchainConfig, | ||
| consensusConfig = config.generic, | ||
| blockPreparator = blockPreparator, | ||
| difficultyCalculator |
| pubKeyBytes <- sig.publicKey(headerHash) | ||
| } yield ByteString.fromArrayUnsafe(pubKeyBytes) | ||
|
|
||
| maybePubKey.fold(false)(pubKey => allowedMiners.contains(pubKey)) |
| def validateSignature(blockHeader: BlockHeader, allowedMiners: Set[ByteString]): Boolean = { | ||
| val signature = blockHeader.extraData.takeRight(ECDSASignature.EncodedLength) | ||
| val blockHeaderWithoutSig = | ||
| blockHeader.copy(extraData = blockHeader.extraData.dropRight(ECDSASignature.EncodedLength)) |
Contributor
There was a problem hiding this comment.
should these .copy calls in this file be methods on BlockHeader?
| with ObjectGenerators | ||
| with SecureRandomBuilder { | ||
| "RestrictedEthashSigner" should "sign and validate correct header" in { | ||
| forAll(blockHeaderGen, genKey(secureRandom)) { (header, key) => |
Contributor
There was a problem hiding this comment.
should similar tests be written against RestrictedEthashHeaderValidator?
Contributor
Author
There was a problem hiding this comment.
tbh i am not sure that is possible (or desirable) as RestrictedEthashBlockHeaderValidator inherits from EthashBlockHeaderValidator which mean method validate validates proof of work therefore to property test it one should mine valid blocks which would take too much time.
Thats why I had split that way:
- property test for signing and validating to test various different inputs
RestrictedEthashBlockHeaderValidatorSpecwhich tests differentif-elseparts fromRestrictedEthashBlockHeaderValidatorplus the case which test against someone who would try to re-sign header after pow is mined
There was a problem hiding this comment.
On the BlockHeaderValidatorSpec we used:
class BlockValidatorWithPowMocked(blockchainConfig: BlockchainConfig)
extends BlockHeaderValidatorSkeleton(blockchainConfig) {
override protected def difficulty: DifficultyCalculator = difficultyCalculator
override def validateEvenMore(
blockHeader: BlockHeader,
parentHeader: BlockHeader
): Either[BlockHeaderError, BlockHeaderValid] = Right(BlockHeaderValid)
}
Something like that might be useful for what you are saying
| "RestrictedEthashSigner" should "sign and validate correct header" in { | ||
| forAll(blockHeaderGen, genKey(secureRandom)) { (header, key) => | ||
| val signedHeader = RestrictedEthashSigner.signHeader(header, key) | ||
| val keyAsBytes = ByteString.fromArrayUnsafe( |
Contributor
There was a problem hiding this comment.
io.iohk.ethereum.crypto#keyPairToByteArrays and friends instead?
|
|
||
| it should "fail to validate header with invalid key" in new TestSetup { | ||
| val allowedKey = crypto.generateKeyPair(secureRandom) | ||
| val keyBytes = ByteString.fromArrayUnsafe( |
| it should "fail to validate header signed with wrong key" in { | ||
| forAll(blockHeaderGen, genKey(secureRandom), genKey(secureRandom)) { (header, correctKey, wrongKey) => | ||
| val signedHeader = RestrictedEthashSigner.signHeader(header, correctKey) | ||
| val wrongKeyAsBytes = ByteString.fromArrayUnsafe( |
|
|
||
| it should "fail to validate header re-signed by valid signer" in new TestSetup { | ||
| val allowedKey = crypto.generateKeyPair(secureRandom) | ||
| val keyBytes = ByteString.fromArrayUnsafe( |
Simplify RestrictedEthash validator Few re-namings Use crypto.utils instead of manual key conversions
ntallar
suggested changes
Nov 20, 2020
ntallar
left a comment
ntallar
left a comment
There was a problem hiding this comment.
Only some minor comments, worked well while testing it! Will take a look at the tests next week
ntallar
suggested changes
Nov 24, 2020
ntallar
left a comment
ntallar
left a comment
There was a problem hiding this comment.
Only a minor comment, apart from which LGTM!
| with ObjectGenerators | ||
| with SecureRandomBuilder { | ||
| "RestrictedEthashSigner" should "sign and validate correct header" in { | ||
| forAll(blockHeaderGen, genKey(secureRandom)) { (header, key) => |
There was a problem hiding this comment.
On the BlockHeaderValidatorSpec we used:
class BlockValidatorWithPowMocked(blockchainConfig: BlockchainConfig)
extends BlockHeaderValidatorSkeleton(blockchainConfig) {
override protected def difficulty: DifficultyCalculator = difficultyCalculator
override def validateEvenMore(
blockHeader: BlockHeader,
parentHeader: BlockHeader
): Either[BlockHeaderError, BlockHeaderValid] = Right(BlockHeaderValid)
}
Something like that might be useful for what you are saying
Remove allowed keys option from etc-chain.conf Add additional validation error Add comments to special headers in test
KonradStaniec
force-pushed
the
etcm-366/consensus-white-listing
branch
from
71dbcd5 to
78209f3
Compare
ntallar
approved these changes
Nov 25, 2020
ntallar
left a comment
ntallar
left a comment
There was a problem hiding this comment.
LGTM!
.\\ //.
. \ \ / /.
.\ ,\ /` /,.-
-. \ /'/ / .
` - `-' \ -
'. /.\`
- .-
:`//.'
.`.'
.' BP
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Non-standard ethereum PoW consensus protocol, which allows restricting list of possible miners.
Main differences from basic PoW consensus protocol:
Each miner, signs header data before mining i.e prepared header without mixHash and Nonce, and appends this
signature to blockheader.extraData field. Only such prepared header is mined upon.
Each validator, checks (in addition to standard blockheader validations):