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

feat: allow delaying transactions in ResponsePrepareProposal #717

Merged
merged 7 commits into from
Dec 18, 2023

Conversation

lklimek
Copy link
Collaborator

@lklimek lklimek commented Dec 14, 2023

Issue being fixed or feature implemented

ABCI App must check if the size of transactions does not exceed max limit.
If it does, it should reject the transaction. Unfortunately, only abci.TxRecord_REMOVED, action is available that removed the tx from mempool.

This should not be a problem in a system where there are many validators, as the tx should be added to next block by another proposer (which didn't remove it from mempool yet). However, for a relatively small validator set, this can lead to dropped transactions in some edge cases.

What was done?

Introduced abci.TxRecord_DELAYED, which should remove the transaction from proposal/block, while still leaving it in the mempool.

Refactored kvstore and e2e example apps to better support testing of transactions.

Refactored logs to only include hash of tx, not full tx

How Has This Been Tested?

Added e2e test that tests this feature.

Breaking Changes

None - only new action added in ResponsePrepareProposal, but the system should work just fine with apps that don't support this action (as it's in response).

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation

For repository code-owners and collaborators only

  • I have assigned this pull request to a milestone

shumkov
shumkov previously approved these changes Dec 15, 2023
Copy link
Member

@shumkov shumkov left a comment

Choose a reason for hiding this comment

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

Nice! 👍

Copy link

@markin-io markin-io left a comment

Choose a reason for hiding this comment

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

utACK

@lklimek lklimek merged commit 96de171 into v0.14-dev Dec 18, 2023
16 of 19 checks passed
@lklimek lklimek deleted the feat/requeue-rejected-txs branch December 18, 2023 16:18
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