feat(prospective-parachains): Implement `FragmentChain` #4337

EclesioMeloJunior · 2024-11-21T22:13:18Z

Changes

Implements Fragment Chain a data structure used by prospective-parachains subsystems in order to keep the best backable chain of parachain blocks
The goal is to always choose the best candidate to back on-chain

Tests

Tests coverage from Polkadot-SDK

go test -timeout 10m github.com/ChainSafe/gossamer/dot/parachain/prospective-parachains/fragment-chain

Issues

Closes feat(prospective-parachains) implement Fragment Chain #4314

- Added: Candidate Entry - (inclusion_emulator): Added ProspectiveCandidate, Modification error and Fragment Validity error

haikoschol

just some types, nits & questions

dot/parachain/util/.DS_Store

dot/parachain/prospective-parachains/fragment-chain/errors.go

dot/parachain/prospective-parachains/fragment-chain/fragment_chain.go

dot/parachain/types/async_backing.go

dot/parachain/util/inclusion-emulator/inclusion_emulator.go

dot/parachain/prospective-parachains/fragment-chain/fragment_chain.go

dot/parachain/types/types.go

dot/parachain/util/inclusion-emulator/inclusion_emulator.go

dot/parachain/prospective-parachains/fragment-chain/fragment_chain.go

axaysagathiya

I found a lot of unused/unnecessary methods. Can you please remove unused/unnecessary methods?
Also if you need some logic only in test please consider creating test helper function.

dot/parachain/prospective-parachains/fragment-chain/fragment_chain.go

jimjbrettj

lgtm!

dot/parachain/prospective-parachains/fragment-chain/fragment_chain.go

dot/parachain/types/async_backing.go

…ssamer into eclesio/fragment-chain-impl

axaysagathiya · 2024-12-12T14:32:51Z

dot/parachain/types/async_backing.go

@@ -17,3 +22,90 @@ type AsyncBackingParams struct {
 	// When async backing is disabled, the only valid value is 0.
 	AllowedAncestryLen uint32 `scale:"2"`
 }
