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.

Sign up for GitHub

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

Fork choice fixes 5 #1381

Merged
merged 5 commits into from
Jul 28, 2020
Merged

Fork choice fixes 5 #1381

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d7aa228
limit attestations kept in attestation pool
arnetheduck Jul 27, 2020
c27fb5b
remove CandidateChains.justifiedState
arnetheduck Jul 27, 2020
819686f
remove BlockPools.Head object
arnetheduck Jul 28, 2020
8385616
avoid quadratic quarantine loop
arnetheduck Jul 28, 2020
6ae9421
fix
arnetheduck Jul 28, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion beacon_chain/attestation_aggregation.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ proc isValidAttestation*(
# therefore propagate faster, thus reordering their arrival in some nodes
let attestationBlck = pool.blockPool.getRef(attestation.data.beacon_block_root)
if attestationBlck.isNil:
debug "block doesn't exist in block pool"
debug "Block not found"
pool.blockPool.addMissing(attestation.data.beacon_block_root)
return false

Expand Down
173 changes: 50 additions & 123 deletions beacon_chain/attestation_pool.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,15 @@

import
# Standard libraries
std/[algorithm, deques, sequtils, tables, options],
std/[algorithm, deques, sequtils, sets, tables, options],
# Status libraries
chronicles, stew/[byteutils], json_serialization/std/sets,
chronicles, stew/[byteutils], json_serialization/std/sets as jsonSets,
# Internal
./spec/[beaconstate, datatypes, crypto, digest, helpers],
./block_pool, ./block_pools/candidate_chains, ./beacon_node_types,
./fork_choice/fork_choice

export beacon_node_types
export beacon_node_types, sets

logScope: topics = "attpool"

Expand All @@ -39,7 +39,7 @@ proc init*(T: type AttestationPool, blockPool: BlockPool): T =
doAssert blockPool.heads.len == 1, "Init only supports a single history"

var blocks: seq[BlockRef]
var cur = blockPool.head.blck
var cur = blockPool.head
while cur != blockPool.finalizedHead.blck:
blocks.add cur
cur = cur.parent
Expand All @@ -65,65 +65,11 @@ proc init*(T: type AttestationPool, blockPool: BlockPool): T =
finalized_root = shortlog(blockPool.finalizedHead.blck.root)

T(
mapSlotsToAttestations: initDeque[AttestationsSeen](),
blockPool: blockPool,
unresolved: initTable[Eth2Digest, UnresolvedAttestation](),
forkChoice: forkChoice
)

proc slotIndex(
pool: var AttestationPool, state: BeaconState, attestationSlot: Slot): int =
## Grow and garbage collect pool, returning the deque index of the slot

# We keep a sliding window of attestations, roughly from the last finalized
# epoch to now, because these are the attestations that may affect the voting
# outcome. Some of these attestations will already have been added to blocks,
# while others are fresh off the network.
# TODO only the latest vote of each validator counts. Can we use that somehow?
logScope: pcs = "atp_slot_maintenance"

doAssert attestationSlot >= pool.startingSlot,
"""
We should have checked in addResolved that attestation is newer than
finalized_slot and we never prune things before that, per below condition!
""" &
", attestationSlot: " & $shortLog(attestationSlot) &
", startingSlot: " & $shortLog(pool.startingSlot)

if pool.mapSlotsToAttestations.len == 0:
# Because the first attestations may arrive in any order, we'll make sure
# to start counting at the last finalized epoch start slot - anything
# earlier than that is thrown out by the above check
info "First attestation!",
attestationSlot = shortLog(attestationSlot)
pool.startingSlot =
state.finalized_checkpoint.epoch.compute_start_slot_at_epoch()

if pool.startingSlot + pool.mapSlotsToAttestations.lenu64 <= attestationSlot:
trace "Growing attestation pool",
attestationSlot = shortLog(attestationSlot),
startingSlot = shortLog(pool.startingSlot)

# Make sure there's a pool entry for every slot, even when there's a gap
while pool.startingSlot + pool.mapSlotsToAttestations.lenu64 <= attestationSlot:
pool.mapSlotsToAttestations.addLast(AttestationsSeen())

if pool.startingSlot <
state.finalized_checkpoint.epoch.compute_start_slot_at_epoch():
debug "Pruning attestation pool",
startingSlot = shortLog(pool.startingSlot),
finalizedSlot = shortLog(
state.finalized_checkpoint
.epoch.compute_start_slot_at_epoch())

# TODO there should be a better way to remove a whole epoch of stuff..
while pool.startingSlot <
state.finalized_checkpoint.epoch.compute_start_slot_at_epoch():
pool.mapSlotsToAttestations.popFirst()
pool.startingSlot += 1

int(attestationSlot - pool.startingSlot)

func processAttestation(
pool: var AttestationPool, participants: HashSet[ValidatorIndex],
block_root: Eth2Digest, target_epoch: Epoch) =
Expand All @@ -137,61 +83,56 @@ func addUnresolved(pool: var AttestationPool, attestation: Attestation) =
attestation: attestation,
)

