Clique: get signers by block num #2610

jochem-brouwer · 2023-03-30T17:25:15Z

I added a test, which currently fails, it messes up the _cliqueLatestSignerStates somehow (after adding block 2, it adds a new signer to block 2, but also edits the signers of block 0?)

Note: we most likely have this problem with the "latest block signers", and the votes as well.

This PR currently allows to sync Goerli > 5476.

There is another problem with the current implementation: in case of a reorg, the active signers are all still remembered. However, if due to this reorg the signers at block X (where block X is now remembered of the previous change) change, this messes up the signer state internally. It should on reorg actually delete these signers above the block (need to investigate this claim though).

Note to self: local branch clique-by-blocknum-new

codecov · 2023-03-30T20:11:56Z

Codecov Report

❗ No coverage uploaded for pull request base (develop-v7@fd2e4ea). Click here to learn what that means.
The diff coverage is n/a.

Additional details and impacted files

Flag	Coverage Δ
block	`90.44% <0.00%> (?)`
blockchain	`90.55% <0.00%> (?)`
common	`95.76% <0.00%> (?)`
devp2p	`91.72% <0.00%> (?)`
evm	`79.48% <0.00%> (?)`
statemanager	`89.58% <0.00%> (?)`
trie	`90.39% <0.00%> (?)`
tx	`94.45% <0.00%> (?)`
util	`83.54% <0.00%> (?)`
vm	`84.51% <0.00%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

jochem-brouwer · 2023-03-30T23:45:30Z

Ok, I think I fixed the CI for blockchain.

The usage of cliqueActiveSigners(blockNum: bigint) are: if you input blockNum this will return whatever signers are active on that blockNum. In case a new signer gets activated/deactivated, this is referenced in the next block. This change of behavior is not a problem if there are no reorgs. However, if there are reorgs, this change should be taken into account.

acolytec3

This all looks great. Have a few clarifying questions but would love to merge this once final.

acolytec3 · 2023-04-10T12:15:15Z

packages/blockchain/src/consensus/clique.ts

@@ -233,17 +244,24 @@ export class CliqueConsensus implements Consensus {
      }
    }

+    this._cliqueLatestSignerStates.sort((a, b) => (a[0] > b[0] ? 1 : -1))


Is it likely that the clique signer states will get out of order and require us to potentially sort them twice in a single call to this update method?

I added this just to be sure, but looking at the code, it trims the cliqueLatestSignerStates after the first sort (so they are already ordered by block number) and thus removes latest signer states if they are below a certain threshold number (so the order should still be fine). I think we can remove.

acolytec3 · 2023-04-10T12:16:12Z

packages/blockchain/src/consensus/clique.ts

+          i++
+        }
+        // eslint-disable-next-line no-empty
+      } catch (e) {}


What's the error that could occur here?

I should have commented because now I don't remember. I think it was something when I tried to sync Goerli, and then for some reason signerState[0] was undefined, which throws the method. (If you want I can re-investigate)

acolytec3 · 2023-04-10T12:17:54Z

packages/blockchain/src/consensus/clique.ts

    const signers = this._cliqueLatestSignerStates
    if (signers.length === 0) {
      return []
    }
-    return [...signers[signers.length - 1][1]]
+    for (let i = signers.length - 1; i >= 0; i--) {
+      if (signers[i][0] < blockNum) {


Just clarify my understanding here, we're just returning the most recent signer state, (which might be from a block older than the current chain head if no signer state change occurs on the current block, right?)

I think you got the idea here, but just for clarity:

This solves the reorg problem, because before it always returned whatever active signers there were at current chain head. (On client this could result in execution vs block import being out of sync which thus require different active signers if you verify them)

Note that _cliqueLatestSignerStates is sorted

We return the first candidate which has active signers < requested block num (so the state we want)

Note that signers always has 1 member

acolytec3 · 2023-04-10T12:18:29Z

packages/blockchain/src/consensus/clique.ts

@@ -455,6 +478,7 @@ export class CliqueConsensus implements Consensus {
    this._cliqueLatestSignerStates = this._cliqueLatestSignerStates.filter(
      (s) => s[0] <= blockNumber
    )
+    this._cliqueLatestSignerStates.sort((a, b) => (a[0] > b[0] ? 1 : -1))


How often to signer states get out of order? Seems like we do a lot of sorting.

On each reorg they will be out-of-order and will mess up the latest states (this fixes reorgs in case voting changes). However since this is in the delete method and the assumption is that the signer state is always sorted, this should only pop items from the end of the array, so might indeed remove.

acolytec3 · 2023-04-10T12:23:39Z

packages/blockchain/test/clique.spec.ts

+    for (let i = 1; i < blocks.length; i++) {
+      await blockchain.putBlock(blocks[i])
+    }
+    st.end()


Should we add a simple check to validate the signer states at some of the historical blocks here or is that sufficiently covered by the reorg tests at the bottom?

Oh wow, the diff got completely messed up. Now it looks like I wrote a gigantic amount of new tests which is not the case. I must have accidentally moved some stuff around. I only added reorg tests and this specific test does not reorg. Note that if you putBlock and the state is wrong (either on reorg or not), then putBlock will throw. I will try to fix the diff.

Nvm, I had to move some logic into methods to create the addNextBlockReorg function. This specific test, I think I added this to test things internally. I think for completeness, lets keep it, but I think if we run this on the old version (without reorg logic) this test still passes anyways.

acolytec3 · 2023-04-10T12:29:16Z

packages/blockchain/test/clique.spec.ts

+        'address C added to signers'
+      )
+      st.ok(
+        equalsBytes((await blockchain.getCanonicalHeadBlock()).hash(), headBlockUnforked.hash())


Complete nit but these equalsBytes calls can be replaced with st.deepEquals...

Oh really? I did not know that :)

jochem-brouwer · 2023-04-10T18:46:05Z

I will pick up these points and cleanup. I will also not fix this epoch reorg thing, but I will comment out the test and leave it there for educational purposes, and explicitly comment that we do not support certain reorg situations, like epoch reorg. (And possibly others but I will not research).

In case you linearly import a PoA chain now, this should be fine. However, at tip of chain if there are reorgs, failure might occur. (Note that this is still somewhat fixable: you can rewind the chain back to latest epoch change block, read clique state from there, and then just replay the chain, which should now fix the issue on the reorg). So it might be unstable at tip of PoA chains.

acolytec3 · 2023-04-10T21:15:47Z

Great, the couple of edits look good. Are we ready to merge?

jochem-brouwer · 2023-04-10T21:56:00Z

Yup.

jochem-brouwer · 2023-04-10T21:56:38Z

(sorry did not update the PR status, just did 😄 )

acolytec3

Lgtm

* blockchain/clique: add support for clique signers by block num * blockchain: clique sort clique latest signer states * client: fix build * blockchain/clique: fix tests and add test * blockchain/clique: fix clique bugs * blockchain/tests/clique: fix tests * blockchain: clique: add reorg test * blockchain/clique: add reorg over epoch test (fails) [no ci] * replace equalsBytes with deepEquals * blockchain/clique: address comments * blockchain/clique: ensure tests are ran --------- Co-authored-by: acolytec3 <17355484+acolytec3@users.noreply.github.com>

jochem-brouwer added PR state: WIP package: blockchain labels Mar 30, 2023

jochem-brouwer force-pushed the clique-by-blocknum branch from 751f5c0 to 1ab62c0 Compare March 30, 2023 18:01

jochem-brouwer force-pushed the clique-by-blocknum branch from d850394 to 2641d88 Compare March 30, 2023 21:08

Base automatically changed from devp2p-fixes to develop-v7 April 4, 2023 17:28

jochem-brouwer added 6 commits April 5, 2023 17:11

blockchain/clique: add support for clique signers by block num

48cb53c

blockchain: clique sort clique latest signer states

d344d90

client: fix build

43457f9

blockchain/clique: fix tests and add test

efab9ee

blockchain/clique: fix clique bugs

0e28a79

blockchain/tests/clique: fix tests

604f264

jochem-brouwer force-pushed the clique-by-blocknum branch from a684d5a to 604f264 Compare April 5, 2023 15:13

jochem-brouwer added 2 commits April 5, 2023 21:24

blockchain: clique: add reorg test

2a064e2

blockchain/clique: add reorg over epoch test (fails) [no ci]

a98c80b

acolytec3 previously approved these changes Apr 10, 2023

View reviewed changes

replace equalsBytes with deepEquals

4bd488d

acolytec3 dismissed their stale review via 4bd488d April 10, 2023 12:33

jochem-brouwer added 2 commits April 10, 2023 22:07

blockchain/clique: address comments

8c96d06

blockchain/clique: ensure tests are ran

924a888

jochem-brouwer marked this pull request as ready for review April 10, 2023 21:56

jochem-brouwer added PR state: needs review and removed PR state: WIP labels Apr 10, 2023

acolytec3 approved these changes Apr 10, 2023

View reviewed changes

acolytec3 merged commit a42c10e into develop-v7 Apr 10, 2023

holgerd77 deleted the clique-by-blocknum branch April 11, 2023 15:19

holgerd77 added the target: develop-v7 label Apr 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clique: get signers by block num #2610

Clique: get signers by block num #2610

jochem-brouwer commented Mar 30, 2023 •

edited

Loading

codecov bot commented Mar 30, 2023 •

edited

Loading

jochem-brouwer commented Mar 30, 2023

acolytec3 left a comment

acolytec3 Apr 10, 2023

jochem-brouwer Apr 10, 2023

acolytec3 Apr 10, 2023

jochem-brouwer Apr 10, 2023

acolytec3 Apr 10, 2023

jochem-brouwer Apr 10, 2023

acolytec3 Apr 10, 2023

jochem-brouwer Apr 10, 2023

acolytec3 Apr 10, 2023

jochem-brouwer Apr 10, 2023

jochem-brouwer Apr 10, 2023

acolytec3 Apr 10, 2023

jochem-brouwer Apr 10, 2023

jochem-brouwer commented Apr 10, 2023

acolytec3 commented Apr 10, 2023

jochem-brouwer commented Apr 10, 2023

jochem-brouwer commented Apr 10, 2023

acolytec3 left a comment

Clique: get signers by block num #2610

Clique: get signers by block num #2610

Conversation

jochem-brouwer commented Mar 30, 2023 • edited Loading

codecov bot commented Mar 30, 2023 • edited Loading

Codecov Report

jochem-brouwer commented Mar 30, 2023

acolytec3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jochem-brouwer commented Apr 10, 2023

acolytec3 commented Apr 10, 2023

jochem-brouwer commented Apr 10, 2023

jochem-brouwer commented Apr 10, 2023

acolytec3 left a comment

Choose a reason for hiding this comment

jochem-brouwer commented Mar 30, 2023 •

edited

Loading

codecov bot commented Mar 30, 2023 •

edited

Loading