Part 3 of Aligning Beacon Chain with Latest 2.1 - Processing Attestations #423

terencechain · 2018-08-21T04:20:18Z

Background

The primary load on beacon chain is attestations. To process an attestation means to attest parent block of the beacon chain, or attest block hash of the shard chain. Every beacon node has the responsibility to process all the attestations coming from a beacon block. That's how it calculates active and crystallized state.

Design

This PR implements the processAttestation inside processBlock function. Here's todos:

Remove checks for active and crystallized state roots. It was invalid, we are suppose to compute new state hashes, not compare with existing canonical state hashes
Verify AttestationRecord slot number is less than current block number and greater than block number that's one CYCLE_LENGTH before
Compute parent hashes based on active state's recent block hash array and oblique parent hashes from block
Get attestation indices that match the correct shard ID
Verify attester bitfield from AttestationRecord is the same length as attestation indices
Hash the following message hash((slot % CYCLE_LENGTH).to_bytes(8, 'big') + parent_hashes + shard_id + shard_block_hash)
Test and coverages

This reverts commit 72a5dd1.

codecov · 2018-08-21T17:30:58Z

Codecov Report

Merging #423 into master will increase coverage by 2.55%.
The diff coverage is 94.28%.

@@            Coverage Diff             @@
##           master     #423      +/-   ##
==========================================
+ Coverage   73.67%   76.23%   +2.55%     
==========================================
  Files           7       35      +28     
  Lines         680     2188    +1508     
==========================================
+ Hits          501     1668    +1167     
- Misses        112      366     +254     
- Partials       67      154      +87

Impacted Files	Coverage Δ
beacon-chain/node/node.go	`58.97% <ø> (ø)`	⬆️
beacon-chain/blockchain/service.go	`70.37% <0%> (-0.88%)`	⬇️
beacon-chain/types/block.go	`93.54% <0%> (ø)`
beacon-chain/utils/checkbit.go	`100% <100%> (ø)`
beacon-chain/casper/validator.go	`100% <100%> (ø)`	⬆️
validator/beacon/service.go	`100% <100%> (ø)`
validator/node/node.go	`40.21% <100%> (ø)`
beacon-chain/blockchain/core.go	`75.55% <100%> (+5.49%)`	⬆️
beacon-chain/casper/incentives.go	`100% <100%> (+7.69%)`	⬆️
... and 28 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 2ee9ec2...490bf84. Read the comment docs.

…validate_attestation

… validate_attestation

rauljordan · 2018-08-22T05:41:27Z

beacon-chain/blockchain/core.go

+	return signedParentHashes
+}
+
+// getAttestatorIndices returns the attestor committee of based from attestation's shard ID  and slot number.


getAttesterIndices*

rawfalafel · 2018-08-22T13:10:24Z

beacon-chain/blockchain/core.go

+
+		// Get all the block hashes up to cycle length.
+		parentHashes := b.getSignedParentHashes(block, attestation)
+		attesterIndices, err := b.getAttestatorIndices(attestation)


b.getAttesterIndices 😛

… validate_attestation

prestonvanloon · 2018-08-23T00:08:04Z

beacon-chain/blockchain/core.go

