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

TransactionsHandle propagation commands should not adhere to caching #12079

Merged

Conversation

Parikalp-Bhardwaj
Copy link
Contributor

@Parikalp-Bhardwaj Parikalp-Bhardwaj commented Oct 25, 2024

Closes #12033

Description:
This pull request addresses the issue where manual propagation commands in TransactionsHandle were adhering to caching, preventing forced broadcasts. The following changes have been made:

  • Added PropgateKind Enum:
    Introduced a PropagateKind enum with variants Basic and Forced to control caching behavior during transaction propagation.

  • Modified Propagation Methods:
    Updated methods such as propagate_full_transactions_to_peer, propagate_hashes_to, and propagate_transactions to accept a PropgateKind parameter.

  • Updated Manual Propagation Commands:
    In the on_command method, manual propagation commands now use PropgateKind::Basic to bypass the cache, allowing forced broadcasts.

  • Ensured Regular Propagation Respects Cache:
    Regular propagation commands continue to use PropgateKind::Forced, maintaining the caching mechanism to optimize network usage.

  • Updated Tests:
    Modified existing tests and added new ones to cover scenarios with both cache policies, ensuring that the caching behavior is as expected.

@mattsse Please review the changes and let me know if any adjustments are needed.

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

cool, functionally this is correct.

only have style nits

crates/net/network/src/transactions/mod.rs Outdated Show resolved Hide resolved
Comment on lines 263 to 267
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
enum CachePolicy {
IgnoreCache,
RespectCache,
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd like to rename this slightly to something like

enum PropgateKind {
  /// Default propgation mode, filters out txs that we already sent or received
  Basic,
  /// Always propagate, even if we already sent or received the txs.
  Forced,
}

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

ah, I see we already have an enum that is used with that name.

we need a new enum just for this, with the two variants. this should not change the existing enum

@Parikalp-Bhardwaj
Copy link
Contributor Author

@mattsse done, Created a new enum PropgateKind with the two variants.

@mattsse mattsse force-pushed the fix/transaction-propagation-caching branch from 9f31bdc to 7f297b8 Compare October 27, 2024 14:49
@mattsse mattsse force-pushed the fix/transaction-propagation-caching branch from 1e1a2ae to 452e053 Compare October 27, 2024 14:56
@mattsse mattsse added this pull request to the merge queue Oct 27, 2024
@mattsse mattsse added the A-networking Related to networking in general label Oct 27, 2024
Merged via the queue into paradigmxyz:main with commit 0c51609 Oct 27, 2024
40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-networking Related to networking in general
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TransactionsHandle propagation commands should not adhere to caching
2 participants