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

sync2: ATX integration #6448

Open
wants to merge 3 commits into
base: sync2/fix-multipeer
Choose a base branch
from
Open

sync2: ATX integration #6448

wants to merge 3 commits into from

Conversation

ivan4th
Copy link
Contributor

@ivan4th ivan4th commented Nov 11, 2024


Motivation

We need more efficient ATX sync.

Description

This adds set reconciliation for ATXs.
There are per-epoch syncers, with lower FPTree depth (16 by default)
used for older epochs and greater FPTree depth (21 by default) used
for current epoch.
Both active syncv2 and passive (server-only) syncv2 are disabled by
default. It is possible to enable syncv2 in server-only or
full (active) mode.

Test Plan

Test on testnet and mainnet nodes

TODO

  • Explain motivation or link existing issue(s)
  • Test changes and document test plan
  • Update documentation as needed
  • Update changelog as needed

This adds set reconciliation for ATXs.
There are per-epoch syncers, with lower FPTree depth (16 by default)
used for older epochs and greater FPTree depth (21 by default) used
for current epoch.
Both active syncv2 and passive (server-only) syncv2 are disabled by
default. It is possible to enable syncv2 in server-only or
full (active) mode.
Comment on lines +82 to +87
oldAtxSyncCfg := sync2.DefaultConfig()
oldAtxSyncCfg.MultiPeerReconcilerConfig.SyncInterval = time.Hour
oldAtxSyncCfg.MaxDepth = 16
newAtxSyncCfg := sync2.DefaultConfig()
newAtxSyncCfg.MaxDepth = 21
newAtxSyncCfg.MultiPeerReconcilerConfig.SyncInterval = 5 * time.Minute
Copy link
Member

@fasmat fasmat Nov 11, 2024

Choose a reason for hiding this comment

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

I don't like how this config is structured, what is the meaning of OldAtxSyncCfg and NewAtxSyncCfg?
Is there maybe a better name for them?

Why are they part of "main" -> "Syncer" -> "V2"? Shouldn't they be part of the "main" -> "Syncer" config object?

Also if these are the defaults anyway, no need to overwrite them with the same values here again 🙂

Copy link
Member

Choose a reason for hiding this comment

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

EDIT: fixed spelling

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This might cause confusion as there's currently V1 sync for ATXs, too, which is also configured in the syncer config.
When V2 is used in server-only mode, both V1 and V2 syncs are used at the same time.

Copy link
Contributor Author

@ivan4th ivan4th Nov 11, 2024

Choose a reason for hiding this comment

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

The idea is to use reasonable defaults so that most users will not have to adjust sync settings.
Then at some time in the future we might want to restructure the config to remove all the v1 stuff and keep v2 sync only, moving it out of this v2 field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

old vs new in this context means old epochs vs the current epoch

Copy link
Member

Choose a reason for hiding this comment

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

Ah so it is actually PrevEpochSyncConfig and CurrentEpochSyncConfig? 🙂

Copy link
Member

Choose a reason for hiding this comment

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

The idea is to use reasonable defaults so that most users will not have to adjust sync settings. Then at some time in the future we might want to restructure the config to remove all the v1 stuff and keep v2 sync only, moving it out of this v2 field.

The reasonable defaults are already specified as part of the syncer.DefaultConfig, my point was that we don't need to overwrite this values again with the same values in config.MainnetConfig 🙂

Copy link
Member

Choose a reason for hiding this comment

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

This might cause confusion as there's currently V1 sync for ATXs, too, which is also configured in the syncer config. When V2 is used in server-only mode, both V1 and V2 syncs are used at the same time.

But this is a technical detail that I think should not be exposed via the config, or at least in a different way. Maybe instead of V2 we can call it something like "reconciliationSync" or similar. Especially since "V2" has negative connotations: https://en.wikipedia.org/wiki/V-2_rocket

Copy link
Contributor Author

@ivan4th ivan4th Nov 11, 2024

Choose a reason for hiding this comment

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

Actually there's a bit of confusion re current vs prev, and I was imprecise in my above comment.
When a new epoch starts, activations don't start to be published in that epoch immediately, and consequently, the more efficient (but more memory and CPU hungry per missing/extra set element) sync is to be applied to the new epoch some time later (EpochEndFraction in the existing sync config). That's why the "new" epoch is not necessarily the current one, and the "old" epoch is not necessarily the previous. We could probably come up with better naming but I'm unsure current / prev is the right choice here.

Otherwise agree, will fix

Copy link

codecov bot commented Nov 11, 2024

Codecov Report

Attention: Patch coverage is 76.08069% with 83 lines in your changes missing coverage. Please review.

Project coverage is 79.8%. Comparing base (042d0d7) to head (294bc6c).

Files with missing lines Patch % Lines
sync2/atxs.go 75.1% 40 Missing and 8 partials ⚠️
syncer/syncer.go 74.5% 21 Missing and 7 partials ⚠️
fetch/fetch.go 0.0% 4 Missing ⚠️
fetch/mesh_data.go 72.7% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@                  Coverage Diff                  @@
##           sync2/fix-multipeer   #6448     +/-   ##
=====================================================
- Coverage                 79.9%   79.8%   -0.1%     
=====================================================
  Files                      352     353      +1     
  Lines                    46209   46534    +325     
=====================================================
+ Hits                     36928   37172    +244     
- Misses                    7193    7254     +61     
- Partials                  2088    2108     +20     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants