feat: Ignore P2P messages previously seen

[PhilWindle]

Please read contributing guidelines and remove this line.

[mralj]

Hey! I'm probably missing something, but this solution feels too complicated.
IMO, a much easier solution would be poor man's LRU.

1. We cap cache by TX count, not time. I don't see a massive benefit in defining this by time instead of count. Is it UX? Because it's easier to reason about the 24hrs or 12hrs, or should I configure the cache to hold 100k transactions or 1M?
2. We have a Set and a Queue
3. If tx already exists in the Set, do nothing, else
4. If set.length >= N, pop from Queue and remove from Set
5. Add to Set and push to Queue

This seems much less complicated and will guarantee removing the oldest transactions first. And uses half of the memory of the current solution

The downsides I see:

1. As mentioned, maybe a bit worse UX
2. JS doesn't have Queues, so the perf will suffer when we hit the cache size limit and constantly call shift(). To combat this, I see 2 options
   2.1. Proper off-shelf queue implementation, but if you don't like this:
   2.2. We could set the cache size to 2N (memory-wise, it will be similar to the current solution), and when we hit limit of 2N cached txs, we prune half - effectively amortizing the cost of the queue.shift()]
   2.3. Implement queue ourselves - but then this gets more complicated than the current solution 😅

P.S.

We could still have the Set/Queue solution without a sliding window if we store, e.g., [hash, timestamp] in the queue and check if the first element of the queue is old enough to execute prune when inserting. IMO it will still be simpler and use less memory :)

[PhilWindle]

Hey! I'm probably missing something, but this solution feels too complicated. IMO, a much easier solution would be poor man's LRU.

1. We cap cache by TX count, not time. I don't see a massive benefit in defining this by time instead of count. Is it UX? Because it's easier to reason about the 24hrs or 12hrs, or should I configure the cache to hold 100k transactions or 1M?
2. We have a Set and a Queue
3. If tx already exists in the Set, do nothing, else
4. If set.length >= N, pop from Queue and remove from Set
5. Add to Set and push to Queue

This seems much less complicated and will guarantee removing the oldest transactions first. And uses half of the memory of the current solution

The downsides I see:

1. As mentioned, maybe a bit worse UX
2. JS doesn't have Queues, so the perf will suffer when we hit the cache size limit and constantly call shift(). To combat this, I see 2 options
   2.1. Proper off-shelf queue implementation, but if you don't like this:
   2.2. We could set the cache size to 2N (memory-wise, it will be similar to the current solution), and when we hit limit of 2N cached txs, we prune half - effectively amortizing the cost of the queue.shift()]
   2.3. Implement queue ourselves - but then this gets more complicated than the current solution 😅

P.S.

We could still have the Set/Queue solution without a sliding window if we store, e.g., [hash, timestamp] in the queue and check if the first element of the queue is old enough to execute prune when inserting. IMO it will still be simpler and use less memory :)

I agree with this. I went for a time-based approach as it intuitive to say 'store N minutes of messages'. But I don't think there is inherently any reason to view the store in this way and a simpler 'store N messages' will suffice. I will also create a cache per topic too.

I think we can get rid of the shifting all together by using a circular array?

[mralj]

I think we can get rid of the shifting all together by using a circular array?

Ring buffer FTW 😄

[alexghr]

I like the new approach

[mralj]

LGTM.
I have left 2 nitpicks 😅

[mralj]

Nitpick: I'd add expect(validator.queue.length === 10) just to be sure this is not growing indefinitely.

[mralj]

This could be simplified to const validator = this.msgIdSeenValidators[msg.topic]; given that we check taht validator exist: if(!validator ...).

[alexghr]

We tried this but TopicType.tx is just a prefix of msg.topic (the topic of the actual message contains the compressed version string as a suffix)