Add "proposed" optional flag to `getValidatorsAt` #3531

iansuvak · 2024-11-08T20:31:26Z

Why this should be merged

I'm not convinced that it should but opening this up for discussion.

Adding this interface was discussed together with adding platform.GetProposedHeight interface in #3530 This is a convenience method to allow for passing "proposed" instead of a numeric height literal to GetValidatorsAt.

The idea is to make it safer to publicly expose GetValidatorsAt interface. This by itself doesn't accomplish it but a different proxy service could use this same interface and call GetProposedHeight and return the cached value if this was called with the same resolved height and subnet.

How this works

Makes the GetValidatorsAt accept either numeric height or a "proposed" string literal that first calls getMinimumHeight and uses that instead of the numerical value. The service side is backward compatible but the client interface has changed.

I'm also not sure if this solution is cleaner than just adding a new boolean flag field to the request. I don't think that the platformVM service uses this kind of type overloading anywhere else.

How this was tested

Added a unit test.

Need to be documented in RELEASES.md?

Added a note to the RELEASES.md but this also introduces a breaking change to the client and not sure what's the best way to handle communication around that

marun

Why was it necessary to propose a replacement for 3497 instead of evolving that PR?
I think it would be cleaner to add a new field instead of adding the complexity of a multi-type field. What suggested this approach?

marun · 2024-11-13T13:12:00Z

tests/e2e/p/validator_sets.go

@@ -102,6 +102,7 @@ var _ = e2e.DescribePChain("[Validator Sets]", func() {
 						tc.DefaultContext(),
 						constants.PrimaryNetworkID,
 						height,
+						false,


(No action required) When using a literal value that doesn't provide context as to what it represents, maybe add a comment (e.g. false, // isProposed)?

iansuvak · 2024-11-13T14:17:11Z

Why was it necessary to propose a replacement for 3497 instead of evolving that PR?

The pivot to this approach was to re-use the already existing field and make it more easily cacheable on the backend and keep the number of externally exposed RPC methods limited if they are accomplishing similar things. Given the pivot in strategy we opted for a new PR.

I think it would be cleaner to add a new field instead of adding the complexity of a multi-type field. What suggested this approach?

I'm open to either solution. The reasoning behind accepting multi-type field was since it's already an established pattern in the industry, see eth_getBlockByNumber which accepts latest, earliest etc. but if the consensus is to have a separate field I'd be happy to add a separate optional flag or boolean field

ceyonur

generally LGTM, we probably need to update RELEASES.md

vms/platformvm/service.md

ceyonur · 2024-11-13T15:02:36Z

vms/platformvm/service.go

@@ -1689,7 +1689,7 @@ func (s *Service) GetTimestamp(_ *http.Request, _ *struct{}, reply *GetTimestamp

 // GetValidatorsAtArgs is the response from GetValidatorsAt
 type GetValidatorsAtArgs struct {
-	Height   avajson.Uint64 `json:"height"`
+	Height   avajson.Height `json:"height"`


I guess this is to prevent breaking change for that args. However I wonder if a separate bool flag or even using negative values would make this more straightforward.

Yeah indeed, that + custom parsing is to allow either json field. The idea was that eth supports this kind of thing already in eth_getBlockByNumber

But happy to hear the votes either way!

It's probably better to have backwards compatibility, so I'm fine with this or using negative values. Should we also add latest here?

a new field would be optional regardless so I don't think it would impact backwards compatibility since the go client is already not backwards compatible in this implementation.

Re: latest I do think it makes sense to add but would prefer to do it as a follow up to get this change in before etna rollout. I do think it's slightly odd that platform.getValidatorsAt({height: "latest"}) and platform.getCurrentValidators{} would then return different types but I guess asking for them in the shorter return format that getValidatorsAt uses is a valid usecase

utils/json/height.go

StephenButtolph

Couple small nits then lgtm

StephenButtolph · 2024-11-15T19:15:59Z

vms/platformvm/client.go

 	// GetValidatorsAt returns the weights of the validator set of a provided
-	// subnet at the specified height.
+	// subnet at the specified height or at proposerVM height if [isProposed] is true.


I think this comment needs to be updated now.

Also - thoughts on changing height uint64 to height api.Height? It feels like right now the MaxUint64 behavior might feel odd

I made the change since I don't feel strongly one way or another. I agree that the behavior might be unintuitive but accepting height uint64 had the benefit of not changing the interface at all.

vms/platformvm/service.go

Co-authored-by: Stephen Buttolph <[email protected]> Signed-off-by: Ian Suvak <[email protected]>

vms/platformvm/service_test.go

Signed-off-by: Ian Suvak <[email protected]> Co-authored-by: Stephen Buttolph <[email protected]>

iansuvak added 4 commits November 7, 2024 09:47

server side changes

f1bb8bc

client side changes

c9661bd

add unit test for getValidatorsAt

448eb0e

add a proposerVM height bump case to the test and update docs

e10d3a9

iansuvak requested review from marun and StephenButtolph as code owners November 8, 2024 20:31

iansuvak added 2 commits November 8, 2024 15:54

lint

a39dba2

lint

8f56151

marun reviewed Nov 13, 2024

View reviewed changes

ceyonur reviewed Nov 13, 2024

View reviewed changes

iansuvak added 3 commits November 13, 2024 10:15

Merge remote-tracking branch 'origin/master' into validators-at-proposed

9dbaf39

update releases and readme

58b0968

num -> int

06bf25f

iansuvak requested a review from ceyonur November 13, 2024 20:05

StephenButtolph reviewed Nov 15, 2024

View reviewed changes

utils/json/height.go Outdated Show resolved Hide resolved

Merge remote-tracking branch 'origin/master' into validators-at-proposed

4b3f5d5

iansuvak self-assigned this Nov 15, 2024

review feedback -> use maxuint as api.ProposedHeight

3a4bc8e

iansuvak requested review from StephenButtolph and marun November 15, 2024 17:09

iansuvak added 2 commits November 15, 2024 12:37

lint

b91042b

Merge branch 'master' into validators-at-proposed

88627c1

StephenButtolph reviewed Nov 15, 2024

View reviewed changes

iansuvak and others added 3 commits November 15, 2024 14:19

Update vms/platformvm/service.go

4d611d6

Co-authored-by: Stephen Buttolph <[email protected]> Signed-off-by: Ian Suvak <[email protected]>

review feedback

5aaca41

lint fix

d653f16

StephenButtolph approved these changes Nov 15, 2024

View reviewed changes

marun approved these changes Nov 15, 2024

View reviewed changes

vms/platformvm/service_test.go Show resolved Hide resolved

marun added this pull request to the merge queue Nov 15, 2024

Merged via the queue into master with commit ae6b476 Nov 15, 2024
23 checks passed

marun deleted the validators-at-proposed branch November 15, 2024 20:22

michaelkaplan13 mentioned this pull request Nov 15, 2024

Add getProposedValidators RPC method #3497

Closed

iansuvak mentioned this pull request Nov 15, 2024

Fix json parsing of GetValidatorsAtArgs #3551

Merged

tsachiherman pushed a commit that referenced this pull request Jan 29, 2025

Add "proposed" optional flag to getValidatorsAt (#3531)

2d2973b

Signed-off-by: Ian Suvak <[email protected]> Co-authored-by: Stephen Buttolph <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add "proposed" optional flag to `getValidatorsAt` #3531

Add "proposed" optional flag to `getValidatorsAt` #3531

Uh oh!

iansuvak commented Nov 8, 2024 •

edited

Loading

Uh oh!

marun left a comment

Uh oh!

marun Nov 13, 2024

Uh oh!

iansuvak commented Nov 13, 2024

Uh oh!

ceyonur left a comment

Uh oh!

Uh oh!

Uh oh!

ceyonur Nov 13, 2024

Uh oh!

iansuvak Nov 13, 2024

Uh oh!

iansuvak Nov 13, 2024

Uh oh!

ceyonur Nov 14, 2024

Uh oh!

iansuvak Nov 14, 2024

Uh oh!

Uh oh!

StephenButtolph left a comment

Uh oh!

StephenButtolph Nov 15, 2024

Uh oh!

iansuvak Nov 15, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Add "proposed" optional flag to getValidatorsAt #3531

Add "proposed" optional flag to getValidatorsAt #3531

Uh oh!

Conversation

iansuvak commented Nov 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why this should be merged

How this works

How this was tested

Need to be documented in RELEASES.md?

Uh oh!

marun left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iansuvak commented Nov 13, 2024

Uh oh!

ceyonur left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

StephenButtolph left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Add "proposed" optional flag to `getValidatorsAt` #3531

Add "proposed" optional flag to `getValidatorsAt` #3531

iansuvak commented Nov 8, 2024 •

edited

Loading