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

Reduce time taken to collect VerifiedTransactions #1563

Merged
merged 5 commits into from
Oct 7, 2024
Merged

Conversation

shawn-zil
Copy link
Contributor

@shawn-zil shawn-zil commented Oct 4, 2024

The original execute_block() collected verified transactions transactions from the Proposal block by re-verifying each transaction in the Proposal block. This triggers the recover_signer() library call for each transaction in the Proposal. It then fills up any missing ones from the mempool by cloning.

This PR flips this behaviour around to collect verified transactions first from the mempool, which already contains pre-verified transactions. It then fills up any missing ones from the Proposal block by only re-verifying those missing from the mempool.

This speeds things up by several orders of magnitude, when the transactions are already pre-verified and present in the mempool, with the worst case scenario being similar in timing to the previous behaviour e.g.

before

zilliqanode1      | 2024-10-04T01:31:05.720473Z  INFO zilliqa::consensus: 2635: 95 ELAPSED_VER 323.269721ms
zilliqanode0      | 2024-10-04T01:31:05.730972Z  INFO zilliqa::consensus: 2635: 95 ELAPSED_VER 323.800224ms
zilliqanode3      | 2024-10-04T01:31:05.756535Z  INFO zilliqa::consensus: 2635: 95 ELAPSED_VER 323.695312ms
zilliqanode2      | 2024-10-04T01:31:05.763893Z  INFO zilliqa::consensus: 2635: 95 ELAPSED_VER 325.994903ms

after

zilliqanode1      | 2024-10-04T02:17:40.488820Z  INFO zilliqa::consensus: 2653: 96 ELAPSED_INS 192.081µs
zilliqanode2      | 2024-10-04T02:17:40.510038Z  INFO zilliqa::consensus: 2653: 96 ELAPSED_INS 197.46µs
zilliqanode3      | 2024-10-04T02:17:40.519616Z  INFO zilliqa::consensus: 2653: 96 ELAPSED_INS 208.291µs
zilliqanode0      | 2024-10-04T02:17:40.790467Z  INFO zilliqa::consensus: 2653: 96 ELAPSED_INS 327.814331ms

The time taken to collect about ~95 VerifiedTransactions is about 325ms across all nodes (before) and is about 200us (if already present in the pool) to ~325ms (if not present in the pool).

As this is a small change that makes a measurable impact, with the worst case similar as before, I thought it would be good to merge this one first.

zilliqa/src/consensus.rs Outdated Show resolved Hide resolved
zilliqa/src/consensus.rs Outdated Show resolved Hide resolved
zilliqa/src/consensus.rs Outdated Show resolved Hide resolved
@shawn-zil shawn-zil added this pull request to the merge queue Oct 7, 2024
Merged via the queue into main with commit 7a24a13 Oct 7, 2024
5 checks passed
@shawn-zil shawn-zil deleted the 1322-execute-block branch October 7, 2024 11:14
@shawn-zil
Copy link
Contributor Author

shawn-zil commented Nov 26, 2024

Also impacts #1322 #1874

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.

2 participants