Client/Block: Clique PoA fixes #1088
Conversation
Codecov Report
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
I am as close as one can get: 😄
|
holgerd77
force-pushed
the
client-block-poa-fixes
branch
from
3475290
to
1de3d5f
Compare
|
Yeah, yeah, yeah client_goerli_ruzn.mov🎉 🎉 🎉 🥳 |
|
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. |
| @@ -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, {})), | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. 😄
|
First time sync stops on Goerli is with Update: Some numbers from within the library: Can't spot what's wrong on first sight. Another update: Ah, there seems to be a new signer Maybe it's the order in blockchain of the |
|
(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?) |
|
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 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). |
That could definitely be it, it looks like |
…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
force-pushed
the
client-block-poa-fixes
branch
from
8ba0292
to
08ffcd8
Compare
|
(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) |
…o annotate errors
…F behavior on outer common updates
…e HF behavior on outer common updates
…dforkByBlockNumber is passed in static constructors, create blocks in blockchain with hardforkByBlockNumber option consistently
…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
|
This still needs a review as well. |
|
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. |
No description provided.