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

1045 - additional fork handling #1048

Merged
merged 6 commits into from
Nov 8, 2021

Conversation

piotrkosecki
Copy link
Contributor

resolves #1045

This PR adds another fork handling behaviour, as described in the ticket #1045 and also adds big maps realated tables invalidation. Also fixes tests and some minor things.

fork-handling {
backtrack-levels: 100 //how many levels back we should check for the forks
backtrack-levels: ${?CONSEIL_LORRE_FORK_HANDLING_BAKCTRACK_LEVELS}
backtrack-interval: 120 // every how many iterations we should check for forks
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure 100 levels will be enough every 120 iterations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've used default values from the #1045 but to be honest to get right values here is trickier than it seems.
For example:
Let's assume that blocks on depth 10 and greater from head are safe from forks.
We started Lorre which is fully indexed and there are no new blocks in the node.
Backtracking forks check looks at the last 10 indexed blocks and everything is ok.
Then it waits number of intervals for a next check. Now we need to estimate amount of blocks that will be baked during this time.
And that depends on few things: sleep_interval and backtrack-interval.
I believe with those assumptions we should make backtrack-levels to be ((Amount_of_blocks_baked_during(sleep_interval) + Amount_of_blocks_baked_during_processing) * backtrack-interval) + 10 + 1.
And that should be an estimate.
We can tune it afterwards, but I definitely think we should follow this rule: backtrack-levels > backtrack-interval

@vishakh
Copy link
Contributor

vishakh commented Nov 3, 2021 via email

@sonarqubecloud
Copy link

sonarqubecloud bot commented Nov 3, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
3.3% 3.3% Duplication

@piotrkosecki
Copy link
Contributor Author

The external user will be puzzled if they are missing all token data, no?

On Wed, Nov 3, 2021 at 8:40 AM Piotr Kosecki @.> wrote: @.* commented on this pull request. ------------------------------ In conseil-lorre/src/main/resources/application.conf <#1048 (comment)>: > fork-handling-is-on: false fork-handling-is-on: ${?CONSEIL_LORRE_FORK_DETECTION_ENABLED} - registered-tokens-is-on: true + registered-tokens-is-on: false I turned it off because of my tests, but now thinking about it if we are publishing it to the public as an external user I would probably want this to be turned off. I can change it if you wish. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#1048 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHDKW27GT7KVCMS5SHKAUTUKEUVFANCNFSM5G7WHR3Q .

no, this will get token data from json file which has basic registered tokens config(and in the future hopefully more)

@vishakh vishakh merged commit 8a8a329 into master Nov 8, 2021
@vishakh vishakh added this to the 2022_r1 milestone Mar 11, 2022
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.

Add additional fork handling behavior
2 participants