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.

Sign up for GitHub

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

Jump to bottom

Add fuzzing for transaction methods #2453

Merged
merged 2 commits into from
Dec 23, 2019
Merged

Add fuzzing for transaction methods #2453

merged 2 commits into from
Dec 23, 2019

Conversation

jberci
Copy link
Contributor

@jberci jberci commented Dec 6, 2019 •
edited
Loading

Closes #2245
Closes #2246

@jberci jberci force-pushed the jberci/feature/consensus-fuzz branch from 3e96ced to 08cb3ca Compare December 9, 2019 08:56
kostko
kostko reviewed Dec 9, 2019
View reviewed changes
Copy link
Member

@kostko kostko left a comment

Choose a reason for hiding this comment

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

Added some comments. Also, should the fuzz thing be excluded from coverage (see .codecov.yml)?

go/consensus/tendermint/apps/fuzz/fuzz.go Outdated Show resolved Hide resolved
go/consensus/tendermint/apps/fuzz/fuzz.go Outdated Show resolved Hide resolved
go/consensus/tendermint/apps/fuzz/fuzz.go Outdated Show resolved Hide resolved
go/consensus/tendermint/apps/fuzz/fuzz.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Dec 9, 2019 •
edited
Loading

Codecov Report

Merging #2453 into master will decrease coverage by 0.28%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2453      +/-   ##
==========================================
- Coverage   67.06%   66.77%   -0.29%     
==========================================
  Files         324      324              
  Lines       29880    29880              
==========================================
- Hits        20038    19952      -86     
- Misses       7353     7455     +102     
+ Partials     2489     2473      -16
Impacted Files Coverage Δ
go/storage/mkvs/urkel/writelog/iterator.go 76.36% <0%> (-16.37%) ⬇️
go/worker/storage/service_external.go 47.61% <0%> (-9.53%) ⬇️
go/consensus/api/grpc.go 57.02% <0%> (-6.62%) ⬇️
go/consensus/tendermint/roothash/roothash.go 63.67% <0%> (-6%) ⬇️
go/oasis-node/cmd/debug/byzantine/byzantine.go 38.91% <0%> (-5.41%) ⬇️
go/storage/api/grpc.go 64.84% <0%> (-5.03%) ⬇️
go/worker/storage/committee/node.go 73.84% <0%> (-4.09%) ⬇️
go/storage/mkvs/urkel/prefetch.go 59.32% <0%> (-3.39%) ⬇️
go/oasis-node/cmd/debug/byzantine/merge.go 74.68% <0%> (-2.54%) ⬇️
go/worker/common/p2p/p2p.go 66.21% <0%> (-1.36%) ⬇️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6dd7628...f3d0cb1. Read the comment docs.

kostko
kostko reviewed Dec 9, 2019
View reviewed changes
go/consensus/tendermint/apps/fuzz/fuzz.go Outdated Show resolved Hide resolved
go/consensus/tendermint/abci/mux.go Outdated Show resolved Hide resolved
go/consensus/tendermint/apps/fuzz/fuzz.go Outdated Show resolved Hide resolved
go/consensus/tendermint/apps/fuzz/fuzz.go Outdated Show resolved Hide resolved
go/consensus/tendermint/abci/mux.go Outdated Show resolved Hide resolved
go/consensus/tendermint/abci/mux.go Outdated Show resolved Hide resolved
@jberci jberci force-pushed the jberci/feature/consensus-fuzz branch 6 times, most recently from 57f6ea3 to 336fdc1 Compare December 17, 2019 13:27
kostko
kostko reviewed Dec 17, 2019
View reviewed changes
Copy link
Member

@kostko kostko left a comment

Choose a reason for hiding this comment

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

I think this looks good, rebase on master and I'll take another look.

go/consensus/tendermint/abci/mux.go Outdated Show resolved Hide resolved
go/consensus/tendermint/fuzz/fuzz.go Show resolved Hide resolved
@jberci jberci force-pushed the jberci/feature/consensus-fuzz branch from 336fdc1 to b93f9f7 Compare December 18, 2019 13:27
kostko
kostko reviewed Dec 18, 2019
View reviewed changes
go/storage/fuzz/fuzz.go Show resolved Hide resolved
go/storage/fuzz/fuzz.go Outdated
defer os.RemoveAll(localDB)

// Create the storage backend service.
storage, err := storage.New(context.Background(), localDB, identity, nil, nil)
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to only create the database once and have it shared between invocations? Or am I mistaken in how Fuzz is used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The fuzzer doesn't really provide any per-worker startup/shutdown hooks. I could use init for startup, but I can't see any solution for cleanup, which we'd need to properly close the database. When I tried running with just one database (and a single worker process), the library decided to stop working with a resource temporarily unavailable.

@jberci jberci force-pushed the jberci/feature/consensus-fuzz branch from b93f9f7 to e089f99 Compare December 19, 2019 15:22
@jberci jberci marked this pull request as ready for review December 19, 2019 15:26
ptrus
ptrus reviewed Dec 19, 2019
View reviewed changes
Copy link
Member

@ptrus ptrus left a comment

Choose a reason for hiding this comment

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

should we setup this to be run daily/weekly/continiously on buildkite?

go/consensus/tendermint/abci/mux.go Outdated Show resolved Hide resolved
go/consensus/tendermint/abci/mux.go Outdated Show resolved Hide resolved
@kostko
Copy link
Member

kostko commented Dec 19, 2019

should we setup this to be run daily/weekly/continiously on buildkite?

Yeah, but not as part of this PR. @jberci can you file an issue for running fuzzers on CI every once in a while.

@jberci jberci force-pushed the jberci/feature/consensus-fuzz branch from e089f99 to a3fc65d Compare December 20, 2019 12:02
@jberci jberci force-pushed the jberci/feature/consensus-fuzz branch 2 times, most recently from 0a69653 to 195822d Compare December 23, 2019 10:52
@jberci jberci force-pushed the jberci/feature/consensus-fuzz branch from 195822d to f3d0cb1 Compare December 23, 2019 11:16
@jberci jberci merged commit 329e023 into master Dec 23, 2019
@jberci jberci deleted the jberci/feature/consensus-fuzz branch December 23, 2019 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ptrus ptrus ptrus left review comments

@kostko kostko kostko approved these changes

@peterjgilbert peterjgilbert Awaiting requested review from peterjgilbert peterjgilbert is a code owner

@Yawning Yawning Awaiting requested review from Yawning

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fuzz storage interface Fuzz consensus interface
3 participants
@jberci @kostko @ptrus