proc addResolved(pool: var AttestationPool, blck: BlockRef, attestation: Attestation) =
logScope:
attestation = shortLog(attestation)

doAssert blck.root == attestation.data.beacon_block_root

# TODO Which state should we use to validate the attestation? It seems
# reasonable to involve the head being voted for as well as the intended
# slot of the attestation - double-check this with spec
func candidateIdx(pool: AttestationPool, slot: Slot): Option[uint64] =
if slot >= pool.startingSlot and
slot < (pool.startingSlot + pool.candidates.lenu64):
some(slot mod pool.candidates.lenu64)
else:
none(uint64)

# TODO: filter valid attestation as much as possible before state rewind
# TODO: the below check does not respect the inclusion delay
# we should use isValidAttestationSlot instead
if blck.slot > attestation.data.slot:
notice "Invalid attestation (too new!)",
blockSlot = shortLog(blck.slot)
proc updateCurrent(pool: var AttestationPool, wallSlot: Slot) =
if wallSlot + 1 < pool.candidates.lenu64:
return

if attestation.data.slot < pool.startingSlot:
# It can happen that attestations in blocks for example are included even
# though they no longer are relevant for finalization - let's clear
# these out
debug "Old attestation",
startingSlot = pool.startingSlot
if pool.startingSlot + pool.candidates.lenu64 - 1 > wallSlot:
error "Current slot older than attestation pool view, clock reset?",
poolSlot = pool.startingSlot, wallSlot
return

# if not isValidAttestationSlot(attestation.data.slot, blck.slot):
# # Logging in isValidAttestationSlot
# return
# As time passes we'll clear out any old attestations as they are no longer
# viable to be included in blocks

# Check that the attestation is indeed valid
if (let v = check_attestation_slot_target(attestation.data); v.isErr):
debug "Invalid attestation", err = v.error
return
let newWallSlot = wallSlot + 1 - pool.candidates.lenu64
for i in pool.startingSlot..newWallSlot:
pool.candidates[i.uint64 mod pool.candidates.lenu64] = AttestationsSeen()

# Get a temporary state at the (block, slot) targeted by the attestation
updateStateData(
pool.blockPool, pool.blockPool.tmpState,
BlockSlot(blck: blck, slot: attestation.data.slot),
true)
pool.startingSlot = newWallSlot

template state(): BeaconState = pool.blockPool.tmpState.data.data
proc addResolved(
pool: var AttestationPool, blck: BlockRef, attestation: Attestation,
wallSlot: Slot) =
# Add an attestation whose parent we know
logScope:
attestation = shortLog(attestation)

# TODO inefficient data structures..
updateCurrent(pool, wallSlot)

doAssert blck.root == attestation.data.beacon_block_root

let candidateIdx = pool.candidateIdx(attestation.data.slot)
if candidateIdx.isNone:
debug "Attestation slot out of range",
startingSlot = pool.startingSlot
return

var cache = getEpochCache(blck, state)
let
attestationSlot = attestation.data.slot
idx = pool.slotIndex(state, attestationSlot)
attestationsSeen = addr pool.mapSlotsToAttestations[idx]
epochRef = pool.blockPool.dag.getEpochRef(blck, attestation.data.target.epoch)
attestationsSeen = addr pool.candidates[candidateIdx.get]
validation = Validation(
aggregation_bits: attestation.aggregation_bits,
aggregate_signature: attestation.signature)
participants = get_attesting_indices(
state, attestation.data, validation.aggregation_bits, cache)
epochRef, attestation.data, validation.aggregation_bits)

