Store Incoming Blocks and Calculated States in DAG #422

rauljordan · 2018-08-21T02:32:25Z

Hi all,

This PR is part of a series in creating the fork choice rule for the Beacon Chain. Currently, we treat every incoming block as canonical (that is, we set the canonical active/crystallized states by deriving them from incoming blocks and save them to persistent storage). A key part of the beacon node is being able to apply a fork choice rule to a DAG of blocks/states at each slot number to derive a canonical chain. This PR also abstracts all core casper functionality into a /beacon-chain/casper package as pure functions with related tests.

This is part of #255.

Additionally, nodes should not sync beacon states between each other but should instead compute them from received blocks (the only exception to this is initial sync where nodes should share the crystallized state between each other until they are synced to the latest head).

Scope

This PR will do the following:

Remove active/crystallized state sync
Store processed blocks and calculated states into a DAG structure
Add important TODOs for other PRs to begin work in with respect to implementing fork-choice
Create an extremely "naive" fork choice where we continually pick the first block inserted in this DAG at each block number as canonical in order to maintain feature parity with the master branch

Persisting DAG across sessions for continuing where fork-choice left off will be done in a separate PR as part of #429.

Separate PRs will be needed to actually derive a canonical chain using the real fork choice rule.

How to Run

First, make sure you have a running geth miner. Then, start from a fresh beacondatadir. Run as follows:

bazel run //beacon-chain -- \
  --web3provider ws://127.0.0.1:8546 \
  --datadir /your/beacon/datadir \
  --rpc-port 5000 \
  --validator \
  --simulator

You'll see incoming blocks being put into a DAG and a fork choice (albeit quite naive) being applied at each new received slot.

rawfalafel · 2018-08-22T02:14:02Z

beacon-chain/blockchain/service.go

-		return fmt.Errorf("could not hash incoming block: %v", err)
+		log.Debugf("Could not hash incoming block: %v", err)
+	}
+	if block.SlotNumber() > c.lastSlot && block.SlotNumber() > 1 {


We need to establish patterns for how to safely access properties concurrently. Take this one for example: ProcessBlock is called by the sync service and checks the value of c.lastSlot, but blockProcessing could easily be modifying it at the same time, leading to a nasty race condition.

Not sure what the actionable item is yet, but ideally these fields are grouped together into distinct goroutines so that we don't have to use locks.

I think that all of the actual "block processing" should be inside the blockProcessing goroutine. That solves 2 potential issues: 1) Eliminates possibility of a race condition, 2) unblocks the sync goroutine, which is the one calling this function.

The goroutine should be split up so that updating head and writing to disk happens in the first loop, and then state transition happens in the second loop.

This function should also be renamed to PreProcessBlock or something along those lines, since there really souldn't be any heavy lifting in this function.

This is excellent feedback @rawfalafel, thank you. Completely agree with moving everything into one routine. When you say:

writing to disk happens in the first loop, and then state transition happens in the second loop

do you mean having a goroutine with different select cases handling each of these items?

yea, that's what i meant

updateHead should come after c.chain.CanProcessBlock. Doesn't make sense to call updateHead if validation fails.

My understanding of the order of operations:

Validate new block

Persist block to disk (not state)

Run fork choice rule

Update active/crystallized state if the head changes

Does that sound correct?

Great, addressed this!

rawfalafel

Looks good! I have one comment that stood out to me, although this looks sound overall.

rawfalafel · 2018-08-22T02:57:18Z

beacon-chain/blockchain/service.go

-		return fmt.Errorf("could not hash incoming block: %v", err)
+		log.Debugf("Could not hash incoming block: %v", err)
+	}
+	if block.SlotNumber() > c.lastSlot && block.SlotNumber() > 1 {


I think that all of the actual "block processing" should be inside the blockProcessing goroutine. That solves 2 potential issues: 1) Eliminates possibility of a race condition, 2) unblocks the sync goroutine, which is the one calling this function.

The goroutine should be split up so that updating head and writing to disk happens in the first loop, and then state transition happens in the second loop.

This function should also be renamed to PreProcessBlock or something along those lines, since there really souldn't be any heavy lifting in this function.

terencechain · 2018-08-22T04:11:52Z

beacon-chain/blockchain/service.go

+	// We store processed blocks and states into a slice by SlotNumber.
+	// For example, at slot 5, we might have received 10 different blocks,
+	// and a canonical chain must be derived from this DAG.
+	processedBlocksBySlot             map[uint64][]*types.Block


These processedXXXBySlot fields are temporary, they will later be replaced by tree right? if yes, can we make a note of that in the comment?

Great, I'll add comments to clarify

terencechain · 2018-08-22T04:15:00Z

