Merge forkchoice getAllAncestorNodes and getAllNonAncestorNodes

[twoeths]

Describe the bug

This is from archiveBlocks.ts, it involves both getAllAncestorNodes and getAllNonAncestorNodes api, each has to do its own forkchoice loop which is not efficient

  const finalizedCanonicalBlocks = forkChoice.getAllAncestorBlocks(finalizedCheckpoint.rootHex);
  const finalizedNonCanonicalBlocks = forkChoice.getAllNonAncestorBlocks(finalizedCheckpoint.rootHex);

Expected behavior

these 2 apis should be merged and return both ancestor + non-ancestor nodes so that it only loop through forkchoice once

Steps to reproduce

No response

Additional context

No response

Operating system

Linux

Lodestar version or commit hash

v1.10.0

[Knos01]

First time exploring lodestar, I'm gonna start from this issue!

[bomanaps]

Hello @nflaig am from WIEP, is this issue WIP or can I work on it?

[nflaig]

just reassigned the issue, thanks for taking a look at this

[bomanaps]

Hello @nflaig my intended approach on this is; Combined Both Loops into One

• Introduced Two Separate Lists: Reuse getNodesBetween() work on If blockRoot is not found, return { ancestors: [], nonAncestors: [] } immediately. add a kind of check to prevent unnecessary function calls when parent is undefined. Question: Do I need to replace the two API calls with the new function with somethimg like this-

const { ancestors: finalizedCanonicalBlocks, nonAncestors: finalizedNonCanonicalBlocks } =
    forkChoice.getAllAncestorAndNonAncestorBlocks(finalizedCheckpoint.rootHex);

[nflaig]

@bomanaps sounds good to me, I have not looked at this in detail myself but if the implementation achieves the goal to only loop through fork choice nodes once then it goes in the right direction.

The function signature looks good to me and yes you would replace the two calls with a single function call. We can refine the naming / function signature once the PR is up and get more eyes on it.