Pull Genesis parsing out of state #2284

abi87 · 2023-11-09T18:11:16Z

Why this should be merged

Building setup for unit test in platformVm is cumbersome. Once of the reasons is that we use genesisBytes to initialize the platformVM state. This forces us to build genesis and serialize it in UTs.
This PR drop the bytes and pass a genesisState data structure, thus simplifying the UTs context. This PR kills 130 LOC without giving up any functionality.
Thanks @joshua-kim and @marun for the discussions that brought me to these changes.

How this works

Let state.State accept a genesis state instead of its bytes, moving the unmarshalling to the client

How this was tested

CI

abi87 · 2023-11-09T18:41:01Z

vms/platformvm/vm.go

 	vm.state, err = state.New(
 		vm.db,
-		genesisBytes,
+		g,


by pulling the byte parsing out of state and passing a well defined object instead, we open the way to simplify state testing. State is surprisingly not well tests (with a few notable exceptions), and part of the reason is the complex building of genesis state. This PR solves this problem

abi87 · 2023-11-09T18:41:46Z

vms/platformvm/validators/manager_benchmark_test.go

-	avaxAssetID := ids.GenerateTestID()
-	genesisTime := time.Now().Truncate(time.Second)
-	genesisEndTime := genesisTime.Add(28 * 24 * time.Hour)
-
-	addr, err := address.FormatBech32(constants.UnitTestHRP, ids.GenerateTestShortID().Bytes())
-	require.NoError(err)
-
-	genesisValidators := []api.GenesisPermissionlessValidator{{
-		GenesisValidator: api.GenesisValidator{
-			StartTime: json.Uint64(genesisTime.Unix()),
-			EndTime:   json.Uint64(genesisEndTime.Unix()),
-			NodeID:    ids.GenerateTestNodeID(),
-		},
-		RewardOwner: &api.Owner{
-			Threshold: 1,
-			Addresses: []string{addr},
-		},
-		Staked: []api.UTXO{{
-			Amount:  json.Uint64(2 * units.KiloAvax),
-			Address: addr,
-		}},
-		DelegationFee: reward.PercentDenominator,
-	}}
-
-	buildGenesisArgs := api.BuildGenesisArgs{
-		NetworkID:     json.Uint32(constants.UnitTestID),
-		AvaxAssetID:   avaxAssetID,
-		UTXOs:         nil,
-		Validators:    genesisValidators,
-		Chains:        nil,
-		Time:          json.Uint64(genesisTime.Unix()),
-		InitialSupply: json.Uint64(360 * units.MegaAvax),
-		Encoding:      formatting.Hex,
-	}
-
-	buildGenesisResponse := api.BuildGenesisReply{}
-	platformvmSS := api.StaticService{}
-	require.NoError(platformvmSS.BuildGenesis(nil, &buildGenesisArgs, &buildGenesisResponse))
-
-	genesisBytes, err := formatting.Decode(buildGenesisResponse.Encoding, buildGenesisResponse.Bytes)
-	require.NoError(err)


just moved to buildGenesisTest + dropped the formatting nonsense.

abi87 · 2023-11-09T18:42:52Z

vms/platformvm/txs/executor/helpers_test.go

-func buildGenesisTest(ctx *snow.Context) []byte {
-	genesisUTXOs := make([]api.UTXO, len(preFundedKeys))
+func buildGenesisTest(ctx *snow.Context) *genesis.Genesis {
+	genesisUtxos := make([]*genesis.UTXO, len(preFundedKeys))


these lines are basically what platformvmSS.BuildGenesis used to do. Pulled up here and in other helpers_test.go files. In a following PR a drop the code duplication and centralize genesis creation in UTs

abi87 · 2023-11-09T18:43:49Z

vms/platformvm/genesis/genesis.go

@@ -22,6 +24,8 @@ type Genesis struct {
 	Timestamp     uint64    `serialize:"true"`
 	InitialSupply uint64    `serialize:"true"`
 	Message       string    `serialize:"true"`
+
+	GenesisID ids.ID `serialize:"false"`


we're changing a genesis related object but as long as changes are not serialized it's fine

Kinda want to see if we can remove this field, wrote a comment in state related to this

abi87 · 2023-11-09T18:44:06Z

vms/platformvm/genesis/genesis.go

@@ -39,5 +43,7 @@ func Parse(genesisBytes []byte) (*Genesis, error) {
 			return nil, err
 		}
 	}
+
+	gen.GenesisID = hashing.ComputeHash256Array(genesisBytes)


this is what we used to do in state package to derive genesisID. Just moved here

vms/platformvm/block/builder/helpers_test.go

vms/platformvm/block/executor/helpers_test.go

vms/platformvm/block/builder/helpers_test.go

joshua-kim · 2023-11-21T20:04:29Z

vms/platformvm/state/state.go

 	// Create the genesis block and save it as being accepted (We don't do
 	// genesisBlock.Accept() because then it'd look for genesisBlock's
 	// non-existent parent)
-	genesisID := hashing.ComputeHash256Array(genesisBytes)
-	genesisBlock, err := block.NewApricotCommitBlock(genesisID, 0 /*height*/)
+	genesisBlock, err := block.NewApricotCommitBlock(g.GenesisID, 0 /*height*/)


Instead of having genesis id as a field attached to Genesis, I think it would be cleaner to just hash the genesis file here and have the caller pass in the id. This way we don't have to have the ID field in Genesis which is only ever used here anyways. I think the ID is more of a property of the genesis block than the genesis data itself

@joshua-kim I don't really have a file to hash here, I have a byte slice. I understand your point about the ID being a block property.
I have replace the GenesisID attribute with GenesisBytes one and hashed it to get the genesis block ID wherever needed.
LMK if this is acceptable

joshua-kim · 2023-11-21T20:08:10Z

vms/platformvm/block/builder/helpers_test.go

-				EndTime:   json.Uint64(defaultValidateEndTime.Unix()),
-				NodeID:    nodeID,
+	vdrs := txheap.NewByEndTime()
+	for idx, nodeID := range genesisNodeIDs {


We initialize genesisNodeIDs in init(), so I think it would make sense for us to also initialize them here as well. Maybe we can move that blob of code down here?

But then what if we write a test that does not really use the environment? The genesisNodeIDs content would change depending on the order tests are executed right?
This is the reason I'd keep them in the init,

joshua-kim · 2023-11-21T20:09:48Z

vms/platformvm/block/executor/helpers_test.go

@@ -395,67 +394,81 @@ func defaultFx(clk *mockable.Clock, log logging.Logger, isBootstrapped bool) fx.
 	return res
 }

-func buildGenesisTest(ctx *snow.Context) []byte {
-	genesisUTXOs := make([]api.UTXO, len(preFundedKeys))
+func buildGenesisTest(ctx *snow.Context) *genesis.Genesis {


I actually didn't reailze we have a lot of genesis helpers to create pre-funded genesis. Wondering if it makes sense for us to consolidate this into a single place somewhere and export it out of a package. It could be something like:

package somewhere func NewPrefundedGenesis(keys ...*secp2561.PrivateKey) (*TestGenesis, error) { ... } type *TestGenesis struct { *Genesis Keys []*secp256k1.PrivateKey }

I'm not sure what package i'd put his in. Maybe we can make a package like genesis/test or put this in platformvm somewhere.

This is what I do in #2107 for genesis helpers and subsequent PRs for other test helpers.
Sorry we got the code duplication here, just trying to segment changes limit PRs scope

Also historically, duplication arised when I repackaged platformvm but the way test parameters were set were slightly different across packaged. #2107 and subsequent PR remove duplicated code and fixes test to make them work with the default values and not ony with the specific values used in different packages.

dhrubabasu

I believe our current model for VMs is they are initialized with genesis bytes: https://docs.avax.network/build/vm/create/golang-vm-complex#virtual-machine-methods

This feels like a step in the wrong direction. I think to simplify testing, we could instead have a defaultGenesisBytes as a UT constant.

marun · 2023-12-18T19:02:33Z

Why does the configuration for testing have to mirror non-test scenarios?

It's not like the temporary networks we use for e2e testing mirror the deployment semantics for fuji or mainnet.

dhrubabasu · 2023-12-18T19:06:03Z

Why does the configuration for testing have to mirror non-test scenarios?

This PR modifies non-test files (specifically, adjusting the state struct to take in a genesis struct instead of bytes)

abi87 · 2023-12-19T09:54:31Z

I believe our current model for VMs is they are initialized with genesis bytes: https://docs.avax.network/build/vm/create/golang-vm-complex#virtual-machine-methods

This feels like a step in the wrong direction. I think to simplify testing, we could instead have a defaultGenesisBytes as a UT constant.

@dhrubabasu I am not changing the VM interface. Rather I am moving the byte parsing out of the state package.
I think right now the state package is under tested. One of the reasons is that is pretty curmbersome to create ad-hoc genesis content for tests. I want to write test genesis with no validators for instance, which are only useful in tests and while I can to it now, I have to go through the whole byte formattin shenanigans. Having a simpler struct where I directly specify the genesis content would make for much smaller tests.
Also defaultGenesisBytes as a UT constant would not offer enough flexibility for testing IMO

dhrubabasu · 2023-12-19T15:27:32Z

I think we can create a helper function that returns the genesisBytes. We shouldn't need to modify the state struct to create a test genesis with no validators. For example, this code works and uses the static service we use in defaultGenesis now. I feel abstracting this into a genesistest pkg helper would consolidate a lot of the code without modifying non-test code.

func main() {
	buildGenesisArgs := api.BuildGenesisArgs{
		Encoding:      formatting.Hex,
		NetworkID:     json.Uint32(constants.UnitTestID),
		AvaxAssetID:   ids.GenerateTestID(),
		UTXOs:         []api.UTXO{}, // No UTXOs
		Validators:    []api.GenesisPermissionlessValidator{}, // No Validators
		Chains:        nil,
		Time:          json.Uint64(time.Date(1997, 1, 1, 0, 0, 0, 0, time.UTC).Unix()),
		InitialSupply: json.Uint64(360 * units.MegaAvax),
	}

	buildGenesisResponse := api.BuildGenesisReply{}
	platformvmSS := api.StaticService{}
	err := platformvmSS.BuildGenesis(nil, &buildGenesisArgs, &buildGenesisResponse)
	if err != nil {
		panic(err)
	}

	genesisBytes, err := formatting.Decode(buildGenesisResponse.Encoding, buildGenesisResponse.Bytes)
	if err != nil {
		panic(err)
	}

	fmt.Println(genesisBytes)
}

[0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 50 201 169 0 4 254 250 23 183 36 0 0 0 0]

github-actions · 2024-03-03T00:00:42Z

This PR has become stale because it has been open for 30 days with no activity. Adding the lifecycle/frozen label will cause this PR to ignore lifecycle events.

cleaned up state ctor

2d2eac2

abi87 requested review from danlaine and StephenButtolph as code owners November 9, 2023 18:11

abi87 self-assigned this Nov 9, 2023

Merge branch 'dev' into pchain_state_ctor_cleanup

f599f94

abi87 requested a review from dhrubabasu November 9, 2023 18:24

abi87 commented Nov 9, 2023

View reviewed changes

Merge branch 'dev' into pchain_state_ctor_cleanup

47f3496

marun approved these changes Nov 12, 2023

View reviewed changes

abi87 added 8 commits November 13, 2023 03:02

Merge branch 'dev' into pchain_state_ctor_cleanup

3da37dc

nit

317cb83

Merge branch 'dev' into pchain_state_ctor_cleanup

d929504

Merge branch 'dev' into pchain_state_ctor_cleanup

37f2130

Merge branch 'dev' into pchain_state_ctor_cleanup

3eba6b7

Merge branch 'dev' into pchain_state_ctor_cleanup

b61fa88

Merge branch 'dev' into pchain_state_ctor_cleanup

b5dd88a

Merge branch 'dev' into pchain_state_ctor_cleanup

18b101f

joshua-kim reviewed Nov 21, 2023

View reviewed changes

abi87 added 2 commits November 22, 2023 01:21

Merge branch 'dev' into pchain_state_ctor_cleanup

1d8437d

replaced GenesisID with GenesisBytes attribute

e4c3328

abi87 requested a review from joshua-kim November 22, 2023 14:09

abi87 added 3 commits November 23, 2023 01:23

Merge branch 'dev' into pchain_state_ctor_cleanup

dfe422c

Merge branch 'dev' into pchain_state_ctor_cleanup

0bc87ac

Merge branch 'dev' into pchain_state_ctor_cleanup

64bfb9f

abi87 added 7 commits December 3, 2023 05:08

Merge branch 'dev' into pchain_state_ctor_cleanup

19a9fd5

Merge branch 'dev' into pchain_state_ctor_cleanup

80ad266

Merge branch 'dev' into pchain_state_ctor_cleanup

e0ab6c8

Merge branch 'dev' into pchain_state_ctor_cleanup

2a4ba56

Merge branch 'dev' into pchain_state_ctor_cleanup

c3fef3a

Merge branch 'dev' into pchain_state_ctor_cleanup

bc0b84f

Merge branch 'dev' into pchain_state_ctor_cleanup

9efc31d

abi87 linked an issue Dec 9, 2023 that may be closed by this pull request

Make P-chain UTs maintanable #2453

Open

abi87 added 5 commits December 12, 2023 07:26

Merge branch 'dev' into pchain_state_ctor_cleanup

1362114

Merge branch 'dev' into pchain_state_ctor_cleanup

b9c227a

Merge branch 'dev' into pchain_state_ctor_cleanup

5db63ac

Merge branch 'dev' into pchain_state_ctor_cleanup

2bbe45a

Merge branch 'dev' into pchain_state_ctor_cleanup

3e1890c

dhrubabasu reviewed Dec 18, 2023

View reviewed changes

Merge branch 'dev' into pchain_state_ctor_cleanup

e2aaa5e

abi87 added 2 commits January 2, 2024 16:59

Merge branch 'dev' into pchain_state_ctor_cleanup

ab1cd0b

Merge branch 'dev' into pchain_state_ctor_cleanup

f87a7e4

abi87 changed the title ~~P-chain UTs consolidation 0 - Pulled Genesis parsing out of state~~ Pull Genesis parsing out of state Jan 2, 2024

abi87 added 5 commits January 4, 2024 01:20

Merge branch 'dev' into pchain_state_ctor_cleanup

19cf59d

Merge branch 'dev' into pchain_state_ctor_cleanup

afc5b39

Merge branch 'dev' into pchain_state_ctor_cleanup

b30633d

Merge branch 'dev' into pchain_state_ctor_cleanup

f56646b

Merge branch 'dev' into pchain_state_ctor_cleanup

267dfa4

github-actions bot added the lifecycle/stale label Mar 3, 2024

aaronbuchwald closed this Dec 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pull Genesis parsing out of state #2284

Pull Genesis parsing out of state #2284

abi87 commented Nov 9, 2023

abi87 Nov 9, 2023 •

edited

Loading

abi87 Nov 9, 2023

abi87 Nov 9, 2023

abi87 Nov 9, 2023

joshua-kim Nov 21, 2023

abi87 Nov 9, 2023

joshua-kim Nov 21, 2023 •

edited

Loading

abi87 Nov 22, 2023 •

edited

Loading

joshua-kim Nov 21, 2023

abi87 Nov 22, 2023

joshua-kim Nov 21, 2023 •

edited

Loading

abi87 Nov 22, 2023

abi87 Nov 22, 2023

dhrubabasu left a comment •

edited

Loading

marun commented Dec 18, 2023 •

edited

Loading

dhrubabasu commented Dec 18, 2023

abi87 commented Dec 19, 2023 •

edited

Loading

dhrubabasu commented Dec 19, 2023 •

edited

Loading

github-actions bot commented Mar 3, 2024

Pull Genesis parsing out of state #2284

Pull Genesis parsing out of state #2284

Conversation

abi87 commented Nov 9, 2023

Why this should be merged

How this works

How this was tested

abi87 Nov 9, 2023 • edited Loading

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

joshua-kim Nov 21, 2023 • edited Loading

Choose a reason for hiding this comment

abi87 Nov 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshua-kim Nov 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhrubabasu left a comment • edited Loading

Choose a reason for hiding this comment

marun commented Dec 18, 2023 • edited Loading

dhrubabasu commented Dec 18, 2023

abi87 commented Dec 19, 2023 • edited Loading

dhrubabasu commented Dec 19, 2023 • edited Loading

github-actions bot commented Mar 3, 2024

abi87 Nov 9, 2023 •

edited

Loading

joshua-kim Nov 21, 2023 •

edited

Loading

abi87 Nov 22, 2023 •

edited

Loading

joshua-kim Nov 21, 2023 •

edited

Loading

dhrubabasu left a comment •

edited

Loading

marun commented Dec 18, 2023 •

edited

Loading

abi87 commented Dec 19, 2023 •

edited

Loading

dhrubabasu commented Dec 19, 2023 •

edited

Loading