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

fix: 🐛 update spam handling to avoid inconsistencies after runtime upgrades #338

Merged
merged 6 commits into from
Jan 28, 2025

Conversation

TDemeco
Copy link
Contributor

@TDemeco TDemeco commented Jan 23, 2025

This PR changes the way we detect if the chain is currently being spammed or not to solve issue #26:

  • Currently, we have a storage value that contains the count of not-full blocks in the last BlockFullnessPeriod blocks. We add one to it if the previous block (current_block - 1) was not full, and substract one from it if the block current_block - BlockFullnessPeriod - 1 was not full.
  • Now we have instead a storage value PastBlocksStatus that contains a bounded vector of boolean elements and size BlockFullnessPeriod. Each index of the vector represents a block (the first element represents the oldest block of the set current_block - BlockFullnessPeriod - 1, the last element the newest block of the set current_block - 1) and each value represents whether that block was considered full or not.
  • Each new block where the chain is not current undergoing a multiblock migration (so the on_poll hook gets executed):
    • The oldest block of the vector gets removed (only if the vector is full, to avoid errors when starting from genesis).
    • The previous block gets checked to see if it was considered spammed. If it was, pushes a true value to the bounded vector, otherwise pushes a false value.
    • After the changes to the bounded vector, the amount of non-full blocks get counted and, depending if it's under the minimum stipulated by the runtime or not, the chain is considered spammed or not, and the ChallengesTickerPaused storage gets set or cleared accordingly.
    • Finally, update the PastBlocksStatus storage with the new vector.

Note: ideally, we would use a BitVec to be more efficient storage-wise, but there's no current implementation of a bounded BitVec, and since we are modifying the vector in every block, it gets included in the PoV so it cannot be unbounded

@TDemeco TDemeco requested a review from ffarall January 23, 2025 19:52
@TDemeco TDemeco requested a review from santikaplan January 24, 2025 15:27
Copy link
Contributor

@santikaplan santikaplan left a comment

Choose a reason for hiding this comment

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

All good! Some minor comments, good to merge!

*
* This is used to check if the network is presumably under a spam attack.
* Each element in the vector represents a block, and is `true` if the block was full, and `false` if it was not.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Each element in the vector represents a block, and is `true` if the block was full, and `false` if it was not.
* Each element in the vector represents a block, and is `true` if the block was full, and `false` otherwise.

Comment on lines 370 to 372
// Setting the `PastBlocksStatus` bounded vector to contain as the first element a block considered full, then
// exactly the minimum non full blocks, and then all full blocks, so that when adding the new non full block
// the chain goes from being considered spammed to not spammed.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Setting the `PastBlocksStatus` bounded vector to contain as the first element a block considered full, then
// exactly the minimum non full blocks, and then all full blocks, so that when adding the new non full block
// the chain goes from being considered spammed to not spammed.
// Setting the `PastBlocksStatus` bounded vector to contain, as the first element, a block considered full, followed by
// exactly the minimum required non-full blocks, and then all full blocks, so that when adding the new non-full block,
// the chain transitions from being considered spammed to not spammed.

*
* This is used to check if the network is presumably under a spam attack.
* Each element in the vector represents a block, and is `true` if the block was full, and `false` if it was not.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Each element in the vector represents a block, and is `true` if the block was full, and `false` if it was not.
* Each element in the vector represents a block, and is `true` if the block was full, and `false` otherwise.

///
/// This is used to check if the network is presumably under a spam attack.
/// Each element in the vector represents a block, and is `true` if the block was full, and `false` if it was not.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Each element in the vector represents a block, and is `true` if the block was full, and `false` if it was not.
/// Each element in the vector represents a block, and is `true` if the block was full, and `false` otherwise.

@TDemeco TDemeco merged commit 7e2f2d5 into main Jan 28, 2025
25 checks passed
@TDemeco TDemeco deleted the fix/check-spamming-block-handling branch January 28, 2025 04:58
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.

3 participants