Skip to content

lnwire: prune dead code #7690

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

morehouse
Copy link
Collaborator

No description provided.

morehouse added 3 commits May 12, 2023 11:52
PkScript is a relic from the time before BOLTs [1] and is unused
anywhere but tests today.

[1] lightningnetwork@fc9ebb5
We use specialized functions to encode/decode ShortChanIDEncoding, so
the cases for this type in ReadElement and WriteElement are dead. It
appears the case in ReadElement was dead on arrival [1] while the case
in write element became dead later on.

[1] https://github.com/lightningnetwork/lnd/pull/1106/files
After lightningnetwork#4884, many of the cases in WriteElement are now dead.
@yyforyongyu yyforyongyu added no-changelog code health Related to code commenting, refactoring, and other non-behaviour improvements labels May 15, 2023
@guggero
Copy link
Collaborator

guggero commented May 15, 2023

How did you determine this code to be dead? We're using parts of the lnwire encoding/decoding code in other projects such as Loop and Pool as well, so I'd need to cross check. Maybe other projects do as well? Might be worth doing a quick GitHub search for calls to lnwire.WriteElement or lnwire.WriteElements...

@morehouse
Copy link
Collaborator Author

I only verified the code is unused within LND.

From a quick search on GitHub, indeed some of these are used by Pool. IME GitHub's search sucks and doesn't return complete results, so it's hard to know what's used by projects outside of Pool/Loop.

And actually anyone could be using any publicly exported function or type from any LND package, whether their project is on GitHub or not. Which brings up some important questions -- how much do we care about breaking LND-as-a-library use cases? Is there a deprecation policy?

There is already a TODO in the code to remove WriteElement completely. So arguably this function has been "deprecated" for years now. If you're still interested in removing some of this code, I can take a closer look at Pool and Loop and leave in any code they still use.

@guggero
Copy link
Collaborator

guggero commented May 15, 2023

Hmm, so if the goal is to get rid of WriteElements and ReadElements (wasn't aware we were working towards that), then I guess we should just move the code we need in Pool over to that repo and then get rid of the dead code here.
I did a quick search of my own and was only able to find Pool as a user of that library.
Do you have time to create a PR for that in the Pool repo? Happy to review it. Then we can go ahead with this one.

I only verified the code is unused within LND.

Question out of interest: How did you verify that? Since the call site in question doesn't have any compile time guarantees. Just from passing unit/integration tests or by inspecting each existing call site?

@morehouse
Copy link
Collaborator Author

Do you have time to create a PR for that in the Pool repo? Happy to review it. Then we can go ahead with this one.

Sure.

Question out of interest: How did you verify that? Since the call site in question doesn't have any compile time guarantees. Just from passing unit/integration tests or by inspecting each existing call site?

I inspected call sites and removed unused code, then verified all tests still pass.

@morehouse
Copy link
Collaborator Author

Pool refactor: lightninglabs/pool#449

@saubyk saubyk added the P3 might get fixed, nice to have label Oct 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code health Related to code commenting, refactoring, and other non-behaviour improvements no-changelog P3 might get fixed, nice to have
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants