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

Ensure Block Processing Failures Return an Error #2325

Merged
merged 8 commits into from
Apr 21, 2019
Merged

Conversation

rauljordan
Copy link
Contributor

No tracking issue. We have a problem in which the following code in block_processing.go in our blockchain package would not return an error if it failed:

beaconState, err = c.ApplyBlockStateTransition(ctx, block, beaconState)
	if err != nil {
		switch err.(type) {
		case *BlockFailedProcessingErr:
			// If the block fails processing, we mark it as blacklisted and delete it from our DB.
			c.beaconDB.MarkEvilBlockHash(blockRoot)
			if err := c.beaconDB.DeleteBlock(block); err != nil {
				return nil, fmt.Errorf("could not delete bad block from db: %v", err)
			}
		default:
			return beaconState, fmt.Errorf("could not apply block state transition: %v", err)
		}
	}

@rauljordan rauljordan self-assigned this Apr 21, 2019
Copy link
Member

@prestonvanloon prestonvanloon left a comment

Choose a reason for hiding this comment

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

Any regression test?

@rauljordan
Copy link
Contributor Author

on it

@codecov
Copy link

codecov bot commented Apr 21, 2019

Codecov Report

Merging #2325 into master will increase coverage by 0.33%.
The diff coverage is 50%.

@@            Coverage Diff             @@
##           master    #2325      +/-   ##
==========================================
+ Coverage   68.44%   68.78%   +0.33%     
==========================================
  Files         118      117       -1     
  Lines        9428     9207     -221     
==========================================
- Hits         6453     6333     -120     
+ Misses       2277     2191      -86     
+ Partials      698      683      -15

if _, err := chainService.ReceiveBlock(context.Background(), block); err == nil {
t.Error("Expected block to fail processing, received nil")
_, err = chainService.ReceiveBlock(context.Background(), block)
if err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this test can pass if err is nil

prestonvanloon
prestonvanloon previously approved these changes Apr 21, 2019
@rauljordan rauljordan merged commit 1df15f7 into master Apr 21, 2019
@rauljordan rauljordan deleted the return-err branch April 21, 2019 22:38
terencechain pushed a commit that referenced this pull request Apr 22, 2019
* ensure block failed processing returns an error

* fixed test

* test assertion corrected

* comments

* fix tests

* imports
prestonvanloon pushed a commit that referenced this pull request Apr 23, 2019
* update attestation related protos

* use root for hash32

* fixed a few typos

* review comments

* update historical batch, deposit and blk header fields

* Add nogo to introduce built time linting (#2317)

* Add nogo and fix lint issues

* Run gazelle

* better gazelle

* ignore external struct tags

* Exclude all third party code from unsafeptr (#2321)

* Fix Assingments Bug (#2320)

* fix

* fix tests

* Add feature flag to toggle gossip sub in p2p (#2322)

* add feature flag to enable gossip sub in p2p

* invert the enable/disable logic

* add the flag in k8s and fix tests

* gazellle

* return empty config if nil

* Prevent Canceling Goroutines in Validator Client (#2324)

* do not cancel assignments goroutines

* exclude rule

* disable lostcancel for now

* Fix Pending Attestations RPC Call (#2304)

* pending atts

* use proposal slot

* attestation inclusion fix

* lint

* advance state transitions

* gazelle

* lint tests pass

* Do Not Update Validator Registry on Nil Block (#2326)

* no registry update if block is nil

* regression test

* lint

* Ensure Block Processing Failures Return an Error (#2325)

* ensure block failed processing returns an error

* fixed test

* test assertion corrected

* comments

* fix tests

* imports

* rebase

* add spec badge. Thanks to ChainSafe/lodestar#166 for the idea :)

* Update Crosslink Protobuf Fields (#2313)

* rebase

* rebase

* rebase

* Prevent Canceling Goroutines in Validator Client (#2324)

* do not cancel assignments goroutines

* exclude rule

* disable lostcancel for now

* starting proposer slashing

* starting attester slashing

* revert gen file

* Update types.pb.go
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.

4 participants