Skip to content
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

Set network coordinates from BeaconChain #927

Merged
merged 34 commits into from
Apr 11, 2023
Merged

Conversation

samuelmanzanera
Copy link
Member

Description

This PR implement the full integration of the network patch in the beacon by building the latency matrix from all the subsets to determine the global network patch.

Fixes #72

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

After running two nodes, the default network patch (being the geo patch) should change based on the latency between the nodes and be persisted into the memory table for the P2P view.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@samuelmanzanera samuelmanzanera force-pushed the compute_beacon_latency_matrix branch 3 times, most recently from 1ac19d8 to 3cfac2c Compare March 9, 2023 08:24
@samuelmanzanera samuelmanzanera changed the title Compute beacon latency matrix Set network coordinates from BeaconChain Mar 16, 2023
@samuelmanzanera samuelmanzanera force-pushed the compute_beacon_latency_matrix branch 2 times, most recently from 6efbe25 to 2caa99c Compare March 17, 2023 10:22
Copy link
Member

@Neylix Neylix left a comment

Choose a reason for hiding this comment

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

Complex to understand as there is a lot of math and sorted list with index ...

But I can see something that may goes wrong, when the subset enter in handle_summary function, it request the network stats of other nodes. Then when stats are retrieved the pop_slots function is called on SummaryCache which empty the ets table and remove the backup.
But when a node request network stats, the result is streamed from the ets table of SummaryCache but if the requested node quickly requested network patches and pop the slots, it will not be able to respond to the GetNetworkStats message

lib/archethic/beacon_chain.ex Outdated Show resolved Hide resolved
lib/archethic/beacon_chain.ex Outdated Show resolved Hide resolved
lib/archethic/beacon_chain/network_coordinates.ex Outdated Show resolved Hide resolved
lib/archethic/beacon_chain/summary.ex Show resolved Hide resolved
@samuelmanzanera
Copy link
Member Author

Complex to understand as there is a lot of math and sorted list with index ...

But I can see something that may goes wrong, when the subset enter in handle_summary function, it request the network stats of other nodes. Then when stats are retrieved the pop_slots function is called on SummaryCache which empty the ets table and remove the backup. But when a node request network stats, the result is streamed from the ets table of SummaryCache but if the requested node quickly requested network patches and pop the slots, it will not be able to respond to the GetNetworkStats message

It might be true, we might schedule the removal of the cache a bit later, maybe after the self-repair scheduling time.

@Neylix
Copy link
Member

Neylix commented Mar 24, 2023

I got this error after some time :
image

A patch has only 2 digits (log in network_distance function from P2P)

Copy link
Member

@Neylix Neylix left a comment

Choose a reason for hiding this comment

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

It looks like the problem of the pathc with only 2 digits is still there

lib/archethic/beacon_chain/network_coordinates.ex Outdated Show resolved Hide resolved
lib/archethic/beacon_chain/subset.ex Outdated Show resolved Hide resolved
lib/archethic/beacon_chain/subset/summary_cache.ex Outdated Show resolved Hide resolved
Copy link
Member

@Neylix Neylix left a comment

Choose a reason for hiding this comment

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

Working well now, I'm just thinking we should create an issue or maybe do it in this one, to add the network patch in the beaconChainSummary graphql API. (in the transform function)

lib/archethic/beacon_chain/subset.ex Show resolved Hide resolved
lib/archethic/beacon_chain/summary_aggregate.ex Outdated Show resolved Hide resolved
lib/archethic/beacon_chain/summary_aggregate.ex Outdated Show resolved Hide resolved
lib/archethic/beacon_chain/summary_aggregate.ex Outdated Show resolved Hide resolved
lib/archethic/p2p.ex Show resolved Hide resolved
@Neylix Neylix force-pushed the compute_beacon_latency_matrix branch from 27d59af to 7532120 Compare April 7, 2023 15:04
@Neylix Neylix merged commit 9d8b4a8 into develop Apr 11, 2023
@Neylix Neylix deleted the compute_beacon_latency_matrix branch April 11, 2023 13:22
@Neylix Neylix mentioned this pull request Apr 17, 2023
@samuelmanzanera samuelmanzanera added the feature New feature request label May 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fetch other beacon summary to build network coordinates map
3 participants