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: limit transaction size #6154

Merged

Conversation

hansieodendaal
Copy link
Contributor

@hansieodendaal hansieodendaal commented Feb 18, 2024

Description

Added checks to ensure that transactions negotiated between parties, transactions to self and imported transactions do not exceed the RPC frame size limit when the transaction is broadcast to the base node. This can easily happen when lots of dust inputs are collected to be spent. If a too-large transaction is detected in any of the transaction service protocols it will be cancelled so that the user can try to create a new transaction with different parameters, for example, to reduce the recipient amount.

Edit:

  • Added a wallet sqlite memory type connection to speed up tests with large amounts of db activity
  • Used fake outputs without valid bulletproof range proofs in the spend_dust tests to speed it up even more as suggested by @SWvheerden

Motivation and Context

See #6108

How Has This Been Tested?

Added unit tests:

  • test_spend_dust_to_self_in_oversized_transaction
  • test_spend_dust_to_other_in_oversized_transaction
  • test_spend_dust_happy_path

What process can a PR reviewer use to test or verify this change?

Code walk-through
Review unit tests

Breaking Changes

  • None
  • Requires data directory on base node to be deleted
  • Requires hard fork
  • Other - Please specify

@hansieodendaal hansieodendaal requested a review from a team as a code owner February 18, 2024 05:46
@hansieodendaal hansieodendaal marked this pull request as draft February 18, 2024 05:46
Copy link

github-actions bot commented Feb 18, 2024

Test Results (CI)

    3 files    117 suites   36m 52s ⏱️
1 262 tests 1 262 ✅ 0 💤 0 ❌
3 778 runs  3 778 ✅ 0 💤 0 ❌

Results for commit 0b9197e.

♻️ This comment has been updated with latest results.

@ghpbot-tari-project ghpbot-tari-project added P-acks_required Process - Requires more ACKs or utACKs P-reviews_required Process - Requires a review from a lead maintainer to be merged labels Feb 18, 2024
Copy link

github-actions bot commented Feb 18, 2024

Test Results (Integration tests)

 2 files  + 2  11 suites  +11   26m 40s ⏱️ + 26m 40s
29 tests +29  28 ✅ +28  0 💤 ±0  1 ❌ +1 
30 runs  +30  29 ✅ +29  0 💤 ±0  1 ❌ +1 

For more details on these failures, see this check.

Results for commit 0b9197e. ± Comparison against base commit 97fc7b3.

♻️ This comment has been updated with latest results.

@hansieodendaal hansieodendaal force-pushed the ho_limit_transaction_size branch 2 times, most recently from 287f722 to 460c677 Compare February 20, 2024 09:43
@hansieodendaal hansieodendaal marked this pull request as ready for review February 20, 2024 09:43
@hansieodendaal hansieodendaal force-pushed the ho_limit_transaction_size branch 3 times, most recently from 7112d23 to 227cdcf Compare February 20, 2024 11:58
@ghpbot-tari-project ghpbot-tari-project removed the P-reviews_required Process - Requires a review from a lead maintainer to be merged label Feb 26, 2024
Added checks to ensure that transactions to self or that are negotiated between
parties do not exceed the RPC frame size limit.
@ghpbot-tari-project ghpbot-tari-project added the P-reviews_required Process - Requires a review from a lead maintainer to be merged label Feb 29, 2024
@ghpbot-tari-project ghpbot-tari-project removed the P-reviews_required Process - Requires a review from a lead maintainer to be merged label Mar 1, 2024
@SWvheerden SWvheerden merged commit abd64d8 into tari-project:development Mar 1, 2024
14 of 16 checks passed
@hansieodendaal hansieodendaal deleted the ho_limit_transaction_size branch March 1, 2024 06:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-acks_required Process - Requires more ACKs or utACKs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants