Attest immediately if block from proposer - fix #966 #1014
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,7 +7,7 @@ | |
|
|
||
| import | ||
| # Standard library | ||
| os, tables, strutils, times, atomics, | ||
|
|
||
| # Nimble packages | ||
| stew/[objects, bitseqs], stew/shims/macros, | ||
|
|
@@ -352,9 +352,21 @@ proc broadcastAggregatedAttestations( | |
| state.genesis_validators_root)) | ||
| node.network.broadcast(node.topicAggregateAndProofs, signedAP) | ||
|
|
||
| func isBlockFromExpectedProposerAtSlot(blck: BlockRef, targetSlot: Slot): bool = | ||
| ## Returns true if the block is from the expected proposer | ||
| ## | ||
| ## This is true if-and-only-if the block slot matches the target slot | ||
|
There was a problem hiding this comment. it doesn't check the proposer actually - ie if someone from an alternate history posts a block, this check will misfire There was a problem hiding this comment.
So what can happen is:
There was a problem hiding this comment. mm.. ok - so it's only because it's used on the head block that the function lives up to its name - I wonder if it wouldn't be more simple to just have the slot comparison with a comment that we're waiting for the the proposed block of that particular slot. |
||
| blck.slot == targetSlot | ||
|
|
||
| template displayParent(blck: BlockRef): string = | ||
| if blck.parent.isNil: | ||
| "orphan" | ||
| else: | ||
| shortLog(blck.parent.root) | ||
|
|
||
| proc handleValidatorDuties*( | ||
| node: BeaconNode, head: BlockRef, lastSlot, slot: Slot): Future[BlockRef] {.async.} = | ||
| ## Perform validator duties - create blocks, vote and aggregate existing votes | ||
| if node.attachedValidators.count == 0: | ||
| # Nothing to do because we have no validator attached | ||
| return head | ||
|
|
@@ -406,10 +418,75 @@ proc handleValidatorDuties*( | |
|
|
||
| # https://github.com/ethereum/eth2.0-specs/blob/v0.11.1/specs/phase0/validator.md#attesting | ||
| # A validator should create and broadcast the attestation to the associated | ||
| # attestation subnet when either | ||
| # (a) the validator has received a valid | ||
| # block from the expected block proposer for the assigned slot or | ||
| # (b) one-third of the slot has transpired (`SECONDS_PER_SLOT / 3` seconds | ||
| # after the start of slot) -- whichever comes first. | ||
|
|
||
| # Poor man's AsyncChannel to notify timeout | ||
| # TODO: refactor with a Producer->Consumer mode of operation | ||
| var timedOut: Atomic[bool] | ||
| timedOut.store(false, moRelaxed) | ||
| let timeOutNotifChannel = timedOut.addr | ||
|
|
||
| proc waitForProposedBlock(node: BeaconNode, slot: Slot, timeOutNotifChannel: ptr Atomic[bool]): Future[void] {.async.} = | ||
|
There was a problem hiding this comment.
|
||
| # Wait until the head is from the expected proposer | ||
| # TODO: refactor with a Producer->Consumer mode of operation | ||
| var head = node.updateHead(logNoUpdate = true) | ||
| while not head.isBlockFromExpectedProposerAtSlot(slot) and not timeOutNotifChannel[].load(moRelaxed): | ||
| await sleepAsync(chronos.milliseconds(50)) # Timers are expensive and it takes 300~600ms to process a block at the moment | ||
| head = node.updateHead(logNoUpdate = false) | ||
|
|
||
| const attCutoff = chronos.seconds(SECONDS_PER_SLOT.int64 div 3) | ||
| let cutoffFromNow = node.beaconClock.fromNow(slot.toBeaconTime(attCutoff)) | ||
|
|
||
| if cutoffFromNow.inFuture: | ||
| let foundBlockInTime = await withTimeout( | ||
|
There was a problem hiding this comment. what cancels the poll loop on timeout? There was a problem hiding this comment. I'm not clear on how to implement timeout with Chronos: It contains a Otherwise I can use an AsyncChannel but I would be surprised if there wasn't a simple solution, cc @cheatfate There was a problem hiding this comment. yes but who the proposer is depends on which history you're following - isn't this "shortcut" only for when the proposer is from the same history as you? There was a problem hiding this comment. was this supposed to be here? #1014 (comment), is this unclear or did I miss something? |
||
| waitForProposedBlock(node, slot, timeOutNotifChannel), | ||
| cutoffFromNow.offset | ||
| ) | ||
|
|
||
| # Reload the head - this should be the same as in `waitForProposedBlock` | ||
| head = node.updateHead() | ||
|
|
||
| if foundBlockInTime: | ||
| info "Found a block from expected proposer, attesting immediately", | ||
| block_root = shortlog(head.root), | ||
| parent = head.displayParent(), | ||
| slot = head.slot, | ||
| justified = shortLog(head.justified_checkpoint), | ||
| finalized = shortLog(head.finalized_checkpoint) | ||
| else: | ||
| timeOutNotifChannel[].store(true, moRelaxed) | ||
|
|
||
| info "Timeout when waiting 2s from an expected block, attesting", | ||
| block_root = shortlog(head.root), | ||
| parent = head.displayParent(), | ||
| slot = head.slot, | ||
| justified = shortLog(head.justified_checkpoint), | ||
| finalized = shortLog(head.finalized_checkpoint) | ||
| else: | ||
| # Reload the head | ||
| head = node.updateHead() | ||
|
|
||
| info "Late for attesting", | ||
| block_root = shortlog(head.root), | ||
| parent = head.displayParent(), | ||
| slot = head.slot, | ||
| justified = shortLog(head.justified_checkpoint), | ||
| finalized = shortLog(head.finalized_checkpoint), | ||
| blockFromExpectedProposer = head.isBlockFromExpectedProposerAtSlot(slot) | ||
|
|
||
| # Attest to the head we found | ||
| handleAttestations(node, head, slot) | ||
|
|
||
| # https://github.com/ethereum/eth2.0-specs/blob/v0.11.1/specs/phase0/validator.md#broadcast-aggregate | ||
| # If the validator is selected to aggregate (is_aggregator), then they | ||
| # broadcast their best aggregate as a SignedAggregateAndProof to the global | ||
| # aggregate channel (beacon_aggregate_and_proof) two-thirds of the way | ||
| # through the slot-that is, SECONDS_PER_SLOT * 2 / 3 seconds after the start | ||
| # of slot. | ||
| template sleepToSlotOffset(extra: chronos.Duration, msg: static string) = | ||
| let | ||
| fromNow = node.beaconClock.fromNow(slot.toBeaconTime(extra)) | ||
|
|
@@ -423,19 +500,9 @@ proc handleValidatorDuties*( | |
| await sleepAsync(fromNow.offset) | ||
|
|
||
| # Time passed - we might need to select a new head in that case | ||
| head = node.updateHead(logNoUpdate = false) | ||
|
|
||
| # TODO: this should probably be independent from block-proposal and signing | ||
| if slot > 2: | ||
| sleepToSlotOffset( | ||
| seconds(int64(SECONDS_PER_SLOT * 2) div 3), | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
something I forgot until we talked about it in the other PR:
BlockRefis not the right granularity for these checkpoints:epoch- that means you need to attach them to a(BlockRef, Slot)tuple, orBlockSlot, more simply, the issue being that epoch processing happens regardless if there's a block or not on the epoch boundary..