-
Notifications
You must be signed in to change notification settings - Fork 494
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
consensus/bor: eliminate contract calls for current span #1444
base: develop
Are you sure you want to change the base?
Conversation
@@ -364,7 +364,7 @@ func getMockedSpanner(t *testing.T, validators []*valset.Validator) *bor.MockSpa | |||
spanner := bor.NewMockSpanner(gomock.NewController(t)) | |||
spanner.EXPECT().GetCurrentValidatorsByHash(gomock.Any(), gomock.Any(), gomock.Any()).Return(validators, nil).AnyTimes() | |||
spanner.EXPECT().GetCurrentValidatorsByBlockNrOrHash(gomock.Any(), gomock.Any(), gomock.Any()).Return(validators, nil).AnyTimes() | |||
spanner.EXPECT().GetCurrentSpan(gomock.Any(), gomock.Any()).Return(&span.Span{0, 0, 0}, nil).AnyTimes() | |||
spanner.EXPECT().GetCurrentSpan(gomock.Any(), gomock.Any()).Return(&span.Span{0, 0, 0}, nil).MaxTimes(0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fyi, this change makes sure that GetCurrentSpan
isn't called even once and the tests still works suggesting that the new implementation is replacable.
I tried adding a new e2e tests but given default configs, it'll mine ~6400 blocks which will take more time (>1m) and fails under certain cases (which needs more debugging). I believe existing tests should cover most of the cases but if there's a need for a new test and the time is manageable, I don't mind adding it. Moreover, I can make the time short but we'll have to expose public functions from bor to override the span length and 0th span end as tests assume a smaller test size (this is also something @lucca30 commented above). |
Description
This PR replaces the logic to check and commit new spans.
Currently, in every sprint start, we make a contract call to fetch the current span. We check against that span if we need to commit a new one and if yes, we commit it to the contract.
We can easily avoid the contract calls every sprint and make the check very simple and intuitive. Because the span length hasn't changed since genesis, we can simply use a formula to find out which span we're currently in and whether we need to commit a new one.
Changes
Breaking changes
Please complete this section if any breaking changes have been made, otherwise delete it
Nodes audience
In case this PR includes changes that must be applied only to a subset of nodes, please specify how you handled it (e.g. by adding a flag with a default value...)
Checklist
Cross repository changes
Testing
Manual tests
Please complete this section with the steps you performed if you ran manual tests for this functionality, otherwise delete it
Additional comments
Please post additional comments in this section if you have them, otherwise delete it