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

Feature/disable tx gossip during sync #5850

Merged
merged 9 commits into from
Jun 22, 2023

Conversation

LukaszRozmej
Copy link
Member

Resolves #5391

Changes

  • Introduce ITxGossipPolicy that can decide if tx's can be gossiped and if particullar transaction can be gossiped (requested feature by user for a plugin)
  • Based on the policy don't gossip the transactions in the pool
  • Based on policy ignore transactions or transactions hashes that are gossiped to the node
  • Create a policy that allows gossiping transaction depending on SyncMode

Types of changes

What types of changes does your code introduce?

  • Bugfix (a non-breaking change that fixes an issue)
  • New feature (a non-breaking change that adds functionality)
  • Breaking change (a change that causes existing functionality not to work as expected)
  • Optimization
  • Refactoring
  • Documentation update
  • Build-related changes
  • Other: Description

Testing

Requires testing

  • Yes
  • No

If yes, did you write tests?

  • Yes
  • No

@benaadams
Copy link
Member

Benchmarks project compile error

Nethermind/Nethermind.Network.Benchmark/Eth62ProtocolHandlerBenchmarks.cs(68,88): 
error CS0104: 'ShouldGossip' is an ambiguous reference between 
'Nethermind.Consensus.ShouldGossip' and 'Nethermind.TxPool.ShouldGossip' [/home/runner/work/nethermind/nethermind/src/Nethermind/Nethermind.Network.Benchmark/Nethermind.Network.Benchmark.csproj]

Copy link
Contributor

@marcindsobczak marcindsobczak left a comment

Choose a reason for hiding this comment

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

Looks fine, but we have regression in hive devp2p TestTransaction (nethermind)
https://github.com/NethermindEth/nethermind/actions/runs/5342947680/jobs/9685386532
(other fails are same as on master).
I tried 3 times and we failed that test in all of them

POSDAO tests are both fine
Engine tests are in progress

@marcindsobczak
Copy link
Contributor

Engine tests are fine (some fails, but nothing more than on master)

@LukaszRozmej
Copy link
Member Author

Looks fine, but we have regression in hive devp2p TestTransaction (nethermind) https://github.com/NethermindEth/nethermind/actions/runs/5342947680/jobs/9685386532 (other fails are same as on master). I tried 3 times and we failed that test in all of them

POSDAO tests are both fine Engine tests are in progress

Works now: https://github.com/NethermindEth/nethermind/actions/runs/5344093085/jobs/9688040889

@LukaszRozmej LukaszRozmej merged commit 66ee09e into master Jun 22, 2023
61 of 62 checks passed
@LukaszRozmej LukaszRozmej deleted the feature/disable_tx_gossip_during_sync branch June 22, 2023 10:37
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.

Ignore transactions messages until state is synced
4 participants