Skip to content

chainimport[1/3]: import block and filter headers on startup before falling back to P2P synchronization #320

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

chainimport[1/3]: import block and filter headers on startup before falling back to P2P synchronization #320

wants to merge 9 commits into from

Conversation

mohamedawnallah
Copy link
Contributor

@mohamedawnallah mohamedawnallah commented Jun 14, 2025 •
edited
Loading

Description

This PR serves two main purposes:

  1. Refactor related code and remove duplicated logic
  2. Fail gracefully on DB header writes

Towards #70.
Next #317.

Motivation and Context

This opportunity for improvement was identified while addressing Oli's feedback #317 (comment).

@mohamedawnallah mohamedawnallah force-pushed the failGracefullyOnDBHeaderWrites branch 6 times, most recently from ff4ead7 to b3b80de Compare June 14, 2025 09:29
@mohamedawnallah
Copy link
Contributor Author

mohamedawnallah commented Jun 14, 2025 •
edited
Loading

I have run this CI a few times - sometimes the tests succeed and other times they fail. Not 100% sure if the changes in this PR introduced this flakiness or if they are "natural" flakes. Further investigation is needed.

Logs for failed CI tests:
https://gist.github.com/mohamedawnallah/f3e4ead6da3c23ec1368212ba840f030

@mohamedawnallah mohamedawnallah mentioned this pull request Jun 14, 2025
Open
6 tasks
@mohamedawnallah mohamedawnallah force-pushed the failGracefullyOnDBHeaderWrites branch 5 times, most recently from 6c0b7e1 to c1bf101 Compare June 17, 2025 14:29
@mohamedawnallah
Copy link
Contributor Author

mohamedawnallah commented Jun 17, 2025 •
edited
Loading

@guggero – How am I supposed to move this PR forward? There are no flaky test cases when running this multiple times. It is now ready for review. This PR mainly addresses your comment in the mentioned PR #317 (comment) while refactoring related code

@guggero guggero requested review from ziggie1984 and guggero June 18, 2025 08:24
@guggero
Copy link
Member

guggero commented Jun 18, 2025

Yeah, looks ready. Assigned myself and @ziggie1984 for review.

guggero
guggero reviewed Jun 19, 2025
View reviewed changes
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did a first pass, looks to be pretty far along.
To add additional integration test coverage, it would be good to create an lnd (draft!) PR in your repo or the main repo that bumps the Neutrino library to this PR, to see if the full Neutrino integration test runs properly.

@saubyk saubyk added this to lnd v0.20 Jun 26, 2025
@saubyk saubyk moved this to In review in lnd v0.20 Jun 26, 2025
@mohamedawnallah mohamedawnallah force-pushed the failGracefullyOnDBHeaderWrites branch 4 times, most recently from 353b7d0 to b0ed3f7 Compare July 1, 2025 14:22
@mohamedawnallah mohamedawnallah force-pushed the failGracefullyOnDBHeaderWrites branch from b0ed3f7 to 73a471b Compare July 4, 2025 20:48
@mohamedawnallah
Copy link
Contributor Author

/gemini review

@mohamedawnallah mohamedawnallah mentioned this pull request Jul 7, 2025
Closed
9 tasks
@mohamedawnallah
Copy link
Contributor Author

mohamedawnallah commented Jul 7, 2025 •
edited
Loading

Why gemini not triggered? reopening this PR

@mohamedawnallah mohamedawnallah force-pushed the failGracefullyOnDBHeaderWrites branch 7 times, most recently from 12a9c3e to 5ea4198 Compare July 18, 2025 21:25
@mohamedawnallah
Copy link
Contributor Author

/gemini review

gemini-code-assist[bot]
gemini-code-assist bot reviewed Jul 18, 2025
View reviewed changes
Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This is a great refactoring that significantly improves the structure and robustness of the header storage logic. The introduction of interfaces, consolidation of related logic, and improved error handling are all excellent changes. The test suite has also been substantially improved with better mocking and more comprehensive test cases.

I've found one critical issue with incorrect error formatting that could lead to a panic. Once that's addressed, this PR will be in great shape.

@mohamedawnallah mohamedawnallah force-pushed the failGracefullyOnDBHeaderWrites branch from 5ea4198 to 40e9054 Compare July 18, 2025 21:30
@mohamedawnallah
Copy link
Contributor Author

/gemini review

gemini-code-assist[bot]
gemini-code-assist bot reviewed Jul 18, 2025
View reviewed changes
Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

The pull request introduces refactoring and improvements, including interfaces for header stores, testify mocks, and consolidated platform-specific code. The error handling for database writes is a crucial addition for robustness. However, there are potential issues related to data consistency in the filterHeaderStore's rollback logic and error handling in NewBlockHeaderStore.

@mohamedawnallah mohamedawnallah force-pushed the failGracefullyOnDBHeaderWrites branch 2 times, most recently from 21dd256 to afc723a Compare July 18, 2025 21:51
@mohamedawnallah
Copy link
Contributor Author

mohamedawnallah commented Jul 18, 2025 •
edited
Loading

Thanks a lot @guggero for the last round of review and your patience reviewing my work. I've addressed the feedback also I (re)organized git commits addressing this feedback comment #320 (comment) as a learning exercise. I think the PR is now in a good shape for another round of review 🙇‍♂️

@mohamedawnallah mohamedawnallah requested a review from guggero July 18, 2025 21:59
@mohamedawnallah mohamedawnallah force-pushed the failGracefullyOnDBHeaderWrites branch from afc723a to da926b2 Compare July 19, 2025 05:07
guggero
guggero reviewed Jul 23, 2025
View reviewed changes
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice, thanks a lot for the improved commit structure. Now ready very well!
Just one final question, other than that LGTM 🎉

@mohamedawnallah mohamedawnallah requested a review from guggero July 23, 2025 08:36
guggero
guggero approved these changes Jul 23, 2025
View reviewed changes
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🎉

@saubyk saubyk requested a review from Roasbeef July 24, 2025 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments

@guggero guggero guggero approved these changes

@ziggie1984 ziggie1984 Awaiting requested review from ziggie1984

@Roasbeef Roasbeef Awaiting requested review from Roasbeef

At least 2 approving reviews are required to merge this pull request.

Assignees

@mohamedawnallah mohamedawnallah

Labels
header side loading issues/prs related to side loading of headers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mohamedawnallah @guggero @Roasbeef @lightninglabs-deploy @saubyk