ACP-77: Implement validator state #3388

StephenButtolph · 2024-09-13T14:29:11Z

Why this should be merged

ACP-77 introduces a new type of validator: the SubnetOnlyValidator.

This PR introduces this new validator type to the P-chain state.

How this works

`SubnetOnlyValidator`

For the new SubnetOnlyValidator type, the validator can either be active (EndAccumulatedFee != 0) or inactive (EndAccumulatedFee == 0).

active validators are handled similarly to other validator types. They can be sampled during consensus, can contribute to block production, can contribute to ICM (Warp) messages, and are held in memory to efficiently perform these operations. To limit the memory pressure introduced by active validators, the number of active validators is limited.

Unlike active validators, inactive validators are not sampled during consensus, can not contribute to block production, and can not contribute to ICM (Warp) messages. This allows inactive validators to be removed from memory and only kept on disk. Each L1 will have an entry in their validator set representing the cumulative amount of inactive stake.

Removing a SubnetOnlyValidator is performed simply by setting their Weight to be 0.

State Modifications

The P-chain state is broken into 2 implementations:

The on-disk representation state
Potential changes to the on-disk representation diff

The typical flow for performing a change is:

Create a diff on top of either the state or another diff.
Apply the diff to the parent, either the state or another diff.
Atomically commit changes made to state to disk.

Contrary to prior validator implementations within the state package, this PR attempts to provide validation of the requested changes.

Returned errors from PutSubnetOnlyValidator should result in no changes to the state.

PutSubnetOnlyValidator succeeding should result in a valid state representation, regardless of the expected API behavior.

The only exception to this is documented with:

// If an SoV with the same validationID as a previously removed SoV is
// added, the behavior is undefined.

It is difficult to handle this case within the state without introducing non-deterministic error reporting because validationIDs are deleted from disk when the state is deleted. This means that the state code would need to handle all of the fields of an SoV to change on demand. However, that would require additional changes to the validator manager and other parts of the code (which make assumptions around ValidationID implying a static subnetID + nodeID pair).

The in-memory validator manager and historical validator set diffs are only updated during the atomic disk commitment changes.

How this was tested

Added unit tests.

Need to be documented in RELEASES.md?

Added P-chain configs:

l1-weights-cache-size
l1-inactive-validators-cache-size
l1-subnet-id-node-id-cache-size

Co-authored-by: Darioush Jalali <[email protected]> Signed-off-by: Ceyhun Onur <[email protected]>

Signed-off-by: Ceyhun Onur <[email protected]>

Co-authored-by: Darioush Jalali <[email protected]> Signed-off-by: Ceyhun Onur <[email protected]>

…ection

ceyonur

LGTM!
(I left a question for the caching in #3516 (comment))

vms/platformvm/state/state.go

yacovm · 2024-11-04T22:26:29Z

vms/platformvm/state/state.go

+					maybeSOVOverhead = wrappers.BoolLen + sovOverhead
+					entryOverhead    = ids.IDLen + maybeSOVOverhead
+				)
+				if maybeSOV.IsNothing() {


under what circumstance is it nothing? Both here and in general (line 196 in subnet_only_validator.go)

This cache stores hits as maybe.Some and misses as maybe.Nothing. So, if someone performs a delete or a get which results in not found, maybe.Nothing would be cached.

yacovm · 2024-11-04T22:51:56Z

vms/platformvm/state/diff.go

+		return false, fmt.Errorf("%w: %s", ErrMissingParentState, d.parentID)
+	}
+
+	return parentState.HasSubnetOnlyValidator(subnetID, nodeID)


this might seem as a weird question, but - how deep is the expected recursion here?

The recursion ends at the lastAcceptedState and starts at the most recently verified block, so typically the recursion is very shallow.

yacovm · 2024-11-04T23:01:53Z

vms/platformvm/state/diff.go

+		return weight, nil
+	}
+
+	parentState, ok := d.stateVersions.GetState(d.parentID)


can you explain the high level idea of doing this backwards traversal in search of the subnet, both here and in the next function?

