nethermind icon indicating copy to clipboard operation
nethermind copied to clipboard

Throttling for networking of blob transactions

Open emlautarom1 opened this issue 1 year ago • 5 comments

Fixes #6284

Changes

  • Support configurable throttling for networking of blob transactions

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)
  • [x] Optimization
  • [ ] Refactoring
  • [ ] Documentation update
  • [ ] Build-related changes
  • [ ] Other: Description

Testing

Requires testing

  • [x] Yes
  • [ ] No

If yes, did you write tests?

  • [x] Yes
  • [ ] No

Documentation

Requires documentation update

  • [ ] Yes
  • [x] No

Requires explanation in Release Notes

  • [ ] Yes
  • [x] No

emlautarom1 avatar Nov 23 '23 15:11 emlautarom1

I've explored a couple of designs for throttling sending responses:

  • Use a decorator (ex. ThrottledProtocolHandler) that wraps Eth66ProtocolHandler. This gets complicated quickly due to inheritance (we cannot easily override the methods that "send"). We end up with code that hacks around internals of the hierarchy.
  • Use a decorator (ex. ThrottledPacketSender) that wraps PacketSender, which is used internally to send responses. This works but then all messages sent get throttled. If this is the behavior that we want then I think it's a better overall solution that modifying Eth66ProtocolHandler as we're currently doing

emlautarom1 avatar Nov 23 '23 16:11 emlautarom1

I think throttling in all messages send would be fine, but we need to limit it by amount of data / time, instead of number of messages / second. Messages have different sizes and we shouldn't count message with blob tx the same like e.g. message with one legacy tx without any data.

marcindsobczak avatar Nov 24 '23 08:11 marcindsobczak

For throttling messages sent, we would need to update the PacketSender used here:

https://github.com/NethermindEth/nethermind/blob/ad54c5f7dd91b0ca9bce0683e60a5f65c1bb4117/src/Nethermind/Nethermind.Network/Rlpx/NettyHandshakeHandler.cs#L183-L183

As for throttling messages received, we would need to update the factory method:

https://github.com/NethermindEth/nethermind/blob/ad54c5f7dd91b0ca9bce0683e60a5f65c1bb4117/src/Nethermind/Nethermind.Network/ProtocolsManager.cs#L203

It's an open question from where we should pick the throttling values (hardcoded or user configurable).

emlautarom1 avatar Nov 28 '23 17:11 emlautarom1

Would be great to add throttling setup to NetworkConfig and make accepted amount of data configurable. Would be also great to make 0 as limitless - this way we could make 0 as default and merge it to the master soon (so throttling will be disabled by default) and on dencun devnets we would test it in various configurations and then decide what should be our default value and update default config in another PR later.

~~Also if we are limiting all messages (not only blob-related), we need to pass it to all eth handlers (from eth62 up to eth68), not only eth68. Open question if we should extend it to other protocols like snap - IMO not, let's stick with just eth protocol~~ ah in case of received txs we are limiting it only as throttling of eth68 hashes, that's ok, we can stick with it and no need to edit other handlers than eth68

marcindsobczak avatar Nov 30 '23 14:11 marcindsobczak

A thing to consider is the netty implementation of similar requirement.

flcl42 avatar Jan 30 '24 10:01 flcl42