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

[api] Change block APIs to by default return all transactions #13365

Merged
merged 1 commit into from
May 24, 2024
Merged

[api] Change block APIs to by default return all transactions #13365

merged 1 commit into from
May 24, 2024

Conversation

gregnazario
Copy link
Contributor

Description

The block size has a limit, and therefore, the limit has a limit, where the transactions API would likely be more variable. Therefore, it should be fine to make the block API return the entire block.

Type of Change

  • New feature
  • Bug fix
  • Breaking change
  • Performance improvement
  • Refactoring
  • Dependency update
  • Documentation update
  • Tests

Which Components or Systems Does This Change Impact?

  • Validator Node
  • Full Node (API, Indexer, etc.)
  • Move/Aptos Virtual Machine
  • Aptos Framework
  • Aptos CLI/SDK
  • Developer Infrastructure
  • Other (specify)

How Has This Been Tested?

Changing default, it has not been tested yet.

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 21, 2024 •
edited
Loading

⏱️ 38m total CI duration on this PR
Job Cumulative Duration Recent Runs
rust-targeted-unit-tests 16m 🟩 
rust-lints 10m 🟩 
run-tests-main-branch 4m 🟩 
rust-move-tests 4m 🟩 
general-lints 2m 🟩 
check-dynamic-deps 1m 🟩 
semgrep/ci 24s 🟩 
file_change_determinator 16s 🟩 
file_change_determinator 15s 🟩 
permission-check 4s 🟩 
permission-check 3s 🟩 
permission-check 3s 🟩 
permission-check 3s 🟩 

🚨 1 job on the last run was significantly faster/slower than expected

Job Duration vs 7d avg Delta
rust-lints 10m 6m +54%

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

@gregnazario gregnazario requested a review from davidiw May 21, 2024 02:20
davidiw
davidiw reviewed May 21, 2024
View reviewed changes
Copy link
Contributor

@davidiw davidiw left a comment

Choose a reason for hiding this comment

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

well the caveat here being that blocksizes can be up to 10K or more txns, so in theory that could be well over a mega... thoughts?

@gregnazario
Copy link
Contributor Author

well the caveat here being that blocksizes can be up to 10K or more txns, so in theory that could be well over a mega... thoughts?

The limit right now is 1900 txns. It could be over a mega, but given that there's already a function that waits a second, I'm not super concerned for now. Choosing UX for this for now.

