all snapshot unit tests succeed

details:
  for extra verbosity compile as: nim c -r -d:debug [..] test_clique.nim

nimbus/p2p/clique.nim

@@ -214,7 +214,7 @@ proc verifyCascadingFields(c: var Clique; header: BlockHeader;
   if (header.blockNumber mod c.cfg.epoch.u256) == 0:
     let
       signersList = snap.value.signers
-      extraList = header.extraData.extraDataSigners
+      extraList = header.extraData.extraDataAddresses
     if signersList != extraList:
       return err((errMismatchingCheckpointSigners,""))
 

nimbus/p2p/clique/clique_utils.nim

@@ -89,7 +89,7 @@ proc cliqueResultErr*(w: CliqueError): CliqueResult =
   err(w)
 
 
-proc extraDataSigners*(extraData: Blob): seq[EthAddress] =
+proc extraDataAddresses*(extraData: Blob): seq[EthAddress] =
   ## Extract signer addresses from extraData header field
 
   proc toEthAddress(a: openArray[byte]; start: int): EthAddress {.inline.} =

nimbus/p2p/clique/recent_snaps.nim

@@ -76,44 +76,51 @@ proc say(d: RecentDesc; v: varargs[string,`$`]) =
     if d.debug:
       stderr.write "*** " & v.join & "\n"
 
+proc say(rs: var RecentSnaps; v: varargs[string,`$`]) =
+  ## Debugging output
+  ppExceptionWrap:
+    if rs.debug:
+      stderr.write "*** " & v.join & "\n"
+
 proc getPrettyPrinters(d: RecentDesc): var PrettyPrinters =
   ## Mixin for pretty printers, see `clique/clique_cfg.pp()`
   d.cfg.prettyPrint
 
-# clique/clique.go(394): if number == 0 || (number%c.config.Epoch [..]
-proc canDiskCheckPointOk(d: RecentDesc): bool {.
-                         gcsafe, raises: [Defect,RlpError].} =
-  # If we're at the genesis, snapshot the initial state.
+
+proc canDiskCheckPointOk(d: RecentDesc):
+                        bool {.inline, raises: [Defect,RlpError].} =
+
+  # clique/clique.go(394): if number == 0 || (number%c.config.Epoch [..]
   if d.args.blockNumber.isZero:
+    # If we're at the genesis, snapshot the initial state.
     return true
-  # Alternatively if we're at a checkpoint block without a parent
-  # (light client CHT), or we have piled up more headers than allowed
-  # to be re-orged (chain reinit from a freezer), consider the
-  # checkpoint trusted and snapshot it.
+
   if (d.args.blockNumber mod d.cfg.epoch) == 0:
+    # Alternatively if we're at a checkpoint block without a parent
+    # (light client CHT), or we have piled up more headers than allowed
+    # to be re-orged (chain reinit from a freezer), consider the
+    # checkpoint trusted and snapshot it.
     if FULL_IMMUTABILITY_THRESHOLD < d.local.headers.len:
       return true
     if d.cfg.dbChain.getBlockHeaderResult(d.args.blockNumber - 1).isErr:
       return true
 
-proc isCheckPointOk(number: BlockNumber): bool =
-  (number mod CHECKPOINT_INTERVAL) == 0
-
 # ------------------------------------------------------------------------------
 # Private functions
 # ------------------------------------------------------------------------------
 
-# clique/clique.go(383): if number%checkpointInterval == 0 [..]
-proc tryDiskSnapshot(d: RecentDesc; snap: var Snapshot): bool =
-  if d.args.blockNumber.isCheckPointOk:
+proc tryLoadDiskSnapshot(d: RecentDesc; snap: var Snapshot): bool {.inline.} =
+  # clique/clique.go(383): if number%checkpointInterval == 0 [..]
+  if (d.args.blockNumber mod CHECKPOINT_INTERVAL) == 0:
     if snap.loadSnapshot(d.cfg, d.args.blockHash).isOk:
       trace "Loaded voting snapshot from disk",
         blockNumber = d.args.blockNumber,
         blockHash = d.args.blockHash
       return true
 
-proc tryDiskCheckPoint(d: RecentDesc; snap: var Snapshot): bool  {.
-                       gcsafe, raises: [Defect,RlpError].} =
+
+proc tryStoreDiskCheckPoint(d: RecentDesc; snap: var Snapshot):
+                           bool {.gcsafe, raises: [Defect,RlpError].} =
   if d.canDiskCheckPointOk:
     # clique/clique.go(395): checkpoint := chain.GetHeaderByNumber [..]
     let checkPoint = d.cfg.dbChain.getBlockHeaderResult(d.args.blockNumber)
@@ -121,8 +128,8 @@ proc tryDiskCheckPoint(d: RecentDesc; snap: var Snapshot): bool  {.
       return false
     let
       hash = checkPoint.value.hash
-      signersList = checkPoint.value.extraData.extraDataSigners
-    snap.initSnapshot(d.cfg, d.args.blockNumber, hash, signersList)
+      accountList = checkPoint.value.extraData.extraDataAddresses
+    snap.initSnapshot(d.cfg, d.args.blockNumber, hash, accountList)
     snap.setDebug(d.debug)
 
     if snap.storeSnapshot.isOk:
@@ -148,12 +155,12 @@ proc initRecentSnaps*(rs: var RecentSnaps;
 
       while true:
         # If an on-disk checkpoint snapshot can be found, use that
-        if d.tryDiskSnapshot(snap):
+        if d.tryLoadDiskSnapshot(snap):
           # clique/clique.go(386): snap = s
           break
 
         # Save checkpoint e.g. when at the genesis ..
-        if d.tryDiskCheckPoint(snap):
+        if d.tryStoreDiskCheckPoint(snap):
           # clique/clique.go(407): log.Info("Stored [..]
           break
 
@@ -164,7 +171,7 @@ proc initRecentSnaps*(rs: var RecentSnaps;
           header = d.args.parents[^1]
 
           # clique/clique.go(416): if header.Hash() != hash [..]
-          if header.hash != d.args.blockHash and
+          if header.hash        != d.args.blockHash or
              header.blockNumber != d.args.blockNumber:
             return err((errUnknownAncestor,""))
           d.args.parents.setLen(d.args.parents.len-1)
@@ -189,15 +196,16 @@ proc initRecentSnaps*(rs: var RecentSnaps;
       block:
         # clique/clique.go(434): snap, err := snap.apply(headers)
         d.say "recentSnaps => applySnapshot([",
-              d.local.headers.mapIt("#" & $it.blockNumber.truncate(int))
-                  .join(",").string, "])"
+          d.local.headers.mapIt("#" & $it.blockNumber.truncate(int))
+            .join(",").string, "])"
         let rc = snap.applySnapshot(d.local.headers)
         d.say "recentSnaps => applySnapshot() => ", rc.pp
         if rc.isErr:
           return err(rc.error)
 
       # If we've generated a new checkpoint snapshot, save to disk
-      if snap.blockNumber.isCheckPointOk and 0 < d.local.headers.len:
+      if (snap.blockNumber mod CHECKPOINT_INTERVAL) == 0 and
+         0 < d.local.headers.len:
         var rc = snap.storeSnapshot
         if rc.isErr:
           return err(rc.error)
@@ -222,6 +230,7 @@ proc setDebug*(rs: var RecentSnaps; debug: bool) =
 proc getRecentSnaps*(rs: var RecentSnaps; args: RecentArgs): auto {.
                      gcsafe, raises: [Defect,CatchableError].} =
   ## Get snapshot from cache or disk
+  rs.say "getRecentSnap #", args.blockNumber.truncate(uint64)
   rs.cache.getLruItem:
     RecentDesc(cfg:   rs.cfg,
                debug: rs.debug,

nimbus/p2p/clique/snapshot.nim

@@ -30,6 +30,7 @@ import
   ./clique_cfg,
   ./clique_defs,
   ./clique_poll,
+  ./clique_utils,
   ./ec_recover,
   chronicles,
   eth/[common, rlp, trie/db]
@@ -202,7 +203,7 @@ proc applySnapshot*(s: var Snapshot;
                     headers: openArray[BlockHeader]): CliqueResult {.
                       gcsafe, raises: [Defect,CatchableError].} =
   ## Initialises an authorization snapshot `snap` by applying the `headers`
-  ## to the argument snapshot `s`.
+  ## to the argument snapshot desciptor `s`.
 
   s.say "applySnapshot ", s.pp(headers).join("\n" & ' '.repeat(18))
 
@@ -238,7 +239,23 @@ proc applySnapshot*(s: var Snapshot;
 
     # Remove any votes on checkpoint blocks
     if (number mod s.cfg.epoch) == 0:
-      s.data.ballot.initCliquePoll
+      # clique/snapshot.go(210): snap.Votes = nil
+      s.say "applySnapshot epoch => reset, state=", s.pp(41)
+
+      # This part differs from the go implementation in that the `signer`
+      # list is re-assigned. The original implementation silently assumes
+      # that the `signer` list is the same as the previous one but this is
+      # not enforced.
+      #
+      # The eip225 discussion has it as: [..] where every epoch transition
+      # flushes all pending votes. Furthermore, these epoch transitions can
+      # also act as stateless checkpoints containing the list of current
+      # authorized signers within the header extra-data. This permits clients
+      # to sync up based only on a checkpoint hash without having to replay
+      # all the voting that was done on the chain up to that point. It also
+      # allows the genesis header to fully define the chain, containing the
+      # list of initial signers.
+      s.data.ballot.initCliquePoll(header.extraData.extraDataAddresses)
       s.data.ballot.setDebug(s.data.debug)
 
     # Delete the oldest signer from the recent list to allow it signing again

tests/test_clique.nim

@@ -28,18 +28,19 @@ proc cliqueMain*(noisy = defined(debug)) =
   suite "Clique PoA Snapshot":
     var
       pool = newTesterPool()
-      skipSet = {20, 23, 24}
-      testSet = {0 .. 99}
+      skipSet = {999}
+      testSet = {0 .. 999}
 
     pool.setDebug(noisy)
 
     # clique/snapshot_test.go(379): for i, tt := range tests {
     for tt in voterSamples.filterIt(it.id in testSet):
-      pool.say "\n"
-      test &"Snapshots {tt.id}: {tt.info.substr(0,50)}...":
+
+      test &"Snapshots {tt.id: 2}: {tt.info.substr(0,50)}...":
+        pool.say "\n"
 
         if tt.id in skipSet:
-          echo &"Test {tt.id} skipped"
+          skip()
 
         else:
           # Assemble a chain of headers from the cast votes