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

Remove unused dBFT APIs #104

Merged
merged 9 commits into from
Mar 8, 2024
Merged

Remove unused dBFT APIs #104

merged 9 commits into from
Mar 8, 2024

Conversation

AnnaShaleva
Copy link
Member

@AnnaShaleva AnnaShaleva commented Mar 6, 2024

This PR removes exported APIs from interfaces declared in the dbft package. Those APIs are remove that are not used by dBFT. If dBFT user wants to access some of removed dbft payloads APIs, then it should cast dbft payload interface to the corresponding locally-defined payload implementation.

Regarding to the rest of APIs, @roman-khimov, need your opinion:

  • Timer related Config API is currently unused in both NeoGo and Bane (WithTimer, WithTimestampIncrement). Default timer implementation is used, thus, timer API may be removed (except WithSecondsPerBlock). However, in general it's nice to have the ability to customize timer. What is the desired behaviour? (I vote to keep it).
  • VerifyPrepareResponse is currently unused by NeoGo and Bane, no verification is being performed, but it may be useful for future Bane consensus, thus I'd suggest to keep it.

Close #84.

@AnnaShaleva AnnaShaleva removed the request for review from roman-khimov March 6, 2024 12:40
@AnnaShaleva
Copy link
Member Author

If everything is OK wrt these movements, then I'll update the documentation.

Copy link

codecov bot commented Mar 6, 2024

Codecov Report

Attention: Patch coverage is 70.73171% with 72 lines in your changes are missing coverage. Please review.

Project coverage is 62.80%. Comparing base (a2fdfa4) to head (d504ad8).
Report is 1 commits behind head on master.

Files Patch % Lines
internal/consensus/consensus.go 0.00% 32 Missing ⚠️
config.go 80.55% 14 Missing ⚠️
internal/simulation/main.go 0.00% 12 Missing ⚠️
timer/timer.go 80.00% 4 Missing ⚠️
internal/consensus/block.go 0.00% 3 Missing ⚠️
internal/consensus/recovery_message.go 66.66% 3 Missing ⚠️
internal/consensus/consensus_message.go 50.00% 2 Missing ⚠️
dbft.go 97.05% 1 Missing ⚠️
internal/consensus/constructors.go 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #104      +/-   ##
==========================================
- Coverage   63.60%   62.80%   -0.80%     
==========================================
  Files          26       27       +1     
  Lines        1547     1546       -1     
==========================================
- Hits          984      971      -13     
- Misses        495      508      +13     
+ Partials       68       67       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@roman-khimov roman-khimov left a comment

Choose a reason for hiding this comment

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

I'd move timer.Timer to dbft.Timer (the interface itself) and then provide an implementation in timer. VerifyPrepareResponse is OK to keep.

block.go Show resolved Hide resolved
config.go Show resolved Hide resolved
consensus_message.go Show resolved Hide resolved
context.go Show resolved Hide resolved
dbft.go Outdated Show resolved Hide resolved
internal/block/block.go Outdated Show resolved Hide resolved
internal/consensus/block.go Outdated Show resolved Hide resolved
recovery_request.go Outdated Show resolved Hide resolved
NextConsensus API is not used by the users of dBFT. NextConsensus
handling (proposal, verification and agrreement) is moved to the upper
level of dBFT users. Starting from this commit NextConsensus
verification should be performed by dBFT user manually in
WithVerifyPrepareRequest callback.

A part of #84.

Signed-off-by: Anna Shaleva <[email protected]>
@AnnaShaleva AnnaShaleva force-pushed the reduce-apis branch 2 times, most recently from 7a66df1 to f1951ae Compare March 6, 2024 14:31
Version context field is always set to 0. There is no API to change
Version in dBFT context. Given a possibility of Block version change, we
either need to introduce this Version initializer or need to move
Version handling out of dBFT. The latter case is chosen, because in this
case block/PrepareRequest version can be handled directly by node
depending on block index fetched from dBFT context.

Also remove Version() block API, it's unused.

A part of #84.

Signed-off-by: Anna Shaleva <[email protected]>
It's unused by dBFT, and thus, it's not needed in this interface. If
this property is used by dBFT user, then the user need to convert dBFT
Block to user's Block and retrieve all necessary info.

A part of #84.

Signed-off-by: Anna Shaleva <[email protected]>
All dBFT payloads implementations should be kept in the same package or
have exported fields. It's inevitable since dBFT user have to refer to
payload fields via converting dBFT interfaces to user-defined local
structures.

dBFT test (`dbft_test` package) imports custom payloads implementations
from `internal`. Since all payloads will be moved to the same package,
we need to split this package from `main` package of simulation.

A consequence of #84.

Signed-off-by: Anna Shaleva <[email protected]>
It's needed to be able to cast dBFT payload interfaces to local
user-defined payloads implementations during consensus service
functioning.

A psrt of #84.

Signed-off-by: Anna Shaleva <[email protected]>
It's unused by dBFT and thus, should be removed from Block interface. If
user need to access this data, then he should convert dbft.Block to the
local user-defined Block implementation.

A part of #84.

Signed-off-by: Anna Shaleva <[email protected]>
This API is unused by dBFT. If a user needs to access ChangeView
timestamp, he has to convert dbft.ChangeView to the user-defined
ChangeView implementation.

A part of #84.

Signed-off-by: Anna Shaleva <[email protected]>
timer.go Outdated Show resolved Hide resolved
timer.go Outdated Show resolved Hide resolved
Store Timer interface along with other dBFT interfaces and provide
default timer implementation in `timer` package. Create `dbft.HV`
interface and configuration for its customisation. Provide default
implementation of `dbft.HV` in `timer` package.

A part of #84.

Signed-off-by: Anna Shaleva <[email protected]>
Also add a record to CHANGELOG.md.

A part of #84.

Signed-off-by: Anna Shaleva <[email protected]>
@roman-khimov roman-khimov merged commit 7e15b1f into master Mar 8, 2024
10 checks passed
@roman-khimov roman-khimov deleted the reduce-apis branch March 8, 2024 06:49
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.

Refactor dBFT APIs
2 participants