From 1eb79c34c6506db71a6678e79d0fe08f462d25c7 Mon Sep 17 00:00:00 2001 From: Jordan Hrycaj Date: Wed, 12 Jan 2022 15:55:36 +0000 Subject: [PATCH] Fixing Win64/CI unit test segfault why: Previously, the function 'snapshot_desc.loadSnapshot()' contained the equivalent of 'eth.decode(@[],SnapshotData)' for some type 'SnapshotData' which should result in an exception of type 'RlpTypeMismatch'. Before mid October, this worked for all systems on the Github CI. Since then, a segfault message in the Github CI can be reproduced on all 64bit Windows wuns when running 'build/all_tests ' after the failed 'make test' directive (the latter one needs to be extended by '|| true'.) This error cannot be reproduced on my local Win7/64 system with the same MSYS2 and gcc 11.2.0 compiler. The fix is, rather than catching an exception, to explicitly check the first argument of 'eth.decode(@[],SnapshotData)' and act if it is empty. also: removed some obsolete {.inline.} annotations. --- nimbus/p2p/chain/persist_blocks.nim | 2 +- nimbus/p2p/clique/clique_snapshot.nim | 64 +++++++++++--------- nimbus/p2p/clique/snapshot/snapshot_desc.nim | 43 +++++++------ 3 files changed, 63 insertions(+), 46 deletions(-) diff --git a/nimbus/p2p/chain/persist_blocks.nim b/nimbus/p2p/chain/persist_blocks.nim index 2f0a45343..a1349f8db 100644 --- a/nimbus/p2p/chain/persist_blocks.nim +++ b/nimbus/p2p/chain/persist_blocks.nim @@ -63,7 +63,7 @@ proc persistBlocksImpl(c: Chain; headers: openarray[BlockHeader]; let validationResult = vmState.processBlock(c.clique, header, body) - + when not defined(release): if validationResult == ValidationResult.Error and body.transactions.calcTxRoot == header.txRoot: diff --git a/nimbus/p2p/clique/clique_snapshot.nim b/nimbus/p2p/clique/clique_snapshot.nim index 602637841..04bd281d4 100644 --- a/nimbus/p2p/clique/clique_snapshot.nim +++ b/nimbus/p2p/clique/clique_snapshot.nim @@ -66,13 +66,20 @@ logScope: # Private debugging functions, pretty printing # ------------------------------------------------------------------------------ -proc say(d: var LocalSnaps; v: varargs[string,`$`]) {.inline.} = +template say(d: var LocalSnaps; v: varargs[untyped]): untyped = discard - # uncomment body to enable - #d.c.cfg.say v + # uncomment body to enable, note that say() prints on + # d.c.cfg.say v +proc pp(a: Hash256): string = + if a == BLANK_ROOT_HASH: + "*blank-root*" + elif a == EMPTY_SHA3: + "*empty-sha3*" + else: + a.data.mapIt(it.toHex(2)).join[56 .. 63].toLowerAscii -proc pp(q: openArray[BlockHeader]; n: int): string {.inline.} = +proc pp(q: openArray[BlockHeader]; n: int): string = result = "[" if 5 < n: result &= toSeq(q[0 .. 2]).mapIt("#" & $it.blockNumber).join(", ") @@ -81,38 +88,40 @@ proc pp(q: openArray[BlockHeader]; n: int): string {.inline.} = result &= toSeq(q[0 ..< n]).mapIt("#" & $it.blockNumber).join(", ") result &= "]" -proc pp(b: BlockNumber, q: openArray[BlockHeader]; n: int): string {.inline.} = +proc pp(b: BlockNumber, q: openArray[BlockHeader]; n: int): string = "#" & $b & " + " & q.pp(n) -proc pp(q: openArray[BlockHeader]): string {.inline.} = +proc pp(q: openArray[BlockHeader]): string = q.pp(q.len) -proc pp(b: BlockNumber, q: openArray[BlockHeader]): string {.inline.} = +proc pp(b: BlockNumber, q: openArray[BlockHeader]): string = b.pp(q, q.len) -proc pp(h: BlockHeader, q: openArray[BlockHeader]; n: int): string {.inline.} = +proc pp(h: BlockHeader, q: openArray[BlockHeader]; n: int): string = "headers=(" & h.blockNumber.pp(q,n) & ")" -proc pp(h: BlockHeader, q: openArray[BlockHeader]): string {.inline.} = +proc pp(h: BlockHeader, q: openArray[BlockHeader]): string = h.pp(q,q.len) -proc pp(t: var LocalPath; w: var LocalSubChain): string {.inline.} = +proc pp(t: var LocalPath; w: var LocalSubChain): string = var (a, b) = (w.first, w.top) if a == 0 and b == 0: b = t.chain.len "trail=(#" & $t.snaps.blockNumber & " + " & t.chain[a ..< b].pp & ")" -proc pp(t: var LocalPath): string {.inline.} = +proc pp(t: var LocalPath): string = var w = LocalSubChain() t.pp(w) +proc pp(err: CLiqueError): string = + "(" & $err[0] & "," & err[1] & ")" + # ------------------------------------------------------------------------------ # Private helpers # ------------------------------------------------------------------------------ -proc maxCheckPointLe(d: var LocalSnaps; - number: BlockNumber): BlockNumber {.inline.} = +proc maxCheckPointLe(d: var LocalSnaps; number: BlockNumber): BlockNumber = let epc = number mod d.c.cfg.ckpInterval if epc < number: number - epc @@ -120,16 +129,13 @@ proc maxCheckPointLe(d: var LocalSnaps; # epc == number => number < ckpInterval 0.u256 -proc isCheckPoint(d: var LocalSnaps; - number: BlockNumber): bool {.inline.} = +proc isCheckPoint(d: var LocalSnaps; number: BlockNumber): bool = (number mod d.c.cfg.ckpInterval) == 0 -proc isEpoch(d: var LocalSnaps; - number: BlockNumber): bool {.inline.} = +proc isEpoch(d: var LocalSnaps; number: BlockNumber): bool = (number mod d.c.cfg.epoch) == 0 -proc isSnapshotPosition(d: var LocalSnaps; - number: BlockNumber): bool {.inline.} = +proc isSnapshotPosition(d: var LocalSnaps; number: BlockNumber): bool = # clique/clique.go(394): if number == 0 || (number%c.config.Epoch [..] if d.isEpoch(number): if number.isZero: @@ -147,7 +153,7 @@ proc isSnapshotPosition(d: var LocalSnaps; # ------------------------------------------------------------------------------ proc findSnapshot(d: var LocalSnaps): bool - {.inline, gcsafe, raises: [Defect,CatchableError].} = + {.gcsafe, raises: [Defect,CatchableError].} = ## Search for a snapshot starting at current header starting at the pivot ## value `d.start`. The snapshot returned in `trail` is a clone of the ## cached snapshot and can be modified later. @@ -166,7 +172,7 @@ proc findSnapshot(d: var LocalSnaps): bool let number = header.blockNumber - # Check whether the snapshot was recently visited and cahed + # Check whether the snapshot was recently visited and cached if d.c.recents.hasLruSnaps(hash): let rc = d.c.recents.getLruSnaps(hash) if rc.isOK: @@ -184,7 +190,7 @@ proc findSnapshot(d: var LocalSnaps): bool let rc = d.c.cfg.loadSnapshot(hash) if rc.isOk: d.trail.snaps = rc.value.cloneSnapshot - d.say "findSnapshot disked ", d.trail.pp + d.say "findSnapshot on disk ", d.trail.pp trace "Loaded voting snapshot from disk", blockNumber = number, blockHash = hash @@ -196,7 +202,8 @@ proc findSnapshot(d: var LocalSnaps): bool if d.isSnapshotPosition(number): # clique/clique.go(395): checkpoint := chain.GetHeaderByNumber [..] d.trail.snaps = d.c.cfg.newSnapshot(header) - if d.trail.snaps.storeSnapshot.isOK: + let rc = d.trail.snaps.storeSnapshot + if rc.isOK: d.say "findSnapshot ", d.trail.pp info "Stored voting snapshot to disk", blockNumber = number, @@ -230,7 +237,7 @@ proc findSnapshot(d: var LocalSnaps): bool proc applyTrail(d: var LocalSnaps): CliqueOkResult - {.inline, gcsafe, raises: [Defect,CatchableError].} = + {.gcsafe, raises: [Defect,CatchableError].} = ## Apply any `trail` headers on top of the snapshot `snap` if d.subChn.first < d.subChn.top: block: @@ -239,7 +246,8 @@ proc applyTrail(d: var LocalSnaps): CliqueOkResult let rc = d.trail.snaps.snapshotApplySeq( d.trail.chain, d.subChn.top-1, d.subChn.first) if rc.isErr: - d.say "applyTrail snaps=#",d.trail.snaps.blockNumber, " err=",$rc.error + d.say "applyTrail snaps=#", d.trail.snaps.blockNumber, + " err=", rc.error.pp return err(rc.error) d.say "applyTrail snaps=#", d.trail.snaps.blockNumber @@ -250,7 +258,7 @@ proc applyTrail(d: var LocalSnaps): CliqueOkResult if rc.isErr: return err(rc.error) - d.say "updateSnapshot chechkpoint #", d.trail.snaps.blockNumber + d.say "applyTrail chechkpoint #", d.trail.snaps.blockNumber trace "Stored voting snapshot to disk", blockNumber = d.trail.snaps.blockNumber, blockHash = d.trail.snaps.blockHash @@ -258,7 +266,7 @@ proc applyTrail(d: var LocalSnaps): CliqueOkResult proc updateSnapshot(d: var LocalSnaps): SnapshotResult - {.gcsafe, raises: [Defect,CatchableError].} = + {.gcsafe, raises: [Defect,CatchableError].} = ## Find snapshot for header `d.start.header` and assign it to the LRU cache. ## This function was expects thet the LRU cache already has a slot allocated ## for the snapshot having run `getLruSnaps()`. @@ -406,7 +414,7 @@ proc cliqueSnapshot*(c: Clique;hash: Hash256; c.cliqueSnapshotSeq(hash,list) proc cliqueSnapshot*(c: Clique; header: Blockheader): SnapshotResult - {.inline,gcsafe,raises: [Defect,CatchableError].} = + {.gcsafe,raises: [Defect,CatchableError].} = ## Short for `cliqueSnapshot(c,header,@[])` var blind: seq[Blockheader] c.cliqueSnapshotSeq(header, blind) diff --git a/nimbus/p2p/clique/snapshot/snapshot_desc.nim b/nimbus/p2p/clique/snapshot/snapshot_desc.nim index 0e1723688..5746ff1f9 100644 --- a/nimbus/p2p/clique/snapshot/snapshot_desc.nim +++ b/nimbus/p2p/clique/snapshot/snapshot_desc.nim @@ -80,13 +80,13 @@ proc signersList(s: Snapshot): string = # Private functions needed to support RLP conversion # ------------------------------------------------------------------------------ -proc append[K,V](rw: var RlpWriter; tab: Table[K,V]) {.inline.} = +proc append[K,V](rw: var RlpWriter; tab: Table[K,V]) = rw.startList(tab.len) for key,value in tab.pairs: rw.append((key,value)) proc read[K,V](rlp: var Rlp; - Q: type Table[K,V]): Q {.inline, raises: [Defect,CatchableError].} = + Q: type Table[K,V]): Q {.raises: [Defect,CatchableError].} = for w in rlp.items: let (key,value) = w.read((K,V)) result[key] = value @@ -165,23 +165,23 @@ proc newSnapshot*(cfg: CliqueCfg; header: BlockHeader): Snapshot = # Public getters # ------------------------------------------------------------------------------ -proc cfg*(s: Snapshot): CliqueCfg {.inline.} = +proc cfg*(s: Snapshot): CliqueCfg = ## Getter s.cfg -proc blockNumber*(s: Snapshot): BlockNumber {.inline.} = +proc blockNumber*(s: Snapshot): BlockNumber = ## Getter s.data.blockNumber -proc blockHash*(s: Snapshot): Hash256 {.inline.} = +proc blockHash*(s: Snapshot): Hash256 = ## Getter s.data.blockHash -proc recents*(s: Snapshot): var AddressHistory {.inline.} = +proc recents*(s: Snapshot): var AddressHistory = ## Retrieves the list of recently added addresses s.data.recents -proc ballot*(s: Snapshot): var Ballot {.inline.} = +proc ballot*(s: Snapshot): var Ballot = ## Retrieves the ballot box descriptor with the votes s.data.ballot @@ -189,11 +189,11 @@ proc ballot*(s: Snapshot): var Ballot {.inline.} = # Public setters # ------------------------------------------------------------------------------ -proc `blockNumber=`*(s: Snapshot; number: BlockNumber) {.inline.} = +proc `blockNumber=`*(s: Snapshot; number: BlockNumber) = ## Getter s.data.blockNumber = number -proc `blockHash=`*(s: Snapshot; hash: Hash256) {.inline.} = +proc `blockHash=`*(s: Snapshot; hash: Hash256) = ## Getter s.data.blockHash = hash @@ -207,13 +207,22 @@ proc loadSnapshot*(cfg: CliqueCfg; hash: Hash256): ## Load an existing snapshot from the database. var s = Snapshot(cfg: cfg) try: - s.data = s.cfg.db.db - .get(hash.cliqueSnapshotKey.toOpenArray) - .decode(SnapshotData) + let rlpData = s.cfg.db.db.get(hash.cliqueSnapshotKey.toOpenArray) + + # The following check is only needed for Github/CI for 64bit Windows (not + # reproducible on my local Win7 -- jordan). What normally happens when + # `rlpData == @[]` holds is that the library function `eth.readImpl()` + # throws an `RlpTypeMismatch` error as the inner function `isList()` fails. + # On Github CI for 64bit Windows, the unit test crashes with a segmentation + # fault. + if rlpData.len == 0: + return err((errSnapshotLoad,"")) + + s.data = rlpData.decode(SnapshotData) s.data.ballot.debug = s.cfg.debug except CatchableError as e: - return err((errSnapshotLoad,e.msg)) - result = ok(s) + return err((errSnapshotLoad, $e.name & ": " & e.msg)) + ok(s) # clique/snapshot.go(104): func (s *Snapshot) store(db [..] proc storeSnapshot*(s: Snapshot): CliqueOkResult {.gcsafe,raises: [Defect].} = @@ -222,14 +231,14 @@ proc storeSnapshot*(s: Snapshot): CliqueOkResult {.gcsafe,raises: [Defect].} = s.cfg.db.db .put(s.data.blockHash.cliqueSnapshotKey.toOpenArray, rlp.encode(s.data)) except CatchableError as e: - return err((errSnapshotStore,e.msg)) - result = ok() + return err((errSnapshotStore, $e.name & ": " & e.msg)) + ok() # ------------------------------------------------------------------------------ # Public deep copy # ------------------------------------------------------------------------------ -proc cloneSnapshot*(s: Snapshot): Snapshot {.inline.} = +proc cloneSnapshot*(s: Snapshot): Snapshot = ## Clone the snapshot Snapshot( cfg: s.cfg, # copy ref