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.

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

Port #4063 to master #4086

Merged
merged 6 commits into from
Mar 24, 2023
Merged

Port #4063 to master #4086

merged 6 commits into from
Mar 24, 2023

Conversation

jordanschalm
Copy link
Member

@codecov-commenter
Copy link

codecov-commenter commented Mar 22, 2023

Codecov Report

Merging #4086 (530e2d6) into master (d352e86) will increase coverage by 0.44%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4086      +/-   ##
==========================================
+ Coverage   53.45%   53.90%   +0.44%     
==========================================
  Files         832      621     -211     
  Lines       77809    55458   -22351     
==========================================
- Hits        41596    29892   -11704     
+ Misses      32883    23180    -9703     
+ Partials     3330     2386     -944     
Flag Coverage Δ
unittests 53.90% <100.00%> (+0.44%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...nsensus/hotstuff/committees/consensus_committee.go 69.72% <ø> (ø)
module/epochs/epoch_lookup.go 71.61% <ø> (ø)
...nsensus/hotstuff/signature/block_signer_decoder.go 86.20% <100.00%> (+4.38%) ⬆️

... and 223 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Contributor

@peterargue peterargue left a comment

Choose a reason for hiding this comment

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

thanks @jordanschalm!

@@ -83,11 +84,12 @@ func (s *blockSignerDecoderSuite) Test_UnexpectedCommitteeException() {
// It should propagate the sentinel error model.ErrViewForUnknownEpoch from Committee.
func (s *blockSignerDecoderSuite) Test_UnknownEpoch() {
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add a couple more tests to cover the 3 error cases:

  1. ParentView's epoch is not in cache, but block exists
  2. IdentitiesByEpoch returns non-sentinel
  3. IdentitiesByBlock returns an error

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added these in a follow-up PR: #4093

Your comment made me realize that the implementation didn't differentiate between storage exceptions and unknown input blocks.

Copy link
Member

@AlexHentschel AlexHentschel left a comment

Choose a reason for hiding this comment

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

👍

would you mind fixing the typo here:

consensus/hotstuff/committee.go Outdated Show resolved Hide resolved
@jordanschalm
Copy link
Member Author

bors merge

@bors bors bot merged commit 27188ec into master Mar 24, 2023
@bors bors bot deleted the jordan/port-4063-to-master branch March 24, 2023 16:06
bors bot added a commit that referenced this pull request Apr 4, 2023
4093: Check reference at snapshot creation r=jordanschalm a=jordanschalm

This PR extends #4086, adding checks to snapshot creation to differentiate between exceptions and unknown blocks in BlockSignerDecoder. 
- Updates `protocol.Snapshot` to guarantee that `ErrUnknownSnapshotReference` is returned in all cases where a snapshot's reference is unknown
  - Add existence checking methods to `storage/common`, `storage.Cache`
- Updates BlockSignerDecoder to differentiate between errors from an unknown input block and exception
- Misc: marks all unexpected errors in `storage/common` as exceptions
- Misc: update `flow-emu` version to include onflow/flow-emulator#280 (avoid changes to `Headers` being breaking)

4140: Storage layer test refactor r=janezpodhostnik a=janezpodhostnik

Extracting pieces from #3736 so its easier to digest and merge.

Co-authored-by: Jordan Schalm <jordan@dapperlabs.com>
Co-authored-by: Janez Podhostnik <janez.podhostnik@gmail.com>
bors bot added a commit that referenced this pull request Apr 4, 2023
4093: Check reference at snapshot creation r=jordanschalm a=jordanschalm

This PR extends #4086, adding checks to snapshot creation to differentiate between exceptions and unknown blocks in BlockSignerDecoder. 
- Updates `protocol.Snapshot` to guarantee that `ErrUnknownSnapshotReference` is returned in all cases where a snapshot's reference is unknown
  - Add existence checking methods to `storage/common`, `storage.Cache`
- Updates BlockSignerDecoder to differentiate between errors from an unknown input block and exception
- Misc: marks all unexpected errors in `storage/common` as exceptions
- Misc: update `flow-emu` version to include onflow/flow-emulator#280 (avoid changes to `Headers` being breaking)

Co-authored-by: Jordan Schalm <jordan@dapperlabs.com>
bors bot added a commit that referenced this pull request Apr 5, 2023
4093: Check reference at snapshot creation r=jordanschalm a=jordanschalm

This PR extends #4086, adding checks to snapshot creation to differentiate between exceptions and unknown blocks in BlockSignerDecoder. 
- Updates `protocol.Snapshot` to guarantee that `ErrUnknownSnapshotReference` is returned in all cases where a snapshot's reference is unknown
  - Add existence checking methods to `storage/common`, `storage.Cache`
- Updates BlockSignerDecoder to differentiate between errors from an unknown input block and exception
- Misc: marks all unexpected errors in `storage/common` as exceptions
- Misc: update `flow-emu` version to include onflow/flow-emulator#280 (avoid changes to `Headers` being breaking)

Co-authored-by: Jordan Schalm <jordan@dapperlabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants