dsmr: replace hard coded validators with ChainState #1894

tsachiherman · 2025-01-27T20:14:24Z

What ?

This PR replaces the hard coded validators list and few more parameters that were previously hard-coded into the node with a dynamic ChainState interface.

The node test is currently have an implementation for that interface, which is used during testing.

Node creation

The node constructor which used to look like this:

func New[T Tx](
	log logging.Logger,
	nodeID ids.NodeID,
	networkID uint32,
	chainID ids.ID,
	pk *bls.PublicKey,
	signer warp.Signer,
	chunkStorage *ChunkStorage[T],
	getChunkClient *p2p.Client,
	getChunkSignatureClient *p2p.Client,
	chunkCertificateGossipClient *p2p.Client,
	validators []Validator, // TODO remove hard-coded validator set
	lastAccepted Block,
	quorumNum uint64,
	quorumDen uint64,
	timeValidityWindow TimeValidityWindow[*emapChunkCertificate],
	validityWindowDuration time.Duration,
)

looks now like that:

func New[T Tx](
	log logging.Logger,
	nodeID ids.NodeID,
	chainState ChainState,
	pk *bls.PublicKey,
	signer warp.Signer,
	chunkStorage *ChunkStorage[T],
	getChunkClient *p2p.Client,
	getChunkSignatureClient *p2p.Client,
	chunkCertificateGossipClient *p2p.Client,
	lastAccepted Block,
	timeValidityWindow TimeValidityWindow[*emapChunkCertificate],
	validityWindowDuration time.Duration,
)

and the ChainState is defined as :

type ChainState interface {
	GetNetworkParams() (networdID uint32, subnetID, chainID ids.ID)
	GetSignatureParams(ctx context.Context) (validators warp.CanonicalValidatorSet, quorumNum uint64, quorumDen uint64, err error)
	GetValidatorSet(ctx context.Context) (map[ids.NodeID]*snowValidators.GetValidatorOutput, error)
}

…va-labs/hypersdk into tsachi/configurable-chunk-rate-limiter

…ava-labs/hypersdk into tsachi/configurable-chunk-rate-limiter2

aaronbuchwald · 2025-01-29T20:43:30Z

tests/e2e/e2e.go

+			// Tsachi : The following line was commented out, as it's no longer supported by avalanchego.
+			// require.NoError(syncNode.SaveAPIPort())


Could we remove this comment as it's not relevant after this change?

For comments like this, could we make them on the diff in the PR as opposed to putting them in the code itself where they will go out of date once they're merged?

We can remove the TODO above this as well since it was documenting that we should be able to get rid of this call.

I was trying to figure out if this is no longer needed. Can you take a look https://github.com/ava-labs/avalanchego/pull/3650/files and let me know what you think ?

Hmm it looks like with the new behavior the URI to access the node may change, so this does not look correct to me.

In the test above, we have saved the syncNodeURI and added it to uris, which we use throughout the tests below, so if that URI is not guaranteed to be reachable after the restart we may fail.

If AvalancheGo deprecated support for this feature, then we'll need to make sure to either switch away from saving them to uris and syncNodeURI and creating them on demand from the updated node instance or update those variables in place after the restart.

This seems to explain the failing e2e sync test: https://github.com/ava-labs/hypersdk/actions/runs/13078267879/job/36495962625?pr=1894#step:4:312

x/dsmr/node.go

aaronbuchwald · 2025-01-29T21:00:28Z

x/dsmr/node.go

+type ChainState interface {
+	GetNetworkParams() (networdID uint32, subnetID, chainID ids.ID)
+	GetSignatureParams(ctx context.Context) (validators warp.CanonicalValidatorSet, quorumNum uint64, quorumDen uint64, err error)
+	GetLatestValidatorSet(ctx context.Context) (map[ids.NodeID]*snowValidators.GetValidatorOutput, error)
+}


Can we break this into individual accessors rather than grouping them based on what we will need at different points? This way when we need networkID and chainID, we can access those two rather than grouping all the network params defined by this function together.

In the future, we may want to make the rules configurable based on the block timestamp and fetch the validator set based off of the P-Chain height (both fine to leave as is for now). WIP for adding ProposerVM block context into the block is here: #1897.

For now, can we break this up to fetch the immutable params individually ie. networkID, subnetID, chainID, quorumNum, and quorumDenom. Can we also switch to two functions to fetch the canonical validator set and check whether a specific nodeID is a validator (can add P-Chain height as a parameter in the future)?

joshua-kim · 2025-01-29T22:53:47Z

examples/morpheusvm/go.mod

