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

docs: comments about consensus concurrency control #45

Merged
merged 2 commits into from
Apr 24, 2024

Conversation

aakoshh
Copy link
Contributor

@aakoshh aakoshh commented Apr 15, 2024

Closes #44

Added comments to all examples, the main README and the Buffer to mention that Tower layering should be used to control the concurrency of the Consensus service if the implementation of request handling in call of the actual application service is asynchronous.

An example of an asynchronous implementation is here, which exhibited transaction reordering.

Implementation notes

I found that I needed to do the following combo to make it work with ABCI++ in v0.37:

  • Pass ServiceBuilder::new().buffer(block_max_msgs + 3).concurrency_limit(1).service(consensus) to the ServiceBuilder, where the buffer serves to allow all messages in a block lifecycle to be queued up without blocking Connection::run
  • Enforce the block_max_msgs setting in prepare_proposal and process_proposal so we don't get blocks which would violate that assumption.
  • Notably without .buffer, we got a deadlock because the responses weren't processed by Connection, and I suppose this applies to undersized buffers as well, which is why I added the proposal handling too.

This shouldn't be an issue with v0.38 which uses finalize_block.

I thought about adding it as an example to kvstore_037 but it would needlessly complicate it, as it's immune by the virtue of being synchronous.

@erwanor erwanor requested review from erwanor and cratelyn April 24, 2024 17:32
Copy link

@cratelyn cratelyn left a comment

Choose a reason for hiding this comment

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

thank you much for writing this @aakoshh. this looks great to me ✔️

@cratelyn cratelyn changed the title DOCS: Comments about consensus concurrency control docs: comments about consensus concurrency control Apr 24, 2024
@cratelyn cratelyn merged commit d18d3e6 into penumbra-zone:main Apr 24, 2024
4 checks passed
@cratelyn cratelyn removed the request for review from erwanor April 24, 2024 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Out-of-order transaction execution
2 participants