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

v2.2: txview: run status and age checks on incoming transactions (backport of #4506) #4978

Open
wants to merge 1 commit into
base: v2.2
Choose a base branch
from

Conversation

mergify[bot]
Copy link

@mergify mergify bot commented Feb 13, 2025

Problem

  • view transaction parsing option in banking stage does not run status/age checks before inserting transactions into the buffer
  • this means that aged out, high priority, transactions may push out lower-priority but still valid transactions

Summary of Changes

  • Insert all valid transactions into the map on receive (map only, not priority queue)
  • Check ages in batches (batch size is equal to the extra capacity we reserved in the map)
  • Insert only valid transaction's id into priority queue, drop lowest priority if at capacity

Fixes #


This is an automatic backport of pull request #4506 done by [Mergify](https://mergify.com).

@mergify mergify bot requested a review from a team as a code owner February 13, 2025 19:39
@apfitzge
Copy link

2435504 due to #4954 not merged yet.

@jstarry jstarry self-requested a review February 14, 2025 15:23
@apfitzge apfitzge force-pushed the mergify/bp/v2.2/pr-4506 branch from 2435504 to db41cc9 Compare February 14, 2025 15:41
@apfitzge
Copy link

rebased on v2.2 after #4954 was merged. No longer needed 2435504

Copy link

@jstarry jstarry left a comment

Choose a reason for hiding this comment

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

Let's bake this on a node before recommending uptake by any clusters

Comment on lines -87 to +94
/// Pushes a transaction id into the priority queue. If the queue is full, the lowest priority
/// transaction will be dropped (removed from the queue and map).
/// Returns `true` if a packet was dropped due to capacity limits.
fn push_id_into_queue(&mut self, priority_id: TransactionPriorityId) -> bool;
/// Pushes transaction ids into the priority queue. If the queue if full,
/// the lowest priority transactions will be dropped (removed from the
/// queue and map) **after** all ids have been pushed.
/// To avoid allocating, the caller should not push more than
/// [`EXTRA_CAPACITY`] ids in a call.
/// Returns the number of dropped transactions.
fn push_ids_into_queue(
&mut self,
priority_ids: impl Iterator<Item = TransactionPriorityId>,
) -> usize;

Choose a reason for hiding this comment

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

i think we could've got away without some of this refactor stuff for the bp-targeted changes

Copy link

Choose a reason for hiding this comment

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

This was necessary, lots of context in the original pr

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.

3 participants