Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Validating minimal required gas for a transaction #2937

Merged
merged 4 commits into from
Oct 28, 2016
Merged

Validating minimal required gas for a transaction #2937

merged 4 commits into from
Oct 28, 2016

Conversation

tomusdrw
Copy link
Collaborator

@tomusdrw tomusdrw commented Oct 28, 2016 •
edited
Loading

Closes #2880

Tomasz Drwięga added 2 commits October 28, 2016 13:07
@tomusdrw tomusdrw added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Oct 28, 2016
@rphmeier
Copy link
Contributor

LGTM. Might be useful to add a comment warning not to use any expensive checks for gas_required (e.g. executing the transaction to find how much it really costs) as that would be a DoS vector. Looks like this is fine, since it just checks the base gas requirement for CALL/CREATE?

@rphmeier rphmeier added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Oct 28, 2016
gavofyork
gavofyork reviewed Oct 28, 2016
View reviewed changes
ethcore/src/miner/banning_queue.rs
gas_estimator: &G,
) -> Result<TransactionImportResult, Error>
where F: Fn(&Address) -> AccountDetails,
G: Fn(&SignedTransaction) -> U256,
Copy link
Contributor

Choose a reason for hiding this comment

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

no spaces mixing in with the tabs!!

gavofyork
gavofyork reviewed Oct 28, 2016
View reviewed changes
ethcore/src/miner/transaction_queue.rs
gas_estimator: &G,
) -> Result<TransactionImportResult, Error>
where F: Fn(&Address) -> AccountDetails,
G: Fn(&SignedTransaction) -> U256,
Copy link
Contributor

Choose a reason for hiding this comment

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

no mixing spaces with tabs!

@gavofyork gavofyork added A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Oct 28, 2016
@gavofyork
Copy link
Contributor

check them spaces!

@tomusdrw tomusdrw added A0-pleasereview 🤓 Pull request needs code review. and removed A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. labels Oct 28, 2016
@tomusdrw
Copy link
Collaborator Author

Fixed

@gavofyork gavofyork added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Oct 28, 2016
@gavofyork gavofyork merged commit 0f03342 into master Oct 28, 2016
@gavofyork gavofyork deleted the basegas branch October 28, 2016 14:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@gavofyork gavofyork gavofyork left review comments

Assignees
No one assigned
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tomusdrw @rphmeier @gavofyork