Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use custom codec for validator metadata #1510

Merged
merged 81 commits into from
Oct 23, 2023
Merged

Use custom codec for validator metadata #1510

merged 81 commits into from
Oct 23, 2023

Conversation

abi87
Copy link
Contributor

@abi87 abi87 commented May 16, 2023 •
edited
Loading

Why this should be merged

In preparation for pending stakers drop #2088, we need P-chain validators' metadata to have their own codec, so to allow upgrading metadata version without affecting txs or blocks codec

How this works

Just replace txs.Codec and blocks.GenesisCodec usage with validators metadata's specific codec. Also introduced scaffolding for delegator metadata, to be used in #2175

How this was tested

No Regression on CI

@abi87 abi87 marked this pull request as ready for review May 16, 2023 17:33
@abi87 abi87 self-assigned this May 16, 2023
@abi87 abi87 requested review from ceyonur and dhrubabasu May 16, 2023 17:34
abi87
abi87 commented May 16, 2023
View reviewed changes
api/keystore/codec.go
@@ -11,7 +11,7 @@ import (

const (
maxPackerSize = 1 * units.GiB // max size, in bytes, of something being marshalled by Marshal()
maxSliceLength = 256 * 1024
maxSliceLength = linearcodec.DefaultMaxSliceLength
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just removed some code duplication, once linearcodec.DefaultMaxSliceLength is exported

@abi87 abi87 linked an issue May 17, 2023 that may be closed by this pull request
Continuous Staking #1426
Open
ceyonur
ceyonur reviewed May 17, 2023
View reviewed changes
vms/platformvm/state/validator_metadata.go Outdated
Comment on lines 37 to 43
c := linearcodec.New([]string{v0tag}, linearcodec.DefaultMaxSliceLength)
validatorMetadataCodec = codec.NewManager(math.MaxInt32)

err := validatorMetadataCodec.RegisterCodec(validatorMetadataCodecV0, c)
if err != nil {
panic(err)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we separate the codec to another file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I want to do that. I think keeping in the very same file suggests that this codec ought to be used to validatorMetadata only? (note that in continuous staking I will have to add a delegatorMetadata and use the very same codec)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont know if there is any implication. I wouldnt ask if this was a plain struct, but the init func looks a bit weird. Nothing really serious though.

Copy link
Contributor Author

@abi87 abi87 Jul 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I use the init function to initialize this code the same way we do with txs.Codec or blocks.Codec. The init is executed before any other code, so that we are guaranteed that codecs are well initialized when node starts

vms/platformvm/state/validator_metadata.go Outdated

var _ validatorState = (*metadata)(nil)
v0tag = "v0"
validatorMetadataCodecV0 = uint16(0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we rename this to something like V0CodecVersion?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it's mouthful but I just want to signal that it should be used for validatorsMetadata only. Should I try and move all of this to an internal subpackage and simplify naming?

Copy link
Contributor

@ceyonur ceyonur Jul 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if it wont be too much work, then yea it can be cleaner.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'll be quite some work, which I intend to do in a following PR, after stakers update operations are merged.

@abi87 abi87 requested a review from ceyonur May 17, 2023 10:50
@abi87 abi87 mentioned this pull request May 18, 2023
Closed
@abi87 abi87 mentioned this pull request Jun 8, 2023
Closed
@abi87 abi87 mentioned this pull request Oct 17, 2023
Closed
2 tasks
abi87
abi87 commented Oct 18, 2023
View reviewed changes
vms/platformvm/state/state.go
Comment on lines +2182 to +2186
metadata := &delegatorMetadata{
txID: staker.TxID,
PotentialReward: staker.PotentialReward,
}
if err := writeDelegatorMetadata(currentDelegatorList, metadata); err != nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just encapsulating delegator metadata writing into delegatorMetadata object

@StephenButtolph StephenButtolph added this to the v1.10.14 milestone Oct 23, 2023
@StephenButtolph StephenButtolph changed the title detached validator metadata codec Use custom codec for validator metadata Oct 23, 2023
@abi87 abi87 force-pushed the validator_metadata_codec branch from a397f9d to 172c8e0 Compare October 23, 2023 17:14
@abi87 abi87 force-pushed the validator_metadata_codec branch from 172c8e0 to a6ae416 Compare October 23, 2023 17:17
@StephenButtolph StephenButtolph merged commit d3287dd into dev Oct 23, 2023
@StephenButtolph StephenButtolph deleted the validator_metadata_codec branch October 23, 2023 19:09
joshua-kim added a commit that referenced this pull request Oct 27, 2023
@joshua-kim
Squashed commit of the following:
a4e9c00 
commit a4cee60
Author: Dan Laine <[email protected]>
Date:   Fri Oct 27 11:37:05 2023 -0400

    `merkledb` -- don't pass `BranchFactor` to `encodeDBNode` (#2217)

    Signed-off-by: Dan Laine <[email protected]>

commit cacbb9b
Author: Dhruba Basu <[email protected]>
Date:   Thu Oct 26 16:57:56 2023 -0400

    Move all blst function usage to `bls` pkg (#2222)

    Signed-off-by: Dan Laine <[email protected]>
    Co-authored-by: Dan Laine <[email protected]>

commit 3b213fc
Author: Dhruba Basu <[email protected]>
Date:   Thu Oct 26 16:53:26 2023 -0400

    Add json marshal tests to existing serialization tests in `platformvm/txs` pkg (#2227)

commit a6448ac
Author: Dhruba Basu <[email protected]>
Date:   Thu Oct 26 16:48:00 2023 -0400

    Update `golangci-lint` to `v1.55.1` (#2228)

commit fe74ed1
Author: Dan Laine <[email protected]>
Date:   Thu Oct 26 12:54:34 2023 -0400

    `merkledb` -- shift nit (#2218)

    Signed-off-by: Dan Laine <[email protected]>

commit e933587
Author: David Boehm <[email protected]>
Date:   Thu Oct 26 11:40:57 2023 -0400

    Reduce allocations on insert and remove (#2201)

    Signed-off-by: David Boehm <[email protected]>
    Signed-off-by: Dan Laine <[email protected]>
    Co-authored-by: Darioush Jalali <[email protected]>
    Co-authored-by: Dan Laine <[email protected]>

commit 638000c
Author: Stephen Buttolph <[email protected]>
Date:   Thu Oct 26 02:08:05 2023 -0400

    Update versions for v1.10.14 (#2225)

commit 8463690
Author: Stephen Buttolph <[email protected]>
Date:   Thu Oct 26 00:12:05 2023 -0400

    Improve logging for block verification failure (#2224)

commit 787f0b6
Author: Stephen Buttolph <[email protected]>
Date:   Wed Oct 25 17:35:17 2023 -0400

    Fix unexpected unlock (#2221)

commit 1f779af
Author: Stephen Buttolph <[email protected]>
Date:   Wed Oct 25 17:13:25 2023 -0400

    Revert networking AllowConnection change (#2219)

commit f020a05
Author: Dhruba Basu <[email protected]>
Date:   Wed Oct 25 16:51:24 2023 -0400

    Add `TransferSubnetOwnershipTx` (#2178)

    Signed-off-by: Dhruba Basu <[email protected]>

commit 150ffae
Author: Dan Laine <[email protected]>
Date:   Wed Oct 25 16:39:10 2023 -0400

    Add pebble database implementation (#1999)

    Co-authored-by: Dhruba Basu <[email protected]>
    Co-authored-by: Stephen Buttolph <[email protected]>

commit de168b1
Author: Joshua Kim <[email protected]>
Date:   Wed Oct 25 15:47:48 2023 -0400

    Add log for ungraceful shutdown on startup (#2215)

    Signed-off-by: Joshua Kim <[email protected]>

commit cd77a1e
Author: Dhruba Basu <[email protected]>
Date:   Wed Oct 25 13:27:43 2023 -0400

    Remove `aggregate` struct (#2213)

commit 128757d
Author: Ceyhun Onur <[email protected]>
Date:   Wed Oct 25 02:03:45 2023 +0300

    Move the overridden manager into the node (#2199)

    Signed-off-by: Ceyhun Onur <[email protected]>
    Co-authored-by: Alberto Benegiamo <[email protected]>
    Co-authored-by: Stephen Buttolph <[email protected]>

commit 7903676
Author: Ceyhun Onur <[email protected]>
Date:   Tue Oct 24 20:44:01 2023 +0300

    Remove contains from validator manager interface (#2198)

    Signed-off-by: Ceyhun Onur <[email protected]>
    Signed-off-by: Stephen Buttolph <[email protected]>
    Co-authored-by: Alberto Benegiamo <[email protected]>
    Co-authored-by: Stephen Buttolph <[email protected]>

commit 963aeb0
Author: Joshua Kim <[email protected]>
Date:   Tue Oct 24 12:54:31 2023 -0400

    Update TestDialContext to use ManuallyTrack (#2209)

    Signed-off-by: Joshua Kim <[email protected]>

commit e337dda
Author: Stephen Buttolph <[email protected]>
Date:   Tue Oct 24 12:42:05 2023 -0400

    Remove duplicate networking check (#2204)

    Signed-off-by: Dan Laine <[email protected]>
    Co-authored-by: Dan Laine <[email protected]>

commit 020e802
Author: Stephen Buttolph <[email protected]>
Date:   Mon Oct 23 17:37:14 2023 -0400

    Add RSA max key length test (#2205)

commit d3287dd
Author: Alberto Benegiamo <[email protected]>
Date:   Mon Oct 23 12:09:10 2023 -0700

    Use custom codec for validator metadata (#1510)

commit 3a1dcca
Author: Stephen Buttolph <[email protected]>
Date:   Mon Oct 23 13:47:37 2023 -0400

    Update local network readme (#2203)

commit 7b7931b
Author: Ceyhun Onur <[email protected]>
Date:   Mon Oct 23 20:36:47 2023 +0300

    Redesign validator set management to enable tracking all subnets (#1857)

    Signed-off-by: Ceyhun Onur <[email protected]>
    Co-authored-by: Alberto Benegiamo <[email protected]>
    Co-authored-by: Stephen Buttolph <[email protected]>

commit 4cd7051
Author: Alberto Benegiamo <[email protected]>
Date:   Mon Oct 23 08:48:56 2023 -0700

    Shutdown TimeoutManager during node Shutdown (#1707)

    Signed-off-by: Alberto Benegiamo <[email protected]>
    Co-authored-by: Dan Laine <[email protected]>
    Co-authored-by: Stephen Buttolph <[email protected]>
    Co-authored-by: Joshua Kim <[email protected]>

commit 6624270
Author: Joshua Kim <[email protected]>
Date:   Mon Oct 23 11:37:03 2023 -0400

    Add Heap Set (#2136)

    Signed-off-by: Joshua Kim <[email protected]>
    Signed-off-by: Stephen Buttolph <[email protected]>
    Co-authored-by: Stephen Buttolph <[email protected]>
    Co-authored-by: Dhruba Basu <[email protected]>
    Co-authored-by: dboehm-avalabs <[email protected]>
    Co-authored-by: David Boehm <[email protected]>
    Co-authored-by: Alberto Benegiamo <[email protected]>

commit 804f45b
Author: Stephen Buttolph <[email protected]>
Date:   Fri Oct 20 14:16:44 2023 -0400

    Move selectStartGear lock from Handler into Engines (#2182)

commit 7ed450b
Author: Joshua Kim <[email protected]>
Date:   Fri Oct 20 09:55:40 2023 -0400

    Implement Heap Map (#2137)

    Signed-off-by: Joshua Kim <[email protected]>
    Co-authored-by: Alberto Benegiamo <[email protected]>
    Co-authored-by: Stephen Buttolph <[email protected]>

commit a21d0cf
Author: Stephen Buttolph <[email protected]>
Date:   Thu Oct 19 18:13:46 2023 -0400

    Move HealthCheck lock from Handler into Engines (#2173)

commit 26b1505
Author: Stephen Buttolph <[email protected]>
Date:   Thu Oct 19 17:48:22 2023 -0400

    Move Shutdown lock from Handler into Engines (#2179)

commit 8c6b9d3
Author: Dhruba Basu <[email protected]>
Date:   Thu Oct 19 15:36:31 2023 -0400

    Add tests for BanffBlock serialization (#2194)

commit d9460de
Author: Dan Laine <[email protected]>
Date:   Thu Oct 19 15:26:16 2023 -0400

    Deprecate keystore config (#2195)

commit a9c260b
Author: David Boehm <[email protected]>
Date:   Thu Oct 19 14:44:51 2023 -0400

    Merkle db Make Paths only refer to lists of nodes (#2143)

    Signed-off-by: David Boehm <[email protected]>
    Co-authored-by: Darioush Jalali <[email protected]>

commit 0faab95
Author: Joshua Kim <[email protected]>
Date:   Thu Oct 19 14:43:29 2023 -0400

    Update P2P proto docs (#2181)

    Signed-off-by: Joshua Kim <[email protected]>
    Co-authored-by: Stephen Buttolph <[email protected]>

commit 927c23d
Author: Dan Laine <[email protected]>
Date:   Thu Oct 19 13:07:46 2023 -0400

    Deprecate IPC configs (#2168)

    Co-authored-by: Stephen Buttolph <[email protected]>

commit 3b843a3
Author: Stephen Buttolph <[email protected]>
Date:   Thu Oct 19 12:14:58 2023 -0400

    Update cgo usage (#2184)

Signed-off-by: Joshua Kim <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dhrubabasu dhrubabasu dhrubabasu approved these changes

@StephenButtolph StephenButtolph StephenButtolph approved these changes

@JacobEverly JacobEverly JacobEverly approved these changes

@danlaine danlaine Awaiting requested review from danlaine

@ceyonur ceyonur Awaiting requested review from ceyonur

Assignees

@abi87 abi87

Labels
continuous staking vm This involves virtual machines
Projects
No open projects
Ski Patrol
Archived in project
Milestone
v1.10.14
Development

Successfully merging this pull request may close these issues.

Continuous Staking
5 participants
@abi87 @dhrubabasu @ceyonur @StephenButtolph @JacobEverly