@@ -3,8 +3,8 @@ module github.com/ava-labs/hypersdk/examples/morpheusvm
 go 1.22.8

 require (
-	github.com/ava-labs/avalanchego v1.11.13-0.20241230212828-6dea1b366756
-	github.com/ava-labs/hypersdk v0.0.1
+	github.com/ava-labs/avalanchego v1.12.3-warp-verify5.0.20250129181352-a1aff06d0fef


Why do we have to update avalanchego to use the new localsigner? I think we can replace the usage of the hard-coded validator set w/out a version upgrade

It's not about the upgrade to localsigner. The goal here was to be able to verify the signature c.Signature.Verify without providing it with pseudo interface. ( i.e. provide it with the data it truly needs to consume ). see ava-labs/avalanchego#3679 for more details.

joshua-kim · 2025-01-29T22:56:25Z

x/dsmr/node.go

+	GetNetworkParams() (networdID uint32, subnetID, chainID ids.ID)
+	GetSignatureParams(ctx context.Context) (validators warp.CanonicalValidatorSet, quorumNum uint64, quorumDen uint64, err error)


Why are we making this a part of this interface? If it's to support forking live networks I think this is a premature abstraction and as a tradeoff it bloats this interface (and if we wanted to support that I think this interface would need some time parameter/block height parameter to support that). IMO this interface should just return validator sets.

yes, it's meant to support live networks. I believe that @aaronbuchwald has some additions he plans to add on top of that.

If we want to support forking live networks I think we need some timestamp/block height parameter in this interface then

joshua-kim · 2025-01-29T22:57:50Z

x/dsmr/node_test.go

+}
+
+func (*testChainState) GetNetworkParams() (uint32, ids.ID, ids.ID) {
+	return networkID /*subnetID*/, ids.Empty, chainID


nit: don't leave these comments

joshua-kim · 2025-01-29T22:58:05Z

x/dsmr/node_test.go

+		ctx,
+		pChain,
+		0,
+		ids.Empty, /*subnetID*/


same nit here

joshua-kim · 2025-01-29T22:58:55Z

x/dsmr/node_test.go

+	return networkID /*subnetID*/, ids.Empty, chainID
+}
+
+func (t *testChainState) getValidatorSet() map[ids.NodeID]*validators.GetValidatorOutput {


I feel like this introduces complexity, can we just cache this in the testChainState struct instead of having to re-call this function every time you want to get the validator set

The method is being used only in a single place ( within GetCanonicalValidatorSet ), and exclusively used for testing purposes. It doesn't seems that we would gain much by caching the map here. We could use a map as the backing storage ( instead of a slice ).

aaronbuchwald · 2025-01-31T19:18:38Z

x/dsmr/node_test.go

+	pChain := &validatorstest.State{
+		GetSubnetIDF: func(context.Context, ids.ID) (ids.ID, error) {
+			return t.GetSubnetID(), nil
+		},
+		GetValidatorSetF: func(
+			context.Context,
+			uint64,
+			ids.ID,
+		) (map[ids.NodeID]*validators.GetValidatorOutput, error) {
+			return t.getValidatorSet(), nil
+		},
+	}
+
+	canonicalValidators, err := warp.GetCanonicalValidatorSetFromSubnetID(


if we need this, can we include it in the testChainState type rather than constructing it on demand in this function?

sure. it would require a constructor, but no prob.

aaronbuchwald · 2025-02-03T19:40:30Z

x/dsmr/node.go

@@ -157,15 +152,16 @@ func (n *Node[T]) BuildChunk(
 		return ErrEmptyChunk
 	}

+	networkID, chainID := n.chainState.GetNetworkID(), n.chainState.GetChainID()


Could we break this into two separate lines?

aaronbuchwald · 2025-02-03T19:42:20Z

x/dsmr/node_test.go

+	canonicalValidators, err := warp.GetCanonicalValidatorSetFromSubnetID(
+		ctx,
+		t,
+		0,
+		ids.Empty,
+	)
+	if err != nil {
+		return warp.CanonicalValidatorSet{}, err
+	}
+	return canonicalValidators, err


can this be a single line?

aaronbuchwald · 2025-02-03T19:43:24Z

x/dsmr/node_test.go

+	quorumDen  uint64
+}
+
+func newTestChainState(validatorsSlice []Validator, quorumNum, quorumDen uint64) *testChainState { //nolint:unparam


This nolint label should not be necessary since each of the parameters is used, no?

in the test, we only use (1,1) for the quorum params. Its ok, since we don't really want to test that, but the linter alert anyway.

tsachiherman added 30 commits December 19, 2024 12:42

update

a68de91

Merge branch 'main' into tsachi/configurable-chunk-rate-limiter

6670ef4

update

Loading
Loading status checks…

4a125fa

update comments

d4d799f

update

d4e7b78

Merge branch 'main' into tsachi/configurable-chunk-rate-limiter

Loading
Loading status checks…

e295065

Merge branch 'main' into tsachi/configurable-chunk-rate-limiter

Loading
Loading status checks…

858fd9c

remove the NoVerifier

5eba267

Merge branch 'tsachi/configurable-chunk-rate-limiter' of github.com:a…

Loading
Loading status checks…

c0725c0

…va-labs/hypersdk into tsachi/configurable-chunk-rate-limiter

update

Loading
Loading status checks…

88c0a29

update per review feedback

b7e0dc8

Merge branch 'main' into tsachi/configurable-chunk-rate-limiter

Loading
Loading status checks…

6f1ad99

update per CR

Loading
Loading status checks…

412398d

Merge branch 'main' into tsachi/configurable-chunk-rate-limiter2

Loading
Loading status checks…

13cadf7

update

Loading
Loading status checks…

1599942

update

e41a8af

lint

Loading
Loading status checks…

d272577

Merge branch 'main' into tsachi/configurable-chunk-rate-limiter2

Loading
Loading status checks…

f579de1

update

5a84dbe

Merge branch 'tsachi/configurable-chunk-rate-limiter2' of github.com:…

Loading
Loading status checks…

371756e

…ava-labs/hypersdk into tsachi/configurable-chunk-rate-limiter2

Merge branch 'main' into tsachi/configurable-chunk-rate-limiter2

Loading
Loading status checks…

10c897f

Merge branch 'main' into tsachi/configurable-chunk-rate-limiter2

Loading
Loading status checks…

a1a9f75

Merge branch 'main' into tsachi/configurable-chunk-rate-limiter2

Loading
Loading status checks…

0a2f0b0

Merge branch 'main' into tsachi/configurable-chunk-rate-limiter2

f9420f8

Merge branch 'tsachi/configurable-chunk-rate-limiter2' of github.com:…

Loading
Loading status checks…

ab4b0e7

…ava-labs/hypersdk into tsachi/configurable-chunk-rate-limiter2

update

Loading
Loading status checks…

778ffec

update per PR review

Loading
Loading status checks…

6307fd8

update

Loading
Loading status checks…

3cc2f3c

temp;

8e5558f

Merge branch 'main' into tsachi/chainstate

Loading
Loading status checks…

82ae745

update

6c9f6f7

tsachiherman changed the title ~~wip : chainstate~~ dsmr: replace hard coded validators with ChainState Jan 29, 2025

tsachiherman added 3 commits January 29, 2025 14:38

update

c03f2b2

update

0f01c2d

update

7e6e941

tsachiherman marked this pull request as ready for review January 29, 2025 19:59

tsachiherman requested review from aaronbuchwald and joshua-kim as code owners January 29, 2025 19:59

tsachiherman self-assigned this Jan 29, 2025

aaronbuchwald requested changes Jan 29, 2025

View reviewed changes

joshua-kim requested changes Jan 29, 2025

View reviewed changes

tsachiherman added 8 commits January 30, 2025 10:50

update per review feedback.,

51ac5e9

add some nit

2c25b8b

Merge branch 'main' into tsachi/chainstate

d9d8829

update

baa3316

attempt to work around missing SaveAPIPort()

875c279

update

fcd0ac1

update

38124aa

Merge branch 'main' into tsachi/chainstate

493e982

aaronbuchwald reviewed Jan 31, 2025

View reviewed changes

tsachiherman added 4 commits February 3, 2025 13:24

Merge branch 'main' into tsachi/chainstate

8135fe3

rollback unwanted change.

8d63e75

update

435c873

lint

a36e0a0

aaronbuchwald reviewed Feb 3, 2025

View reviewed changes

update

29343b1

tsachiherman requested a review from aaronbuchwald February 3, 2025 20:20

aaronbuchwald approved these changes Feb 3, 2025

View reviewed changes

aaronbuchwald merged commit 9b7e1ef into main Feb 3, 2025
17 checks passed

aaronbuchwald deleted the tsachi/chainstate branch February 3, 2025 20:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dsmr: replace hard coded validators with ChainState #1894

dsmr: replace hard coded validators with ChainState #1894

tsachiherman commented Jan 27, 2025 •

edited

Loading

aaronbuchwald Jan 29, 2025

aaronbuchwald Jan 29, 2025

tsachiherman Jan 29, 2025

aaronbuchwald Jan 31, 2025

aaronbuchwald Jan 31, 2025

aaronbuchwald Jan 29, 2025

tsachiherman Jan 30, 2025

joshua-kim Jan 29, 2025

tsachiherman Jan 30, 2025

joshua-kim Jan 29, 2025 •

edited

Loading

tsachiherman Jan 30, 2025

joshua-kim Jan 30, 2025 •

edited

Loading

joshua-kim Jan 29, 2025

joshua-kim Jan 29, 2025

joshua-kim Jan 29, 2025

tsachiherman Jan 30, 2025

aaronbuchwald Jan 31, 2025

tsachiherman Feb 3, 2025

aaronbuchwald Feb 3, 2025

aaronbuchwald Feb 3, 2025

aaronbuchwald Feb 3, 2025

tsachiherman Feb 3, 2025

		// Tsachi : The following line was commented out, as it's no longer supported by avalanchego.
		// require.NoError(syncNode.SaveAPIPort())

		GetNetworkParams() (networdID uint32, subnetID, chainID ids.ID)
		GetSignatureParams(ctx context.Context) (validators warp.CanonicalValidatorSet, quorumNum uint64, quorumDen uint64, err error)

dsmr: replace hard coded validators with ChainState #1894

dsmr: replace hard coded validators with ChainState #1894

Conversation

tsachiherman commented Jan 27, 2025 • edited Loading

What ?

Node creation

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

joshua-kim Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshua-kim Jan 30, 2025 • 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

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

tsachiherman commented Jan 27, 2025 •

edited

Loading

joshua-kim Jan 29, 2025 •

edited

Loading

joshua-kim Jan 30, 2025 •

edited

Loading