Parallelize header download across peers to fetch headers within a chain's checkpointed region.

Chinwendu20 commented 1 year ago

This pull request parellizes header download across peers within a chain's checkpointed region.

If neutrino's chain has checkpoints and its header tip is within that checkpointed region, the blockmanager batches multiple getheaders requests and sends them to the work manager. Even though we fetch in parallel we must validate and write to the DB in order. The processBlKHeaderInCheckPtRegionInOrder loop handles the fetched headers and writes to the DB in order. After fetching headers in parallel within the checkpointed region, the headers in the uncheckpointed region is fetched synchronously as before.

The changes are detailed in the commit messages.

Fixing https://github.com/lightninglabs/neutrino/issues/71

Chinwendu20 commented 11 months ago

Thanks a lot for running the workflow, please I have fixed the issues. I would appreciate it if you rerun the workflow @yyforyongyu

Chinwendu20 commented 11 months ago

Hello @positiveblue, this PR is ready for review.

Chinwendu20 commented 10 months ago

First off I just want to say congratulations for putting up this PR in the first place. This code isn't the most approachable and it's not organized in a way that made your task easy. The task you set out to solve is an important one for getting this technology into more people's hands around the world.

That said, I do believe there is a ton of work to do on this PR before we can get it merged. While I did go ahead and leave a bunch of comments on individual items I saw, I think that there is some more general work here to be done in terms of organization and so I strongly recommend attacking that before and use my comments as examples of what I'm talking about rather than a TODO list for what to tidy up.

Overall the biggest thing I'm seeing is that this code does not try to isolate concerns enough. When we are doing software engineering, the only way we can build up ever larger structures and not have them fall apart is because we can package up complexity in a way that it doesn't leak. Abstractly this allows us to entirely forget about how a piece of code works internally and only have to reason about how we interact with it. Concretely this means a few things.

As much as possible try to write functions that take data in as arguments and return data out via return values. This allows the readers of your API to understand the contract, rather than having to read the code itself, which defeats the purpose of packaging complexity

Try to make each function and each struct only responsible for one thing. It is OK to have nested structs within each other. It is not OK to have 2 conceptually distinct subsystems that are sharing a struct

Naming things well makes the difference between people being able to follow your thoughts and not. Try to name things that help people understand its role not its representation. After all, if you do your job in complexity packaging properly, readers should never have to care about its representation, but they will have to care a lot about how to use something. Secondly, in a statically typed language such as go, the type system will give us extra information on how something is represented, so there is no need to duplicate that information in the name.

Avoid mixing "validation" code with "computation" code. You will inevitably need to sanitize and validate data, after it is validated you may wish to do some computation over that data. It may be tempting to package these things into the same function but it blurs the requirements your true API contract. When you write a function with a good function signature (precise argument and return types), it is often the case that a reader of your code does not have to read the implementation at all. This is a good thing. If you do the validation inside the function, it will allow you to be more permissive in the types that function accepts. While this might seem like a good thing at first, it makes it harder to understand what the code does.

As far as concrete design goes, I'll offer a bit of guidance here, and we can follow up on the LND Slack to iron out some details.

Conceptually you are trying to take the workload of the blockManager/workManager and do the following:

get an outline of the full scope of work you need to do by getting the block headers and filter checkpoints

creating a bunch of jobs to go fetch those series' of filter headers

allocating those jobs to preferred peers

tracking the progress of each of these jobs

gluing the entire filter header chain together to make sure it lines up

Ideally you should be able to point to a single function that does each of these things. As I was reading the code, I was unable to. Perhaps this is a failure of my depth of understanding of the codebase, but even so, if I can't follow what is going on, it is likely others will have trouble as well. I want to reiterate, making this readable is not an easy task and the fact that you were able to get this far is quite an accomplishment, so don't be discouraged. It will take some thought to reimagine the approach to this PR but code is simply organized thought and you will be able to bring everything you've learned into the next revision. You also do not have to do this without support. You can ask questions in this thread, or better yet, reach out via Slack to run ideas by me. Looking forward to hearing from you.

Thanks for taking out your time to review this code and offer your invaluable suggestions. I guess if a lot of people are finding it hard to follow the code then it is a call of concern.

This PR basically just makes adjustments to the query package to enable the workmanager orchestrate the process of fetching block headers across various peers in a non-synchronous way. The pull request can be broken down into two parts:

Adjustments made to the query package.
Adding and modifying functions in the blockmanager to begin fetching headers in the chain's checkpointed region leveraging the blockmanager's work dispatcher. Previously this dispatcher was inly used for fetching cfheaders but now we have included block headers.

Trying to reimagine and work on your various concerns and so I have responded to your comments on the code.

I would be happy to understand your perspective better and use yours and the community's invaluable insights to help make this PR better.

lightninglabs-deploy commented 4 months ago

@yyforyongyu: review reminder @positiveblue: review reminder @kcalvinalvin: review reminder @chinwendu20, remember to re-request review from reviewers when ready