Update snapshot smoke test

details:
  can initialise & load all tests

todo:
  double check tests that are supposed to return error
  follow up succesful voting results

nimbus/p2p/clique.nim

@@ -634,6 +634,12 @@ proc snapshotInternal*(c: var Clique; number: BlockNumber; hash: Hash256;
 proc cfgInternal*(c: var Clique): auto =
   c.cfg
 
+proc pp*(rc: var Result[Snapshot,CliqueError]; indent = 0): string =
+  if rc.isOk:
+    rc.value.pp(indent)
+  else:
+    "(error: " & rc.error.pp & ")"
+
 # ------------------------------------------------------------------------------
 # End
 # ------------------------------------------------------------------------------

nimbus/p2p/clique/clique_cfg.nim

@@ -26,6 +26,7 @@ import
   ethash,
   random,
   sequtils,
+  stew/results,
   stint,
   strutils,
   times
@@ -82,6 +83,19 @@ proc newCliqueCfg*(dbChain: BaseChainDB; period = BLOCK_PERIOD;
 # Debugging
 # ------------------------------------------------------------------------------
 
+proc pp*(v: CliqueError): string =
+  ## Pretty print error
+  result = $v[0]
+  if v[1] != "":
+    result &=  " => " & v[1]
+
+proc pp*(v: CliqueResult): string =
+  ## Pretty print result
+  if v.isOk:
+    "OK"
+  else:
+    v.error.pp
+
 proc pp*(p: var PrettyPrinters; v: BlockNonce): string =
   ## Pretty print nonce
   p.nonce(v)

nimbus/p2p/clique/clique_poll.nim

@@ -35,7 +35,7 @@ type
     authorize*: bool          ## authorization type,  whether to authorize or
                               ## deauthorize the voted account
 
-  Tally = tuple
+  Tally = object
     authorize: bool
     signers: Table[EthAddress,Vote]
 
@@ -118,17 +118,22 @@ proc addVote*(t: var CliquePoll; vote: Vote) =
   ##    from the list of authorised signers.
   t.authRemoved = false
 
+  echo ">>> addVote 1"
+
   # clique/snapshot.go(147): if !s.validVote(address, [..]
   if not t.validVote(vote.address, vote.authorize):
     # Voting has no effect
     return
 
   # clique/snapshot.go(253): if snap.cast(header.Coinbase, [..]
+  echo ">>> addVote 2"
 
   # Collect vote
   var numVotes = 0
   if not t.votes.hasKey(vote.address):
-    t.votes[vote.address] = (vote.authorize, {vote.signer: vote}.toTable)
+    t.votes[vote.address] = Tally(
+      authorize: vote.authorize,
+      signers: {vote.signer: vote}.toTable)
     numVotes = 1
   elif t.votes[vote.address].authorize == vote.authorize:
     t.votes[vote.address].signers[vote.signer] = vote
@@ -136,12 +141,16 @@ proc addVote*(t: var CliquePoll; vote: Vote) =
   else:
     return
 
+  echo ">>> addVote 3"
+
   # clique/snapshot.go(262): if tally := snap.Tally[header.Coinbase]; [..]
 
   # Vote passed, update the list of authorised signers if enough votes
   if numVotes < t.authSignersThreshold:
     return
 
+  echo ">>> addVote 4"
+
   var obsolete = @[vote.address]
   if vote.authorize:
     # Has minimum votes, so add it
@@ -162,6 +171,18 @@ proc addVote*(t: var CliquePoll; vote: Vote) =
   for key in obsolete:
     t.votes.del(key)
 
+  echo ">>> addVote done"
+
+# ------------------------------------------------------------------------------
+# Test interface
+# ------------------------------------------------------------------------------
+
+proc votesInternal*(t: var CliquePoll): seq[(EthAddress,EthAddress,Vote)] =
+  for account in toSeq(t.votes.keys).sorted(EthAscending):
+    let tally = t.votes[account]
+    for signer in toSeq(tally.signers.keys).sorted(EthAscending):
+      result.add (account, signer, tally.signers[signer])
+
 # ------------------------------------------------------------------------------
 # End
 # ------------------------------------------------------------------------------

nimbus/p2p/clique/clique_utils.nim

@@ -30,7 +30,7 @@ import
   ./clique_defs,
   algorithm,
   eth/[common, rlp],
-  stew/results,
+  stew/[objects, results],
   stint,
   strformat,
   times
@@ -46,12 +46,6 @@ type
 # Private helpers
 # ------------------------------------------------------------------------------
 
-proc toEthAddress(a: openArray[byte]; start: int): EthAddress =
-  ## Concert seq[..] => Array[..]
-  doAssert start + EthAddress.len <= a.len
-  for n in 0 ..< EthAddress.len:
-    result[n] = a[start + n]
-
 proc gasLimitBounds(limit: GasInt): (GasInt, GasInt) =
   ## See also utils.header.gasLimitBounds()
   let
@@ -98,12 +92,18 @@ proc cliqueResultErr*(w: CliqueError): CliqueResult =
 
 proc extraDataSigners*(extraData: Blob): seq[EthAddress] =
   ## Extract signer addresses from extraData header field
-  if EXTRA_VANITY + EXTRA_SEAL < extraData.len:
+
+  proc toEthAddress(a: openArray[byte]; start: int): EthAddress {.inline.} =
+    toArray(EthAddress.len, a[start ..< start + EthAddress.len])
+
+  if EXTRA_VANITY + EXTRA_SEAL < extraData.len and
+      ((extraData.len - (EXTRA_VANITY + EXTRA_SEAL)) mod EthAddress.len) == 0:
     var addrOffset = EXTRA_VANITY
-    while addrOffset + EthAddress.len <= EXTRA_SEAL:
+    while addrOffset + EthAddress.len <= extraData.len - EXTRA_SEAL:
       result.add extraData.toEthAddress(addrOffset)
       addrOffset += EthAddress.len
 
+
 proc getBlockHeaderResult*(c: BaseChainDB;
                            number: BlockNumber): Result[BlockHeader,void] =
   ## Slightly re-phrased dbChain.getBlockHeader(..) command
@@ -112,6 +112,7 @@ proc getBlockHeaderResult*(c: BaseChainDB;
     return ok(header)
   result = err()
 
+
 # core/types/block.go(343): func (b *Block) WithSeal(header [..]
 proc withHeader*(b: EthBlock; header: BlockHeader): EthBlock =
   ## New block with the data from `b` but the header replaced with the

nimbus/p2p/clique/recent_snaps.nim

@@ -172,10 +172,10 @@ proc initRecentSnaps*(rs: var RecentSnaps;
         swap(d.local.headers[i], d.local.headers[^(1+i)])
       block:
         # clique/clique.go(434): snap, err := snap.apply(headers)
-        echo ">>> applySnapshot(",
+        echo ">>> calling applySnapshot(",
                    d.local.headers.mapIt(it.blockNumber.truncate(int)), ")"
         let rc = snap.applySnapshot(d.local.headers)
-        echo "<<< applySnapshot() => ", rc.repr
+        echo "<<< calling applySnapshot() => ", rc.pp
         if rc.isErr:
           return err(rc.error)
 

nimbus/p2p/clique/snapshot.nim

@@ -30,9 +30,12 @@ import
   ./clique_defs,
   ./clique_poll,
   ./ec_recover,
+  algorithm,
   chronicles,
   eth/[common, rlp, trie/db],
   sequtils,
+  strformat,
+  strutils,
   tables,
   times
 
@@ -56,7 +59,7 @@ type
 {.push raises: [Defect,CatchableError].}
 
 logScope:
-  topics = "clique snapshot"
+  topics = "clique PoA snapshot"
 
 # ------------------------------------------------------------------------------
 # Pretty printers for debugging
@@ -66,6 +69,62 @@ proc getPrettyPrinters(s: var Snapshot): var PrettyPrinters =
   ## Mixin for pretty printers
   s.cfg.prettyPrint
 
+proc pp(s: var Snapshot; h: var AddressHistory): string =
+  toSeq(h.keys)
+    .mapIt(it.truncate(uint64))
+    .sorted
+    .mapIt("#" & $it & ":" & s.pp(h[it.u256]))
+    .join(",")
+
+proc pp(s: var Snapshot; v: Vote): string =
+  proc authorized(b: bool): string =
+    if b: ",auhorized" else: ""
+  "{" & &"signer={s.pp(v.signer)}" &
+        &",address={s.pp(v.address)}" &
+        &",blockNumber={v.blockNumber.truncate(uint64)}" &
+        &"{authorized(v.authorize)}" & "}"
+
+proc pp(s: var Snapshot;
+        v: (EthAddress,EthAddress,Vote); delim: string): string =
+  &"(account={s.pp(v[0])}" &
+    &"{delim}signer={s.pp(v[1])}" &
+    &"{delim}vote={s.pp(v[2])})"
+
+proc pp(s: var Snapshot;
+        w: seq[(EthAddress,EthAddress,Vote)]; sep1, sep2: string): string =
+  w.mapIt(s.pp(it,sep1)).join(sep2)
+
+proc pp(s: var Snapshot;
+        w: seq[(EthAddress,EthAddress,Vote)]; indent = 0): string =
+  let
+    sep1 = if 0 < indent: "\n" & ' '.repeat(indent) else: ","
+    sep2 = if 0 < indent: "\n" & ' '.repeat(indent) else: " "
+  s.pp(w,sep1,sep2)
+
+# ------------------------------------------------------------------------------
+# Public pretty printers
+# ------------------------------------------------------------------------------
+
+proc pp*(s: var Snapshot; delim: string): string =
+  ## Pretty print descriptor
+  let
+    sep1 = if 0 < delim.len: delim
+           else: ";"
+    sep2 = if 0 < delim.len and delim[0] == '\n': delim & ' '.repeat(8)
+           else: ";"
+    sep3 = if 0 < delim.len and delim[0] == '\n': delim & ' '.repeat(7)
+           else: ";"
+  &"(blockNumber=#{s.data.blockNumber}" &
+    &"{sep1}recents=[{s.pp(s.data.recents)}]" &
+    &"{sep1}votes=[" &
+      s.pp(s.data.ballot.votesInternal,sep2,sep3) &
+      "])"
+
+proc pp*(s: var Snapshot; indent = 0): string =
+  ## Pretty print descriptor
+  let delim = if 0 < indent: "\n" & ' '.repeat(indent) else: " "
+  s.pp(delim)
+
 # ------------------------------------------------------------------------------
 # Private functions needed to support RLP conversion
 # ------------------------------------------------------------------------------
@@ -97,7 +156,7 @@ proc initSnapshot*(s: var Snapshot; cfg: CliqueCfg;
   s.data.recents = initTable[BlockNumber,EthAddress]()
   s.data.ballot.initCliquePoll(signers)
   echo ">>> initSnapshot #", number.truncate(uint64),
-    " >> ", s.pp(signers), " >> ", s.pp(s.data.ballot.authSigners)
+    " >> ", s.pp(s.data.ballot.authSigners)
 
 proc initSnapshot*(cfg: CliqueCfg; number: BlockNumber; hash: Hash256;
                    signers: openArray[EthAddress]): Snapshot =
@@ -142,7 +201,7 @@ proc applySnapshot*(s: var Snapshot;
   ## Initialises an authorization snapshot `snap` by applying the `headers`
   ## to the argument snapshot `s`.
 
-  echo ">>> applySnapshot ", s.pp(headers)
+  echo ">>> applySnapshot ", s.pp(headers).join("\n" & ' '.repeat(18))
 
   # Allow passing in no headers for cleaner code
   if headers.len == 0:
@@ -183,13 +242,16 @@ proc applySnapshot*(s: var Snapshot;
       if limit <= number:
         s.data.recents.del(number - limit)
 
-    echo "<<< applySnapshot 2"
+    echo "<<< applySnapshot 2 snap=", s.pp(26)
 
     # Resolve the authorization key and check against signers
     let signer = ? s.cfg.signatures.getEcRecover(header)
     echo "<<< applySnapshot 3 ", s.pp(signer)
+
     if not s.data.ballot.isAuthSigner(signer):
+      echo "<<< applySnapshot 3 => fail ", s.pp(29)
       return err((errUnauthorizedSigner,""))
+
     echo "<<< applySnapshot 4"
     for recent in s.data.recents.values:
       if recent == signer:
@@ -199,21 +261,25 @@ proc applySnapshot*(s: var Snapshot;
     echo "<<< applySnapshot 5"
 
     # Header authorized, discard any previous vote from the signer
+    # clique/snapshot.go(233): for i, vote := range snap.Votes {
     s.data.ballot.delVote(signer = signer, address = header.coinbase)
 
     # Tally up the new vote from the signer
+    # clique/snapshot.go(244): var authorize bool
     var authOk = false
     if header.nonce == NONCE_AUTH:
       authOk = true
     elif header.nonce != NONCE_DROP:
       return err((errInvalidVote,""))
-    s.data.ballot.addVote:
+    let vote = Vote(address:     header.coinbase,
-      Vote(address:     header.coinbase,
+                    signer:      signer,
-           signer:      signer,
+                    blockNumber: number,
-           blockNumber: number,
+                    authorize:   authOk)
-           authorize:   authOk)
+    echo "<<< applySnapshot calling addVote ", s.pp(vote)
+    # clique/snapshot.go(253): if snap.cast(header.Coinbase, authorize) {
+    s.data.ballot.addVote(vote)
 
-    echo "<<< applySnapshot 6"
+    echo "<<< applySnapshot 6 ", s.pp(21)
 
     # clique/snapshot.go(269): if limit := uint64(len(snap.Signers)/2 [..]
     if s.data.ballot.authSignersShrunk:

tests/test_clique.nim

@@ -65,19 +65,19 @@ proc cliqueMain*(noisy = defined(debug)) =
   ## Tests that Clique signer voting is evaluated correctly for various simple
   ## and complex scenarios, as well as that a few special corner cases fail
   ## correctly.
-  suite "Clicque PoA":
+  suite "Clique PoA Snapshot":
-    var pool = newTesterPool()
+    var
-    const maxID = 2
+      pool = newTesterPool()
+      testSet = {0 .. 99}
 
     # clique/snapshot_test.go(379): for i, tt := range tests {
     for tt in voterSamples:
-      if maxId < tt.id:
+      if tt.id in testSet:
-        echo "Tests stopped"
+        test &"Snapshots {tt.id}: {tt.info.substr(0,50)}...":
-        break
+          var snap = pool.initSnapshot(tt, noisy)
-
+          if snap.isErr:
-      test &"Snapshots {tt.id}: {tt.info.substr(0,50)}...":
+            # FIXME: double check error behavior
-        var snap = pool.initSnapshot(tt, noisy)
+            check snap.error[0] == tt.failure
-        check snap.isOk
 
 
 when isMainModule:

tests/test_clique/pool.nim

@@ -26,7 +26,6 @@ import
   sequtils,
   stew/objects,
   strformat,
-  strutils,
   tables,
   times
 
@@ -144,12 +143,14 @@ proc ppNonce(ap: TesterPool; v: BlockNonce): string =
 
 proc ppAddress(ap: TesterPool; v: EthAddress): string =
   ## Pretty print address
-  result = ap.findName(v)
+  if v.isZero:
-  if result == "":
+    result = "@0"
-    if v.isZero:
+  else:
-      result = "@0"
+    let a = ap.findName(v)
+    if a == "":
+      result = &"@{v}"
     else:
-      result = $v
+      result = &"@{a}"
 
 proc ppExtraData(ap: TesterPool; v: Blob): string =
   ## Visualise `extraData` field
@@ -190,10 +191,11 @@ proc ppExtraData(ap: TesterPool; v: Blob): string =
 
 proc ppBlockHeader(ap: TesterPool; v: BlockHeader; delim: string): string =
   ## Pretty print block header
+  let sep = if 0 < delim.len: delim else: ";"
   &"(blockNumber=#{v.blockNumber.truncate(uint64)}" &
-    delim & &"coinbase={ap.ppAddress(v.coinbase)}" &
+    &"{sep}coinbase={ap.ppAddress(v.coinbase)}" &
-    delim & &"nonce={ap.ppNonce(v.nonce)}" &
+    &"{sep}nonce={ap.ppNonce(v.nonce)}" &
-    delim & &"extraData={ap.ppExtraData(v.extraData)})"
+    &"{sep}extraData={ap.ppExtraData(v.extraData)})"
 
 proc initPrettyPrinters(pp: var PrettyPrinters; ap: TesterPool) =
   pp.nonce =       proc(v:BlockNonce):            string = ap.ppNonce(v)
@@ -367,7 +369,11 @@ proc getPrettyPrinters*(t: TesterPool): var PrettyPrinters =
 # ------------------------------------------------------------------------------
 
 #[
-let tt = voterSamples[0]
+
+import
+  algorithm, strutils
+
+let tt = voterSamples.filterIt(it.id == 21)[0]
 
 var p = newTesterPool()
 p.resetVoterChain(tt.signers)
@@ -377,10 +383,14 @@ p.commitVoterChain
 
 let topHeader = p.topVoterHeader
 
-echo "*** adresses: ", toSeq(p.names.pairs).mapIt(&"{it[1]}:{it[0]}").join(", ")
+echo "*** adresses: ", toSeq(p.names.pairs)
+                         .mapIt(&"{it[1]}:{it[0]}")
+                         .sorted
+                         .join("\n" & ' '.repeat(14))
 echo "     genesis: ", p.pp(p.chain.getBlockHeader(0.u256),15)
 echo "   topHeader: ", p.pp(topHeader,15)
 
 var snap = p.snapshot(topHeader.blockNumber, topHeader.hash, @[])
-echo ">>> ", snap
+echo ">>> snap=", snap.pp(10)
+
 ]#

tests/test_clique/voter_samples.nim

@@ -325,7 +325,7 @@ const
                  TesterVote(signer: "B"),
                  TesterVote(signer: "A", checkpoint: @["A", "B", "C"]),
                  TesterVote(signer: "A", newbatch: true)],
-    failure: errRecentlySigned)]
+      failure: errRecentlySigned)]
 
 static:
   # For convenience, make sure that IDs are increasing