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.

Sign up for GitHub

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

Jump to bottom

reverse state proof siblings #13284

Merged
merged 1 commit into from
May 16, 2024
Merged

reverse state proof siblings #13284

merged 1 commit into from
May 16, 2024
+103 −87

Conversation

msmouse
Copy link
Contributor

@msmouse msmouse commented May 15, 2024 •
edited

Description

Our implementation outputs the new order natually without the need for
reversing.

Lucky that we didn't expose the state proof to outside of the node

The range proof unfortunately is already exposed, so not gonna be
changed.

Type of Change

  • Refactoring

Which Components or Systems Does This Change Impact?

  • Validator Node

How Has This Been Tested?

unit tests updated.
checked code as far as I can, and I don't think the proof is exposed anywhere, including db backups.

Key Areas to Review

Checklist

  • I have read and followed the CONTRIBUTING doc
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I identified and added all stakeholders and component owners affected by this change as reviewers
  • I tested both happy and unhappy path of the functionality
  • I have made corresponding changes to the documentation

@trunk-io Trunk.io
Copy link

trunk-io bot commented May 15, 2024 •
edited

⏱️ 13h 36m total CI duration on this PR
Job Cumulative Duration Recent Runs
rust-targeted-unit-tests 2h 43m ⬜ 🟥 🟥 🟩 🟩 (+4 more)
execution-performance / single-node-performance 1h 58m 🟩 🟥 🟩 🟩 
rust-move-tests 1h 45m 🟩 🟩 🟩 🟩 🟩 (+4 more)
forge-framework-upgrade-test / forge 1h 8m 🟩 
rust-lints 52m 🟩 🟩 🟥 🟩 🟩 (+4 more)
windows-build 45m 🟩 
rust-images / rust-all 41m 🟩 🟥 🟩 🟩 
single-node-performance 40m 🟥 🟥 
run-tests-main-branch 37m 🟩 🟩 🟩 🟩 🟩 (+4 more)
rust-smoke-tests 33m 🟩 
general-lints 16m 🟩 🟩 🟩 🟩 🟩 (+4 more)
test-target-determinator 15m 🟩 🟩 🟩 🟩 
execution-performance / test-target-determinator 15m 🟩 🟩 🟩 🟩 
forge-e2e-test / forge 14m 🟩 
forge-compat-test / forge 13m 🟩 
check-dynamic-deps 12m 🟩 🟩 🟩 🟩 🟩 (+4 more)
test-target-determinator 7m 🟩 🟩 
cli-e2e-tests / run-cli-tests 6m 🟩 
check 4m 🟩 
rust-build-cached-packages 4m 🟩 
semgrep/ci 4m 🟩 🟩 🟩 🟩 🟩 (+4 more)
file_change_determinator 2m 🟩 🟩 🟩 🟩 🟩 (+4 more)
file_change_determinator 1m 🟩 🟩 🟩 🟩 🟩 (+4 more)
node-api-compatibility-tests / node-api-compatibility-tests 50s 🟩 
file_change_determinator 40s 🟩 🟩 🟩 🟩 
permission-check 24s 🟩 🟩 🟩 🟩 🟩 (+4 more)
permission-check 24s 🟩 🟩 🟩 🟩 🟩 (+4 more)
permission-check 24s 🟩 🟩 🟩 🟩 🟩 (+4 more)
permission-check 22s 🟩 🟩 🟩 🟩 🟩 (+4 more)
permission-check 16s 🟩 🟩 🟩 🟩 
determine-docker-build-metadata 8s 🟩 🟩 🟩 🟩 

🚨 2 jobs on the last run were significantly faster/slower than expected

Job Duration vs 7d avg Delta
execution-performance / single-node-performance 39m 24m +61%
rust-build-cached-packages 4m 5m -22%

settings ⋅ feedback ⋅ docs ⋅ learn more about trunk.io

Base automatically changed from 0509-alden-slice-node to 0514-alden-revert-expected-tps May 15, 2024 04:36
This was referenced May 15, 2024
Merged
Merged
@msmouse
Copy link
Contributor Author

msmouse commented May 15, 2024 •
edited

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @msmouse and the rest of your teammates on Graphite Graphite

@msmouse msmouse mentioned this pull request May 15, 2024
Merged
8 tasks
Base automatically changed from 0514-alden-revert-expected-tps to main May 15, 2024 07:46
@msmouse msmouse force-pushed the 0514-alden-sibling-rev branch 3 times, most recently from 30683ca to fb462d7 Compare May 15, 2024 20:40
@msmouse msmouse added the CICD:run-execution-performance-full-test Run execution performance test (full version) label May 15, 2024
@msmouse
reverse state proof siblings
83c00f6 
Our implementation outputs the new order natually without the need for
reversing.

Lucky that we didn't expose the state proof to outside of the node

The range proof unfortunately is already exposed, so not gonna be
changed.
@msmouse msmouse marked this pull request as ready for review May 15, 2024 22:46
@msmouse msmouse requested review from a team and zekun000 May 15, 2024 22:46
@msmouse msmouse enabled auto-merge (squash) May 16, 2024 16:52
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions
Copy link
Contributor

