Refactor dBFT APIs #84

AnnaShaleva · 2023-11-17T14:23:58Z

Is your feature request related to a problem? Please describe.

Some of dBFT APIs are not used since WithNewBlockFromContext addition. E.g. WithGetConsensusAddress always returns an empty address in NeoGo.

Describe the solution you'd like

We need to review all unused dBFT APIs and remove them.

The text was updated successfully, but these errors were encountered:

dBFT doesn't use validators got from this call to GetValidators callback, because NeoGo doesn't properly set WithGetConsensusAddress, and thus this call can be safely skipped. Instead, NeoGo fills NextConsensus field by itself in NewBlockFromContext callback. This commit technically doesn't perform any functional changes and doesn't affect the problem described in #3253 in any way. This commit is just a removal of the code that was never used by NeoGo library. This commit is a direct consequence of nspcc-dev/dbft#84. Signed-off-by: Anna Shaleva <[email protected]>

AnnaShaleva · 2024-03-04T10:50:07Z

Also implement #97 (comment).

roman-khimov · 2024-03-04T12:10:17Z

That's mostly about setters, we want some set of New* and then interface would just expose the data to dbft. Setters are useless here, we never change these values except for initialization of a new payload (either generated one to send or received from the network).

Ref. #84 (comment). Signed-off-by: Anna Shaleva <[email protected]>

A part of #84. Signed-off-by: Anna Shaleva <[email protected]>

Ref. #84 (comment). Signed-off-by: Anna Shaleva <[email protected]>

A part of #84. Signed-off-by: Anna Shaleva <[email protected]>

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]>

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. Close #84. Signed-off-by: Anna Shaleva <[email protected]>

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]>

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]>

This API is unused. In it's essence it's important for the dBFT to know that RecoveryRequest is received, no timestamp information required for its handling. A part of #84. Signed-off-by: Anna Shaleva <[email protected]>

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]>

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]>

Store Timer interface along with other dBFT interfaces and provide default timer implementation 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]>