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

[Execution] Add TransactionDeduper trait and implementation using (hash, signature) #8367

Merged
merged 11 commits into from
May 31, 2023

Conversation

bchocho
Copy link
Contributor

@bchocho bchocho commented May 25, 2023

Description

Adding TransactionDeduper trait and an implementation. The dedup is done within a block, just before transaction shuffle. It's guarded by an onchain config.

The purpose is to not send duplicate transactions to execution. While execution correctness is not affected by duplicates (and other work removed false error logs that fired due to them), it's possible that duplicate transactions could hurt throughput of parallel execution. By deduping ahead of time, we don't have to worry about that.

The implementation finds duplicates by matching (raw txn bcs hash, signature). Because calculating hash can be relatively expensive, it is only done when a shallow match is found of (account, seq_no), and it's done in parallel.

Overhead (as seen on forge):

  • When there are no duplicates, the dedup is negligible -- it takes ~2ms per second.
  • When there are ~100 duplicates per block, the dedup takes ~10ms per second.

Test Plan

Added unit tests.

@bchocho bchocho added the CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR label May 25, 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.

@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.

@bchocho
Copy link
Contributor Author

bchocho commented May 26, 2023

Results look good:

I'm turning this off to prepare for landing, and it will have to be enabled onchain.

@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.

@bchocho bchocho removed the CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR label May 31, 2023
@bchocho bchocho enabled auto-merge (squash) May 31, 2023 01:06
@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 compat success on testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> cd4f892cc42c383bbde0e51e62ca42af0065691c

Compatibility test results for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> cd4f892cc42c383bbde0e51e62ca42af0065691c (PR)
1. Check liveness of validators at old version: testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b
compatibility::simple-validator-upgrade::liveness-check : 10256 TPS, 3675 ms latency, 6300 ms p99 latency,no expired txns
2. Upgrading first Validator to new version: cd4f892cc42c383bbde0e51e62ca42af0065691c
compatibility::simple-validator-upgrade::single-validator-upgrade : 6039 TPS, 6856 ms latency, 10400 ms p99 latency,no expired txns
3. Upgrading rest of first batch to new version: cd4f892cc42c383bbde0e51e62ca42af0065691c
compatibility::simple-validator-upgrade::half-validator-upgrade : 5931 TPS, 6754 ms latency, 8800 ms p99 latency,no expired txns
4. upgrading second batch to new version: cd4f892cc42c383bbde0e51e62ca42af0065691c
compatibility::simple-validator-upgrade::rest-validator-upgrade : 1559 TPS, 32778 ms latency, 40200 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> cd4f892cc42c383bbde0e51e62ca42af0065691c passed
Test Ok

@github-actions
Copy link
Contributor

✅ Forge suite land_blocking success on cd4f892cc42c383bbde0e51e62ca42af0065691c

performance benchmark : 5677 TPS, 6969 ms latency, 27700 ms p99 latency,(!) expired 124 out of 2424360 txns
Test Ok

@github-actions
Copy link
Contributor

✅ Forge suite framework_upgrade success on aptos-node-v1.3.0_3fc3d42b6cfe27460004f9a0326451bcda840a60 ==> cd4f892cc42c383bbde0e51e62ca42af0065691c

Compatibility test results for aptos-node-v1.3.0_3fc3d42b6cfe27460004f9a0326451bcda840a60 ==> cd4f892cc42c383bbde0e51e62ca42af0065691c (PR)
Upgrade the nodes to version: cd4f892cc42c383bbde0e51e62ca42af0065691c
framework_upgrade::framework-upgrade::full-framework-upgrade : 6153 TPS, 6699 ms latency, 18700 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for aptos-node-v1.3.0_3fc3d42b6cfe27460004f9a0326451bcda840a60 ==> cd4f892cc42c383bbde0e51e62ca42af0065691c passed
Test Ok

