-
Notifications
You must be signed in to change notification settings - Fork 622
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
[Epoch Sync] Add a Nayduck test for Epoch Sync. #12237
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #12237 +/- ##
==========================================
- Coverage 71.72% 71.62% -0.11%
==========================================
Files 833 837 +4
Lines 166700 167141 +441
Branches 166700 167141 +441
==========================================
+ Hits 119573 119718 +145
- Misses 41902 42198 +296
Partials 5225 5225
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
chain/chain/src/store_validator.rs
Outdated
let epoch_sync_boundary = if let Ok(Some(epoch_sync_proof)) = | ||
store.get_ser::<EpochSyncProof>(DBCol::EpochSyncProof, &[]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sanity check: Is it ok to ignore errors here?
mini nit: not sure if any better but you can consider .ok().flatten().map() and splitting into multiple lines to improve readability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should .expect instead, indeed.
@@ -436,6 +436,8 @@ impl EpochSync { | |||
.choose(&mut rand::thread_rng()) | |||
.ok_or_else(|| Error::Other("No peers to request epoch sync from".to_string()))?; | |||
|
|||
tracing::info!(peer_id=?peer.peer_info.id, "Bootstrapping node via epoch sync"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice
pytest/tests/sanity/epoch_sync.py
Outdated
for height, block_hash in utils.poll_blocks(node0, | ||
timeout=SYNC_FROM_BLOCK * 2, | ||
poll_interval=0.1): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mini nit: If you add a trailing comma after the last arg the formatting will be a bit nicer - imo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eh, it leaves a dangling ): on its own line. I feel the existing way is better.
@@ -0,0 +1,68 @@ | |||
#!/usr/bin/env python3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine but you can consider using unitttest like so:
https://github.com/near/nearcore/blob/master/pytest/README.md#creating-new-tests
pytest/tests/sanity/epoch_sync.py
Outdated
poll_interval=0.1): | ||
if height >= SYNC_FROM_BLOCK: | ||
break | ||
ctx.send_moar_txs(block_hash, 1, False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes me chuckle every time
pytest/tests/sanity/epoch_sync.py
Outdated
{x: node_config for x in range(3)}) | ||
|
||
node0 = spin_up_node(config, near_root, node_dirs[0], 0) | ||
node1 = spin_up_node(config, near_root, node_dirs[1], 1, boot_node=node0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought node1 wouldn't be started until after the delay.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahhh, so I had to use two nodes because the TxContext class needs that and I have no idea why 🤷♂️
pytest/tests/sanity/epoch_sync.py
Outdated
break | ||
ctx.send_moar_txs(block_hash, 1, False) | ||
|
||
node1 = spin_up_node(config, near_root, node_dirs[2], 2, boot_node=node0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I think it's the third node that does the state sync? Can you rename to node2 and update the comment at the beginning of the file please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, thanks for catching that.
pytest/tests/sanity/epoch_sync.py
Outdated
utils.wait_for_blocks(node1, target=CATCHUP_BLOCK, timeout=(CATCHUP_BLOCK - SYNC_FROM_BLOCK) * 2) | ||
|
||
# Verify that we did bootstrap using epoch sync (rather than header sync). | ||
tracker.check('Bootstrapped from epoch sync') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nifty
You should also add your new test to nayduck by adding it to one of the config files (sanity.txt or similar). |
Oh right, of course! |
Most of the complexity here is changes to the StoreValidator: