fix(lib/babe): fix timing for transition between epochs

[noot]

Changes

• fix timing for transition between epochs, sometimes epoch was getting started slightly too early, so added a time.Until to wait until the actual start time
• update block production to use first 2/3 of slot duration, last 1/3 is reserved for finality as per substrate

Tests

go test ./lib/babe

also, can run 2 nodes locally to test this out, let it run until it reaches the second epoch (epoch 1):

./bin/gossamer --key alice
./bin/gossamer --key bob --basepath /tmp/bob --port 7002

Issues

• related to devnet: ensure blocks are finalized  #1587
• related to Fix block production timing logic #1628

[codecov]

Codecov Report

Merging #1636 (1058a9a) into development (3c18e47) will decrease coverage by 0.34%.
The diff coverage is 20.83%.

@@               Coverage Diff               @@
##           development    #1636      +/-   ##
===============================================
- Coverage        58.29%   57.95%   -0.35%     
===============================================
  Files              172      174       +2     
  Lines            16905    17056     +151     
===============================================
+ Hits              9854     9884      +30     
- Misses            5279     5399     +120     
- Partials          1772     1773       +1     

Flag	Coverage Δ
unit-tests	57.95% <20.83%> (-0.35%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
lib/babe/types.go	0.00% <0.00%> (-42.86%)	⬇️
lib/common/uint128.go	82.35% <0.00%> (-1.65%)	⬇️
lib/keystore/mock_keyring.go	0.00% <0.00%> (ø)
lib/keystore/mock_keystore.go	0.00% <0.00%> (ø)
lib/babe/babe.go	53.97% <51.28%> (+8.43%)	⬆️
lib/babe/build.go	50.65% <100.00%> (ø)
lib/babe/epoch.go	66.66% <100.00%> (+1.28%)	⬆️
... and 2 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3c18e47...1058a9a. Read the comment docs.

[timwu20]

nit: seems odd that there are multiple two places in the code where paused is set to true. Also having both pause and paused seems unnecessary.

[noot]

I'll change the other place to just call Pause() instead. it needs the paused bool in the Resume function to get if the service is paused already or not, is there a better way to do that?

[arijitAD]

You can probably close the pause channel and reinitialize it on resume.

func (b *Service) IsPaused() {
	select {
	case: <-b.pause
		return true
	default:
		return false
	}
}

func (b *Service) Pause() {
	close(b.pause)
}

func otherFunc() {
	select {
	case <-time.After(time.Until(epochStartTime)):
	case <-b.ctx.Done():
		return nil
	case <-b.pause:
		return nil
	}
}

[timwu20]

does this service need to halt if invokeBlockAuthoring exits? If this function returned an error, appropriate actions could be taken in that case in initiate.

[noot]

yeah it should probably panic or logger.Crit. I can update that

[timwu20]

is it possible to move this into another function? This seems like something that happens infrequently and can be tested independently?

[noot]

it would happen if the node is down for over an epoch or if it's starting up for the first time on a network that's past epoch 0, which part would you put in a separate function? just this if check?

[edwardmack]

Great work, lgtm.
Testing running two nodes I see the transition to epoch 1:

INFO[06-15|10:34:40] epoch complete!                          pkg=babe completed epoch=0 upcoming epoch=1 caller=babe.go:463
INFO[06-15|10:34:40] initiating epoch                         pkg=babe number=1 first slot of epoch=541255882 caller=babe.go:385
INFO[06-15|10:34:40] current epoch                            pkg=babe epoch=1 slots into epoch=11 caller=babe.go:426
INFO[06-15|10:34:43] 💤 node waiting                        pkg=network peer count=1 head=7 finalised=0 caller=sync.go:325

Is it normal for transition epochs after only 7 blocks?
Is in ok that finilised = 0?

[arijitAD]

You can probably close the pause channel and reinitialize it on resume.

func (b *Service) IsPaused() {
	select {
	case: <-b.pause
		return true
	default:
		return false
	}
}

func (b *Service) Pause() {
	close(b.pause)
}

func otherFunc() {
	select {
	case <-time.After(time.Until(epochStartTime)):
	case <-b.ctx.Done():
		return nil
	case <-b.pause:
		return nil
	}
}

[arijitAD]

Instead of 1/3, Can we allocate a fixed time for block finalization?

[noot]

what do you mean exactly? 1/3 of the slot is a fixed slot duration, this is what substrate does afaik

[arijitAD]

I meant 500ms or so. Instead of 1/3 of slot.duration

[noot]

no that would not be ideal, substrate uses 1/3 of the slot duration, you can see their implementation on the issue

[noot]

@edwardmack yep that is normal, since the genesis file expects 6 block producers and not enough nodes are online to finalise. if you change the genesis to have only 2-3 block producers then there will be more blocks

[arijitAD]

The lock is not required.

[arijitAD]

Remove the error

[noot]

there might be a chance that Pause() is called when the b.pause channel is already close, thus causing a close of closed channel panic

[arijitAD]

No need to take lock here.

[arijitAD]

return errors.New("service has been stopped")

S -> s
Error starts with smallcase letters.

[noot]

for your first comment, the lock is here so there isn't a send on a closed channel. maybe there is a better way to implement that? either way, I think I will be removing this section anyways and doing a refactor

[arijitAD]

Acquire lock after checking b.IsPaused

[noot]

I think this function should be atomic in the case that Pause or Resume are called concurrently by multiple processes, process A might see it's not paused, acquire the lock, then do b.initiate() but in between seeing it's not paused an acquiring the lock another process might also see the same thing, thus causing b.initiate() to be called twice which would be bad

[arijitAD]

Also, make blockChan buffered channel. #1642

[github-actions]

🎉 This PR is included in version 0.6.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