Consider changing BlockSyncStorage interface in block sync to rely on GetLastCommittedBlockHeight instead of LastCommittedBlockHeight

[talkol]

The block sync separate unit relies on the block storage service via this interface:

type BlockSyncStorage interface {
	LastCommittedBlockHeight() primitives.BlockHeight
	CommitBlock(ctx context.Context, input *services.CommitBlockInput) (*services.CommitBlockOutput, error)
	ValidateBlockForCommit(ctx context.Context, input *services.ValidateBlockForCommitInput) (*services.ValidateBlockForCommitOutput, error)
}

Most of these functions are public functions of the service defined in the service proto defining its public well-known interface.

The only exception is LastCommittedBlockHeight.

The service has a public known function in the protos called GetLastCommittedBlockHeight. This function also takes the context. Consider for esthetic reasons to switch to it instead of the internal version LastCommittedBlockHeight.

This would also enable us to make LastCommittedBlockHeight lowercase and not exported. It's a smell to export service methods that aren't defined in the protos.

[netoneko]

It was done because there is a lot of boxing/unboxing one has to do in case of calling GetLastCommittedBlockHeight vs LastCommittedBlockHeight. Of course we could move this boxing/unboxing into block sync instead of keeping it inside of the block storage.

[jlevison]

acutally this got me looking at the code and i found a bug in GetLastCommittedBlockHeight .. the return value holds both height and timestamp, and there exists a case where it would return the wrong timestamp for the requested block (race condition) - i'll fix that, and i will play around with refactoring as the issue describes here and see how it feels

[jlevison]

@talkol , this is waiting for a spec change as we discussed, see orbs-network/orbs-spec#107

[jlevison]

@tal, asking as i cannot remember right now, you decided this should not be changed and the tests were changed, LastCommittedBlockHeight was removed as far as i know, right?

[electricmonk]

@ronnno please evaluate and either close or assign for Q32019 milestone

[OdedWx]

Will close the discussion in spec #109.

[ronnno]

This issue is stale. was fixed by @danokoch in rev 0346001 on 2018-11-05