sync2: ATX integration #6448
sync2: ATX integration #6448
Conversation
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.
ivan4th
requested review from
dshulyak,
fasmat,
poszu,
acud and
jellonek
as code owners
| oldAtxSyncCfg := sync2.DefaultConfig() | ||
| oldAtxSyncCfg.MultiPeerReconcilerConfig.SyncInterval = time.Hour | ||
| oldAtxSyncCfg.MaxDepth = 16 | ||
| newAtxSyncCfg := sync2.DefaultConfig() | ||
| newAtxSyncCfg.MaxDepth = 21 | ||
| newAtxSyncCfg.MultiPeerReconcilerConfig.SyncInterval = 5 * time.Minute |
There was a problem hiding this comment.
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 🙂
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
old vs new in this context means old epochs vs the current epoch
There was a problem hiding this comment.
Ah so it is actually PrevEpochSyncConfig and CurrentEpochSyncConfig? 🙂
There was a problem hiding this comment.
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
v2field.
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 🙂
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Renamed to v2 part to reconc-sync (ReconcSync).
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## sync2/fix-multipeer #6448 +/- ##
=====================================================
Coverage ? 79.8%
=====================================================
Files ? 353
Lines ? 46805
Branches ? 0
=====================================================
Hits ? 37366
Misses ? 7327
Partials ? 2112 ☔ View full report in Codecov by Sentry. |
It turned out that sync interactions are happening rather quickly, and thus it is not really practical to try and begin handling arriving keys (e.g. ATX IDs) during sync itself. Moreover, on-the-fly key handling was actually only used to register peer IDs for each ATX ID to fetch the actual blob from, and that can be done in the handler's Commit() method just as well.
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