banool
banool approved these changes May 23, 2024
View reviewed changes
config/src/config/api_config.rs
@@ -53,6 +53,8 @@ pub struct ApiConfig {
pub max_submit_transaction_batch_size: usize,
/// Maximum page size for transaction paginated APIs
pub max_transactions_page_size: u16,
/// Maximum page size for block transaction APIs
pub max_block_transactions_page_size: u16,
Copy link
Contributor

Choose a reason for hiding this comment

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

Call me crazy but is u16 too small? I can imagine a world where MAX_SENDING_BLOCK_TXNS is larger than u16 max. Maybe either make this u32 or cast MAX_SENDING_BLOCK_TXNS to u16 safely (ceil).

@gregnazario
[api] Change block APIs to by default return all transactions
2b01ffc 
The block size has a limit, and therefore, the limit has a limit,
where the transactions API would likely be more variable.  Therefore,
it should be fine to make the block API return the entire block.
@gregnazario gregnazario enabled auto-merge (squash) May 24, 2024 16:25
@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 3ffe0986b5fe4acb76544ae7ae85d73b91a6a411 ==> 2b01ffc71c0f0e9f1efbe9172fc3af480a20c93b

Compatibility test results for 3ffe0986b5fe4acb76544ae7ae85d73b91a6a411 ==> 2b01ffc71c0f0e9f1efbe9172fc3af480a20c93b (PR)
1. Check liveness of validators at old version: 3ffe0986b5fe4acb76544ae7ae85d73b91a6a411
compatibility::simple-validator-upgrade::liveness-check : committed: 5738.59114426694 txn/s, latency: 5746.877244643747 ms, (p50: 5000 ms, p90: 9000 ms, p99: 9700 ms), latency samples: 200700
2. Upgrading first Validator to new version: 2b01ffc71c0f0e9f1efbe9172fc3af480a20c93b
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 3346.4814698458754 txn/s, latency: 9310.932406740601 ms, (p50: 9400 ms, p90: 13900 ms, p99: 14200 ms), latency samples: 138860
3. Upgrading rest of first batch to new version: 2b01ffc71c0f0e9f1efbe9172fc3af480a20c93b
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 1199.44766752595 txn/s, latency: 21562.45865279299 ms, (p50: 25500 ms, p90: 32400 ms, p99: 33200 ms), latency samples: 73040
4. upgrading second batch to new version: 2b01ffc71c0f0e9f1efbe9172fc3af480a20c93b
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 6356.409529789908 txn/s, latency: 5124.447884320082 ms, (p50: 4800 ms, p90: 8400 ms, p99: 9600 ms), latency samples: 234440
5. check swarm health
Compatibility test for 3ffe0986b5fe4acb76544ae7ae85d73b91a6a411 ==> 2b01ffc71c0f0e9f1efbe9172fc3af480a20c93b passed
Test Ok

@github-actions GitHub Actions
Copy link
Contributor

✅ Forge suite realistic_env_max_load success on 2b01ffc71c0f0e9f1efbe9172fc3af480a20c93b

two traffics test: inner traffic : committed: 7806.035860376967 txn/s, latency: 5018.983644449975 ms, (p50: 4800 ms, p90: 6600 ms, p99: 9900 ms), latency samples: 3374940
two traffics test : committed: 100.03194149929085 txn/s, latency: 1919.6758426966292 ms, (p50: 1800 ms, p90: 2100 ms, p99: 5600 ms), latency samples: 1780
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.213, avg: 0.204", "QsPosToProposal: max: 0.339, avg: 0.266", "ConsensusProposalToOrdered: max: 0.439, avg: 0.421", "ConsensusOrderedToCommit: max: 0.380, avg: 0.367", "ConsensusProposalToCommit: max: 0.816, avg: 0.787"]
Max round gap was 1 [limit 4] at version 1531585. Max no progress secs was 4.506803 [limit 15] at version 1531585.
Test Ok

@github-actions GitHub Actions
Copy link
Contributor

✅ Forge suite framework_upgrade success on 3ffe0986b5fe4acb76544ae7ae85d73b91a6a411 ==> 2b01ffc71c0f0e9f1efbe9172fc3af480a20c93b

Compatibility test results for 3ffe0986b5fe4acb76544ae7ae85d73b91a6a411 ==> 2b01ffc71c0f0e9f1efbe9172fc3af480a20c93b (PR)
Upgrade the nodes to version: 2b01ffc71c0f0e9f1efbe9172fc3af480a20c93b
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1238.3050513011242 txn/s, submitted: 1241.186376818739 txn/s, failed submission: 2.88132551761493 txn/s, expired: 2.88132551761493 txn/s, latency: 2524.1445140504743 ms, (p50: 1800 ms, p90: 4800 ms, p99: 9300 ms), latency samples: 111740
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1122.455261308209 txn/s, submitted: 1124.2800209282993 txn/s, failed submission: 1.8247596200905651 txn/s, expired: 1.8247596200905651 txn/s, latency: 2783.0221499695185 ms, (p50: 2100 ms, p90: 5100 ms, p99: 9600 ms), latency samples: 98420
5. check swarm health
Compatibility test for 3ffe0986b5fe4acb76544ae7ae85d73b91a6a411 ==> 2b01ffc71c0f0e9f1efbe9172fc3af480a20c93b passed
Upgrade the remaining nodes to version: 2b01ffc71c0f0e9f1efbe9172fc3af480a20c93b
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1174.3808351723226 txn/s, submitted: 1176.3860386472895 txn/s, failed submission: 2.005203474966971 txn/s, expired: 2.005203474966971 txn/s, latency: 2670.55291216088 ms, (p50: 2100 ms, p90: 4500 ms, p99: 8100 ms), latency samples: 105420
Test Ok

@gregnazario gregnazario merged commit f869344 into aptos-labs:main May 24, 2024
84 of 98 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidiw davidiw davidiw approved these changes

@banool banool banool approved these changes

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

@0xmaayan 0xmaayan Awaiting requested review from 0xmaayan 0xmaayan is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@gregnazario @davidiw @banool