Client/Block: Clique PoA fixes #1088

holgerd77 · 2021-02-08T12:44:35Z

No description provided.

codecov · 2021-02-08T12:46:12Z

Codecov Report

Merging #1088 (c23f006) into master (4cb7fae) will increase coverage by 1.20%.
The diff coverage is 73.49%.

Flag	Coverage Δ
block	`?`
blockchain	`?`
client	`87.29% <73.68%> (+0.02%)`	⬆️
common	`?`
devp2p	`?`
ethash	`?`
tx	`?`
vm	`82.32% <73.33%> (-1.63%)`	⬇️

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

holgerd77 · 2021-02-08T15:27:46Z

I am as close as one can get: 😄

holgerd77 · 2021-02-08T16:26:58Z

Yeah, yeah, yeah

client_goerli_ruzn.mov

🎉 🎉 🎉 🥳

holgerd77 · 2021-02-08T16:42:02Z

Got it, client is now syncing with the Goerli network. 😄

This is now ready for review, change summary can be taken from the commit messages.

ryanio · 2021-02-08T17:35:34Z

packages/client/lib/net/protocol/ethprotocol.ts

@@ -56,8 +56,6 @@ const messages: Message[] = [
    name: 'BlockHeaders',
    code: 0x04,
    encode: (headers: BlockHeader[]) => headers.map((h) => h.raw()),
-    decode: (headers: BlockHeaderBuffer[]) =>
-      headers.map((h) => BlockHeader.fromValuesArray(h, {})),


Could we just add common here instead of removing the decode function?

headers.map((h) => BlockHeader.fromValuesArray(h, { common: this.chain._common }))

This commit (2ca45cd) feels like it's moving around and adding code to handle the buffers that was otherwise nicely handled here.

I had this implemented before (as announced here #1086 (comment)) but then reverted it because I had to pass config along every single decode method due to typing and then add another round of //es-lint-ignore comments since it wasn't used in most of the other decode functions. So I decided that this would at the end be the cleaner way, especially since this was in an inconsistent state anyhow with the Block decoding also working without a decode method like now done here for the BlockHeader decoding. Wouldn't be a big fan of reverting, but if you have got a cleaner way of doing this feel free to push here, no emotional connections to this code 😋 , just would want this to work "somehow".

Maybe this can be readded (then also together with the Block decoding) along some refactor at some point towards some more type friendly handling of this whole ETH/LES message call handling?

oh yeah passing common into decode would be a lot of passing around. can't we use the common from chain in EthProtocolOptions?

Maybe this can be readded (then also together with the Block decoding) along some refactor at some point towards some more type friendly handling of this whole ETH/LES message call handling?

yes for sure, I will start thinking about this. I think if we inline the messages const into methods on the actual class then it would allow for seamless type info without needing to define the additional type helper EthProtocolMethods.

Ah, yes, you are so right, I didn't see the option that messages could be relatively easily inlined into the Protocol classes. Have removed the old commits via rebase and done this now in 08ffcd8. I had to add a no-invalid-this lint ignore rule here since eslint was complaining, but I checked with a console.log() in the decode message and this is properly resolved and the common object is present and not set to undefined or something.

This is really much nicer now. 😄

holgerd77 · 2021-02-08T22:13:06Z

First time sync stops on Goerli is with Error: invalid clique difficulty (block number=5281 hash=cca147be20ed1c416cfc8b533d15827764a216eef5d2c67f472ee9d143538926), so there still seems to be something wrong with this difficulty-in-turn, difficulty-out-turn check in BlockHeader.validateCliqueDifficulty(), difficulty of the respective block 5281 is 1.

Update:

Some numbers from within the library:
Signer length: 2
Signer index: 1
inTurn: true

Can't spot what's wrong on first sight.

Another update:

Ah, there seems to be a new signer 0x000000568b9b5a365eaa767d42e74ed88915c204 added around that block, so generally an interesting spot for testing PoA mechanics.

Maybe it's the order in blockchain of the validate call and the signer updates? 🤔

holgerd77 · 2021-02-08T22:15:28Z

(side question: so what do you guys think? I would love to add such additional information as in the error message above (with the block number and hash) also to the other block/header error messages (and also expand to other libraries), for now I just kept it to the new error messages to not change anything existing here. Do you think this is acceptable?)

ryanio · 2021-02-08T23:14:49Z

yeah I love the additional context for the error messages, I wonder if we can stick it in some meta field on the error object to not disturb the prior messages, but then I'm not sure if it would output on a toString(). another idea could be to make some kind of error factory that would try to insert this data so we don't have to add it to each error individually.

in general i think the more we use enums for error messages gives us more flexibility to not worry about disturbing the string content. it would also free us to more easily fix typos or improve wording/clarity incrementally. perhaps we could do a sprint to swap error strings for enums in v6. i found geth's code base also does a really good job of defining and documenting their error enums (example 1, example 2).

ryanio · 2021-02-08T23:19:02Z

Maybe it's the order in blockchain of the validate call and the signer updates? 🤔

That could definitely be it, it looks like validate is run before cliqueUpdateVotes and cliqueUpdateLatestBlockSigners in _putBlockOrHeader.

…tion public, added tests and testdata for hash() tests

… quality indicator and being present as an devp2p ETH status value

… blocks, added VM execution PoA tests to client

holgerd77 · 2021-02-09T08:51:15Z

(side note for reviewers: the clique error from above is independent from this PR - just noted it down for convenience since I looked a bit further - so this remains open for review)

holgerd77 · 2021-02-09T09:05:28Z

@ryanio that's a great idea with the error sprint, have added this to #1024. 😄

…o annotate errors

holgerd77 · 2021-02-09T11:46:22Z

@ryanio regarding error annotation this should already be better I guess a738424 ? I think we should be safe to at least already update the already-dynamic error messages which add custom data to a static prefix message (like the invalid transactions: ... error from the Block class.

…F behavior on outer common updates

…e HF behavior on outer common updates

…er option

…dforkByBlockNumber is passed in static constructors, create blocks in blockchain with hardforkByBlockNumber option consistently

…ogger

…patibility checks for clique functionality usage on block library, added reasonable cliqueSigner default (zero address) for default blocks

Block, Tx: always copy Common on instantiation / Fix HF consistency bugs

holgerd77 · 2021-02-11T07:59:30Z

This still needs a review as well.

holgerd77 · 2021-02-11T08:00:01Z

(@ryanio thanks for reviewing #1089)

holgerd77 · 2021-02-11T08:13:40Z

Guys, I will do an admin merge here, otherwise this will get bloated up too much and more and more difficult to review, and I need these changes for continued work this morning. I did a self-review of the remaining changes, I think all are local and unproblematic, the fixed clique hash function change has already be confirmed by the geth team, and the reactivated total difficulty is also obvious if looking at e.g. an Etherscan block where the total difficulty is still present. The common addition to the ETH/LES protocols respectively the block creations has already been reviewed and discussed with @ryanio.

Fee free to do a post-merge review if you are not satisfied with something, I'll happily integrate into a follow-up PR.

holgerd77 added type: bug PR state: WIP type: refactor type: enhancement package: block package: client labels Feb 8, 2021

holgerd77 force-pushed the client-block-poa-fixes branch from 3475290 to 1de3d5f Compare February 8, 2021 15:35

holgerd77 requested review from ryanio, jochem-brouwer and cgewecke February 8, 2021 16:40

ryanio reviewed Feb 8, 2021

View reviewed changes

holgerd77 added 5 commits February 9, 2021 09:24

block -> clique PoA: fixed hash() function, made cliqueSigHash() func…

b301a51

…tion public, added tests and testdata for hash() tests

blockchain -> PoA: reactivated total difficulty for PoA still being a…

6943f0a

… quality indicator and being present as an devp2p ETH status value

block: added error postfixes to the clique validation checks

836c25c

vm, client -> PoA: do not assign block rewards in VM when running PoA…

a619dd2

… blocks, added VM execution PoA tests to client

client: added common to ETH/LES protocol messages

08ffcd8

holgerd77 force-pushed the client-block-poa-fixes branch from 8ba0292 to 08ffcd8 Compare February 9, 2021 08:44

holgerd77 added PR state: needs review and removed PR state: WIP labels Feb 9, 2021

client -> VM execution: fixed HF switch logic and log output

2ea6761

block -> error handling: added BlockHeader._error() helper function t…

a738424

…o annotate errors

holgerd77 added 11 commits February 9, 2021 14:04

tx: make internal copy of common instance provided to remain stable H…

264632b

…F behavior on outer common updates

block: make internal copy of common instance provided to remain stabl…

b715d53

…e HF behavior on outer common updates

client: create Block and BlockHeader objects with hardforkByBlockNumb…

1935427

…er option

block, blockchain: fixed txs being created with the wrong HF when har…

8e00fa2

…dforkByBlockNumber is passed in static constructors, create blocks in blockchain with hardforkByBlockNumber option consistently

client -> VM execution: added hardfork to execution failure log output

010c04d

vm: improved early exit debug message in call or create execution

67f4dfd

vm -> debugging: added vm:state,vm:state:root loggers

4fed17c

vm -> debugging: removed state:root logger (verbose), aded vm:block l…

95546cf

…ogger

vm -> debugging: logger improvements

26274d8

vm -> clique PoA: use signer for fee attribution, added backwards-com…

e5d9cde

…patibility checks for clique functionality usage on block library, added reasonable cliqueSigner default (zero address) for default blocks

Merge pull request #1089 from ethereumjs/block-tx-copy-common

0df1f45

Block, Tx: always copy Common on instantiation / Fix HF consistency bugs

holgerd77 merged commit 6822c08 into master Feb 11, 2021

holgerd77 deleted the client-block-poa-fixes branch February 11, 2021 08:14

This was referenced Feb 11, 2021

Client: PoA Clique (Goerli) run "Error: could not find parent header" #1086

Closed

Client -> Goerli: Sync error "invalid clique difficulty" (block 5281) #1100

Closed

Support PoA chains #689

Closed

holgerd77 mentioned this pull request Mar 24, 2021

EIP-225 Clique Spec Improvements ethereum/EIPs#3426

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Client/Block: Clique PoA fixes #1088

Client/Block: Clique PoA fixes #1088

holgerd77 commented Feb 8, 2021

codecov bot commented Feb 8, 2021 •

edited

Loading

holgerd77 commented Feb 8, 2021

holgerd77 commented Feb 8, 2021

holgerd77 commented Feb 8, 2021

ryanio Feb 8, 2021

holgerd77 Feb 8, 2021 •

edited

Loading

holgerd77 Feb 8, 2021

ryanio Feb 8, 2021

holgerd77 Feb 9, 2021

holgerd77 commented Feb 8, 2021 •

edited

Loading

holgerd77 commented Feb 8, 2021

ryanio commented Feb 8, 2021

ryanio commented Feb 8, 2021

holgerd77 commented Feb 9, 2021

holgerd77 commented Feb 9, 2021

holgerd77 commented Feb 9, 2021

holgerd77 commented Feb 11, 2021

holgerd77 commented Feb 11, 2021

holgerd77 commented Feb 11, 2021

Client/Block: Clique PoA fixes #1088

Client/Block: Clique PoA fixes #1088

Conversation

holgerd77 commented Feb 8, 2021

codecov bot commented Feb 8, 2021 • edited Loading

Codecov Report

holgerd77 commented Feb 8, 2021

holgerd77 commented Feb 8, 2021

holgerd77 commented Feb 8, 2021

ryanio Feb 8, 2021

Choose a reason for hiding this comment

holgerd77 Feb 8, 2021 • edited Loading

Choose a reason for hiding this comment

holgerd77 Feb 8, 2021

Choose a reason for hiding this comment

ryanio Feb 8, 2021

Choose a reason for hiding this comment

holgerd77 Feb 9, 2021

Choose a reason for hiding this comment

holgerd77 commented Feb 8, 2021 • edited Loading

holgerd77 commented Feb 8, 2021

ryanio commented Feb 8, 2021

ryanio commented Feb 8, 2021

holgerd77 commented Feb 9, 2021

holgerd77 commented Feb 9, 2021

holgerd77 commented Feb 9, 2021

holgerd77 commented Feb 11, 2021

holgerd77 commented Feb 11, 2021

holgerd77 commented Feb 11, 2021

codecov bot commented Feb 8, 2021 •

edited

Loading

holgerd77 Feb 8, 2021 •

edited

Loading

holgerd77 commented Feb 8, 2021 •

edited

Loading