We basically don't have a full snapshot of the validators in each state? Why is that?

Performing a full copy of the validator states for each diff would be pretty expensive. The number of changes per block is typically small, but the number total entries being tracked can be quite large.

vms/platformvm/state/state.go

michaelkaplan13 · 2024-11-04T23:21:08Z

vms/platformvm/state/state.go

+// SubnetOnlyValidator with the given validationID. It is guaranteed that any
+// returned validator is either active or inactive.


(No action needed) Aren't all validators always either active or inactive? Not sure I understand the purpose of the comment

This was supposed to signify that no deleted SoVs will be returned from this function (isDeleted() will always return false). Specifically, this is to ensure that !isActive() means inactive (and not deleted)

Suggested change

// SubnetOnlyValidator with the given validationID. It is guaranteed that any

// returned validator is either active or inactive.

// returned validator is either active or inactive (not deleted).

yacovm · 2024-11-04T23:23:00Z

vms/platformvm/state/diff.go

+	// ensures that a subnetID+nodeID pair that was deleted and then re-added in
+	// a single diff can't get reordered into the addition happening first;
+	// which would return an error.
+	for _, sov := range d.sovDiff.modified {


Playing devil's advocate here, we're iterating here over a map, and calling baseState.PutSubnetOnlyValidator which may return with different errors. This means that across different nodes, we may fail differently, no?

Is that a concern?

It is generally unexpected for Apply to ever error.

Apply is called during Accept. If Apply were to return an error, that error would be propagated through Accept into the consensus engine (causing a FATAL).

vms/platformvm/state/state.go

vms/platformvm/state/subnet_only_validator.go

yacovm · 2024-11-05T00:03:33Z

vms/platformvm/state/state.go

+				if priorSOV.Weight < sov.Weight {
+					err = s.validators.AddWeight(sov.SubnetID, nodeID, sov.Weight-priorSOV.Weight)
+				} else if priorSOV.Weight > sov.Weight {
+					err = s.validators.RemoveWeight(sov.SubnetID, nodeID, priorSOV.Weight-sov.Weight)


is priorSOV.Weight == sov.Weight legal? what does it mean?

If the priorSOV.Weight is greater than sov.Weight, then the weight of the validator is being decreased by priorSOV.Weight - sov.Weight units

no but i asked what it means when they're equal, I edited shortly after, probably github didn't refresh my comment

Ah. It is possible for them to be equal. In that case the weight does not need to be modified.

cam-schultz · 2024-11-05T00:46:42Z

vms/platformvm/state/diff.go

+	feeState         gas.State
+	sovExcess        gas.Gas
+	accruedFees      uint64
+	parentActiveSOVs int


nit: numParentActiveSOVs is more descriptive IMO

Or parentActiveSOVCount because a "count" is more descriptive than a "number" 😉

I ended up going with parentNumActiveSOVs.

Because it aligns most with the actual source of this: parentState.NumActiveSubnetOnlyValidators()

cam-schultz · 2024-11-05T00:54:28Z

vms/platformvm/state/diff.go

+	return parentState.WeightOfSubnetOnlyValidators(subnetID)
+}
+
+func (d *diff) GetSubnetOnlyValidator(validationID ids.ID) (SubnetOnlyValidator, error) {


A couple high level questions that may be related:

What triggers a new diff to be created?

Did you consider updating diffs in place, rather than the linked list style approach here?

Diffs are created for a few reasons. But the reason the diffs are designed this way is because each diff (can) represent the changes that are performed by a block. When a block is verified a diff is created. When a block is accepted a diff is applied to the disk state.

Diffs are updated in-place during the execution (verification) of a block - but we wouldn't want a child block to modify the state that will be committed when the parent block is accepted (as the child could be rejected for a different child)

cam-schultz · 2024-11-05T01:01:06Z

vms/platformvm/state/subnet_only_validator.go

+
+	// GetSubnetOnlyValidator returns the validator with [validationID] if it
+	// exists. If the validator does not exist, [err] will equal
+	// [database.ErrNotFound].


It's a bit odd to call out the error type here when that's not enforceable by the interface

I don't think this is particularly unusual... This is done in the standard lib as well. For an example, see SetDeadline here: https://pkg.go.dev/net#Conn

cam-schultz · 2024-11-05T01:34:02Z

vms/platformvm/state/state.go

@@ -2230,6 +2571,16 @@ func (s *state) writeValidatorDiffs(height uint64) error {
 	return nil
 }

+func getOrDefault[K comparable, V any](m map[K]*V, k K) *V {


A comment explaining that the default value is written to the map would be helpful. Or consider renaming to getOrAddDefault

cam-schultz · 2024-11-05T01:37:20Z

vms/platformvm/state/state.go

+	for subnetID, weight := range s.sovDiff.modifiedTotalWeight {
+		var err error
+		if weight == 0 {
+			err = s.weightsDB.Delete(subnetID[:])


What's the expected behavior of WeightOfSubnetOnlyValidators after the entry is deleted from the DB and the cache entry expires? If it's to error, perhaps we should not write to the cache so that WeightOfSubnetOnlyValidators deterministically errors for any subnets with weight 0.

WeightOfSubnetOnlyValidators calls database.WithDefault(database.GetUInt64, s.weightsDB, subnetID[:], 0) which will return 0 if the value is not on disk. So, WeightOfSubnetOnlyValidators deterministically returns 0 for any subnets with weight 0.

vms/platformvm/state/subnet_only_validator.go

darioush · 2024-11-05T01:19:58Z

vms/platformvm/state/state.go

+// SubnetOnlyValidator with the given validationID. It is guaranteed that any
+// returned validator is either active or inactive.


Suggested change

// SubnetOnlyValidator with the given validationID. It is guaranteed that any

// returned validator is either active or inactive.

// returned validator is either active or inactive (not deleted).

vms/platformvm/state/state.go

vms/platformvm/state/state_test.go

vms/platformvm/state/subnet_only_validator.go

ceyonur and others added 30 commits July 24, 2024 16:42

remove subnet IDs from uptime pkg

9c8835b

remove subnet uptimes from platforvm

733d5fd

remove subnet uptimes from p2p

10d8e86

remove subnet uptimes from api

a2f69d0

add tracked bool

ee287c6

remove unnecessary err

a42e48c

remove connected subnet msg

4c542af

Merge branch 'master' into remove-subnetid-uptime-manager

941f536

fix linter

9161864

Merge branch 'master' into remove-subnetid-uptime-manager

dfded5e

rework on tests and reviews

f275c15

fix linter

d9355cc

Update proto/p2p/p2p.proto

46501ad

Co-authored-by: Darioush Jalali <[email protected]> Signed-off-by: Ceyhun Onur <[email protected]>

fix comment

b7459bd

Signed-off-by: Ceyhun Onur <[email protected]>

Update vms/platformvm/service_test.go

b81b737

Co-authored-by: Darioush Jalali <[email protected]> Signed-off-by: Ceyhun Onur <[email protected]>

use disconnect in stop tracking

f6bb383

remove todo comment

7a6f7eb

Merge branch 'master' into remove-subnetid-uptime-manager

64dec9c

Merge branch 'master' into remove-subnetid-uptime-manager

33eb562

remove unused err

cba7a50

remove subnet connector mock

b4955d6

Merge branch 'master' into remove-subnetid-uptime-manager

3c9096e

WIP Add Expiry Replay Protection

1b22842

rename

ad83805

wip

7820c17

Replace iterator equality helper

20ed013

merged

fcb388c

Finish diff implementation

a02b51c

Merge branch 'master' into implement-acp-77-add-validator-replay-prot…

daf45a3

…ection

Finish state implementation

7bda9b1

fix merge

7cbf31b

StephenButtolph changed the base branch from master to add-weight-diff-helpers November 2, 2024 00:00

StephenButtolph mentioned this pull request Nov 2, 2024

ACP-77: Add caching to SoV DB helpers #3516

Merged

Base automatically changed from add-weight-diff-helpers to master November 3, 2024 16:31

ceyonur approved these changes Nov 4, 2024

View reviewed changes

merged

3077356

StephenButtolph commented Nov 4, 2024

View reviewed changes

vms/platformvm/state/state.go Outdated Show resolved Hide resolved

vms/platformvm/state/state.go Outdated Show resolved Hide resolved

improve caching

69837c1

yacovm reviewed Nov 4, 2024

View reviewed changes

michaelkaplan13 reviewed Nov 4, 2024

View reviewed changes

vms/platformvm/state/state.go Outdated Show resolved Hide resolved

michaelkaplan13 reviewed Nov 4, 2024

View reviewed changes

yacovm reviewed Nov 4, 2024

View reviewed changes

michaelkaplan13 reviewed Nov 4, 2024

View reviewed changes

vms/platformvm/state/state.go Outdated Show resolved Hide resolved

michaelkaplan13 reviewed Nov 4, 2024

View reviewed changes

vms/platformvm/state/subnet_only_validator.go Show resolved Hide resolved

nit

d37e9f3

yacovm reviewed Nov 5, 2024

View reviewed changes

num -> net for possibly negative value

9dc642a

cam-schultz reviewed Nov 5, 2024

View reviewed changes

darioush approved these changes Nov 5, 2024

View reviewed changes

darioush reviewed Nov 5, 2024

View reviewed changes

vms/platformvm/state/subnet_only_validator.go Show resolved Hide resolved

marun approved these changes Nov 5, 2024

View reviewed changes

yacovm approved these changes Nov 5, 2024

View reviewed changes

Address PR comments

22de2b1

michaelkaplan13 approved these changes Nov 5, 2024

View reviewed changes

StephenButtolph added this pull request to the merge queue Nov 5, 2024

Merged via the queue into master with commit ca5e0d5 Nov 5, 2024
23 checks passed

StephenButtolph deleted the implement-acp-77-sov-validators-state branch November 5, 2024 16:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ACP-77: Implement validator state #3388

ACP-77: Implement validator state #3388

StephenButtolph commented Sep 13, 2024 •

edited

Loading

ceyonur left a comment

yacovm Nov 4, 2024 •

edited

Loading

StephenButtolph Nov 4, 2024

yacovm Nov 4, 2024

StephenButtolph Nov 4, 2024

yacovm Nov 4, 2024

StephenButtolph Nov 5, 2024

michaelkaplan13 Nov 4, 2024

StephenButtolph Nov 4, 2024

darioush Nov 5, 2024

yacovm Nov 4, 2024

StephenButtolph Nov 5, 2024

yacovm Nov 5, 2024 •

edited

Loading

StephenButtolph Nov 5, 2024

yacovm Nov 5, 2024

StephenButtolph Nov 5, 2024

cam-schultz Nov 5, 2024

richardpringle Nov 5, 2024

StephenButtolph Nov 5, 2024

cam-schultz Nov 5, 2024

StephenButtolph Nov 5, 2024

cam-schultz Nov 5, 2024

StephenButtolph Nov 5, 2024

cam-schultz Nov 5, 2024

StephenButtolph Nov 5, 2024

cam-schultz Nov 5, 2024

StephenButtolph Nov 5, 2024

darioush Nov 5, 2024

		// SubnetOnlyValidator with the given validationID. It is guaranteed that any
		// returned validator is either active or inactive.

ACP-77: Implement validator state #3388

ACP-77: Implement validator state #3388

Conversation

StephenButtolph commented Sep 13, 2024 • edited Loading

Why this should be merged

How this works

SubnetOnlyValidator

State Modifications

How this was tested

Need to be documented in RELEASES.md?

ceyonur left a comment

Choose a reason for hiding this comment

yacovm Nov 4, 2024 • 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

yacovm Nov 5, 2024 • 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

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

StephenButtolph commented Sep 13, 2024 •

edited

Loading

`SubnetOnlyValidator`

yacovm Nov 4, 2024 •

edited

Loading

yacovm Nov 5, 2024 •

edited

Loading