sopel icon indicating copy to clipboard operation
sopel copied to clipboard

[Proposal] Update flood protection logic

Open HumorBaby opened this issue 5 years ago • 4 comments

Flood-prevention measures need to be refactored.

Proposed minimum changes:

  • Move logic into irc.Bot.write
  • Calculate writing backoff (penalty) based on an adequately massaged message (e.g., post-truncation)

Background

RFC 1459 spec on flood control of clients:

8.10 Flood control of clients

   With a large network of interconnected IRC servers, it is quite easy
   for any single client attached to the network to supply a continuous
   stream of messages that result in not only flooding the network, but
   also degrading the level of service provided to others.  Rather than
   require every 'victim' to be provide their own protection, flood
   protection was written into the server and is applied to all clients
   except services.  The current algorithm is as follows:

        * check to see if client's `message timer' is less than
          current time (set to be equal if it is);

        * read any data present from the client;

        * while the timer is less than ten seconds ahead of the current
          time, parse any present messages and penalize the client by
          2 seconds for each message;

   which in essence means that the client may send 1 message every 2
   seconds without being adversely affected.

Current bot layout: (I may have missed some :hushed:)

** = flood-prevention logic is here
+----------------+    +---------------+    +-------------+
|  Sopel.action  +--->+ **Sopel.say** +--->+             |
|  Sopel.msg     |    +---------------+    |             |    +---------------+    +--------------------------+
|  Sopel.reply   |                         | Sopel.write +--->+ irc.Bot.write +--->+ asynchat.async_chat.send |
+----------------+    +---------------+    |             |    +---------------+    +--------------------------+
                      | Sopel.notice  +--->+             |            .
                      | Sopel.part    |    +-------------+           /|\
                      | Sopel.join    |                               |
                      | Sopel.kick ?  |                               |
                      +---------------+                         +-----------+ 
                                                                | CAP,NICK, |
                                                                | PASS,...  |
                                                                +-----------+

Rationale

A few reasons this change is needed:

  1. The current logic calculates a penalty based on the untruncated message. This led to the fiasco of https://github.com/sopel-irc/sopel/issues/1550#issuecomment-481937234 and #1551.
    • The penalty should be based only on the actual part of the message that is being sent to the server.
  2. All the methods that don't use Sopel.say currently, bypass flood-prevention logic.

Future plans

Potential additional changes:

  • Add configurable backoff
  • Add a configurable maximum penalty
  • Allow message burst (with a configurable number and subsequent delays)

HumorBaby avatar Apr 15 '19 18:04 HumorBaby

I'm working on that, but I need #1518 to be merged before I can publish anything.

Exirel avatar May 31 '19 22:05 Exirel

@Exirel Do you still want to work on this for 7.0, or should we defer it to 7.1?

dgw avatar Nov 16 '19 16:11 dgw

Ok, after looking for it, I think it's best to postpone to 7.1 (if not later).

Exirel avatar Nov 16 '19 16:11 Exirel

@HumorBaby I'm about to look at making a small PR for 7.1 that changes the flood penalty calculation to use the truncated message, but regardless of whether that ships or not… I think most of your "Future plans" section is already done, actually.

We have flood_burst_lines (burst quota before triggering Sopel's send-limits), flood_empty_rate (delay between messages when flood-limited), and flood_refill_rate (burst message quota recovery per second) in 7.0 (from #1518); flood_max_wait takes care of a configurable maximum wait time for 7.1 (from #1929). The only thing that might not be present yet is "configurable backoff", but you might consider it partially/wholly obviated by one of the above or remaining new options (flood_penalty_ratio or flood_text_length).

It would benefit Sopel most in the long term to move its flood prevention logic into the IRC backend (so it applies to everything sent, not just what the bot object sends)—a step further than your proposed move into bot.write(). I'm just looking at the other stuff in this issue to see if it's still relevant, or already done in 7.x; those main changes will still wait until 8.0.

dgw avatar Sep 30 '20 05:09 dgw