Skip to content
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

[EIP-1559] Step 5 - Gas price and gas limit block header validation rules #660

Merged
merged 28 commits into from
Apr 9, 2020

Conversation

AbdelStark
Copy link
Contributor

PR description

Implement 2 new AttachedBlockHeaderValidationRule

  • EIP1559BlockHeaderGasPriceValidationRule
  • EIP1559BlockHeaderGasLimitValidationRule

Fixed Issue(s)

fixes #597 #598

Signed-off-by: Abdelhamid Bakhta abdelhamid.bakhta@consensys.net

- hyperledger#430
- hyperledger#440

Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>
Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>

# Conflicts:
#	CHANGELOG.md
Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>
Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>
Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>
Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>
Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>
Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>
Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>
Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>
Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>
Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>
Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>
Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>
…r specified in EIP-1559

Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>
Copy link
Contributor

@shemnon shemnon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The builder pattern for BlockHeaderValidator needs to be preserved. I've left some suggestions on how that can be done.

@@ -99,6 +99,10 @@ private boolean applyRules(
return parent;
}

public void addRule(final AttachedBlockHeaderValidationRule<C> rule) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes the BlockHeaderValidator mutable, and IMHO should not be done. Note we have a builder that has the exact same method, so instead of mutating a produced BlockHeaderValidator we should hook into wherever the builder is being created.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still need to delete this method.

@@ -284,6 +294,8 @@

final BlockHeaderValidator<T> blockHeaderValidator =
blockHeaderValidatorBuilder.apply(difficultyCalculator);
additionalBlockHeaderValidationRules.forEach(blockHeaderValidator::addRule);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will require a tiny bit of replumbing. We can have blockHeaderValidatorBuilder return the builder instead of the fully assembled blockHeaderValidator. Then we call addRule on the builder, then build and store.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

AbdelStark and others added 4 commits April 7, 2020 18:31
Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>
Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>
@AbdelStark
Copy link
Contributor Author

The builder pattern for BlockHeaderValidator needs to be preserved. I've left some suggestions on how that can be done.

Actually i realized with your comment that both EIP1559BlockHeaderGasLimitValidationRule and EIP1559BlockHeaderGasPriceValidationRule are not AttachedBlockHeaderValidationRule but rather DetachedBlockHeaderValidationRule since none of them need the ProtocolContext to perform the validation.

Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>
Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>
@AbdelStark AbdelStark requested a review from shemnon April 8, 2020 12:10
@timbeiko timbeiko added this to the Chupacabra Sprint 62 milestone Apr 8, 2020
Copy link
Contributor

@shemnon shemnon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved with reservations. The issue with how intertwined the difficulty calculator is with all of this feels like it's out of scope for this PR, I would like it fixed but not bad enough to block progress on 1559 work. I opened #687 so we can come back to it later. This PR adds some technical debt to some pre-existing debt, but I don't think we can pay it off before we get this prototype done.

@AbdelStark AbdelStark merged commit 8b1a06f into hyperledger:master Apr 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gas price block header validation rule
3 participants