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

[qs] another refactor #7045

Merged
merged 5 commits into from
Mar 14, 2023
Merged

[qs] another refactor #7045

merged 5 commits into from
Mar 14, 2023

Conversation

zekun000
Copy link
Contributor

@zekun000 zekun000 commented Mar 10, 2023

This commit unifies fragment and proof processing such that we don't differentiate whether the message is for local or remote.

@zekun000 zekun000 force-pushed the zekun/qs branch 3 times, most recently from 78d32e6 to b954345 Compare March 11, 2023 21:20
@zekun000 zekun000 added CICD:run-consensus-only-perf-test Builds consensus-only aptos-node image and uses it to run forge CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR labels Mar 11, 2023
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@zekun000 zekun000 force-pushed the zekun/qs branch 2 times, most recently from fa2206e to a08666c Compare March 12, 2023 03:57
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@@ -116,7 +123,9 @@ impl ProofCoordinator {
validator_verifier: &ValidatorVerifier,
) -> Result<Option<ProofOfStore>, SignedDigestError> {
if !self.digest_to_proof.contains_key(&signed_digest.digest()) {
if signed_digest.info().batch_author == self.peer_id {
if signed_digest.info().batch_author == self.peer_id
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we missed a possible attack. I can make you store incremental signatures for all digests in the system because I can lie about the batch_author in the signed digest. I think the fix is to check signed_digest.info().batch_author against what we store. Probably require an API change to batch_reader.exists.

Copy link
Contributor

Choose a reason for hiding this comment

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

but in order to check we should be storing it, no? so I guess that's why the original algorithm initialized the proofs from self (assuming self is and should be faster) and then appended matching digest/signatures - and I guess that also explains the check of the own signature, but in this case it should have been an assert.

I think that's a bit nicer than trying to check the author - as long as I'm not mistaken and checking the author almost implies that checking own signature would also be okay.

Copy link
Contributor

@gelash gelash Mar 14, 2023

Choose a reason for hiding this comment

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

@igor-aptos since you found the self signature checks - what do you think?

self.digest_to_proof.remove(&signed_digest_info.digest);
batch_ids.push(signed_digest_info.batch_id);
}
if self
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you still call self.expire() in batch_generato.rs?

@zekun000 zekun000 enabled auto-merge (squash) March 14, 2023 05:01
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

✅ Forge suite land_blocking success on 6807fde14f2bd66c9a4f4487ab0fb79870db0ffa

performance benchmark with full nodes : 5810 TPS, 6823 ms latency, 13900 ms p99 latency,(!) expired 320 out of 2481360 txns
Test Ok

@github-actions
Copy link
Contributor

✅ Forge suite compat success on testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 6807fde14f2bd66c9a4f4487ab0fb79870db0ffa

Compatibility test results for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 6807fde14f2bd66c9a4f4487ab0fb79870db0ffa (PR)
1. Check liveness of validators at old version: testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b
compatibility::simple-validator-upgrade::liveness-check : 8106 TPS, 4707 ms latency, 6500 ms p99 latency,no expired txns
2. Upgrading first Validator to new version: 6807fde14f2bd66c9a4f4487ab0fb79870db0ffa
compatibility::simple-validator-upgrade::single-validator-upgrade : 4771 TPS, 8489 ms latency, 10800 ms p99 latency,no expired txns
3. Upgrading rest of first batch to new version: 6807fde14f2bd66c9a4f4487ab0fb79870db0ffa
compatibility::simple-validator-upgrade::half-validator-upgrade : 5195 TPS, 7448 ms latency, 10000 ms p99 latency,no expired txns
4. upgrading second batch to new version: 6807fde14f2bd66c9a4f4487ab0fb79870db0ffa
compatibility::simple-validator-upgrade::rest-validator-upgrade : 6997 TPS, 5498 ms latency, 9400 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 6807fde14f2bd66c9a4f4487ab0fb79870db0ffa passed
Test Ok

@zekun000 zekun000 merged commit 7250a5d into main Mar 14, 2023
@zekun000 zekun000 deleted the zekun/qs branch March 14, 2023 06:50
@github-actions
Copy link
Contributor

✅ Forge suite framework_upgrade success on cb4ba0a57c998c60cbab65af31a64875d2588ca5 ==> 6807fde14f2bd66c9a4f4487ab0fb79870db0ffa

Compatibility test results for cb4ba0a57c998c60cbab65af31a64875d2588ca5 ==> 6807fde14f2bd66c9a4f4487ab0fb79870db0ffa (PR)
Upgrade the nodes to version: 6807fde14f2bd66c9a4f4487ab0fb79870db0ffa
framework_upgrade::framework-upgrade::full-framework-upgrade : 6825 TPS, 5719 ms latency, 8500 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for cb4ba0a57c998c60cbab65af31a64875d2588ca5 ==> 6807fde14f2bd66c9a4f4487ab0fb79870db0ffa passed
Test Ok

.batch_reader
.exists(&signed_digest.digest())
.ok_or(SignedDigestError::WrongAuthor)?;
if batch_author != signed_digest.info().batch_author {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we merge with the if in line 114? => if batch_author != self.peer_id {}

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

Successfully merging this pull request may close these issues.

5 participants