PeerDAS: Implement IncrementalDAS #14109

nalepae · 2024-06-13T16:18:41Z

Please read commit by commit.

The pull requests implements IncrementalDAS.

nisdas · 2024-06-17T09:23:43Z

beacon-chain/core/peerdas/helpers.go

+
+// hypergeomCDF computes the hypergeometric cumulative distribution function.
+// https://en.wikipedia.org/wiki/Hypergeometric_distribution
+func hypergeomCDF(k, M, n, N uint64) float64 {


can we get some unit tests for CDF ?

We could, but this function is actually already 100% tested as part of ExtendedSampleCount.
This function is not used outside of ExtendedSampleCount.
ExtendedSampleCount has its own tests.

Do you still think it’s worth adding tests?

Doesn't need to be extensive, but I think some basic unit tests would be useful for the fact this is a statistical function that we might use in other places in the future.

Fixed in 8a926f3.

nisdas · 2024-06-17T09:28:09Z

beacon-chain/p2p/rpc_topic_mappings.go

@@ -116,7 +116,7 @@ var RPCTopicMappings = map[string]interface{}{
 	// DataColumnSidecarsByRange v1 Message
 	RPCDataColumnSidecarsByRangeTopicV1: new(pb.DataColumnSidecarsByRangeRequest),
 	// DataColumnSidecarsByRoot v1 Message
-	RPCDataColumnSidecarsByRootTopicV1: new(p2ptypes.BlobSidecarsByRootReq),
+	RPCDataColumnSidecarsByRootTopicV1: new(p2ptypes.DataColumnSidecarsByRootReq),


why a new type for this ? Underneath it is the same object type

Because now we don't use any more blob in place of dataColumn.
A solution could be:

Remove the newsly created type

Rename the old typ BlobSidecarsByRootReq into sidecarsByRootReq

What do you think?

ah ok, I think its fine to leave it as you have done it. Our reusing of the blob types was more for code velocity rather than any correctness reasons previously

nisdas · 2024-06-17T09:29:34Z

beacon-chain/sync/service.go

@@ -254,7 +254,7 @@ func (s *Service) Start() {

 	// Run data column sampling
 	if features.Get().EnablePeerDAS {
-		go s.dataColumnSampling(s.ctx)
+		go s.DataColumnSamplingLoop(s.ctx)


maybe call it Routine instead of loop to be in more inline with how we refer to background services. Another word such as Service works too

Fixed in 37a4bba.

nisdas · 2024-06-17T10:06:43Z

beacon-chain/sync/data_columns_sampling.go

-		"missingColumns":   missingColumnsList,
-	}).Warning("Failed to sample some requested columns")
+	// Ramdomize all columns.
+	columns := randomSlice(columnsCount)


we want to only sample columns that we do not custody, correct ? this function would do so for all our columns

That's actually not 100% clear to me.

If we do so, nodes custodying all the columns won't sample any columns.
Node custodying at least 50% of the columns will reconstruct and save the 50% missing.
==> Node custodying 50% of the columns will actually custody 100% of the columns. Should they sample columns as well or not?

It should definitely be distinct, there is 0 benefit in sampling a column you already know is available via custody. If you sample it again it doesn't push anything forward in whether we know a particular block is available.

If we do so, nodes custodying all the columns won't sample any columns.
Node custodying at least 50% of the columns will reconstruct and save the 50% missing.

If you see all the columns from gossip, then there isn't any benefit to sampling them, no ? The benefit I see is if you do not actually see this via gossip.

New design:

If we custody >= 64 columns ==> No need to sample. Details are:

If we receive these columns, then we are able to reconstruct everything ==> No need to sample

If some are missing, then the block won't be eligible for the fork choice ==> No need to samle

If we custody < 64 columns:

We start to sample with incrementalDAS min(16, 64-ourCustodyColumnsCount), as soon as we receive the block, columns we should NOT custody.

Fixed in 92a1bfe.

nisdas · 2024-06-21T12:45:54Z

beacon-chain/sync/data_columns_sampling.go

+		}
+	}
+
+	log.Warning("CCCCCC")


