Add sideloading functionality to neutrino #285

Chinwendu20 · 2023-08-19T14:09:27Z

Related issue #70

This issue adds the sideloading functionality in neutrino. We want to be able to preload the block header and filter header store with headers before neutrino starts up. This optimizes speed.

Change summary:

Introduce a sideload package responsible for sideloading. It fetches headers from the source, validates them, and writes to the store.
Write an implementation for fetching block headers from a binary encoded source.
Decouple block header validation and storage from the block manager.
Decouple the process of finding the next and previous block header checkpoints from the block manager.
Include functionality in the neutrino chain service.

positiveblue

This looks pretty nice! I think we are in the right track here 👍

I left some comments, many are about naming, styling and formatting but there are others about the feature logic that we need to address 🙏

sideload/binary.go

blockmanager.go

Chinwendu20 · 2023-08-22T19:59:59Z

Thanks @positiveblue , please I left comments on your review and pushed some changes in response to your feedback (the ones that I am clear on).

Chinwendu20 · 2023-09-05T11:16:31Z

Hello @positiveblue, have you had the chance to look at this again. If there are no objections to my comment. I will just make fmt on this and include a description of the encoding and push.

positiveblue

Second round done. I think it's taking shape 🥇 but there is still some things to address.

Currently unit test are not passing and the linter is also failing. You can check the github actions to get more details 🙏

Should we include the side loading for cfilters in this PR or you prefer to leave it for a next one?

chainDataLoader/binary.go

blockmanager.go

Chinwendu20 · 2023-09-25T08:54:55Z

Hello can anyone please run the workflow?

Chinwendu20 · 2023-09-27T09:04:26Z

Please can anyone help run the workflow?

Chinwendu20 · 2023-09-27T09:55:28Z

hmm, this works fine locally. I think I would just write a simple unit test instead of the integration test.

Roasbeef · 2023-09-28T20:59:13Z

@Chinwendu20 have you run it with the race condition detector locally?

Chinwendu20 · 2023-09-29T06:57:52Z

Oh no I did not @Roasbeef but I have made some changes now and ran it with the race detector, hopefully, this works on the CI as well. Can anyone please help run the workflow?

Chinwendu20 · 2023-09-29T09:32:01Z

Oh okay I will just mock the reader interface and write a unit test now.

Chinwendu20 · 2023-09-29T19:21:38Z

Please can anyone help run the workflow

Roasbeef

Nice work so far on this feature!

Completed an initial review, with the following jumping out:

Style guide not closely adhered to (godoc string, 80 char columns, etc)
The main set of interfaces can be simplified.
The control flow can be simplified: if we have more headers than in the file, we can terminate. We should check the file before starting up any other sub-system. The file can just have the header be removed, then appended in one step to the end of headers.bin.
We can also use the side loader for the filter headers as well. I think this is where that generic param can actually be used.
No need for custom re-org logic, just load the file if it's relevant, then let the normal p2p logic handle reorgs if needed.

Roasbeef · 2023-10-27T21:59:00Z

chaindataloader/binary.go

+
+First 17 bytes holds the following information:
+
+- First 8 bytes holds the height of the first block header in the binary file (start height).


If we want to compress things a bit more, then we can use a varint here, so: https://github.com/lightningnetwork/lnd/blob/3b7cda9e8d3a493fd548077a6cd6d5b8fa4b76bb/tlv/varint.go#L15

The comment should also be wrapped to 80 character columns as dictated by our style guide. I think it can be compressed to just:

Each file has a header consisting of: firstHeight || lastHeight || chainType.

Also we can just use an integer for the chain type, so assign values 1-4 to: mainnet, testnet, simnet, regtest, etc.

Thanks, I would look into this.

chaindataloader/binary.go

Roasbeef · 2023-10-27T22:01:58Z

chaindataloader/binary.go