+	lastBit := len(attesterIndices)
+	if lastBit%8 != 0 {
+		for i := 0; i < 8-lastBit%8; i++ {
+			voted, err := utils.CheckBit(attestation.AttesterBitfield, lastBit+i)


Can we simplify checkBit not to return an error? It seems reasonable that an out of index bit would return false since the bit at that index is not 1. The error seems unecessary IMO and it would make this method easier to use.

prestonvanloon · 2018-08-23T00:09:10Z

beacon-chain/blockchain/service.go

@@ -153,6 +153,9 @@ func (c *ChainService) ProcessBlock(block *types.Block) error {
 		return nil
 	}
 	if canProcess {
+		if err := c.chain.processAttestations(block); err != nil {


This branch is untested. Can we add a test to ensure the attestations are processed on a block when canProcess is truthy?

Done. Added TestProcessingBlockWithAttestations in blockchain's service_test.go

prestonvanloon · 2018-08-23T00:14:57Z

beacon-chain/utils/checkbit_test.go

+	}
+	for _, tt := range tests {
+		if BitLength(tt.a) != tt.b {
+			t.Errorf("Expected %v, Got %v", tt.b, BitLength(tt.b))


This is great use of test driven tests. One improvement you could make is to structure this error message format to include the method name

t.Errorf("BitLength(%d) = %d, want = %d", tt.a, BitLength(tt.a), tt.b)

nisdas

lgtm

prestonvanloon · 2018-08-23T11:52:01Z

beacon-chain/blockchain/core.go

+			len(attestation.AttesterBitfield), utils.BitLength(len(attesterIndices)))
+	}
+
+	// Validate attestation can not have non-zero trailing bits.


/validate/valid/ ??

prestonvanloon · 2018-08-23T11:56:58Z

beacon-chain/blockchain/core.go

-		return nil, fmt.Errorf("can not return attester set of given height, input height %v has to be in between %v and %v", height, sback, sback+params.CycleLength*2)
+// getBlockHash returns the block hash of a slot.
+func (b *BeaconChain) getBlockHash(slot uint64, block *types.Block) ([]byte, error) {
+	sback := int(block.SlotNumber()) - params.CycleLength*2


What does sback mean?

slot back. getBlockHash can only return within a range of block.Slot - 128 <= slot < block.Slot. Let's say current block is 200, but you wanna get the block hash of slot 50. That won't work because the further you can get is 200 - 128. And that is slot back

Will change sback to slotBack

prestonvanloon · 2018-08-23T11:58:03Z

beacon-chain/blockchain/core.go

+// getBlockHash returns the block hash of a slot.
+func (b *BeaconChain) getBlockHash(slot uint64, block *types.Block) ([]byte, error) {
+	sback := int(block.SlotNumber()) - params.CycleLength*2
+	if !(sback <= int(slot) && int(slot) < sback+params.CycleLength*2) {


Please change sback+params.CycleLength*2 to int(block.SlotNumber())

prestonvanloon · 2018-08-23T12:01:24Z

beacon-chain/blockchain/core.go

+	// Validate attestation can not have non-zero trailing bits.
+	lastBit := len(attesterIndices)
+	if lastBit%8 != 0 {
+		for i := 0; i < 8-lastBit%8; i++ {


I don’t understand this logic. If length is 9, then it would only run check the first bit?

Exactly. Let's say attester_bitfield is initialized in with attester_bitfield = (len(attestation_indices) + 7) // 8 empty bytes. So if committee_indices size is 10, attester_bitfield is 2 bytes, but the meaningful bits are still the left 10 bits. This checking from the spec is for keeping the padding bits clean.

prestonvanloon · 2018-08-23T12:04:26Z

beacon-chain/blockchain/service.go

@@ -146,13 +146,17 @@ func (c *ChainService) ProcessBlock(block *types.Block) error {
 	} else {
 		canProcess, err = c.chain.CanProcessBlock(nil, block, false)
 	}
+	fmt.Println(err)


terencechain added 2 commits August 19, 2018 08:40

Revert "Add Skip Coverage Condition in Coverage.sh (#416)"

d21c81b

This reverts commit 72a5dd1.

begin processing attestation

d6c93f4

terencechain added the Beacon Chain label Aug 21, 2018

terencechain self-assigned this Aug 21, 2018

terencechain changed the title ~~Part 2 of Aligning Core Package with Latest 2.1 - Processing Attestations~~ Part 3 of Aligning Core Package with Latest 2.1 - Processing Attestations Aug 21, 2018

terencechain added 5 commits August 20, 2018 21:36

coverage.sh

810dfaa

Merge branch 'master' of https://github.com/prysmaticlabs/prysm

1f979ae

Merge branch 'master' of https://github.com/prysmaticlabs/prysm

0125529

add coverage.sh back

8a48bdd

Merge branch 'master' into validate_attestation

b3dc96d

terencechain added 6 commits August 21, 2018 20:07

tests

7dd4e52

Merge branch 'master' of https://github.com/prysmaticlabs/prysm into …

b11d38b

…validate_attestation

Merge branch 'validate_attestation' of github.com:terenc3t/prysm into…

a39e871

… validate_attestation

lint

6c48c0d

bazel

8de3557

comment

e0e2a70

rauljordan reviewed Aug 22, 2018

View reviewed changes

terencechain added 2 commits August 21, 2018 22:51

use PutUvarint instead of BigEndian

0e54c46

fixed typo

4b074f6

rawfalafel reviewed Aug 22, 2018

View reviewed changes

terencechain added 4 commits August 22, 2018 06:49

fixed typo

c977417

Merge branch 'master' of https://github.com/prysmaticlabs/prysm

34399e3

s/client/validator

d3a63b2

Merge branch 'validate_attestation' of github.com:terenc3t/prysm into…

cea7cb2

… validate_attestation

prestonvanloon reviewed Aug 23, 2018

View reviewed changes

Addressed Preston's feedback

caaf23f

nisdas approved these changes Aug 23, 2018

View reviewed changes

prestonvanloon reviewed Aug 23, 2018

View reviewed changes

Addressed Preston's feedback

76c7f88

terencechain changed the title ~~Part 3 of Aligning Core Package with Latest 2.1 - Processing Attestations~~ Part 3 of Aligning Beacon Chain with Latest 2.1 - Processing Attestations Aug 24, 2018

terencechain added 4 commits August 23, 2018 22:07

merged with Raul's fork choice stuff

b747ebe

added the old tests back

2600b22

Merge branch 'master' into validate_attestation

92cd648

race issue?

490bf84

terencechain merged commit 1598ae8 into prysmaticlabs:master Aug 24, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Part 3 of Aligning Beacon Chain with Latest 2.1 - Processing Attestations #423

Part 3 of Aligning Beacon Chain with Latest 2.1 - Processing Attestations #423

terencechain commented Aug 21, 2018 •

edited

Loading

codecov bot commented Aug 21, 2018 •

edited

Loading

rauljordan Aug 22, 2018

rawfalafel Aug 22, 2018

prestonvanloon Aug 23, 2018

prestonvanloon Aug 23, 2018

terencechain Aug 23, 2018

prestonvanloon Aug 23, 2018

nisdas left a comment

prestonvanloon Aug 23, 2018

prestonvanloon Aug 23, 2018

terencechain Aug 23, 2018

terencechain Aug 23, 2018

prestonvanloon Aug 23, 2018

prestonvanloon Aug 23, 2018

terencechain Aug 23, 2018 •

edited

Loading

prestonvanloon Aug 23, 2018

Part 3 of Aligning Beacon Chain with Latest 2.1 - Processing Attestations #423

Part 3 of Aligning Beacon Chain with Latest 2.1 - Processing Attestations #423

Conversation

terencechain commented Aug 21, 2018 • edited Loading

Background

Design

codecov bot commented Aug 21, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nisdas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terencechain Aug 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terencechain commented Aug 21, 2018 •

edited

Loading

codecov bot commented Aug 21, 2018 •

edited

Loading

terencechain Aug 23, 2018 •

edited

Loading