✅ Forge suite compat success on 01b24e7e3548382dd25440b39a0438a993387f12 ==> 83c00f61e598b65d2a4ecdf83a2853bb83df34c8

Compatibility test results for 01b24e7e3548382dd25440b39a0438a993387f12 ==> 83c00f61e598b65d2a4ecdf83a2853bb83df34c8 (PR)
1. Check liveness of validators at old version: 01b24e7e3548382dd25440b39a0438a993387f12
compatibility::simple-validator-upgrade::liveness-check : committed: 5714.7992214798305 txn/s, latency: 5135.882757657945 ms, (p50: 4800 ms, p90: 8400 ms, p99: 10800 ms), latency samples: 250720
2. Upgrading first Validator to new version: 83c00f61e598b65d2a4ecdf83a2853bb83df34c8
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 1658.663715934282 txn/s, latency: 17303.804584365847 ms, (p50: 19200 ms, p90: 23200 ms, p99: 23500 ms), latency samples: 88780
3. Upgrading rest of first batch to new version: 83c00f61e598b65d2a4ecdf83a2853bb83df34c8
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 1576.407361595898 txn/s, latency: 18182.95638702461 ms, (p50: 19000 ms, p90: 26500 ms, p99: 26800 ms), latency samples: 89400
4. upgrading second batch to new version: 83c00f61e598b65d2a4ecdf83a2853bb83df34c8
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 3575.3120549893993 txn/s, latency: 8727.260407366071 ms, (p50: 9700 ms, p90: 12600 ms, p99: 12900 ms), latency samples: 143360
5. check swarm health
Compatibility test for 01b24e7e3548382dd25440b39a0438a993387f12 ==> 83c00f61e598b65d2a4ecdf83a2853bb83df34c8 passed
Test Ok

@github-actions GitHub Actions
Copy link
Contributor

✅ Forge suite realistic_env_max_load success on 83c00f61e598b65d2a4ecdf83a2853bb83df34c8

two traffics test: inner traffic : committed: 8190.895131960914 txn/s, latency: 4792.291682514886 ms, (p50: 4500 ms, p90: 5700 ms, p99: 10200 ms), latency samples: 3533520
two traffics test : committed: 99.9085732815396 txn/s, latency: 1830.9577777777777 ms, (p50: 1800 ms, p90: 2000 ms, p99: 2200 ms), latency samples: 1800
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.205, avg: 0.201", "QsPosToProposal: max: 0.227, avg: 0.214", "ConsensusProposalToOrdered: max: 0.462, avg: 0.440", "ConsensusOrderedToCommit: max: 0.360, avg: 0.345", "ConsensusProposalToCommit: max: 0.799, avg: 0.785"]
Max round gap was 1 [limit 4] at version 1834912. Max no progress secs was 4.9253 [limit 15] at version 1834912.
Test Ok

@github-actions GitHub Actions
Copy link
Contributor

✅ Forge suite framework_upgrade success on 01b24e7e3548382dd25440b39a0438a993387f12 ==> 83c00f61e598b65d2a4ecdf83a2853bb83df34c8

Compatibility test results for 01b24e7e3548382dd25440b39a0438a993387f12 ==> 83c00f61e598b65d2a4ecdf83a2853bb83df34c8 (PR)
Upgrade the nodes to version: 83c00f61e598b65d2a4ecdf83a2853bb83df34c8
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1249.553248126269 txn/s, submitted: 1253.792046861209 txn/s, failed submission: 4.238798734940031 txn/s, expired: 4.238798734940031 txn/s, latency: 2382.1116229244776 ms, (p50: 2100 ms, p90: 3900 ms, p99: 5400 ms), latency samples: 112020
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1146.495528213663 txn/s, submitted: 1149.1926359059908 txn/s, failed submission: 2.69710769232777 txn/s, expired: 2.69710769232777 txn/s, latency: 2629.3138796314447 ms, (p50: 2200 ms, p90: 4500 ms, p99: 6400 ms), latency samples: 102020
5. check swarm health
Compatibility test for 01b24e7e3548382dd25440b39a0438a993387f12 ==> 83c00f61e598b65d2a4ecdf83a2853bb83df34c8 passed
Upgrade the remaining nodes to version: 83c00f61e598b65d2a4ecdf83a2853bb83df34c8
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1223.3298555492797 txn/s, submitted: 1227.6768290401037 txn/s, failed submission: 4.346973490824072 txn/s, expired: 4.346973490824072 txn/s, latency: 2493.387871703759 ms, (p50: 2300 ms, p90: 4200 ms, p99: 5800 ms), latency samples: 106940
Test Ok

@msmouse msmouse merged commit 8141697 into main May 16, 2024
99 checks passed
@msmouse msmouse deleted the 0514-alden-sibling-rev branch May 16, 2024 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@areshand areshand areshand approved these changes

@zekun000 zekun000 zekun000 approved these changes

@lightmark lightmark Awaiting requested review from lightmark lightmark is a code owner

@grao1991 grao1991 Awaiting requested review from grao1991 grao1991 is a code owner

Assignees
No one assigned
Labels
CICD:run-execution-performance-full-test Run execution performance test (full version)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@msmouse @areshand @zekun000