beacon-chain/blockchain/service.go

-	go c.run(c.ctx.Done())
+	head, err := c.chain.CanonicalHead()
+	if err != nil {
+		log.Errorf("Could not fetch latest canonical head from DB: %v", err)


if you cant fetch the canonical head, is error too mild? should it be Fatal?

Fatal would shut down the whole node, although you could argue that if the chain head cant be fetched the node is not of much use

Agreed there should be fatal here, it is critical for the entire system to fetch a canonical head

terencechain · 2018-08-22T04:16:19Z

beacon-chain/blockchain/service.go

@@ -237,55 +206,102 @@ func (c *ChainService) CanonicalCrystallizedStateEvent() <-chan *types.Crystalli

 // run processes the changes needed every beacon chain block,


this comment is still about run

terencechain · 2018-08-22T04:17:13Z

beacon-chain/blockchain/service.go

+		case block := <-c.latestProcessedBlock:
+			// 3 steps:
+			// - Compute the active state for the block.
+			// - Compute the crystallized state for the block if epoch transition.


there's no more epoch transition, can you change it to cycle transition?

terencechain · 2018-08-22T04:17:36Z

beacon-chain/blockchain/service.go

+			//
+			// Another routine will run that will continually compute
+			// the canonical block and states from this data structure using the
+			// fork choice rule


rawfalafel

Extracting PoS logic into its own package's a good idea 👍 Noticed a few more things in this PR.

rawfalafel · 2018-08-22T20:09:27Z

beacon-chain/casper/validator.go

+
+// SampleAttestersAndProposers returns lists of random sampled attesters and proposer indices.
+func SampleAttestersAndProposers(seed common.Hash, crystallized *types.CrystallizedState) ([]int, int, error) {
+	attesterCount := math.Min(params.MinCommiteeSize, float64(crystallized.ValidatorsLength()))


Thiis isn’t your code but can you convert math.Min to an if condition? It’s more verbose but we shouldn’t convert to a float. Pretty sure there’s a subtle bug here due to the conversion.

rawfalafel · 2018-08-22T20:16:07Z

beacon-chain/blockchain/service.go

-			activeState, err := c.chain.computeNewActiveState(common.BytesToHash([]byte(string(timestamp))))
+			// Process block as a validator if beacon node has registered, else process block as an observer.
+			if c.validator {
+				canProcess, err = c.chain.CanProcessBlock(c.web3Service.Client(), block, true)


I don't think this comment was addressed. Specifically, this validation step should come before updateHead.

prestonvanloon · 2018-08-22T23:23:56Z

beacon-chain/casper/incentives.go

+
+// CalculateRewardsFFG adjusts validators balances by applying rewards or penalties
+// based on FFG incentive structure.
+func CalculateRewardsFFG(active *types.ActiveState, crystallized *types.CrystallizedState, block *types.Block) error {


Can we drop the FFG suffix? If this package is the FFG implementation of casper then this is redundant.

prestonvanloon · 2018-08-22T23:58:09Z

beacon-chain/blockchain/core.go

+		return nil, err
+	}
+	if headExists {
+		bytes, err := b.db.Get([]byte(canonicalHeadKey))


This code is not tested. Should we have a test for when the head block exists in the database?

prestonvanloon · 2018-08-22T23:59:56Z

beacon-chain/casper/sharding.go

+// ValidatorsByHeightShard splits a shuffled validator list by height and by shard,
+// it ensures there's enough validators per height and per shard, if not, it'll skip
+// some heights and shards.
+func ValidatorsByHeightShard(crystallized *types.CrystallizedState) ([]*BeaconCommittee, error) {


This method has no test coverage. Please add test

prestonvanloon · 2018-08-23T00:00:55Z

beacon-chain/casper/validator.go

+}
+
+// SampleAttestersAndProposers returns lists of random sampled attesters and proposer indices.
+func SampleAttestersAndProposers(seed common.Hash, crystallized *types.CrystallizedState) ([]int, int, error) {


This method is untested

prestonvanloon · 2018-08-23T00:02:26Z

beacon-chain/types/state.go

@@ -112,6 +126,9 @@ func (a *ActiveState) NewPendingAttestation(record *pb.AttestationRecord) {

 // LatestPendingAttestation returns the latest pending attestaton fields.
 func (a *ActiveState) LatestPendingAttestation() *pb.AttestationRecord {


This method is also untested. Please consider adding a test for this method and the new lines added

terencechain

👍 👍 👍

rauljordan added 2 commits August 20, 2018 18:31

begin with a block/state dag approach in prep for fork choice

e614a61

add TODOs for fork choice items, add data structure for forks

c458839

rauljordan added Bigger Project labels Aug 21, 2018

rauljordan added this to the Ruby milestone Aug 21, 2018

rauljordan self-assigned this Aug 21, 2018

rauljordan added 12 commits August 21, 2018 11:03

remove syncing and processing states for incoming block sync

5b17bcc

simulator only broadcasts blocks now, no more state simulation

19cf1f7

fix sim tests, no more state sim

cf891d6

bazel rerun

03cbd09

naive fork choice

a409e71

split update head routine

441d2b2

pesky race conditions

9c9b90b

fork choice rule works

b977893

dag + fork choice working

dc42514

canonical head storage across sessions

2e62c86

todo: save dag

eb85ea1

yay supports forking

840fe13

rauljordan requested review from prestonvanloon, terencechain, nisdas and rawfalafel and removed request for prestonvanloon and terencechain August 21, 2018 23:22

rawfalafel reviewed Aug 22, 2018

View reviewed changes

terencechain requested changes Aug 22, 2018

View reviewed changes

rauljordan added 4 commits August 22, 2018 12:07

refactor blockhashforslot

26fe52c

event feed for incoming blocks in chainservice

a5d686a

use config

3fa0fd0

addressed all comments

a369f09

rawfalafel suggested changes Aug 22, 2018

View reviewed changes

rauljordan added 4 commits August 22, 2018 14:56

fix typo

f1eed9c

address yutaro comment

d174328

Merge branch 'master' into block-dag

e257790

using db interface

53f34ae

prestonvanloon reviewed Aug 23, 2018

View reviewed changes

rauljordan added 11 commits August 22, 2018 22:25

check if parent hash in previous slot DAG

f40a35f

works

097720a

tests

645f7ae

drop ffg suffix

323b582

bazel gazelle

dd22c62

full cov validators by height shard

312e1dd

gazelle

005af28

state tests

1fdd76f

all state tests

28205da

100% cov types and 99% cover casper

001af0d

cov up

7687741

rawfalafel approved these changes Aug 23, 2018

View reviewed changes

rauljordan added 2 commits August 23, 2018 13:04

80% blockchain cov

99ba6b1

fix lint

7996214

terencechain approved these changes Aug 23, 2018

View reviewed changes

terencechain mentioned this pull request Aug 24, 2018

Part 4 of Aligning Beacon Chain with Latest 2.1 - Bootstrapping Genesis #430

Merged

prestonvanloon approved these changes Aug 24, 2018

View reviewed changes

rauljordan merged commit 4a1b5db into prysmaticlabs:master Aug 24, 2018

rauljordan mentioned this pull request Aug 30, 2018

Implement Fork Choice Rule for Beacon Chain #255

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store Incoming Blocks and Calculated States in DAG #422

Store Incoming Blocks and Calculated States in DAG #422

rauljordan commented Aug 21, 2018 •

edited

Loading

rawfalafel Aug 22, 2018

rawfalafel Aug 22, 2018

rauljordan Aug 22, 2018

rawfalafel Aug 22, 2018

rawfalafel Aug 22, 2018

rauljordan Aug 22, 2018

rawfalafel left a comment

rawfalafel Aug 22, 2018

terencechain Aug 22, 2018

rauljordan Aug 22, 2018

terencechain Aug 22, 2018

nisdas Aug 22, 2018

rauljordan Aug 22, 2018

terencechain Aug 22, 2018

terencechain Aug 22, 2018

terencechain Aug 22, 2018

rawfalafel left a comment

rawfalafel Aug 22, 2018

rawfalafel Aug 22, 2018

prestonvanloon Aug 22, 2018

prestonvanloon Aug 22, 2018

prestonvanloon Aug 22, 2018

prestonvanloon Aug 23, 2018

prestonvanloon Aug 23, 2018

terencechain left a comment

		@@ -237,55 +206,102 @@ func (c ChainService) CanonicalCrystallizedStateEvent() <-chan types.Crystalli

		// run processes the changes needed every beacon chain block,

		@@ -112,6 +126,9 @@ func (a ActiveState) NewPendingAttestation(record pb.AttestationRecord) {

		// LatestPendingAttestation returns the latest pending attestaton fields.
		func (a ActiveState) LatestPendingAttestation() pb.AttestationRecord {

Store Incoming Blocks and Calculated States in DAG #422

Store Incoming Blocks and Calculated States in DAG #422

Conversation

rauljordan commented Aug 21, 2018 • edited Loading

Scope

How to Run

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

rawfalafel 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rawfalafel 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

Choose a reason for hiding this comment

terencechain left a comment

Choose a reason for hiding this comment

rauljordan commented Aug 21, 2018 •

edited

Loading