PeerDAS: Fix major bug in dataColumnSidecarsByRangeRPCHandler and allow syncing from full nodes.
#14532
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Before this commit, the node was unable to respond with a data column index higher than the count of stored data columns. For example, if there is 8 data columns stored for a given block, the node was able to respond for data columns indices 1, 3, and 5, but not for 10, 16 or 127. The issue was visible only for full nodes, since super nodes always store 128 data columns.
nalepae
force-pushed
the
initial-sync-rework-2
branch
3 times, most recently
from
a8930c6 to
16b679c
Compare
nalepae
requested review from
terencechain,
rkapka and
nisdas
and removed request for
a team
nalepae
changed the title
dataColumnSidecarsByRangeRPCHandler and allow syncing from full nodes.
(Not only from supernodes.)
nalepae
force-pushed
the
initial-sync-rework-2
branch
from
16b679c to
9f679e8
Compare
nisdas
reviewed
Oct 16, 2024
| missingColumnsByRoot map[[fieldparams.RootLength]byte]map[uint64]bool, | ||
| batchSize uint64, | ||
| bwbSlice bwbSlice) error { | ||
| lastSlot := bwbs[bwbSlice.end].Block.Block().Slot() |
There was a problem hiding this comment.
why is this repeated with endSlot ?
|
|
||
| func (f *blocksFetcher) fetchBwbSliceFromPeers( | ||
| ctx context.Context, | ||
| mu *sync.RWMutex, |
There was a problem hiding this comment.
why are we passing mutexes as an argument ? this looks like an antipattern
There was a problem hiding this comment.
See the other equivalent comment.
| @@ -1089,15 +1201,43 @@ func processDataColumn( | |||
| // - `missingColumnsByRoot` by removing the fetched data columns. | |||
| func (f *blocksFetcher) fetchDataColumnFromPeer( | |||
| ctx context.Context, | |||
| mu *sync.RWMutex, | |||
There was a problem hiding this comment.
Same here, its odd to be passing in mutexes as an argument
There was a problem hiding this comment.
Usually, we use mutexes as struct field because mutexes are protecting struct fields as well.
Here, mu is only protecting concurrent access to bwbs and missingColumnsByRoot, which are not struct fields, but variables defined inside a function. That's why the corresponding mutex mu does not need to be defined as a struct field, and so the mutex is passed as an argument.
There was a problem hiding this comment.
is it possible to pass in the shared resource rather than just the mutex ? Basically have all these items enclosed in a single logical object
nisdas
approved these changes
Oct 16, 2024
nalepae
added a commit
that referenced
this pull request
Oct 23, 2024
…llow syncing from full nodes. (#14532) * `validateDataColumnsByRange`: `current` ==> `currentSlot`. * `validateRequest`: Extract `remotePeer` variable. * `dataColumnSidecarsByRangeRPCHandler`: Small non functional refactor. * `streamDataColumnBatch`: Fix major bug. Before this commit, the node was unable to respond with a data column index higher than the count of stored data columns. For example, if there is 8 data columns stored for a given block, the node was able to respond for data columns indices 1, 3, and 5, but not for 10, 16 or 127. The issue was visible only for full nodes, since super nodes always store 128 data columns. * Initial sync: Fetch data columns from all peers. (Not only from supernodes.) * Nishant's comment: Fix `lastSlot` and `endSlot` duplication. * Address Nishant's comment.
nalepae
added a commit
that referenced
this pull request
Oct 23, 2024
…llow syncing from full nodes. (#14532) * `validateDataColumnsByRange`: `current` ==> `currentSlot`. * `validateRequest`: Extract `remotePeer` variable. * `dataColumnSidecarsByRangeRPCHandler`: Small non functional refactor. * `streamDataColumnBatch`: Fix major bug. Before this commit, the node was unable to respond with a data column index higher than the count of stored data columns. For example, if there is 8 data columns stored for a given block, the node was able to respond for data columns indices 1, 3, and 5, but not for 10, 16 or 127. The issue was visible only for full nodes, since super nodes always store 128 data columns. * Initial sync: Fetch data columns from all peers. (Not only from supernodes.) * Nishant's comment: Fix `lastSlot` and `endSlot` duplication. * Address Nishant's comment.
nalepae
added a commit
that referenced
this pull request
Oct 23, 2024
…llow syncing from full nodes. (#14532) * `validateDataColumnsByRange`: `current` ==> `currentSlot`. * `validateRequest`: Extract `remotePeer` variable. * `dataColumnSidecarsByRangeRPCHandler`: Small non functional refactor. * `streamDataColumnBatch`: Fix major bug. Before this commit, the node was unable to respond with a data column index higher than the count of stored data columns. For example, if there is 8 data columns stored for a given block, the node was able to respond for data columns indices 1, 3, and 5, but not for 10, 16 or 127. The issue was visible only for full nodes, since super nodes always store 128 data columns. * Initial sync: Fetch data columns from all peers. (Not only from supernodes.) * Nishant's comment: Fix `lastSlot` and `endSlot` duplication. * Address Nishant's comment.
nalepae
added a commit
that referenced
this pull request
Oct 24, 2024
…llow syncing from full nodes. (#14532) * `validateDataColumnsByRange`: `current` ==> `currentSlot`. * `validateRequest`: Extract `remotePeer` variable. * `dataColumnSidecarsByRangeRPCHandler`: Small non functional refactor. * `streamDataColumnBatch`: Fix major bug. Before this commit, the node was unable to respond with a data column index higher than the count of stored data columns. For example, if there is 8 data columns stored for a given block, the node was able to respond for data columns indices 1, 3, and 5, but not for 10, 16 or 127. The issue was visible only for full nodes, since super nodes always store 128 data columns. * Initial sync: Fetch data columns from all peers. (Not only from supernodes.) * Nishant's comment: Fix `lastSlot` and `endSlot` duplication. * Address Nishant's comment.
nalepae
added a commit
that referenced
this pull request
Nov 20, 2024
…llow syncing from full nodes. (#14532) * `validateDataColumnsByRange`: `current` ==> `currentSlot`. * `validateRequest`: Extract `remotePeer` variable. * `dataColumnSidecarsByRangeRPCHandler`: Small non functional refactor. * `streamDataColumnBatch`: Fix major bug. Before this commit, the node was unable to respond with a data column index higher than the count of stored data columns. For example, if there is 8 data columns stored for a given block, the node was able to respond for data columns indices 1, 3, and 5, but not for 10, 16 or 127. The issue was visible only for full nodes, since super nodes always store 128 data columns. * Initial sync: Fetch data columns from all peers. (Not only from supernodes.) * Nishant's comment: Fix `lastSlot` and `endSlot` duplication. * Address Nishant's comment.
nalepae
added a commit
that referenced
this pull request
Nov 22, 2024
…llow syncing from full nodes. (#14532) * `validateDataColumnsByRange`: `current` ==> `currentSlot`. * `validateRequest`: Extract `remotePeer` variable. * `dataColumnSidecarsByRangeRPCHandler`: Small non functional refactor. * `streamDataColumnBatch`: Fix major bug. Before this commit, the node was unable to respond with a data column index higher than the count of stored data columns. For example, if there is 8 data columns stored for a given block, the node was able to respond for data columns indices 1, 3, and 5, but not for 10, 16 or 127. The issue was visible only for full nodes, since super nodes always store 128 data columns. * Initial sync: Fetch data columns from all peers. (Not only from supernodes.) * Nishant's comment: Fix `lastSlot` and `endSlot` duplication. * Address Nishant's comment.
nalepae
added a commit
that referenced
this pull request
Nov 22, 2024
…llow syncing from full nodes. (#14532) * `validateDataColumnsByRange`: `current` ==> `currentSlot`. * `validateRequest`: Extract `remotePeer` variable. * `dataColumnSidecarsByRangeRPCHandler`: Small non functional refactor. * `streamDataColumnBatch`: Fix major bug. Before this commit, the node was unable to respond with a data column index higher than the count of stored data columns. For example, if there is 8 data columns stored for a given block, the node was able to respond for data columns indices 1, 3, and 5, but not for 10, 16 or 127. The issue was visible only for full nodes, since super nodes always store 128 data columns. * Initial sync: Fetch data columns from all peers. (Not only from supernodes.) * Nishant's comment: Fix `lastSlot` and `endSlot` duplication. * Address Nishant's comment.
nalepae
added a commit
that referenced
this pull request
Nov 25, 2024
…llow syncing from full nodes. (#14532) * `validateDataColumnsByRange`: `current` ==> `currentSlot`. * `validateRequest`: Extract `remotePeer` variable. * `dataColumnSidecarsByRangeRPCHandler`: Small non functional refactor. * `streamDataColumnBatch`: Fix major bug. Before this commit, the node was unable to respond with a data column index higher than the count of stored data columns. For example, if there is 8 data columns stored for a given block, the node was able to respond for data columns indices 1, 3, and 5, but not for 10, 16 or 127. The issue was visible only for full nodes, since super nodes always store 128 data columns. * Initial sync: Fetch data columns from all peers. (Not only from supernodes.) * Nishant's comment: Fix `lastSlot` and `endSlot` duplication. * Address Nishant's comment.
nalepae
added a commit
that referenced
this pull request
Nov 25, 2024
…llow syncing from full nodes. (#14532) * `validateDataColumnsByRange`: `current` ==> `currentSlot`. * `validateRequest`: Extract `remotePeer` variable. * `dataColumnSidecarsByRangeRPCHandler`: Small non functional refactor. * `streamDataColumnBatch`: Fix major bug. Before this commit, the node was unable to respond with a data column index higher than the count of stored data columns. For example, if there is 8 data columns stored for a given block, the node was able to respond for data columns indices 1, 3, and 5, but not for 10, 16 or 127. The issue was visible only for full nodes, since super nodes always store 128 data columns. * Initial sync: Fetch data columns from all peers. (Not only from supernodes.) * Nishant's comment: Fix `lastSlot` and `endSlot` duplication. * Address Nishant's comment.
nalepae
added a commit
that referenced
this pull request
Nov 27, 2024
…llow syncing from full nodes. (#14532) * `validateDataColumnsByRange`: `current` ==> `currentSlot`. * `validateRequest`: Extract `remotePeer` variable. * `dataColumnSidecarsByRangeRPCHandler`: Small non functional refactor. * `streamDataColumnBatch`: Fix major bug. Before this commit, the node was unable to respond with a data column index higher than the count of stored data columns. For example, if there is 8 data columns stored for a given block, the node was able to respond for data columns indices 1, 3, and 5, but not for 10, 16 or 127. The issue was visible only for full nodes, since super nodes always store 128 data columns. * Initial sync: Fetch data columns from all peers. (Not only from supernodes.) * Nishant's comment: Fix `lastSlot` and `endSlot` duplication. * Address Nishant's comment.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Please read commit by commit, with commit messages.
This PR brings two major changes:
dataColumnSidecarsByRangeRPCHandler. (Read the corresponding commit message for details.)When selecting peers to fetch columns from, the node will select admissible peers with the highest corresponding columns, to minimise the number of peers to fetch columns from.
Remaining task on initial sync: