Author a sibling block in case best block's slot is same as current slot by kishansagathiya · Pull Request #2827 · ChainSafe/gossamer

kishansagathiya · 2022-09-13T09:53:44Z

Changes

Copy of #2726 with the merge fix

There were files ending with _test.go which were not actually tests, but just things that were getting used in test files. I had to remove _test from them in order to use that helper code to be used in other packages.
createTestService in babe tests had some un-necessary conditions. Removed them. It also helps me with the test that I have written.
Remove some extra redundant code that created TransactionState
Used actual block import handler instead of a fake one.

Tests

go test -tags integration github.com/ChainSafe/gossamer

Issues

Primary Reviewer

@timwu20

the current slot, we should author a new block as child of best block's parent. This would create forks in the chain which would get resolved eventually.

that we are claiming at the moment

…lot (#2726)

- Split out (CGO related) helper functions from `imports.go` to `lib/runtime/wasmer/helpers.go` - Move pointer size helper functions to `lib/runtime/wasmer/helpers.go` - Change `toWasmMemorySized` to NOT take a size argument (unneeded) - Clarify all comments for helper functions - Update all error wrappings - Review variable names - Use `ptr` instead of `out` for 32 bit pointers - Use `pointerSize` instead of `span` for 64 bit pointers size - Name return values - Other minor renamings such as `res` to `result`, `enc` to `encodedResult` - Optimizations: - `storageAppend`: use slice capacity allocation, `copy` and remove unneeded `append`s - `toKillStorageResultEnum`: use `copy` instead of `append` - `toWasmMemoryOptional`: remove unneeded variable copy

…lopment

codecov · 2022-09-13T10:18:50Z

Codecov Report

Merging #2827 (bb1a02c) into development (37fda37) will increase coverage by 0.00%.
The diff coverage is 0.00%.

Additional details and impacted files

@@             Coverage Diff              @@
##           development    #2827   +/-   ##
============================================
  Coverage        63.37%   63.38%           
============================================
  Files              217      217           
  Lines            27062    27088   +26     
============================================
+ Hits             17150    17169   +19     
- Misses            8348     8353    +5     
- Partials          1564     1566    +2

…lopment

Co-authored-by: Quentin McGaw <quentin.mcgaw@gmail.com>

kishansagathiya · 2022-09-22T10:36:27Z

@EclesioMeloJunior

@kishansagathiya looking at substrate code I notice they have a method to report equivocations, is it related to this PR? should we implement it? https://github.com/paritytech/substrate/blob/c24431eb6a95197550b30715a4c124be1aea79de/client/consensus/babe/src/lib.rs#L1224

This is a really good find. I went through that piece of code and tried to figure out what it does. It appears to me that we should report equivocations. I have created an issue for that #2853. Take a look at it, let me know your thoughts.

…inSafe/gossamer into kishan/fix/slot-number-must-increase

EclesioMeloJunior

LGTM, just a small unneeded comment

qdm12 · 2022-09-22T13:27:53Z

+			if err != nil {
+				return fmt.Errorf("could not get header for hash %s: %w", parentHeader.ParentHash, err)
+			}
+			if newParentHeader == nil {


Uh should we create an issue for that? Sounds kinda strange we can't find the parent header right?

qdm12 · 2022-09-22T13:35:00Z


+	parent, err := b.getParentForBlockAuthoring(slotNum)
+	if err != nil {
+		return fmt.Errorf("could not get parent for claiming slot %d: %w", slotNum, err)


the slotNum argument should be bundled in the error context by getParentForBlockAuthoring, not here. Also shouldn't it be for block authoring instead of for claiming slot?

Suggested change

return fmt.Errorf("could not get parent for claiming slot %d: %w", slotNum, err)

return fmt.Errorf("could not get parent for block authoring: %w", err)

qdm12 · 2022-09-22T13:40:36Z

 )

+// NewTestService creates a new test core service
+func NewTestService(t *testing.T, cfg *core.Config) *core.Service {


Since this function is used in only one place in the babe package, please remove all the nil checks below and inject a fully configured cfg to NewTestService. All these nil checks are just confusing and prone to error/hard to debug. You might be able to remove unused config fields too eventually.

qdm12 · 2022-09-22T13:44:07Z

-	authorityIndex uint32,
-	preRuntimeDigest *types.PreRuntimeDigest,
-) error {
+func (b *Service) getParentForBlockAuthoring(slotNum uint64) (*types.Header, error) {


please add a unit test for this function

It gets tested TestService_HandleSlotWithSameSlot and TestService_HandleSlotWithLaggingSlot.

These are integration tests, not unit tests. We want a unit test because:

it should cover all code paths (unlike integ tests)

it should be fast

it's easier to spot a bug with a unit test than through an integ test

@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 not handleSlot as well.

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.

Co-authored-by: Eclésio Junior <eclesiomelo.1@gmail.com>

…inSafe/gossamer into kishan/fix/slot-number-must-increase

github-actions · 2022-11-23T18:25:41Z

🎉 This PR is included in version 0.7.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

kishansagathiya and others added 20 commits July 18, 2022 12:11

temp

5cd4263

temp

f57fa84

Merge branch 'development' into kishan/fix/slot-number-must-increase

ca4724f

temp

12fde0b

While authoring a new block, if we find that best block was authored in

ae2fa69

the current slot, we should author a new block as child of best block's parent. This would create forks in the chain which would get resolved eventually.

clean up

cc2ffaf

add a check for current slot is greater than slot in best block

cbc44d2

test for err slot lagging

3de132f

test

a63a073

fix test for errLaggingSlot

aca3b68

test that we create a fork if best block is authored in the same slot

9c9f47b

that we are claiming at the moment

temp

527f515

fixed tests

cecc308

Merge branch 'development' into kishan/fix/slot-number-must-increase

d022f43

Author a sibling block in case best block's slot is same as current s…

540c09a

…lot (#2726)

chore(dot): remove unneeded error functions (#2790)

1edb86b

Merge branch 'development' of github.com:ChainSafe/gossamer into deve…

64e3c03

…lopment

Merge branch 'development' into kishan/fix/slot-number-must-increase

db03cbb

merge fix

95a7a35

kishansagathiya added 9 commits September 14, 2022 13:20

Merge branch 'development' of github.com:ChainSafe/gossamer into deve…

220f5ae

…lopment

Merge branch 'development' into kishan/fix/slot-number-must-increase

37fa232

intermediate

11471e8

it probably fixes the test

e0109dc

Merge branch 'development' into kishan/fix/slot-number-must-increase

93b3057

Use a real block import handler

fdb1fd2

more fix

388678a

improved createTestService

0862104

Merge branch 'development' into kishan/fix/slot-number-must-increase

c99b776

kishansagathiya and others added 7 commits September 21, 2022 16:05

Update lib/babe/babe.go

8155d43

Co-authored-by: Quentin McGaw <quentin.mcgaw@gmail.com>

Update lib/babe/babe.go

d0b2a1b

Co-authored-by: Quentin McGaw <quentin.mcgaw@gmail.com>

Update lib/babe/babe_integration_test.go

03db54d

Co-authored-by: Quentin McGaw <quentin.mcgaw@gmail.com>

Update lib/babe/babe_integration_test.go

45b5fd3

Co-authored-by: Quentin McGaw <quentin.mcgaw@gmail.com>

Update lib/babe/errors.go

a71de07

Co-authored-by: Quentin McGaw <quentin.mcgaw@gmail.com>

fix

37b41ca

split handleSlot function

532c617

kishansagathiya requested a review from qdm12 September 21, 2022 13:15

kishansagathiya and others added 2 commits September 21, 2022 23:20

Update lib/babe/babe_integration_test.go

f56e154

Co-authored-by: Quentin McGaw <quentin.mcgaw@gmail.com>

Update lib/babe/babe_integration_test.go

849ad82

Co-authored-by: Quentin McGaw <quentin.mcgaw@gmail.com>

kishansagathiya mentioned this pull request Sep 22, 2022

Submit and handle equivocation report #2853

Closed

2 tasks

kishansagathiya added 3 commits September 22, 2022 18:31

separate mock interface

9ba1b58

Merge branch 'kishan/fix/slot-number-must-increase' of github.com:Cha…

8dbc936

…inSafe/gossamer into kishan/fix/slot-number-must-increase

some clean up

db736ba

EclesioMeloJunior approved these changes Sep 22, 2022

View reviewed changes

Comment thread lib/babe/babe_integration_test.go Outdated

qdm12 reviewed Sep 22, 2022

View reviewed changes

Update lib/babe/babe_integration_test.go

3366ecc

Co-authored-by: Eclésio Junior <eclesiomelo.1@gmail.com>

timwu20 approved these changes Sep 22, 2022

View reviewed changes

kishansagathiya added 2 commits September 26, 2022 16:52

added mock generate file

65f649e

Merge branch 'kishan/fix/slot-number-must-increase' of github.com:Cha…

e667450

…inSafe/gossamer into kishan/fix/slot-number-must-increase

kishansagathiya commented Sep 26, 2022

View reviewed changes

Comment thread lib/babe/mocks_generate_test.go Outdated

fixed the mock generation command

06313f1

kishansagathiya requested a review from qdm12 September 26, 2022 14:22

qdm12 approved these changes Sep 26, 2022

View reviewed changes

Merge branch 'development' into kishan/fix/slot-number-must-increase

bb1a02c

kishansagathiya merged commit 1262e73 into development Sep 26, 2022

kishansagathiya deleted the kishan/fix/slot-number-must-increase branch September 26, 2022 15:56

kishansagathiya mentioned this pull request Sep 30, 2022

Member Request polkadot-fellows/seeding#31

Merged

	return fmt.Errorf("could not get parent for claiming slot %d: %w", slotNum, err)
	return fmt.Errorf("could not get parent for block authoring: %w", err)

Conversation

kishansagathiya commented Sep 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Tests

Issues

Primary Reviewer

Uh oh!

codecov Bot commented Sep 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kishansagathiya commented Sep 22, 2022

Uh oh!

EclesioMeloJunior left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

qdm12 Sep 22, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

qdm12 Sep 22, 2022

Choose a reason for hiding this comment

Uh oh!

qdm12 Sep 22, 2022

Choose a reason for hiding this comment

Uh oh!

qdm12 Sep 22, 2022

Choose a reason for hiding this comment

Uh oh!

kishansagathiya Sep 22, 2022

Choose a reason for hiding this comment

Uh oh!

qdm12 Sep 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timwu20 Sep 22, 2022

Choose a reason for hiding this comment

Uh oh!

qdm12 Sep 26, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Nov 23, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kishansagathiya commented Sep 13, 2022 •

edited

Loading

codecov Bot commented Sep 13, 2022 •

edited

Loading

qdm12 Sep 22, 2022 •

edited

Loading