-
Notifications
You must be signed in to change notification settings - Fork 112
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
Author a sibling block in case best block's slot is same as current slot #2827
Changes from 58 commits
5cd4263
f57fa84
ca4724f
12fde0b
ae2fa69
cc2ffaf
cbc44d2
3de132f
a63a073
aca3b68
9c9f47b
527f515
cecc308
d022f43
540c09a
1edb86b
7fa9196
64e3c03
db03cbb
95a7a35
220f5ae
37fa232
11471e8
e0109dc
93b3057
fdb1fd2
388678a
0862104
c99b776
26f8769
22d9b2c
3e2aa7f
1589dc9
8ec533c
8448863
ce9e195
bcdf2b5
9a9f26d
b065fb8
264db41
24e69e6
2fd728a
31fa786
881eee0
34800ee
8155d43
d0b2a1b
03db54d
45b5fd3
a71de07
37b41ca
532c617
f56e154
849ad82
9ba1b58
8dbc936
db736ba
3366ecc
65f649e
e667450
06313f1
bb1a02c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -454,32 +454,65 @@ func (b *Service) handleEpoch(epoch uint64) (next uint64, err error) { | |||||
return next, nil | ||||||
} | ||||||
|
||||||
func (b *Service) handleSlot(epoch, slotNum uint64, | ||||||
authorityIndex uint32, | ||||||
preRuntimeDigest *types.PreRuntimeDigest, | ||||||
) error { | ||||||
func (b *Service) getParentForBlockAuthoring(slotNum uint64) (*types.Header, error) { | ||||||
parentHeader, err := b.blockState.BestBlockHeader() | ||||||
if err != nil { | ||||||
return err | ||||||
return nil, fmt.Errorf("could not get best block header: %w", err) | ||||||
} | ||||||
|
||||||
if parentHeader == nil { | ||||||
return errNilParentHeader | ||||||
return nil, errNilParentHeader | ||||||
} | ||||||
|
||||||
atGenesisBlock := b.blockState.GenesisHash().Equal(parentHeader.Hash()) | ||||||
if !atGenesisBlock { | ||||||
bestBlockSlotNum, err := b.blockState.GetSlotForBlock(parentHeader.Hash()) | ||||||
Comment on lines
+468
to
+469
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit maybe move the content of that if block to a separate function? |
||||||
if err != nil { | ||||||
return nil, fmt.Errorf("could not get slot for block: %w", err) | ||||||
} | ||||||
|
||||||
if bestBlockSlotNum > slotNum { | ||||||
return nil, fmt.Errorf("%w: best block slot number is %d and got slot number %d", | ||||||
errLaggingSlot, bestBlockSlotNum, slotNum) | ||||||
} | ||||||
|
||||||
if bestBlockSlotNum == slotNum { | ||||||
// pick parent of best block instead to handle slot | ||||||
newParentHeader, err := b.blockState.GetHeader(parentHeader.ParentHash) | ||||||
if err != nil { | ||||||
return nil, fmt.Errorf("could not get header: %w", err) | ||||||
} | ||||||
if newParentHeader == nil { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit 🤔 can this ever happen? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It probably isn't supposed to happen, but I have seen it happen. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Uh should we create an issue for that? Sounds kinda strange we can't find the parent header right? |
||||||
return nil, fmt.Errorf("%w: for block hash %s", errNilParentHeader, parentHeader.ParentHash) | ||||||
} | ||||||
parentHeader = newParentHeader | ||||||
kishansagathiya marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
} | ||||||
} | ||||||
|
||||||
// there is a chance that the best block header may change in the course of building the block, | ||||||
// so let's copy it first. | ||||||
parent, err := parentHeader.DeepCopy() | ||||||
if err != nil { | ||||||
return err | ||||||
return nil, fmt.Errorf("could not create deep copy of parent header: %w", err) | ||||||
} | ||||||
|
||||||
return parent, nil | ||||||
} | ||||||
|
||||||
func (b *Service) handleSlot(epoch, slotNum uint64, | ||||||
authorityIndex uint32, | ||||||
preRuntimeDigest *types.PreRuntimeDigest, | ||||||
) error { | ||||||
currentSlot := Slot{ | ||||||
start: time.Now(), | ||||||
duration: b.constants.slotDuration, | ||||||
number: slotNum, | ||||||
} | ||||||
|
||||||
parent, err := b.getParentForBlockAuthoring(slotNum) | ||||||
if err != nil { | ||||||
return fmt.Errorf("could not get parent for claiming slot %d: %w", slotNum, err) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the
Suggested change
|
||||||
} | ||||||
b.storageState.Lock() | ||||||
defer b.storageState.Unlock() | ||||||
|
||||||
|
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.
please add a unit test for this function
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.
It gets tested
TestService_HandleSlotWithSameSlot
andTestService_HandleSlotWithLaggingSlot
.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.
These are integration tests, not unit tests. We want a unit test because:
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.
@qdm12 please remember this was a PR that was already approved before. The lack of existing unit tests in the
babe
package is known. I don't think it makes sense to add scope to this PR now by just unit testing this function, and nothandleSlot
as well.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.
I feel like new code added should be unit tested, but up to you if you feel it's not necessary. I wasn't referring to unit testing
handleSlot
, that's indeed out of scope.