+
+	if _, err := b.file.ReadAt(rawBlkHdr, b.offset+(headerfs.BlockHeaderSize*b.tracker)); err != nil {
+		if err == io.EOF {
+			return nil, ErrEndOfFile


I think it can just pass thru io.EOF as normal?

#285 (comment)

blockmanager.go

Roasbeef · 2023-10-27T22:12:51Z

blockmanager.go

+			return
+		default:
+			// Request header
+			header, headerErr := b.sideLoadBlkHdrReader.NextHeader()


Should read batches of headers at a time. We can also verify batches of headers at a time.

Just like it is done in handleheaders ? From what I understand even though we read in batch we would have to verify the headers one after the other.

Yes we can read them in as a batch, verify one by one (validity of header N depends on the validity of header N-1), then write as a batch.

blockmanager.go

Roasbeef · 2023-10-27T22:14:14Z

blockmanager.go

+
+				// Verify checkpoint only if verification is enabled.
+				if b.nextCheckpoint != nil &&
+					node.Height == b.nextCheckpoint.Height {


We should feed all this through our normal header processing logic. Some refactoring might be needed.

You mean refactoring the handleheaders function?

For regular block headers, I think the sideloading should be in blockManager.Start() after the blockHandler goroutine has started. Then it can call handleHeadersMsg with the set of block headers. The headersMsg struct might need to be modified since there's no peer -- maybe a sideloading bool so we can error if things go wrong. This also gets rid of the code duplication

For filter headers, something similar can be done where the sideloading is in Start().

I can't place it after the blockhandler goroutine because we want to sideload before connecting to the network to fetch block headers.

IMO it's better to have this be distinct, as then we don't need to increase the set of responsibilities of the blockManager.

Logic similar to the following can validate all teh headers at oce:

for height := uint32(1); height <= bestBlockHeight; height++ { if currHeader.PrevBlock != prevHeader.BlockHash() { return fmt.Errorf("block header at height %d does "+ "not refrence previous hash %v", height, prevHeader.BlockHash().String()) } parentHeaderCtx := newLightHeaderCtx( int32(height-1), &prevHeader, headerStore, nil, ) skipDifficulty := blockchain.BFFastAdd err = blockchain.CheckBlockHeaderContext( &currHeader, parentHeaderCtx, skipDifficulty, chainCtx, true, ) if err != nil { return fmt.Errorf("error checking block %d header "+ "context: %w", height, err) } err = blockchain.CheckBlockHeaderSanity( &currHeader, chainCtx.params.PowLimit, timeSource, skipDifficulty, ) if err != nil { return fmt.Errorf("error checking block %d header "+ "sanity: %w", height, err) } // TODO: Validate checkpoint and filter headers. prevHeader = currHeader if height > 0 && height%100_000 == 0 { log.Debugf("Validated %d headers", height) } }

We'd then:

Read a chunk of N headers (say 2k or so) from the side loader source.

Validate them all at once.

Write them all to disk directly.

Cool, I thought we did not want to duplicate the functionality of the handleheaders function #285 (comment)

blockmanager.go

linden · 2023-10-27T23:16:12Z

Hi @Chinwendu20, I'd love to implement this into a wallet I'm working on for Joltz Rewards (https://joltzrewards.com).

Let me know if you'd like any support in addressing this PR review. Happy to dedicate time towards it.

Chinwendu20 · 2023-10-28T06:20:03Z

Nice work so far on this feature!

Completed an initial review, with the following jumping out:

Style guide not closely adhered to (godoc string, 80 char columns, etc)

The main set of interfaces can be simplified.

The control flow can be simplified: if we have more headers than in the file, we can terminate. We should check the file before starting up any other sub-system. The file can just have the header be removed, then appended in one step to the end of headers.bin.

We can also use the side loader for the filter headers as well. I think this is where that generic param can actually be used.

No need for custom re-org logic, just load the file if it's relevant, then let the normal p2p logic handle reorgs if needed.

Thanks for the review left some comments and would implement changes that I am clear on.

lightninglabs-deploy · 2024-03-07T23:41:50Z

@Roasbeef: review reminder
@Chinwendu20, remember to re-request review from reviewers when ready

Chinwendu20 · 2024-03-13T11:13:32Z

@ellemouton, you can review now, thank you so much. I have not yet included a commit to remove code that is now redundant in blockmanger. As well as updated the blockmanager to use the validator and writer that was created in this PR.
Please skip formatting comments for now.

I just want to know if it is in line with Laolu's comments concerning encapsulation and using interfaces in the chaindataloader package and if the design is okay in general.

chaindataloader/dataloader.go

ellemouton

@Chinwendu20 - I think the overall idea of the various interfaces & abstractions along with the decoupling from the block manager look good 👍 would be good to get an opinion from @Roasbeef again though.

I think a good next step would be to spend some time getting the PR in a working & readable state. As is, it is hard to see what fits where just by going through the commits. So it is hard to give detailed feedback. As a reviewer, we want to be able to play around with the code. And then it is also easier to give feedback about whether or not an interface looks good.

Adding working tests would be awesome too cause that is often used by reviewers to et familiar with the changes & the reasons for various decisions.

Kudos for tackling this! This is quite a big project 🥇

Chinwendu20 · 2024-03-25T05:27:01Z

sync_test.go

@@ -39,7 +42,7 @@ var (
 	// btclog.LevelOff turns on log messages from the tests themselves as
 	// well. Keep in mind some log messages may not appear in order due to
 	// use of multiple query goroutines in the tests.
-	logLevel    = btclog.LevelOff
+	logLevel    = btclog.LevelInfo
 	syncTimeout = 30 * time.Second


I will revert this

Chinwendu20 · 2024-03-25T05:29:41Z

neutrino.go

+			SkipVerify:    cfg.BlkHdrSideload.SkipVerify,
+			Chkpt:         blockHdrChkptMgr,
+			SideloadRange: SideloadRange,
+		}


Please note that I have not modified the blockmanager yet, if my approach is okay, the PR that I will submit would not have these.

Chinwendu20 · 2024-03-25T05:31:26Z

sideload/sideload.go

+		return curHeight + s.SideloadRange, nil
+	}
+}
+


I am thinking there should be nothing like sideload range and we just fetch from checkpoint to checkpoint just as the blockmanager currently does whether we are verifying or not.

Also this just returns the last height and header that should be fetched in the next fetch. I would fix the comment and name of function.

Chinwendu20 · 2024-04-10T13:26:49Z

sideload/test_utils.go

+
+	require.NoError(
+		t, tlv.WriteVarInt(encodedOsFile, c.StartHeight, &[8]byte{}),
+	)


I would jut use one scratch buffer for these

Chinwendu20 · 2024-04-10T13:28:20Z

sideload/test_utils.go

+// headerBufPool is a pool of bytes.Buffer that will be re-used by the various
+// headerStore implementations to batch their header writes to disk. By
+// utilizing this variable we can minimize the total number of allocations when
+// writing headers to disk.


copy pasta - ignore comment for now.

This commit introduces the `sideload` package, designed to facilitate the sideloading of Bitcoin blockchain headers from external sources. Key components and changes: - **Interfaces and Core Types**: Introduction of several interfaces and types such as `SourceType`, `dataType`, `dataSize`, `HeaderValidator`, `HeaderWriter`, `Checkpoints`, and `LoaderSource` to abstract the concepts of blockchain header validation, storage, and source management. - **Loader Implementation**: The core of the sideload functionality is encapsulated in the `SideLoader` struct, which includes logic for header fetching, validation, and writing. - **Binary Loader for Headers**: An implementation of the LoaderSource interface for binary encoded headers is included in this commit Signed-off-by: Ononiwu Maureen <maureen.ononiwu@outlook.com>

This commit introduces a new `Checkpoints` structure for managing block header checkpoints. Motivation: Decoupling the logic for finding next and previous header checkpoints from the `blockmanager`, facilitating sharing this functionality between the `sideload` package and `blockmanager`, promoting code reuse and consistency across the components. Signed-off-by: Ononiwu Maureen <maureen.ononiwu@outlook.com>

This commit introduces a new structure to decouple the process of validating `wire.BlockHeaders` from the blockmanager. Signed-off-by: Ononiwu Maureen <maureen.ononiwu@outlook.com>

This commit introduces a new structure to decouple the process of writing `wire.BlockHeaders` to the block header store from the blockmanager. Signed-off-by: Ononiwu Maureen <maureen.ononiwu@outlook.com>

This commit adds the sideoading functionality to neutrino's chainservice. Signed-off-by: Ononiwu Maureen <maureen.ononiwu@outlook.com>

Signed-off-by: Ononiwu Maureen <maureen.ononiwu@outlook.com>

Chinwendu20 force-pushed the side branch 2 times, most recently from 24a87f6 to 916eb57 Compare August 20, 2023 04:51

positiveblue self-requested a review August 22, 2023 01:35

positiveblue suggested changes Aug 22, 2023

View reviewed changes

Chinwendu20 force-pushed the side branch from 916eb57 to 60f4af7 Compare August 22, 2023 19:56

Chinwendu20 requested a review from positiveblue August 22, 2023 19:59

Chinwendu20 force-pushed the side branch 3 times, most recently from 214a0e6 to 26ffe03 Compare August 24, 2023 15:34

Roasbeef requested a review from ellemouton September 14, 2023 02:06

positiveblue reviewed Sep 15, 2023

View reviewed changes

Chinwendu20 force-pushed the side branch from 26ffe03 to a87d3d0 Compare September 25, 2023 08:53

Chinwendu20 force-pushed the side branch from a87d3d0 to 4e9e69c Compare September 27, 2023 09:03

Chinwendu20 force-pushed the side branch from 4e9e69c to 2390225 Compare September 29, 2023 06:53

Chinwendu20 force-pushed the side branch from 2390225 to 76952b7 Compare September 29, 2023 19:06

Chinwendu20 requested a review from positiveblue September 30, 2023 08:16

Roasbeef requested review from Crypt-iQ and removed request for positiveblue October 26, 2023 17:23

Roasbeef requested changes Oct 27, 2023

View reviewed changes

Chinwendu20 requested a review from Roasbeef February 15, 2024 22:32

Chinwendu20 force-pushed the side branch from a8b4424 to 3f21ef2 Compare March 13, 2024 06:38

Chinwendu20 marked this pull request as draft March 13, 2024 06:47

Chinwendu20 force-pushed the side branch 3 times, most recently from ee234b0 to 1f87365 Compare March 13, 2024 11:01

Chinwendu20 force-pushed the side branch from 1f87365 to 21d5c47 Compare March 13, 2024 11:18

Chinwendu20 commented Mar 13, 2024

View reviewed changes

chaindataloader/dataloader.go Outdated Show resolved Hide resolved

ellemouton reviewed Mar 13, 2024

View reviewed changes

Chinwendu20 force-pushed the side branch 3 times, most recently from 4ce5ab9 to d5bed2f Compare March 25, 2024 05:21

Chinwendu20 commented Mar 25, 2024

View reviewed changes

Chinwendu20 mentioned this pull request Mar 25, 2024

Add lnd config to sideload headers. lightningnetwork/lnd#8580

Draft

8 tasks

Chinwendu20 force-pushed the side branch 2 times, most recently from 41da324 to 5269c1b Compare March 26, 2024 13:05

Chinwendu20 force-pushed the side branch 3 times, most recently from 55218d5 to dd04f61 Compare April 10, 2024 13:24

Chinwendu20 commented Apr 10, 2024

View reviewed changes

Ononiwu Maureen added 6 commits April 10, 2024 15:25

neutrino: Add block header validator implemenation

8e8c1e0

This commit introduces a new structure to decouple the process of validating `wire.BlockHeaders` from the blockmanager. Signed-off-by: Ononiwu Maureen <maureen.ononiwu@outlook.com>

neutrino: Add block header writer implementation.

f278f17

This commit introduces a new structure to decouple the process of writing `wire.BlockHeaders` to the block header store from the blockmanager. Signed-off-by: Ononiwu Maureen <maureen.ononiwu@outlook.com>

neutrino: Add sideloading to chainservice

a69f275

This commit adds the sideoading functionality to neutrino's chainservice. Signed-off-by: Ononiwu Maureen <maureen.ononiwu@outlook.com>

neutrino: Added itest for sideloading.

9f2a37e

Signed-off-by: Ononiwu Maureen <maureen.ononiwu@outlook.com>

Chinwendu20 force-pushed the side branch from dd04f61 to 9f2a37e Compare April 10, 2024 14:26


		First 17 bytes holds the following information:

		- First 8 bytes holds the height of the first block header in the binary file (start height).

Add sideloading functionality to neutrino #285

Are you sure you want to change the base?

Add sideloading functionality to neutrino #285

Conversation

Chinwendu20 commented Aug 19, 2023 • edited Loading

positiveblue left a comment

Choose a reason for hiding this comment

Chinwendu20 commented Aug 22, 2023 • edited Loading

Chinwendu20 commented Sep 5, 2023

positiveblue left a comment • edited Loading

Choose a reason for hiding this comment

Chinwendu20 commented Sep 25, 2023

Chinwendu20 commented Sep 27, 2023

Chinwendu20 commented Sep 27, 2023

Roasbeef commented Sep 28, 2023

Chinwendu20 commented Sep 29, 2023

Chinwendu20 commented Sep 29, 2023

Chinwendu20 commented Sep 29, 2023

Roasbeef left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Crypt-iQ Oct 31, 2023 • edited Loading

Choose a reason for hiding this comment

Chinwendu20 Nov 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chinwendu20 Dec 4, 2023 • edited Loading

Choose a reason for hiding this comment

linden commented Oct 27, 2023

Chinwendu20 commented Oct 28, 2023

lightninglabs-deploy commented Mar 7, 2024

Chinwendu20 commented Mar 13, 2024

ellemouton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chinwendu20 Mar 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chinwendu20 commented Aug 19, 2023 •

edited

Loading

Chinwendu20 commented Aug 22, 2023 •

edited

Loading

positiveblue left a comment •

edited

Loading

Crypt-iQ Oct 31, 2023 •

edited

Loading

Chinwendu20 Nov 21, 2023 •

edited

Loading

Chinwendu20 Dec 4, 2023 •

edited

Loading

Chinwendu20 Mar 25, 2024 •

edited

Loading