Attest immediately if block from proposer - fix #966 #1014
Conversation
|
|
|
And |
|
Ok so if it's added in |
| @@ -198,6 +202,7 @@ type | |||
| ## has advanced without blocks | |||
|
|
|||
| Head* = object | |||
| # TODO: delete - all BlockRef tracks the justified checkpoint | |||
There was a problem hiding this comment.
well, head still needs to stay..
There was a problem hiding this comment.
The field, but it can be a regular BlockRef
| 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.
process_block_header requires that the correct proposer was used:
So what can happen is:
- (a) You found a block at that exact slot in the blockpool so it was run through
process_block_headerand you can attest to it as it was chacked to be from the expected proposer - (b) You didn't find a block at that exact slot in the blockpool so you vote for one of the head you have but the blck.slot is different from the current slot.
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.
| timeout = cutoffFromNow | ||
|
|
||
| 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 poll() so Chronos can do maintenance there, like stopping to yield the closure iterator, but in my testing the node gets stuck. Pointer appreciated:
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?
mratsim
force-pushed
the
fix-attestation-signing-schedule
branch
from
96ba039 to
ca2a08e
Compare
mratsim
force-pushed
the
fix-attestation-signing-schedule
branch
from
ca2a08e to
afd1650
Compare
| 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.
AsyncEvent that fires on updateHead is likely what you're looking for here - ping @cheatfate - no need for this polling
| @@ -170,6 +170,10 @@ type | |||
|
|
|||
| slot*: Slot # TODO could calculate this by walking to root, but.. | |||
|
|
|||
| # Cache for ProtoArray fork-choice | |||
There was a problem hiding this comment.
something I forgot until we talked about it in the other PR: BlockRef is not the right granularity for these checkpoints: epoch - that means you need to attach them to a (BlockRef, Slot) tuple, or BlockSlot, more simply, the issue being that epoch processing happens regardless if there's a block or not on the epoch boundary..
|
The codebase has changed significantly since this PR was made, and we're now closer to a principled solution to this - we can use #1700 to track the feature - closing PR as the code is no longer relevant, except as reference for a future implementation |
This PR does 2 things:
Fix Attestation duties: always waiting for 2 seconds #966, when we receive a block from the expected proposer, we attest immediately
BlockRef now stores the justified and finalized checkpoints which will avoid rewinding state in the fork choice PR at https://github.com/status-im/nim-beacon-chain/pull/965/files#diff-a007e749710608c06418bb9fcdf140dbR276-R302, https://github.com/status-im/nim-beacon-chain/blob/7dba780a1d2503c04bcaf360b1c1d8410807dd18/beacon_chain/attestation_pool.nim#L276-L302