core: verify headers in AddHeaders() #703
Conversation
Codecov Report
@@ Coverage Diff @@
## master #703 +/- ##
=========================================
Coverage ? 65.72%
=========================================
Files ? 128
Lines ? 11042
Branches ? 0
=========================================
Hits ? 7257
Misses ? 3506
Partials ? 279
Continue to review full report at Codecov.
|
pkg/core/blockchain.go
Outdated
| err = fmt.Errorf("header %v is invalid", h) | ||
| return | ||
| if verify { | ||
| if err = bc.verifyHeader(h, lastHeader); err != nil { |
There was a problem hiding this comment.
I don't think it's a good idea doing verification inside headersOp as it effectively locks bc.headerList and verification can take some substantial amount of time.
There was a problem hiding this comment.
I have moved verification of external input upwards, and inside locked context we can only check for the validity of the first header.
| if want, have := tk.PrivateKey, acc.privateKey.String(); want != have { | ||
| t.Fatalf("expected %s got %s", want, have) | ||
| } | ||
| want, have := tk.Address, acc.Address |
There was a problem hiding this comment.
Do we really need intermediate want/have here?
There was a problem hiding this comment.
No, we don't, like we didn't need it earlier. It seems more readable.
This simplifies tests a bit.
It can lead to unnecessary race conditions and is just a bad practice.
Headers can be malformed so public methods should verify them before adding.
This makes tests less verbose and unifies the style they are written in.
Maybe now there is no need to verify block if it's header matches header in headerlist.