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

fixed verify_uncles error type#11276

Merged
ordian merged 4 commits into
masterfrom
verify-uncles
Nov 22, 2019
Merged

fixed verify_uncles error type#11276
ordian merged 4 commits into
masterfrom
verify-uncles

Conversation

@debris
Copy link
Copy Markdown
Collaborator

@debris debris commented Nov 21, 2019

No description provided.

@debris debris added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Nov 21, 2019
@debris debris requested a review from dvdplm November 21, 2019 05:23
dvdplm
dvdplm reviewed Nov 21, 2019
View reviewed changes
Copy link
Copy Markdown
Collaborator

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

I'm not very familiar with this uncle handling; can you elaborate on what is going on here and why it's incorrect?

Comment thread ethcore/verification/src/verification.rs Outdated
Comment thread ethcore/verification/src/verification.rs
@debris
Copy link
Copy Markdown
Collaborator Author

debris commented Nov 21, 2019

I'm not very familiar with this uncle handling; can you elaborate on what is going on here and why it's incorrect?

The values returned in an error are incorrect and might be misleading. Currently UnlceTooOld::min is equal to UncleTooOld::found and UncleIsBrother::min is bigger than UncleIsBrother::max.

Actually, when I think of it, we can just get error variant UncleOutOfBounds.

@dvdplm
Copy link
Copy Markdown
Collaborator

dvdplm commented Nov 21, 2019

Actually, when I think of it, we can just get error variant UncleOutOfBounds.

You mean use the same error for both == and >? Yeah, unless the "is brother" is important elsewhere I'm not sure it's useful to have two error variants. :)

niklasad1
niklasad1 reviewed Nov 21, 2019
View reviewed changes
Comment thread ethcore/types/src/engines/mod.rs
dvdplm
dvdplm approved these changes Nov 22, 2019
View reviewed changes
Copy link
Copy Markdown
Collaborator

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

lgmt (niklas's question seems relevant though)

Comment thread ethcore/verification/src/verification.rs Outdated
@debris
Copy link
Copy Markdown
Collaborator Author

debris commented Nov 22, 2019

yes, I addressed @niklasad1 comment in the latest commit :)

@debris @dvdplm
Update ethcore/verification/src/verification.rs
4558835 
Co-Authored-By: David <dvdplm@gmail.com>
@niklasad1 niklasad1 added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Nov 22, 2019
@ordian ordian added this to the 2.7 milestone Nov 22, 2019
@ordian ordian merged commit 1986c4e into master Nov 22, 2019
@ordian ordian deleted the verify-uncles branch November 22, 2019 09:26
ordian added a commit that referenced this pull request Nov 25, 2019
@ordian
Merge branch 'master' into dp/chore/use-upstream-rocksdb
c694323 
* master:
  [ethcore]: apply filter when `PendingSet::AlwaysQueue` in `ready_transactions_filtered` (#11227)
  Update lib.rs (#11286)
  Don't prune ancient state when instantiating a Client (#11270)
  fixed verify_uncles error type (#11276)
  ethcore: fix rlp deprecation warnings (#11280)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@dvdplm dvdplm dvdplm approved these changes

@niklasad1 niklasad1 niklasad1 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.

Projects

None yet

Milestone

2.7

Development

Successfully merging this pull request may close these issues.

4 participants

@debris @dvdplm @niklasad1 @ordian