@bchocho bchocho merged commit 44ac535 into main May 31, 2023
@bchocho bchocho deleted the brian/dedup-at-state-computer branch May 31, 2023 01:49
bchocho added a commit that referenced this pull request May 31, 2023
…sh, signature) (#8367)

### Description

Adding TransactionDeduper trait and an implementation. The dedup is done within a block, just before transaction shuffle. It's guarded by an onchain config.

The purpose is to not send duplicate transactions to execution. While execution correctness is not affected by duplicates (and other work removed false error logs that fired due to them), it's possible that duplicate transactions could hurt throughput of parallel execution. By deduping ahead of time, we don't have to worry about that.

The implementation finds duplicates by matching (raw txn bcs hash, signature). Because calculating hash can be relatively expensive, it is only done when a shallow match is found of (account, seq_no), and it's done in parallel.

Overhead (as seen on forge):
* When there are no duplicates, the dedup is negligible -- it takes ~2ms per second.
* When there are ~100 duplicates per block, the dedup takes ~10ms per second.

### Test Plan

Added unit tests.
bchocho added a commit that referenced this pull request Jun 1, 2023
…sh, signature) (#8367)

### Description

Adding TransactionDeduper trait and an implementation. The dedup is done within a block, just before transaction shuffle. It's guarded by an onchain config.

The purpose is to not send duplicate transactions to execution. While execution correctness is not affected by duplicates (and other work removed false error logs that fired due to them), it's possible that duplicate transactions could hurt throughput of parallel execution. By deduping ahead of time, we don't have to worry about that.

The implementation finds duplicates by matching (raw txn bcs hash, signature). Because calculating hash can be relatively expensive, it is only done when a shallow match is found of (account, seq_no), and it's done in parallel.

Overhead (as seen on forge):
* When there are no duplicates, the dedup is negligible -- it takes ~2ms per second.
* When there are ~100 duplicates per block, the dedup takes ~10ms per second.

### Test Plan

Added unit tests.
bchocho added a commit that referenced this pull request Jun 1, 2023
…sh, signature) (#8367) (#8458)

Cherry-pick #8367. Expected to be a noop until onchain config is changed.

### Description

Adding TransactionDeduper trait and an implementation. The dedup is done within a block, just before transaction shuffle. It's guarded by an onchain config.

The purpose is to not send duplicate transactions to execution. While execution correctness is not affected by duplicates (and other work removed false error logs that fired due to them), it's possible that duplicate transactions could hurt throughput of parallel execution. By deduping ahead of time, we don't have to worry about that.

The implementation finds duplicates by matching (raw txn bcs hash, signature). Because calculating hash can be relatively expensive, it is only done when a shallow match is found of (account, seq_no), and it's done in parallel.

Overhead (as seen on forge):
* When there are no duplicates, the dedup is negligible -- it takes ~2ms per second.
* When there are ~100 duplicates per block, the dedup takes ~10ms per second.

### Test Plan

Added unit tests.
gedigi pushed a commit that referenced this pull request Jun 6, 2023
…sh, signature) (#8367)

### Description

Adding TransactionDeduper trait and an implementation. The dedup is done within a block, just before transaction shuffle. It's guarded by an onchain config.

The purpose is to not send duplicate transactions to execution. While execution correctness is not affected by duplicates (and other work removed false error logs that fired due to them), it's possible that duplicate transactions could hurt throughput of parallel execution. By deduping ahead of time, we don't have to worry about that.

The implementation finds duplicates by matching (raw txn bcs hash, signature). Because calculating hash can be relatively expensive, it is only done when a shallow match is found of (account, seq_no), and it's done in parallel.

Overhead (as seen on forge):
* When there are no duplicates, the dedup is negligible -- it takes ~2ms per second.
* When there are ~100 duplicates per block, the dedup takes ~10ms per second.

### Test Plan

Added unit tests.
banool pushed a commit that referenced this pull request Jul 7, 2023
…sh, signature) (#8367)

### Description

Adding TransactionDeduper trait and an implementation. The dedup is done within a block, just before transaction shuffle. It's guarded by an onchain config.

The purpose is to not send duplicate transactions to execution. While execution correctness is not affected by duplicates (and other work removed false error logs that fired due to them), it's possible that duplicate transactions could hurt throughput of parallel execution. By deduping ahead of time, we don't have to worry about that.

The implementation finds duplicates by matching (raw txn bcs hash, signature). Because calculating hash can be relatively expensive, it is only done when a shallow match is found of (account, seq_no), and it's done in parallel.

Overhead (as seen on forge):
* When there are no duplicates, the dedup is negligible -- it takes ~2ms per second.
* When there are ~100 duplicates per block, the dedup takes ~10ms per second.

### Test Plan

Added unit tests.
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.

4 participants