From 90dd86be9a3feba5037dd885e63423475dd3905f Mon Sep 17 00:00:00 2001 From: Jordan Hrycaj Date: Thu, 5 Dec 2024 06:01:57 +0000 Subject: [PATCH] Fc module can update base also when on parent arc (#2911) * Re-org internal descriptor `CanonicalDesc` as `PivotArc` why: Despite its name, `CanonicalDesc` contained a cursor arc (or leg) from the base tree with a designated block (or Header) on its arc members (aka blocks.) The type is used more generally than only for s block on the canonical cursor. Also, the `PivotArc` provides some more fields for caching intermediate data. This simplifies managing extra arguments for some functions. * Remove cruft details: No need to find cursor arc if it is given as function argument. * Rename prototype variables `head: PivotArc` to `pvarc` why: Better reading * Function and code massage, adjust names details: Avoid the syllable `canonical` in function names that do not strictly apply to the canonical chain. So renaming * findCanonicalHead() => findCursorArc() * canonicalChain() => findHeader() * trimCanonicalChain() => trimCursorArc() * Combine `updateBase()` function-args into single `PivotArgs` object why: Will generalise action for more complex scenarios in future. * update `calculateNewBase()` return code type => `PivotArc` why: So it can directly be used as argument into `updateBase()` * Update `calculateNewBase()` for target on parent arc * Update unit tests --- nimbus/core/chain/forked_chain.nim | 276 ++++++++++-------- nimbus/core/chain/forked_chain/chain_desc.nim | 21 +- tests/test_forked_chain.nim | 53 +++- tests/test_forked_chain/chain_debug.nim | 9 +- 4 files changed, 211 insertions(+), 148 deletions(-) diff --git a/nimbus/core/chain/forked_chain.nim b/nimbus/core/chain/forked_chain.nim index c3e2fd168..cb10b0483 100644 --- a/nimbus/core/chain/forked_chain.nim +++ b/nimbus/core/chain/forked_chain.nim @@ -228,10 +228,7 @@ proc writeBaggage(c: ForkedChainRef, target: Hash32) = baseNumber = c.baseHeader.number, baseHash = c.baseHash.short -func updateBase(c: ForkedChainRef, - newBaseHash: Hash32, - newBaseHeader: Header, - canonicalCursorHash: Hash32) = +func updateBase(c: ForkedChainRef, pvarc: PivotArc) = ## Remove obsolete chains, example: ## ## A1 - A2 - A3 D5 - D6 @@ -240,21 +237,23 @@ func updateBase(c: ForkedChainRef, ## \ \ ## C2 - C3 E4 - E5 ## - ## where `B5` is the `canonicalCursor` head. When the `base` is moved to - ## position `[B3]`, both chains `A` and `C` will be removed but not so for - ## `D` and `E`, and chain `B` will be curtailed below `B4`. + ## where `B1..B5` is the `pvarc.cursor` arc and `[B5]` is the `pvarc.pv`. + # + ## The `base` will be moved to position `[B3]`. Both chains `A` and `C` + ## will be removed but not so for `D` and `E`, and `pivot` arc `B` will + ## be curtailed below `B4`. ## var newCursorHeads: seq[CursorDesc] # Will become new `c.cursorHeads` for ch in c.cursorHeads: - if newBaseHeader.number < ch.forkJunction: + if pvarc.pvNumber < ch.forkJunction: # On the example, this would be any of chain `D` or `E`. newCursorHeads.add ch - elif ch.hash == canonicalCursorHash: + elif ch.hash == pvarc.cursor.hash: # On the example, this would be chain `B`. newCursorHeads.add CursorDesc( hash: ch.hash, - forkJunction: newBaseHeader.number + 1) + forkJunction: pvarc.pvNumber + 1) else: # On the example, this would be either chain `A` or `B`. @@ -263,22 +262,27 @@ func updateBase(c: ForkedChainRef, # Cleanup in-memory blocks starting from newBase backward # while blocks from newBase+1 to canonicalCursor not deleted # e.g. B4 onward - c.deleteLineage newBaseHash + c.deleteLineage pvarc.pvHash # Implied deletion of chain heads (if any) c.cursorHeads.swap newCursorHeads - c.baseHeader = newBaseHeader - c.baseHash = newBaseHash + c.baseHeader = pvarc.pvHeader + c.baseHash = pvarc.pvHash -func findCanonicalHead(c: ForkedChainRef, - hash: Hash32): Result[CanonicalDesc, string] = +func findCursorArc(c: ForkedChainRef, hash: Hash32): Result[PivotArc, string] = ## Find the `cursor` arc that contains the block relative to the ## argument `hash`. + ## if hash == c.baseHash: # The cursorHash here should not be used for next step # because it not point to any active chain - return ok(CanonicalDesc(cursorHash: c.baseHash, header: c.baseHeader)) + return ok PivotArc( + pvHash: c.baseHash, + pvHeader: c.baseHeader, + cursor: CursorDesc( + forkJunction: c.baseHeader.number, + hash: c.baseHash)) for ch in c.cursorHeads: var top = ch.hash @@ -286,7 +290,10 @@ func findCanonicalHead(c: ForkedChainRef, c.blocks.withValue(top, val): if ch.forkJunction <= val.blk.header.number: if top == hash: - return ok CanonicalDesc(cursorHash: ch.hash, header: val.blk.header) + return ok PivotArc( + pvHash: hash, + pvHeader: val.blk.header, + cursor: ch) if ch.forkJunction < val.blk.header.number: top = val.blk.header.parentHash continue @@ -294,84 +301,106 @@ func findCanonicalHead(c: ForkedChainRef, err("Block hash is not part of any active chain") -func canonicalChain(c: ForkedChainRef, - hash: Hash32, - headHash: Hash32): Result[Header, string] = - if hash == c.baseHash: +func findHeader( + c: ForkedChainRef; + itHash: Hash32; + headHash: Hash32; + ): Result[Header, string] = + ## Find header for argument `itHash` on argument `headHash` ancestor chain. + ## + if itHash == c.baseHash: return ok(c.baseHeader) - shouldNotKeyError "canonicalChain": - var prevHash = headHash - while prevHash != c.baseHash: - var header = c.blocks[prevHash].blk.header - if prevHash == hash: - return ok(header) - prevHash = header.parentHash - - err("Block hash not in canonical chain") - -func calculateNewBase(c: ForkedChainRef, - finalized: BlockNumber, - headHash: Hash32, - head: CanonicalDesc): BaseDesc = - ## Search for `finalized` searching backwards starting at `head` to find an - ## entry with this block number on the arc (or leg) terminating at `head`. - ## - ## It is required that `finalized` is within the `head` arc, i.e. - ## `cursorHead.forkJunction <= finalized` for the relevant `cursorHead`. - ## - ## The `finalized` entry might be moved backwards so that a minimum - ## distance applies. - ## - ## Discussion: - ## If the distance `finalized..head` is too short, `finalized` will be - ## adjusted backwards and may fall outside the `head` arc. In that case, - ## base remains un-moved. - ## - # It's important to have base at least `baseDistance` behind head - # so we can answer state queries about history that deep. - - let target = min(finalized, - max(head.header.number, c.baseDistance) - c.baseDistance) - - # The distance is less than `baseDistance`, don't move the base - if target <= c.baseHeader.number + c.baseDistance: - return BaseDesc(hash: c.baseHash, header: c.baseHeader) - - # Verify that `target` does not fall outside the `head` arc. It is - # assumed that `finalized` is within the `head` arc. - if target < finalized: - block verifyTarget: - # Find cursor realive to the `head` argument - for ch in c.cursorHeads: - if ch.hash == head.cursorHash: - if ch.forkJunction <= target: - break verifyTarget - break - # Noting to do here - return BaseDesc(hash: c.baseHash, header: c.baseHeader) - + # Find `pvHash` on the ancestor lineage of `headHash` var prevHash = headHash while true: c.blocks.withValue(prevHash, val): - # Note that `cursorHead.forkJunction <= target` + if prevHash == itHash: + return ok(val.blk.header) + prevHash = val.blk.header.parentHash + continue + break + + err("Block not in argument head ancestor lineage") + +func calculateNewBase( + c: ForkedChainRef; + finalized: BlockNumber; + pvarc: PivotArc; + ): PivotArc = + ## It is required that the `finalized` argument is on the `pvarc` arc, i.e. + ## it ranges beween `pvarc.cursor.forkJunction` and + ## `c.blocks[pvarc.cursor.head].number`. + ## + ## The function returns a cursor arc containing a new base position. It is + ## calculated as follows. + ## + ## Starting at the argument `pvarc.pvHead` searching backwards, the new base + ## is the position of the block with number `finalized`. + ## + ## Before searching backwards, the `finalized` argument might be adjusted + ## and made smaller so that a minimum distance to the head on the cursor arc + ## applies. + ## + # It's important to have base at least `baseDistance` behind head + # so we can answer state queries about history that deep. + let target = min(finalized, + max(pvarc.pvNumber, c.baseDistance) - c.baseDistance) + + # Can only increase base block number. + if target <= c.baseHeader.number: + return PivotArc( + pvHash: c.baseHash, + pvHeader: c.baseHeader, + cursor: CursorDesc( + forkJunction: c.baseHeader.number, + hash: c.baseHash)) + + var prevHash = pvarc.pvHash + while true: + c.blocks.withValue(prevHash, val): if target == val.blk.header.number: - return BaseDesc(hash: prevHash, header: val.blk.header) + if pvarc.cursor.forkJunction <= target: + # OK, new base stays on the argument pivot arc. + # :: + # B1 - B2 - B3 - B4 + # / ^ ^ ^ + # base - A1 - A2 - A3 | | | + # | pv CCH + # | + # target + # + return PivotArc( + pvHash: prevHash, + pvHeader: val.blk.header, + cursor: pvarc.cursor) + else: + # The new base (aka target) falls out of the argument pivot branch, + # ending up somewhere on a parent branch. + # :: + # B1 - B2 - B3 - B4 + # / ^ ^ + # base - A1 - A2 - A3 | | + # ^ pv CCH + # | + # target + # + return c.findCursorArc(prevHash).expect "valid cursor arc" prevHash = val.blk.header.parentHash continue break doAssert(false, "Unreachable code, finalized block outside cursor arc") -func trimCanonicalChain(c: ForkedChainRef, - head: CanonicalDesc, - headHash: Hash32) = +func trimCursorArc(c: ForkedChainRef, pvarc: PivotArc) = + ## Curb argument `pvarc.cursor` head so that it ends up at `pvarc.pv`. + ## # Maybe the current active chain is longer than canonical chain shouldNotKeyError "trimCanonicalChain": - var prevHash = head.cursorHash + var prevHash = pvarc.cursor.hash while prevHash != c.baseHash: let header = c.blocks[prevHash].blk.header - if header.number > head.header.number: + if header.number > pvarc.pvNumber: c.blocks.del(prevHash) else: break @@ -382,43 +411,40 @@ func trimCanonicalChain(c: ForkedChainRef, # Update cursorHeads if indeed we trim for i in 0.. baseHeader.number - if newBase.hash == c.cursorHash: + if newBase.pvHash == c.cursorHash: # Paranoid check, guaranteed by `newBase.hash == c.cursorHash` doAssert(not c.stagingTx.isNil) # CL decide to move backward and then forward? - if c.cursorHeader.number < head.header.number: - c.replaySegment(headHash, c.cursorHeader, c.cursorHash) + if c.cursorHeader.number < pvarc.pvNumber: + c.replaySegment(pvarc.pvHash, c.cursorHeader, c.cursorHash) # Current segment is canonical chain - c.writeBaggage(newBase.hash) - c.setHead(headHash, head.header.number) + c.writeBaggage(newBase.pvHash) + c.setHead(pvarc) c.stagingTx.commit() c.stagingTx = nil # Move base to newBase - c.updateBase(newBase.hash, c.cursorHeader, head.cursorHash) + c.updateBase(newBase) # Save and record the block number before the last saved block state. - c.db.persistent(newBase.header.number).isOkOr: + c.db.persistent(newBase.pvNumber).isOkOr: return err("Failed to save state: " & $$error) return ok() # At this point finalizedHeader.number is <= headHeader.number # and possibly switched to other chain beside the one with cursor - doAssert(finalizedHeader.number <= head.header.number) - doAssert(newBase.header.number <= finalizedHeader.number) + doAssert(finalizedHeader.number <= pvarc.pvNumber) + doAssert(newBase.pvNumber <= finalizedHeader.number) # Write segment from base+1 to newBase into database c.stagingTx.rollback() c.stagingTx = c.db.ctx.newTransaction() - if newBase.header.number > c.baseHeader.number: - c.replaySegment(newBase.hash) - c.writeBaggage(newBase.hash) + if newBase.pvNumber > c.baseHeader.number: + c.replaySegment(newBase.pvHash) + c.writeBaggage(newBase.pvHash) c.stagingTx.commit() c.stagingTx = nil # Update base forward to newBase - c.updateBase(newBase.hash, newBase.header, head.cursorHash) - c.db.persistent(newBase.header.number).isOkOr: + c.updateBase(newBase) + c.db.persistent(newBase.pvNumber).isOkOr: return err("Failed to save state: " & $$error) if c.stagingTx.isNil: @@ -593,16 +619,16 @@ proc forkChoice*(c: ForkedChainRef, c.stagingTx = c.db.ctx.newTransaction() # Move chain state forward to current head - if newBase.header.number < head.header.number: - c.replaySegment(headHash) + if newBase.pvNumber < pvarc.pvNumber: + c.replaySegment(pvarc.pvHash) - c.setHead(headHash, head.header.number) + c.setHead(pvarc) # Move cursor to current head - c.trimCanonicalChain(head, headHash) - if c.cursorHash != headHash: - c.cursorHeader = head.header - c.cursorHash = headHash + c.trimCursorArc(pvarc) + if c.cursorHash != pvarc.pvHash: + c.cursorHeader = pvarc.pvHeader + c.cursorHash = pvarc.pvHash ok() diff --git a/nimbus/core/chain/forked_chain/chain_desc.nim b/nimbus/core/chain/forked_chain/chain_desc.nim index 891fe685d..3689fce40 100644 --- a/nimbus/core/chain/forked_chain/chain_desc.nim +++ b/nimbus/core/chain/forked_chain/chain_desc.nim @@ -17,22 +17,17 @@ import type CursorDesc* = object - forkJunction*: BlockNumber - hash*: Hash32 + forkJunction*: BlockNumber ## Bottom or left end of cursor arc + hash*: Hash32 ## Top or right end of cursor arc BlockDesc* = object blk*: Block receipts*: seq[Receipt] - BaseDesc* = object - hash*: Hash32 - header*: Header - - CanonicalDesc* = object - ## Designate some `header` entry on a `CursorDesc` sub-chain named - ## `cursorDesc` identified by `cursorHash == cursorDesc.hash`. - cursorHash*: Hash32 - header*: Header + PivotArc* = object + pvHash*: Hash32 ## Pivot item on cursor arc (e.g. new base) + pvHeader*: Header ## Ditto + cursor*: CursorDesc ## Cursor arc containing `pv` item ForkedChainRef* = ref object stagingTx*: CoreDbTxRef @@ -50,6 +45,10 @@ type # ---------------- +func pvNumber*(pva: PivotArc): BlockNumber = + ## Getter + pva.pvHeader.number + func txRecords*(c: ForkedChainRef): var Table[Hash32, (Hash32, uint64)] = ## Avoid clash with `forked_chain.txRecords()` c.txRecords diff --git a/tests/test_forked_chain.nim b/tests/test_forked_chain.nim index dabacf59c..5a46773d3 100644 --- a/tests/test_forked_chain.nim +++ b/tests/test_forked_chain.nim @@ -241,7 +241,7 @@ proc forkedChainMain*() = check com.wdWritten(blk3) == 3 check chain.validate info & " (9)" - test "newBase == oldBase, fork and keep on that fork": + test "newBase == oldBase, fork and stay on that fork": const info = "newBase == oldBase, fork .." let com = env.newCom() @@ -266,7 +266,7 @@ proc forkedChainMain*() = check chain.latestHash == B7.blockHash check chain.validate info & " (9)" - test "newBase == cursor, fork and keep on that fork": + test "newBase == cursor, fork and stay on that fork": const info = "newBase == cursor, fork .." let com = env.newCom() @@ -294,8 +294,8 @@ proc forkedChainMain*() = check chain.latestHash == B7.blockHash check chain.validate info & " (9)" - test "newBase between oldBase and cursor, fork and keep on that fork": - const info = "newBase between oldBase .." + test "newBase on shorter canonical arc, discard arc with oldBase": + const info = "newBase on shorter canonical .." let com = env.newCom() var chain = newForkedChain(com, com.genesisHeader, baseDistance = 3) @@ -318,6 +318,37 @@ proc forkedChainMain*() = check com.headHash == B7.blockHash check chain.latestHash == B7.blockHash + check chain.baseNumber >= B4.header.number + check chain.cursorHeads.len == 1 + check chain.validate info & " (9)" + + test "newBase on curbed non-canonical arc": + const info = "newBase on curbed non-canonical .." + let com = env.newCom() + + var chain = newForkedChain(com, com.genesisHeader, baseDistance = 5) + check chain.importBlock(blk1).isOk + check chain.importBlock(blk2).isOk + check chain.importBlock(blk3).isOk + check chain.importBlock(blk4).isOk + check chain.importBlock(blk5).isOk + check chain.importBlock(blk6).isOk + check chain.importBlock(blk7).isOk + + check chain.importBlock(B4).isOk + check chain.importBlock(B5).isOk + check chain.importBlock(B6).isOk + check chain.importBlock(B7).isOk + check chain.validate info & " (1)" + + check chain.forkChoice(B7.blockHash, B5.blockHash).isOk + check chain.validate info & " (2)" + + check com.headHash == B7.blockHash + check chain.latestHash == B7.blockHash + check chain.baseNumber > 0 + check chain.baseNumber < B4.header.number + check chain.cursorHeads.len == 2 check chain.validate info & " (9)" test "newBase == oldBase, fork and return to old chain": @@ -374,8 +405,9 @@ proc forkedChainMain*() = check chain.latestHash == blk7.blockHash check chain.validate info & " (9)" - test "newBase between oldBase and cursor, fork and return to old chain, switch to new chain": - const info = "newBase between oldBase and .." + test "newBase on shorter canonical arc, discard arc with oldBase" & + " (ign dup block)": + const info = "newBase on shorter canonical .." let com = env.newCom() var chain = newForkedChain(com, com.genesisHeader, baseDistance = 3) @@ -400,10 +432,12 @@ proc forkedChainMain*() = check com.headHash == B7.blockHash check chain.latestHash == B7.blockHash + check chain.baseNumber >= B4.header.number + check chain.cursorHeads.len == 1 check chain.validate info & " (9)" - test "newBase between oldBase and cursor, fork and return to old chain": - const info = "newBase between oldBase and .." + test "newBase on longer canonical arc, discard arc with oldBase": + const info = "newBase on longer canonical .." let com = env.newCom() var chain = newForkedChain(com, com.genesisHeader, baseDistance = 3) @@ -426,6 +460,9 @@ proc forkedChainMain*() = check com.headHash == blk7.blockHash check chain.latestHash == blk7.blockHash + check chain.baseNumber > 0 + check chain.baseNumber < blk5.header.number + check chain.cursorHeads.len == 1 check chain.validate info & " (9)" test "headerByNumber": diff --git a/tests/test_forked_chain/chain_debug.nim b/tests/test_forked_chain/chain_debug.nim index 2af7dcd6e..4e47b03b9 100644 --- a/tests/test_forked_chain/chain_debug.nim +++ b/tests/test_forked_chain/chain_debug.nim @@ -90,7 +90,8 @@ func pp*(n: BlockNumber): string = n.bnStr func pp*(h: Header): string = h.bnStr func pp*(b: Block): string = b.bnStr func pp*(h: Hash32): string = h.short -func pp*(d: BaseDesc): string = d.header.pp +func pp*(d: BlockDesc): string = d.blk.header.pp +func pp*(d: ptr BlockDesc): string = d[].pp func pp*(q: openArray[Block]): string = q.ppImpl func pp*(q: openArray[Header]): string = q.ppImpl @@ -107,15 +108,15 @@ func pp*(h: Hash32; c: ForkedChainRef): string = return c.baseHeader.pp h.short -func pp*(d: CanonicalDesc; c: ForkedChainRef): string = - "(" & d.cursorHash.header(c).number.pp & "," & d.header.pp & ")" - func pp*(d: CursorDesc; c: ForkedChainRef): string = let (a,b) = (d.forkJunction, d.hash.header(c).number) result = a.bnStr if a != b: result &= ".." & (if b == 0: d.hash.pp else: b.pp) +func pp*(d: PivotArc; c: ForkedChainRef): string = + "(" & d.pvHeader.pp & "," & d.cursor.pp(c) & ")" + func pp*(q: openArray[CursorDesc]; c: ForkedChainRef): string = "{" & q.sorted(c.cmp CursorDesc).mapIt(it.pp(c)).join(",") & "}"