var found = false
for a in attestationsSeen.attestations.mitems():
Expand Down Expand Up @@ -226,7 +167,6 @@ proc addResolved(pool: var AttestationPool, blck: BlockRef, attestation: Attesta
info "Attestation resolved",
attestation = shortLog(attestation),
validations = a.validations.len(),
current_epoch = get_current_epoch(state),
blockSlot = shortLog(blck.slot)

found = true
Expand All @@ -244,11 +184,12 @@ proc addResolved(pool: var AttestationPool, blck: BlockRef, attestation: Attesta

info "Attestation resolved",
attestation = shortLog(attestation),
current_epoch = get_current_epoch(state),
validations = 1,
blockSlot = shortLog(blck.slot)

proc addAttestation*(pool: var AttestationPool, attestation: Attestation) =
proc addAttestation*(pool: var AttestationPool,
attestation: Attestation,
wallSlot: Slot) =
Copy link
Contributor

Choose a reason for hiding this comment

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

Have yet to read the rest but, should wallSlot be a distinct type from Slot if mixing both may lead to subtle bugs?

Copy link
Member Author

Choose a reason for hiding this comment

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

well.. slots are like minutes basically, don't think a specific type is motivated. this code is a little wrong though, it should be a range of slots to handle clock disparity - but that is for a different PR

## Add a verified attestation to the fork choice context
logScope: pcs = "atp_add_attestation"

Expand All @@ -261,15 +202,14 @@ proc addAttestation*(pool: var AttestationPool, attestation: Attestation) =
pool.addUnresolved(attestation)
return

pool.addResolved(blck, attestation)
pool.addResolved(blck, attestation, wallSlot)

proc addForkChoice*(pool: var AttestationPool,
state: BeaconState,
blckRef: BlockRef,
blck: BeaconBlock,
wallSlot: Slot) =
## Add a verified block to the fork choice context
## The current justifiedState of the block pool is used as reference
let state = pool.forkChoice.process_block(
pool.blockPool, state, blckRef, blck, wallSlot)

Expand All @@ -288,29 +228,17 @@ proc getAttestationsForSlot*(pool: AttestationPool, newBlockSlot: Slot):
newBlockSlot = shortLog(newBlockSlot)
return none(AttestationsSeen)

if pool.mapSlotsToAttestations.len == 0: # startingSlot not set yet!
info "No attestations found (pool empty)",
newBlockSlot = shortLog(newBlockSlot)
return none(AttestationsSeen)

let
# TODO in theory we could include attestations from other slots also, but
# we're currently not tracking which attestations have already been included
# in blocks on the fork we're aiming for.. this is a conservative approach
# that's guaranteed to not include any duplicates, because it's the first
# time the attestations are up for inclusion!
attestationSlot = newBlockSlot - MIN_ATTESTATION_INCLUSION_DELAY
candidateIdx = pool.candidateIdx(attestationSlot)

if attestationSlot < pool.startingSlot or
attestationSlot >= pool.startingSlot + pool.mapSlotsToAttestations.lenu64:
if candidateIdx.isNone:
info "No attestations matching the slot range",
attestationSlot = shortLog(attestationSlot),
startingSlot = shortLog(pool.startingSlot),
endingSlot = shortLog(pool.startingSlot + pool.mapSlotsToAttestations.lenu64)
startingSlot = shortLog(pool.startingSlot)
return none(AttestationsSeen)

let slotDequeIdx = int(attestationSlot - pool.startingSlot)
some(pool.mapSlotsToAttestations[slotDequeIdx])
some(pool.candidates[candidateIdx.get()])

proc getAttestationsForBlock*(pool: AttestationPool,
state: BeaconState): seq[Attestation] =
Expand All @@ -337,8 +265,7 @@ proc getAttestationsForBlock*(pool: AttestationPool,
# addResolved, too, the new attestations get added to the end, while in
# these functions, it's reading from the beginning, et cetera. This all
# needs a single unified strategy.
const LOOKBACK_WINDOW = 3
for i in max(1, newBlockSlot.int64 - LOOKBACK_WINDOW) .. newBlockSlot.int64:
for i in max(1, newBlockSlot.int64 - ATTESTATION_LOOKBACK.int64) .. newBlockSlot.int64:
let maybeSlotData = getAttestationsForSlot(pool, i.Slot)
if maybeSlotData.isSome:
insert(attestations, maybeSlotData.get.attestations)
Expand Down Expand Up @@ -390,7 +317,7 @@ proc getAttestationsForBlock*(pool: AttestationPool,
attestationSlot = newBlockSlot - 1
return

proc resolve*(pool: var AttestationPool) =
proc resolve*(pool: var AttestationPool, wallSlot: Slot) =
## Check attestations in our unresolved deque
## if they can be integrated to the fork choice
logScope: pcs = "atp_resolve"
Expand All @@ -412,7 +339,7 @@ proc resolve*(pool: var AttestationPool) =
pool.unresolved.del(k)

for a in resolved:
pool.addResolved(a.blck, a.attestation)
pool.addResolved(a.blck, a.attestation, wallSlot)

proc selectHead*(pool: var AttestationPool, wallSlot: Slot): BlockRef =
let newHead = pool.forkChoice.find_head(wallSlot, pool.blockPool)
Expand Down
Loading