ouroboros-network icon indicating copy to clipboard operation
ouroboros-network copied to clipboard

ChainSync rate-limiting

Open jasagredo opened this issue 3 years ago • 3 comments

Following #423, this PR implements a rate-limiting functionality that can be expressed as:

When a peer does a rollback from a block with block number N and then in less that one second it does another rollback from a block with block number =< N, this second rollback is delayed until one second has passed since the first rollback.

Note that the first commit is merely documentation whereas the second one has the actual code changes.

jasagredo avatar May 18 '21 09:05 jasagredo

We're still doing plenty of design discussion, so I've only looked through this diff quickly.

First: great job updating the comments; I like it.

Second: adding a common wrapper to the state seems reasonable. I don wonder if it might be clearer/simpler to instead add a common field to the three existing state types we already have (Known, Unknown, and ()). (Maybe I'm misremembering, but I think it's only ever those three.) That might keep the types a bit tidier.

nfrisby avatar May 27 '21 23:05 nfrisby

Third: I'm having trouble shaking the thought that we maybe don't need to retain the rollback history when we drain the pipe and go through the find intersection path. Do you have any clarity there as to why we definitely would need to?

Hmm, maybe my discomfort is actually about the drainThePipe part; maybe we need to be using those messages to update the rollback history (and possibly punish) instead of simply discarding them?

nfrisby avatar May 27 '21 23:05 nfrisby

I think this PR is no longer relevant after pipelining work? @amesgen @nfrisby And perhaps even #423 can be closed too?

jasagredo avatar May 24 '22 08:05 jasagredo

Yeah, I think the Google doc you wrote will be the more useful resource when we eventually swing our attention back here. Thanks!

nfrisby avatar Oct 26 '22 17:10 nfrisby