this is not needed

My bad. Fixed in 655f926.

nalepae requested a review from a team as a code owner June 13, 2024 16:18

nalepae requested review from potuz, terencechain and nisdas and removed request for a team June 13, 2024 16:18

nalepae added the peerDAS label Jun 13, 2024

nalepae force-pushed the lossy-das branch 3 times, most recently from 393f615 to d1c581f Compare June 14, 2024 23:23

nalepae changed the title ~~PeerDAS: Implement LossyDAS~~ PeerDAS: Implement IncrementalDAS Jun 14, 2024

nalepae force-pushed the lossy-das branch 3 times, most recently from 09c71fe to 33aba8f Compare June 16, 2024 21:10

nisdas reviewed Jun 17, 2024

View reviewed changes

nalepae force-pushed the lossy-das branch 2 times, most recently from b88b40d to 65f03ac Compare June 17, 2024 12:06

nalepae mentioned this pull request Jun 18, 2024

Fix columns sampling #14118

Merged

nalepae marked this pull request as draft June 19, 2024 15:37

nalepae force-pushed the lossy-das branch 2 times, most recently from bd61881 to 7a5f2bf Compare June 20, 2024 15:46

nalepae added 10 commits June 20, 2024 18:14

ConvertPeerIDToNodeID: Add tests.

36e1f3b

Remove extractNodeID and uses ConvertPeerIDToNodeID instead.

c82a4a6

Implement IncrementalDAS.

1a8d424

DataColumnSamplingLoop ==> DataColumnSamplingRoutine.

37a4bba

HypergeomCDF: Add test.

8a926f3

GetValidCustodyPeers: Optimize and add tests.

e966cbd

Remove blank identifiers.

9c06634

Implement CustodyCountFromRecord.

a393019

Implement TestP2P.CustodyCountFromRemotePeer.

8aa20d7

NewTestP2P: Add swarmt.Option parameters.

20b9072

nalepae force-pushed the lossy-das branch from 7a5f2bf to 36af807 Compare June 20, 2024 17:11

nalepae marked this pull request as ready for review June 20, 2024 17:11

nalepae force-pushed the lossy-das branch from 36af807 to 01394d6 Compare June 20, 2024 17:38

incrementalDAS: Rework and add tests.

92a1bfe

nalepae force-pushed the lossy-das branch from 01394d6 to 92a1bfe Compare June 20, 2024 23:33

nisdas reviewed Jun 21, 2024

View reviewed changes

Remove useless warning.

655f926

nisdas approved these changes Jun 21, 2024

View reviewed changes

nalepae merged commit 913e84d into peerDAS Jun 21, 2024
15 of 16 checks passed

nalepae deleted the lossy-das branch June 21, 2024 13:22

nalepae mentioned this pull request Jun 21, 2024

[PeerDAS] Parallelize data column sampling #14105

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PeerDAS: Implement IncrementalDAS #14109

PeerDAS: Implement IncrementalDAS #14109

nalepae commented Jun 13, 2024 •

edited

Loading

nisdas Jun 17, 2024

nalepae Jun 17, 2024

nalepae Jun 17, 2024

nisdas Jun 17, 2024

nalepae Jun 21, 2024

nisdas Jun 17, 2024

nalepae Jun 17, 2024

nisdas Jun 17, 2024 •

edited

Loading

nisdas Jun 17, 2024

nalepae Jun 17, 2024 •

edited

Loading

nisdas Jun 17, 2024

nalepae Jun 17, 2024

nisdas Jun 17, 2024

nalepae Jun 17, 2024

nalepae Jun 21, 2024

nisdas Jun 21, 2024

nalepae Jun 21, 2024

PeerDAS: Implement IncrementalDAS #14109

PeerDAS: Implement IncrementalDAS #14109

Conversation

nalepae commented Jun 13, 2024 • edited Loading

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

nisdas Jun 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nalepae Jun 17, 2024 • edited Loading

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

nalepae commented Jun 13, 2024 •

edited

Loading

nisdas Jun 17, 2024 •

edited

Loading

nalepae Jun 17, 2024 •

edited

Loading