BaseApp Security Improvements #3801
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #3801 +/- ##
==========================================
Coverage ? 60.94%
==========================================
Files ? 192
Lines ? 14323
Branches ? 0
==========================================
Hits ? 8729
Misses ? 5025
Partials ? 569 |
alexanderbez
requested review from
cwgoes,
ebuchman and
rigelrozanski
as code owners
|
@jackzampolin @cwgoes I think we should get this in for the 0.33 release. Thoughts? |
baseapp/baseapp.go
Outdated
| @@ -510,6 +513,19 @@ func handleQueryCustom(app *BaseApp, path []string, req abci.RequestQuery) (res | |||
| } | |||
| } | |||
|
|
|||
| func (app *BaseApp) validateHeight(req abci.RequestBeginBlock) error { | |||
| if req.Header.Height < 0 { | |||
There was a problem hiding this comment.
I think Tendermint starts at height 1 now.
There was a problem hiding this comment.
True, but can the same be said for other consensus engines?
There was a problem hiding this comment.
Not necessarily, but I think we should panic otherwise, because elsewhere in the SDK we assume that the first block height is 1.
There was a problem hiding this comment.
Fair point -- updated.
There was a problem hiding this comment.
Can a comment be made somewhere or we amend ABCI to stipulate that the first block is at height 1?
There was a problem hiding this comment.
@ValarDragon yes, I'll follow up on this.
|
@cwgoes addressed your comments |
|
A series of small BaseApp security improvements spawned by a lovely review by @ValarDragon.
closes: #3791
closes: #3790
Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
Wrote tests
Updated relevant documentation (
docs/)Added entries in
PENDING.mdwith issue #rereviewed
Files changedin the github PR explorerFor Admin Use: