Skip to content

Commit 774ad73

Browse files
mjfhmjfh
authored
Update pending and finalised registry (#3844)
* Introduce resolved finalised values as pair `(BlockNumber,Hash32)` why Tackle runaway values when the hash from `pendingFCU` does not refer to the block number, anymore. This happened because there was an assumption that the `pendingFCU` corresponds to `latestFinalizedBlockNumber` if the latter one was non-zero. So `pendingFCU` became a misnomer and accordingly was treated wrongly (e.g. in `forkchoiceUpdated()`.) * Cosmetics, update comments and logging, remove cruft
1 parent 1bc91bd commit 774ad73

File tree

7 files changed

+70
-70
lines changed

7 files changed

+70
-70
lines changed

‎execution_chain/beacon/api_handler/api_forkchoice.nim‎

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,9 @@ proc forkchoiceUpdated*(ben: BeaconEngineRef,
8686
warn "Forkchoice requested update to zero hash"
8787
return simpleFCU(PayloadExecutionStatus.invalid)
8888

89-
chain.pendingFCU = update.finalizedBlockHash
89+
# Try updateing the finalised header argument by hash. If unsuccessful,
90+
# the hash will be stored in `pendingFCU`. Otherwise, hash and block
91+
# number will have been stored in `latestFinalized`.
9092
com.resolveFinHash(update.finalizedBlockHash)
9193

9294
# Check whether we have the block yet in our database or not. If not, we'll
@@ -114,8 +116,9 @@ proc forkchoiceUpdated*(ben: BeaconEngineRef,
114116
base = chain.baseNumber,
115117
finHash= update.finalizedBlockHash.short,
116118
safe = update.safeBlockHash.short,
117-
pendingFCU = chain.finHash.short,
118-
resolvedFin= chain.resolvedFinNumber
119+
pendingFCU = chain.pendingFCU.short,
120+
resolvedFinNum = chain.resolvedFinNumber,
121+
resolvedFinHash = chain.resolvedFinHash.short
119122

120123
# Inform the header chain cache (used by the syncer)
121124
com.headerChainUpdate(header, update.finalizedBlockHash)
@@ -166,8 +169,9 @@ proc forkchoiceUpdated*(ben: BeaconEngineRef,
166169
headHash = headHash.short,
167170
headNumber = header.number,
168171
base = chain.baseNumber,
169-
pendingFCU = chain.finHash.short,
170-
resolvedFin= chain.resolvedFinNumber
172+
pendingFCU = chain.pendingFCU.short,
173+
resolvedFinNum = chain.resolvedFinNumber,
174+
resolvedFinHash = chain.resolvedFinHash.short
171175
return validFCU(Opt.none(Bytes8), headHash)
172176

173177
# If the beacon client also advertised a finalized block, mark the local
@@ -224,6 +228,7 @@ proc forkchoiceUpdated*(ben: BeaconEngineRef,
224228
base = chain.baseNumber,
225229
baseHash = chain.baseHash.short,
226230
finalizedHash = finalizedBlockHash.short,
227-
resolvedFin = chain.resolvedFinNumber
231+
resolvedFinNum = chain.resolvedFinNumber,
232+
resolvedFinHash = chain.resolvedFinHash.short
228233

229234
return validFCU(Opt.none(Bytes8), headHash)

‎execution_chain/core/chain/forked_chain.nim‎

Lines changed: 17 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -401,7 +401,8 @@ proc processUpdateBase(c: ForkedChainRef): Future[Result[void, string]] {.async:
401401
base = c.base.number,
402402
baseHash = c.base.hash.short,
403403
pendingFCU = c.pendingFCU.short,
404-
resolvedFin = c.latestFinalizedBlockNumber,
404+
resolvedFinNum = c.latestFinalized.number,
405+
resolvedFinHash = c.latestFinalized.hash.short,
405406
dbSnapshotsCount = c.baseTxFrame.aTx.db.snapshots.len()
406407
else:
407408
debug "Finalized blocks persisted",
@@ -410,7 +411,8 @@ proc processUpdateBase(c: ForkedChainRef): Future[Result[void, string]] {.async:
410411
base = c.base.number,
411412
baseHash = c.base.hash.short,
412413
pendingFCU = c.pendingFCU.short,
413-
resolvedFin = c.latestFinalizedBlockNumber,
414+
resolvedFinNum = c.latestFinalized.number,
415+
resolvedFinHash = c.latestFinalized.hash.short,
414416
dbSnapshotsCount = c.baseTxFrame.aTx.db.snapshots.len()
415417
c.lastBaseLogTime = time
416418
c.persistedCount = 0
@@ -475,8 +477,7 @@ proc validateBlock(c: ForkedChainRef,
475477

476478
if blkHash == c.pendingFCU:
477479
# Resolve the hash into latestFinalizedBlockNumber
478-
c.latestFinalizedBlockNumber = max(blk.header.number,
479-
c.latestFinalizedBlockNumber)
480+
discard c.tryUpdatePendingFCU(blkHash, blk.header.number)
480481

481482
let
482483
# As a memory optimization we move the HashKeys (kMap) stored in the
@@ -490,7 +491,7 @@ proc validateBlock(c: ForkedChainRef,
490491
# TODO shortLog-equivalent for eth types
491492
debug "Validating block",
492493
blkHash, blk = (
493-
parentHash: blk.header.parentHash,
494+
parentHash: blk.header.parentHash.short,
494495
coinbase: blk.header.coinbase,
495496
stateRoot: blk.header.stateRoot,
496497
transactionsRoot: blk.header.transactionsRoot,
@@ -532,14 +533,14 @@ proc validateBlock(c: ForkedChainRef,
532533
let
533534
offset = c.baseDistance + c.persistBatchSize
534535
number =
535-
if offset >= c.latestFinalizedBlockNumber:
536+
if offset >= c.latestFinalized.number:
536537
0.uint64
537538
else:
538-
c.latestFinalizedBlockNumber - offset
539-
if c.pendingFCU != zeroHash32 and
540-
c.base.number < number:
539+
c.latestFinalized.number - offset
540+
if c.base.number < number:
541+
# 0 < number => latestFinalized.hash != zero
541542
let
542-
base = c.calculateNewBase(c.latestFinalizedBlockNumber, c.latest)
543+
base = c.calculateNewBase(c.latestFinalized.number, c.latest)
543544
prevBase = c.base.number
544545

545546
c.updateFinalized(base, base)
@@ -698,7 +699,7 @@ proc importBlock*(c: ForkedChainRef, blk: Block):
698699
# Setting the finalized flag to true here has the effect of skipping the
699700
# stateroot check for performance reasons.
700701
let
701-
isFinalized = blk.header.number <= c.latestFinalizedBlockNumber
702+
isFinalized = blk.header.number <= c.latestFinalized.number
702703
parent = ?(await c.validateBlock(parent, blk, isFinalized))
703704
if c.quarantine.hasOrphans():
704705
c.queueOrphan(parent, isFinalized)
@@ -797,11 +798,11 @@ template queueForkChoice*(c: ForkedChainRef,
797798
await c.queue.addLast(item)
798799
item.responseFut
799800

800-
func finHash*(c: ForkedChainRef): Hash32 =
801-
c.pendingFCU
801+
func resolvedFinHash*(c: ForkedChainRef): Hash32 =
802+
c.latestFinalized.hash
802803

803804
func resolvedFinNumber*(c: ForkedChainRef): uint64 =
804-
c.latestFinalizedBlockNumber
805+
c.latestFinalized.number
805806

806807
func haveBlockAndState*(c: ForkedChainRef, blockHash: Hash32): bool =
807808
## Blocks still in memory with it's txFrame
@@ -899,7 +900,7 @@ proc headerByNumber*(c: ForkedChainRef, number: BlockNumber): Result[Header, str
899900
err("Block not found, number = " & $number)
900901

901902
func finalizedHeader*(c: ForkedChainRef): Header =
902-
c.hashToBlock.withValue(c.pendingFCU, loc):
903+
c.hashToBlock.withValue(c.latestFinalized.hash, loc):
903904
return loc[].header
904905

905906
c.base.header
@@ -911,7 +912,7 @@ func safeHeader*(c: ForkedChainRef): Header =
911912
c.base.header
912913

913914
func finalizedBlock*(c: ForkedChainRef): Block =
914-
c.hashToBlock.withValue(c.pendingFCU, loc):
915+
c.hashToBlock.withValue(c.latestFinalized.hash, loc):
915916
return loc[].blk
916917

917918
c.base.blk

‎execution_chain/core/chain/forked_chain/chain_desc.nim‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -71,9 +71,9 @@ type
7171

7272
pendingFCU* : Hash32
7373
# When we know finalizedHash from CL but has yet to resolve
74-
# the hash into a latestFinalizedBlockNumber
74+
# the hash into a `latestFinalized` hash/number pair
7575

76-
latestFinalizedBlockNumber*: uint64
76+
latestFinalized*: FcuHashAndNumber
7777
# When our latest imported block is far away from
7878
# latestFinalizedBlockNumber, we can move the base
7979
# forward when importing block
@@ -108,10 +108,10 @@ func txRecords*(c: ForkedChainRef): var Table[Hash32, (Hash32, uint64)] =
108108
c.txRecords
109109

110110
func tryUpdatePendingFCU*(c: ForkedChainRef, finHash: Hash32, number: uint64): bool =
111-
if number > c.latestFinalizedBlockNumber:
112-
c.latestFinalizedBlockNumber = number
113-
c.pendingFCU = finHash
111+
c.pendingFCU = zeroHash32
112+
if number > c.latestFinalized.number:
113+
c.latestFinalized = FcuHashAndNumber(number: number, hash: finHash)
114114
return true
115-
false
115+
# false
116116

117117
# End

‎execution_chain/core/chain/forked_chain/chain_serialize.nim‎

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ type
3636
latest: uint
3737
heads: seq[uint]
3838
pendingFCU: Hash32
39-
latestFinalizedBlockNumber: uint64
39+
latestFinalized: FcuHashAndNumber
4040
txRecords: seq[TxRecord]
4141
fcuHead: FcuHashAndNumber
4242
fcuSafe: FcuHashAndNumber
@@ -65,7 +65,7 @@ proc append(w: var RlpWriter, fc: ForkedChainRef) =
6565

6666
w.append(heads)
6767
w.append(fc.pendingFCU)
68-
w.append(fc.latestFinalizedBlockNumber)
68+
w.append(fc.latestFinalized)
6969
w.startList(fc.txRecords.len)
7070
for k, v in fc.txRecords:
7171
w.append(TxRecord(
@@ -90,7 +90,7 @@ proc read(rlp: var Rlp, T: type FcState): T {.raises: [RlpError].} =
9090
rlp.read(result.latest)
9191
rlp.read(result.heads)
9292
rlp.read(result.pendingFCU)
93-
rlp.read(result.latestFinalizedBlockNumber)
93+
rlp.read(result.latestFinalized)
9494
rlp.read(result.txRecords)
9595
rlp.read(result.fcuHead)
9696
rlp.read(result.fcuSafe)
@@ -189,7 +189,7 @@ proc reset(fc: ForkedChainRef, base: BlockRef) =
189189
fc.heads = @[base]
190190
fc.hashToBlock = {base.hash: base}.toTable
191191
fc.pendingFCU = zeroHash32
192-
fc.latestFinalizedBlockNumber = 0'u64
192+
fc.latestFinalized.reset()
193193
fc.txRecords.clear()
194194
fc.fcuHead.reset()
195195
fc.fcuSafe.reset()
@@ -224,8 +224,8 @@ proc serialize*(fc: ForkedChainRef, txFrame: CoreDbTxRef): Result[void, CoreDbEr
224224
latestHash=fc.latest.hash.short,
225225
head=fc.fcuHead.number,
226226
headHash=fc.fcuHead.hash.short,
227-
finalized=fc.latestFinalizedBlockNumber,
228-
finalizedHash=fc.pendingFCU.short,
227+
finalizedNum=fc.latestFinalized.number,
228+
finalizedHash=fc.latestFinalized.hash.short,
229229
blocksSerialized=fc.hashToBlock.len,
230230
heads=fc.heads.toString
231231

@@ -270,14 +270,15 @@ proc deserialize*(fc: ForkedChainRef): Result[void, string] =
270270
fc.heads.add blocks[h]
271271

272272
fc.pendingFCU = state.pendingFCU
273-
fc.latestFinalizedBlockNumber = state.latestFinalizedBlockNumber
273+
fc.latestFinalized = state.latestFinalized
274274
fc.fcuHead = state.fcuHead
275275
fc.fcuSafe = state.fcuSafe
276276

277277
info "Loading block DAG from database",
278278
base=fc.base.number,
279279
pendingFCU=fc.pendingFCU.short,
280-
resolvedFin=fc.latestFinalizedBlockNumber,
280+
resolvedFinNum=fc.latestFinalized.number,
281+
resolvedFinHash=fc.latestFinalized.hash.short,
281282
canonicalHead=fc.fcuHead.number,
282283
safe=fc.fcuSafe.number,
283284
numBlocks=state.numBlocks,
@@ -317,11 +318,11 @@ proc deserialize*(fc: ForkedChainRef): Result[void, string] =
317318
let txFrame = val[].txFrame
318319
?txFrame.fcuSafe(fc.fcuSafe.hash, fc.fcuSafe.number)
319320

320-
fc.hashToBlock.withValue(fc.pendingFCU, val) do:
321+
fc.hashToBlock.withValue(fc.latestFinalized.hash, val) do:
321322
# Restore finalized marker
322323
for it in loopNotFinalized(val[]):
323324
it.finalize()
324325
let txFrame = val[].txFrame
325-
?txFrame.fcuFinalized(fc.pendingFCU, fc.latestFinalizedBlockNumber)
326+
?txFrame.fcuFinalized(fc.latestFinalized.hash, fc.latestFinalized.number)
326327

327328
ok()

‎execution_chain/core/chain/header_chain_cache.nim‎

Lines changed: 15 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -294,6 +294,7 @@ proc resolveFinHash(hc: HeaderChainRef, f: Hash32) =
294294
hc.chain.quarantine.getHeader(f).map(toNumber) or hc.kvt.getNumber(f) or
295295
hc.chain.headerByHash(f).map(toNumber)
296296
).valueOr:
297+
hc.chain.pendingFCU = f
297298
return
298299

299300
if hc.chain.tryUpdatePendingFCU(f, number):
@@ -324,6 +325,7 @@ proc headUpdateFromCL(hc: HeaderChainRef; h: Header; f: Hash32) =
324325
# Update `FC` module
325326
hc.chain.pendingFCU = f
326327
if f == hc.session.headHash:
328+
# Note that `tryUpdatePendingFCU()` wil reset `pendingFCU`.
327329
discard hc.chain.tryUpdatePendingFCU(f, h.number)
328330

329331
# Inform client app about that a new session has started.
@@ -581,30 +583,19 @@ proc commit*(hc: HeaderChainRef): Result[void,string] =
581583
hc.session.mode = locked # update internal state
582584
return ok()
583585

584-
block assignFinalisedChild:
585-
# Use `finalised` only if it is on the header chain as well
586-
let fin = hc.kvt.getHeader(hc.chain.pendingFCU).valueOr:
587-
break assignFinalisedChild
588-
589-
if hc.chain.baseNumber() < fin.number:
590-
# Now, there are two segments of the canonical chain, `base..finalised`
591-
# on# the `FC` module and `ante..finalised` (maybe degraded) on the
592-
# header chain cache.
593-
#
594-
# So `finalised` is on the header chain cache and has a parent on the
595-
# `FC` module.
596-
if hc.chain.hashToBlock.hasKey(hc.chain.pendingFCU):
597-
hc.session.ante = fin
598-
hc.session.mode = locked # update internal state
599-
metrics.set(nec_sync_dangling, fin.number.int64)
600-
return ok()
601-
602-
# Impossible situation!
603-
raiseAssert MsgPfx &
604-
"Missing finalised " & $fin.number & " parent on FC module" &
605-
", base=" & $hc.chain.baseNumber &
606-
", head=" & $hc.session.head.number &
607-
", finalized=" & $hc.chain.latestFinalizedBlockNumber
586+
if hc.chain.pendingFCU != zeroHash32:
587+
hc.resolveFinHash(hc.chain.pendingFCU)
588+
let finNum = hc.chain.latestFinalized.number
589+
590+
# Use `finalised` only if it is on the header chain as well
591+
if hc.chain.baseNumber() < finNum:
592+
let finHash = hc.chain.latestFinalized.hash
593+
hc.session.ante =
594+
if hc.session.head.number <= finNum: hc.session.head
595+
else: hc.kvt.getHeader(finHash).expect "valid header"
596+
hc.session.mode = locked # update internal state
597+
metrics.set(nec_sync_dangling, hc.session.ante.number.int64)
598+
return ok()
608599

609600
hc.session.mode = orphan
610601
err("Parent on FC module has been lost: obsolete branch segment")

‎execution_chain/sync/beacon/worker/blocks/blocks_fetch.nim‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -115,11 +115,12 @@ template fetchBodies*(
115115
# Just return `failed` (no error count or throughput stats)
116116
discard
117117

118-
chronicles.info trEthRecvReceivedBlockBodies & " error", peer,
118+
debug trEthRecvReceivedBlockBodies & " error", peer,
119119
startHash=request.blockHashes[0].short, nReq,
120120
ela=rc.error.elapsed.toStr, state=($buddy.syncState),
121-
errorClass=rc.error.excp, error=rc.error.name, msg=rc.error.msg,
122-
nErrors=buddy.nErrors.fetch.bdy
121+
error=($rc.error.excp & (if rc.error.name.len == 0: ""
122+
else: "(" & rc.error.name & ")")),
123+
msg=rc.error.msg, nErrors=buddy.nErrors.fetch.bdy
123124
break body # return err()
124125

125126
# Evaluate result
@@ -129,7 +130,7 @@ template fetchBodies*(
129130
trace trEthRecvReceivedBlockBodies, peer,
130131
startHash=request.blockHashes[0].short, nReq, nResp=0,
131132
ela=elapsed.toStr, state=($buddy.syncState),
132-
errorClass=(if rc.isErr: $rc.error.excp else: "n/a"),
133+
error=(if rc.isErr: $rc.error.excp else: "n/a"),
133134
nErrors=buddy.nErrors.fetch.bdy
134135
break body # return err()
135136

‎execution_chain/sync/beacon/worker/headers/headers_fetch.nim‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -132,11 +132,12 @@ template fetchHeadersReversed*(
132132
# Just return `failed` (no error count or throughput stats)
133133
discard
134134

135-
chronicles.info trEthRecvReceivedBlockHeaders & ": error", peer,
135+
debug trEthRecvReceivedBlockHeaders & ": error", peer,
136136
req=ivReq, nReq=req.maxResults, hash=topHash.toStr,
137137
ela=rc.error.elapsed.toStr, state=($buddy.syncState),
138-
errorClass=rc.error.excp, error=rc.error.name, msg=rc.error.msg,
139-
nErrors=buddy.nErrors.fetch.hdr
138+
error=($rc.error.excp & (if rc.error.name.len == 0: ""
139+
else: "(" & rc.error.name & ")")),
140+
msg=rc.error.msg, nErrors=buddy.nErrors.fetch.hdr
140141
break body # return err()
141142

142143
# Evaluate result
@@ -146,7 +147,7 @@ template fetchHeadersReversed*(
146147
trace trEthRecvReceivedBlockHeaders, peer, nReq=req.maxResults,
147148
hash=topHash.toStr, nResp=0, ela=elapsed.toStr,
148149
state=($buddy.syncState),
149-
errorClass=(if rc.isErr: $rc.error.excp else: "n/a"),
150+
error=(if rc.isErr: $rc.error.excp else: "n/a"),
150151
nErrors=buddy.nErrors.fetch.hdr
151152
break body # return err()
152153

0 commit comments

Comments
 (0)