diff --git a/nimbus/p2p/clique.nim b/nimbus/p2p/clique.nim index fb14e5b67..a2ee7d7e6 100644 --- a/nimbus/p2p/clique.nim +++ b/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 # ------------------------------------------------------------------------------ diff --git a/nimbus/p2p/clique/clique_cfg.nim b/nimbus/p2p/clique/clique_cfg.nim index d9d2cd164..8aa59d5fe 100644 --- a/nimbus/p2p/clique/clique_cfg.nim +++ b/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) diff --git a/nimbus/p2p/clique/clique_poll.nim b/nimbus/p2p/clique/clique_poll.nim index 257b9581a..8e5223352 100644 --- a/nimbus/p2p/clique/clique_poll.nim +++ b/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 # ------------------------------------------------------------------------------ diff --git a/nimbus/p2p/clique/clique_utils.nim b/nimbus/p2p/clique/clique_utils.nim index 76e20811b..b88349de2 100644 --- a/nimbus/p2p/clique/clique_utils.nim +++ b/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 diff --git a/nimbus/p2p/clique/recent_snaps.nim b/nimbus/p2p/clique/recent_snaps.nim index daa4d335d..ed68a1ac7 100644 --- a/nimbus/p2p/clique/recent_snaps.nim +++ b/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) diff --git a/nimbus/p2p/clique/snapshot.nim b/nimbus/p2p/clique/snapshot.nim index eee6e9528..a862f4124 100644 --- a/nimbus/p2p/clique/snapshot.nim +++ b/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: - Vote(address: header.coinbase, - signer: signer, - blockNumber: number, - authorize: authOk) + let vote = Vote(address: header.coinbase, + signer: signer, + blockNumber: number, + 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: diff --git a/tests/test_clique.nim b/tests/test_clique.nim index 46cfb5fcc..1210782ec 100644 --- a/tests/test_clique.nim +++ b/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": - var pool = newTesterPool() - const maxID = 2 + suite "Clique PoA Snapshot": + var + pool = newTesterPool() + testSet = {0 .. 99} # clique/snapshot_test.go(379): for i, tt := range tests { for tt in voterSamples: - if maxId < tt.id: - echo "Tests stopped" - break - - test &"Snapshots {tt.id}: {tt.info.substr(0,50)}...": - var snap = pool.initSnapshot(tt, noisy) - check snap.isOk + if tt.id in testSet: + test &"Snapshots {tt.id}: {tt.info.substr(0,50)}...": + var snap = pool.initSnapshot(tt, noisy) + if snap.isErr: + # FIXME: double check error behavior + check snap.error[0] == tt.failure when isMainModule: diff --git a/tests/test_clique/pool.nim b/tests/test_clique/pool.nim index 7f1e52fa0..02631711d 100644 --- a/tests/test_clique/pool.nim +++ b/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 result == "": - if v.isZero: - result = "@0" + if v.isZero: + result = "@0" + else: + 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)}" & - delim & &"nonce={ap.ppNonce(v.nonce)}" & - delim & &"extraData={ap.ppExtraData(v.extraData)})" + &"{sep}coinbase={ap.ppAddress(v.coinbase)}" & + &"{sep}nonce={ap.ppNonce(v.nonce)}" & + &"{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) + ]# diff --git a/tests/test_clique/voter_samples.nim b/tests/test_clique/voter_samples.nim index 0e20c2248..527228f4b 100644 --- a/tests/test_clique/voter_samples.nim +++ b/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