+
+// InboundHrmpLimitations constraints on inbound HRMP channels.
+type InboundHrmpLimitations struct {


Do we use this and the below code in any subsystem other than the prospective parachain subsystem?

Currently we only use in prospective-parachains pkg, however since this types are a representation of what the runtime uses to validate the constraints I think placing them here is the better approach, given that they are in the same level as CandidateHash, PersistedValidationData

In this package, we keep all the types that are used in multiple subsystems.

CandidateHashm and PersistedValidationData are the types we use in multiple subsystems.
As I can see In Rust implementation, the InboundHrmpLimitations type is only being used in prospective parachain.
Please move this to prospective parachain package

dot/parachain/types/types.go

axaysagathiya · 2024-12-12T14:36:23Z

dot/parachain/types/types.go

+
+// UpgradeRestriction a possible restriction that prevents a parachain
+// from performing an upgrade
+// TODO: should be scale encoded/decoded


If it has already been addressed, can we remove this to-do comment?
Do we need this type in multiple subsystems?

the type UpgradeRestriction and its variant Present is used to validate the candidate, this validation happens only in prospective-parachains subsystem, this is part of Constraints type and because of that it should be in the types pkg

No,
In this package, we keep all the types used in multiple subsystems.
We will have to move this type, Constraints, to its correct package where it is used.

If you find any other types that are only needed in prospective parachain but have placed them in the types package, please move them to the prospective parachain package.

dot/parachain/prospective-parachains/errors.go

axaysagathiya · 2024-12-13T14:35:34Z

dot/parachain/types/async_backing.go

+// Constraints on the actions that can be taken by a new parachain block. These
+// limitations are implicitly associated with some particular parachain, which should
+// be apparent from usage.
+type Constraints struct {


Let's move this type to the inclusion simulator file.

This package contains types that we need in multiple subsystems.
Because we will never use this in other subsystems, we only use this in prospective parachain.

Move other types you added to this file that are not used in multiple subsystems.

axaysagathiya · 2024-12-13T14:39:05Z

dot/parachain/types/async_backing.go

@@ -17,3 +22,90 @@ type AsyncBackingParams struct {
 	// When async backing is disabled, the only valid value is 0.
 	AllowedAncestryLen uint32 `scale:"2"`
 }
+
+// InboundHrmpLimitations constraints on inbound HRMP channels.
+type InboundHrmpLimitations struct {


In this package, we keep all the types that are used in multiple subsystems.

CandidateHashm and PersistedValidationData are the types we use in multiple subsystems.
As I can see In Rust implementation, the InboundHrmpLimitations type is only being used in prospective parachain.
Please move this to prospective parachain package

axaysagathiya · 2024-12-13T14:49:45Z

dot/parachain/types/types.go

+
+// UpgradeRestriction a possible restriction that prevents a parachain
+// from performing an upgrade
+// TODO: should be scale encoded/decoded


No,
In this package, we keep all the types used in multiple subsystems.
We will have to move this type, Constraints, to its correct package where it is used.

If you find any other types that are only needed in prospective parachain but have placed them in the types package, please move them to the prospective parachain package.

dot/parachain/prospective-parachains/errors.go

axaysagathiya

Some minor changes.
You can merge after addressing the comments.

dot/parachain/prospective-parachains/fragment_chain.go

axaysagathiya · 2024-12-13T15:40:54Z

dot/parachain/types/types.go

+
+// UpgradeRestriction a possible restriction that prevents a parachain
+// from performing an upgrade
+type UpgradeRestriction struct {


Write the unit test to ensure we are getting the correct encoding/decoding.

dot/parachain/prospective-parachains/fragment_chain.go

EclesioMeloJunior added 10 commits November 11, 2024 20:57

feat: starting the fragment chain implementation

afa3e68

- Added: Candidate Entry - (inclusion_emulator): Added ProspectiveCandidate, Modification error and Fragment Validity error

feat: implemented CandidateStorage

404f5fe

feat: create Scope, working on inclusionemulator.Fragment

6648464

wip: implementing validateAgainstConstraints

5119649

feat: implement BackedChain

e072160

wip: FragmentChain struct implementation

0de059a

feat: full FragmentChain implementation done

0388b02

chore: wip fragment chain tests

1b796b8

chore: TestPopulateAndCheckPotential done

f11ee4f

feat: bring polkadot-sdk test coverage to fragment chain

6a001c8

EclesioMeloJunior requested review from jimjbrettj, timwu20, kishansagathiya and P1sar as code owners November 21, 2024 22:13

haikoschol approved these changes Nov 25, 2024

View reviewed changes

EclesioMeloJunior added 3 commits November 26, 2024 18:44

chore: remove dsstore

6b30bc6

chore: change loopt

5f01ccb

Merge branch 'feat/parachain' into eclesio/fragment-chain-impl

987c880

axaysagathiya reviewed Dec 3, 2024

View reviewed changes

chore: fix loop conditional

5abd1e3

This was referenced Dec 3, 2024

feat(prospective-parachains): handle GetMinimumRelayParents signal #4363

Open

feat(prospective-parachains): Implement HandleIntroduceSecondedCandidate #4350

Open

jimjbrettj reviewed Dec 3, 2024

View reviewed changes

dot/parachain/types/types.go Outdated Show resolved Hide resolved

haikoschol reviewed Dec 4, 2024

View reviewed changes

dot/parachain/util/inclusion-emulator/inclusion_emulator.go Outdated Show resolved Hide resolved

dot/parachain/util/inclusion-emulator/inclusion_emulator.go Outdated Show resolved Hide resolved

dot/parachain/util/inclusion-emulator/inclusion_emulator.go Outdated Show resolved Hide resolved

jimjbrettj reviewed Dec 4, 2024

View reviewed changes

dot/parachain/util/inclusion-emulator/inclusion_emulator.go Outdated Show resolved Hide resolved

jimjbrettj reviewed Dec 4, 2024

View reviewed changes

dot/parachain/prospective-parachains/fragment-chain/fragment_chain.go Outdated Show resolved Hide resolved

jimjbrettj reviewed Dec 4, 2024

View reviewed changes

dot/parachain/prospective-parachains/fragment-chain/fragment_chain.go Outdated Show resolved Hide resolved

jimjbrettj reviewed Dec 4, 2024

View reviewed changes

dot/parachain/prospective-parachains/fragment-chain/fragment_chain.go Outdated Show resolved Hide resolved

jimjbrettj reviewed Dec 4, 2024

View reviewed changes

dot/parachain/prospective-parachains/fragment-chain/fragment_chain.go Outdated Show resolved Hide resolved

chore: address comment

54e0adb

EclesioMeloJunior added 5 commits December 9, 2024 13:48

chore: fix TestScopeOnlyTakesAncestorsUpToMin test

041f161

chore: loop until maxDepth + 1

082bb9c

Merge branch 'feat/parachain' into eclesio/fragment-chain-impl

153b7d6

chore: gofmt

7d716c7

chore: address lint warns

1dfeb76

axaysagathiya requested changes Dec 10, 2024

View reviewed changes

chore: removed Unconnected method

ba8ede8

EclesioMeloJunior requested review from axaysagathiya and jimjbrettj December 10, 2024 13:37

chore: remove candidates method

ef35ba9

jimjbrettj approved these changes Dec 10, 2024

View reviewed changes

EclesioMeloJunior added 2 commits December 10, 2024 12:08

chore: added logs

6e28164

Merge branch 'feat/parachain' into eclesio/fragment-chain-impl

b65841c

DanielDDHM approved these changes Dec 10, 2024

View reviewed changes

axaysagathiya reviewed Dec 11, 2024

View reviewed changes

axaysagathiya reviewed Dec 12, 2024

View reviewed changes

dot/parachain/prospective-parachains/fragment-chain/fragment_chain.go Outdated Show resolved Hide resolved

dot/parachain/types/async_backing.go Show resolved Hide resolved

EclesioMeloJunior added 5 commits December 12, 2024 10:22

chore: address comments

30062cf

Merge branch 'eclesio/fragment-chain-impl' of github.com:ChainSafe/go…

8d80d2d

…ssamer into eclesio/fragment-chain-impl

Merge branch 'feat/parachain' into eclesio/fragment-chain-impl

e484624

chore: fix misspelling

42a7345

Merge branch 'eclesio/fragment-chain-impl' of github.com:ChainSafe/go…

87e096c

…ssamer into eclesio/fragment-chain-impl

axaysagathiya reviewed Dec 12, 2024

View reviewed changes

chore: make types unexported

71085e7

EclesioMeloJunior requested a review from axaysagathiya December 12, 2024 17:53

Trigger Build

da18a0b

axaysagathiya reviewed Dec 13, 2024

View reviewed changes

axaysagathiya approved these changes Dec 13, 2024

View reviewed changes

EclesioMeloJunior added 2 commits December 14, 2024 14:44

chore: addressing comments

d16b056

chore: small comments

e5d8019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(prospective-parachains): Implement `FragmentChain` #4337

feat(prospective-parachains): Implement `FragmentChain` #4337

EclesioMeloJunior commented Nov 21, 2024

haikoschol left a comment

axaysagathiya left a comment •

edited

Loading

jimjbrettj left a comment

axaysagathiya Dec 12, 2024

EclesioMeloJunior Dec 12, 2024

axaysagathiya Dec 13, 2024

axaysagathiya Dec 12, 2024

EclesioMeloJunior Dec 12, 2024

axaysagathiya Dec 13, 2024

axaysagathiya Dec 13, 2024

axaysagathiya Dec 13, 2024

axaysagathiya Dec 13, 2024

axaysagathiya left a comment

axaysagathiya Dec 13, 2024

feat(prospective-parachains): Implement FragmentChain #4337

Are you sure you want to change the base?

feat(prospective-parachains): Implement FragmentChain #4337

Conversation

EclesioMeloJunior commented Nov 21, 2024

Changes

Tests

Issues

haikoschol left a comment

Choose a reason for hiding this comment

axaysagathiya left a comment • edited Loading

Choose a reason for hiding this comment

jimjbrettj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axaysagathiya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat(prospective-parachains): Implement `FragmentChain` #4337

feat(prospective-parachains): Implement `FragmentChain` #4337

axaysagathiya left a comment •

edited

Loading