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

Slash packet throttling #462

Merged
merged 210 commits into from
Dec 16, 2022
Merged

Slash packet throttling #462

merged 210 commits into from
Dec 16, 2022

Conversation

shaspitz
Copy link
Contributor

@shaspitz shaspitz commented Nov 10, 2022

Description

Slash packets are now queued on the provider, and handled in a throttled manner.

Closes #404
Closes #595

I've kept #589 and #600 as separate PRs, but they are useful in validating the correctness of this feature. My suggestion is to merge them into main after this PR, once people have time to review them.

Protocol Description and Background

See md file in PR https://github.com/cosmos/interchain-security/blob/ba9965ec5fa8920008e3b1afc593277741e5c9e8/docs/throttle.md

@shaspitz
Copy link
Contributor Author

shaspitz commented Dec 13, 2022

Change entire design of this PR, don't queue anything, drop slash packets when too many come in at once. I like this idea but it is not viable, as it would violate a lot of the guarantees of the spec.

How it is currently already violates a lot of guarantees of the spec. We are going to change the spec to accomodate this. The spec would presumably also be changed if other solutions required it. The precise slash guarantees are anyway a lot a product decision, not an absolute correctness decision.

Don't panic when queue becomes too large. I'm fine taking this path, but it would cause non-deterministic behavior if validators' binaries are panicking (or crashing, or their machines run out of disk space) at different times

Hmm good point about the non determinism! Whatever the decision with the queue size limit, it should be in the doc.

Addressing this comment, this design does not violate a lot of the spec properties compared to alternatives like dropping slash packets. These are definitely valuable conversations, but I think they're best saved for the development of the slash packet throttling spec. I'll add a section in the doc about the max queue limit

shaspitz and others added 2 commits December 12, 2022 17:10
@shaspitz shaspitz marked this pull request as ready for review December 16, 2022 05:27
Copy link
Contributor

@mpoke mpoke left a comment

Choose a reason for hiding this comment

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

Amazing work @smarshall-spitzbart. Check my comments bellow. Nothing major. Can be fixed after this is merged.

timeBzL := len(timeBz)
return AppendMany(
[]byte{GlobalSlashEntryBytePrefix},
sdk.Uint64ToBigEndian(uint64(timeBzL)),
Copy link
Contributor

Choose a reason for hiding this comment

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

This sorts by the number of bytes returned by sdk.FormatTimeBytes. Replace with uint64 as done in e440824

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch! When you have time could you sanity check 3b03354. It should be safe to parse the uint64 time back to a time.Time using recvTime = time.Unix(0, int64(timeBz)).UTC(), correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM. I would be more comfortable though once we have #607 fixed.

x/ccv/provider/keeper/throttle.go Show resolved Hide resolved
x/ccv/provider/keeper/throttle.go Outdated Show resolved Hide resolved
x/ccv/provider/keeper/throttle.go Outdated Show resolved Hide resolved
x/ccv/provider/keeper/throttle.go Show resolved Hide resolved
@shaspitz shaspitz mentioned this pull request Dec 16, 2022
vscMaturedData = []ccvtypes.VSCMaturedPacketData{}
ibcSeqNums = []uint64{}

iteratorLoop:
Copy link
Contributor

Choose a reason for hiding this comment

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

Change the switch to ifs to avoid using a label, becuase most people won't be familiar with labels

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See #611

Copy link
Contributor

@jtremback jtremback left a comment

Choose a reason for hiding this comment

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

LGTM

@jtremback jtremback merged commit ae66785 into main Dec 16, 2022
@jtremback jtremback deleted the circuit-breaker branch December 16, 2022 18:52
@shaspitz shaspitz mentioned this pull request Dec 16, 2022
3 tasks
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.

Diagnose GOC halt Jailing circuit breaker
4 participants