devp2p: ETH-LES class refactor

[scorbajio]

Creates a new super-class to consolidate commonalities between the ETH and LES protocols. Fixes #1450.

[codecov]

Codecov Report

Merging #1600 (5b685d4) into master (94211ce) will not change coverage.
The diff coverage is 90.00%.

Flag	Coverage Δ
block	85.57% <ø> (ø)
blockchain	83.28% <ø> (ø)
client	75.27% <ø> (ø)
common	93.90% <ø> (ø)
devp2p	82.63% <90.00%> (ø)
ethash	90.76% <ø> (ø)
trie	80.02% <ø> (ø)
tx	88.20% <ø> (ø)
util	92.62% <ø> (ø)
vm	81.17% <ø> (ø)

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

[scorbajio]

@ryanio @holgerd77 Anything I'm missing?

[ryanio]

will try to review tomorrow! 🙏

[holgerd77]

Great start, cool, looking forward to have this implemented! 😄

The naming thing is somewhat important, we should give this some thought.

Also have some suggestions for additional unifications, will address in a separate comment.

Generally also really nice to review, thanks for splitting this up in such easy to follow steps, this helps a lot! 🙂

[holgerd77]

Ok, so the initMsgDebuggers() function is actually already a good example for why this PR/work is really useful. This can actually be unified as well, code is slightly different in ETH and LES but for LES this is just because some functionality has been forgotten to be added. 😋 So you can just take the ETH version of the code and unify (I guess with keeping subclass functions passing over the message code enums).

Same goes for _addFirstPeerDebugger() and debug().

Regarding the global debug object, I guess you can also switch over to pass the DEBUG_BASE_NAME in the constructor to the super class and initialize a private (protected) _debug class member instead of the global debug variable. Then the rest of the functions should (probably ?) work.

Member functions which had been private before need to be made protected with this move.

[ryanio]

thanks @ghorbanian! this looks really great. I'll give this a more in-depth review now.

@holgerd77 do you have any further comments?

@ghorbanian do you have anything else you'd like to add? should we move from WIP to "needs review"?

[scorbajio]

thanks @ghorbanian! this looks really great. I'll give this a more in-depth review now.

@holgerd77 do you have any further comments?

@ghorbanian do you have anything else you'd like to add? should we move from WIP to "needs review"?

Should be good to move to "needs review."

[holgerd77]

@ghorbanian Yeah, can agree, PR looks great! 🥳

Maybe you can do the final typing suggestions from Ryan, then I would also say that this is ready to be merged.

Ah, and maybe one last quality assurance thing: did you do a final "live" test run on this within the client (make sure to build before) (see here for guidance), and optimally doing 3-4 runs using these special debug loggers (first peer, IP, so basically the things which can be activated from here)?

All this debugging stuff is not at all covered by tests, and it is otherwise not really detectable if things are or are not running as expected.

But otherwise: again, great work, thanks for keeping up on this for such a long time. 🙂

[scorbajio]

@ghorbanian Yeah, can agree, PR looks great! 🥳

Maybe you can do the final typing suggestions from Ryan, then I would also say that this is ready to be merged.

Ah, and maybe one last quality assurance thing: did you do a final "live" test run on this within the client (make sure to build before) (see here for guidance), and optimally doing 3-4 runs using these special debug loggers (first peer, IP, so basically the things which can be activated from here)?

All this debugging stuff is not at all covered by tests, and it is otherwise not really detectable if things are or are not running as expected.

But otherwise: again, great work, thanks for keeping up on this for such a long time. 🙂

Appreciate the support along the way!

Bear with me as I look into QA and those typing changes, I have a busy week ahead of me.

[ryanio]

awesome @ghorbanian, thanks for updating! there is one merge conflict on packages/devp2p/src/eth/index.ts before the tests can run, but then it LGTM!

[holgerd77]

Ah, there is another PR which happened in parallel here #1643 doing somewhat heavy changes on very much the same code parts - a bit unfortunate.

Think it might be a bit too much to ask to do a rebase/merge resolve here, will try to do myself locally and then re-push a PR, authorship should hopefully be preserved.

[holgerd77]

Or maybe: @ScottyPoi, would you dare to do this? 🙂 This would also make sense, since you already did the work on #1643 and this work here goes in a bit similar direction.

So this is basically a refactoring Amir did here with combining logic from the ETH and LES classes into one super-class. Since there was also some greater consolidation opportunity on parts of the debug code, this is touching a lot of debug code as well as in your PR.

So it would be great if you can fetch the branch from Amir and try to carefully do a git rebase master on this. This will likely get a bit hairy and one need to be careful. 😛 If you don't feel safe enough on rebases feel free to push the ball back. 🙂 There should likely already be enough back assurance though if one gets the tests to pass.

And finally: Amir @ghorbanian, this is not to take this out of your hands or something, but just an offer to share the work load since you also seem busy with other stuff. If you would like to finish yourself here also feel free to speak out loud and clearly. 😀 Again, just an offer where I thought that it might make sense.

[ryanio]

LGTM! thanks @ghorbanian!

[ryanio]

@holgerd77 do you want to give a quick review, ensure everything still works as expected with the debug loggers, etc. and if all looks good, you can merge in?

[holgerd77]

I've the impression that not all DEBUG loggers are working, but I counter-tested on master, and there some loggers are working neither, so this is very likely not related to this PR (so e.g. DEBUG=devp2p:eth:GET_BLOCK_HEADERS npm run client:start -- --network=sepolia shows no output).

So I will merge here, we should address this at some point though (not super-critical though).

Thanks Ghorbanian! 🙂

[ryanio]

I found the logging issue, will open a separate PR to fix