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

Fix Pending Attestations RPC Call #2304

Merged
merged 11 commits into from
Apr 21, 2019
Merged

Conversation

rauljordan
Copy link
Contributor

@rauljordan rauljordan commented Apr 19, 2019

Part of #1586


Description

Write why you are making the changes in this pull request

This PR resolves attestation inclusion problems in blocks. We encountered a major issue where blocks did not have any attestations at epoch boundaries, leading to all sorts of problems and error logs which would eventually trigger reorgs when a previous slot boundary was a skip slot.

Write a summary of the changes you are making

The important change is that when we give the proposer attestations to include, we must give him attestations which will pass state transition processing at the time they are included. That is, we must filter these attestations keeping in mind the beacon state slot at which they are supposed to be included. We can solve the previous problems by increasing state by MIN_ATTESTATION_INCLUSION_DISTANCE when filtering attestations.

@codecov
Copy link

codecov bot commented Apr 19, 2019

Codecov Report

Merging #2304 into master will increase coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2304      +/-   ##
==========================================
+ Coverage   68.84%   68.88%   +0.04%     
==========================================
  Files         117      117              
  Lines        9151     9132      -19     
==========================================
- Hits         6300     6291       -9     
+ Misses       2172     2165       -7     
+ Partials      679      676       -3

@codecov
Copy link

codecov bot commented Apr 19, 2019

Codecov Report

Merging #2304 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2304   +/-   ##
=======================================
  Coverage   68.93%   68.93%           
=======================================
  Files         117      117           
  Lines        9336     9336           
=======================================
  Hits         6436     6436           
  Misses       2204     2204           
  Partials      696      696

@rauljordan rauljordan self-assigned this Apr 19, 2019
@rauljordan rauljordan added Bug Something isn't working Ready For Review labels Apr 19, 2019
terencechain
terencechain previously approved these changes Apr 19, 2019
@prestonvanloon
Copy link
Member

Looks like there are test failures

@rauljordan rauljordan merged commit 0ff335f into master Apr 21, 2019
@rauljordan rauljordan deleted the pending-attestations-inclusion branch April 21, 2019 21:12
terencechain pushed a commit that referenced this pull request Apr 22, 2019
* pending atts

* use proposal slot

* attestation inclusion fix

* lint

* advance state transitions

* gazelle

* lint tests pass
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
Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants