[ACP-99] ValidatorSetManager Solidity Contract

[Nuttymoon]

ACP that defines:

1. A reference implementation for a minimal Solidity smart contract to manage a Subnet’s validator set from an Avalanche EVM chain
2. A reference architecture to easily plug in custom “security modules” on top of this contract (e.g. to implement a PoS Subnet)

Rendered

[aaronbuchwald]

Hmm so it looks like there are two different architectures that we could take here.

1. ValidatorSetManager + Owner PoS/PoA (as suggested in this PR):

• Create the ValidatorSetManager contract to be listed as the owner of a Subnet
• Create an arbitrary owner contract that could be PoS/PoA to call the ValidatorSetManager

2. xL1StakingLibrary + Reference PoS/PoA:

• Create a library that provides high level functions to send and parse the required messages from the P-Chain
• Create reference implementations using the library for PoS/PoA

I'm not sure which implementation would be cleaner or a more standard way to handle things in Solidity, curious for others thoughts on the best way to handle this as well.

I think a lot of this will depend on how much functionality is easily shared between the different reference implementations.

[Nuttymoon]

@aaronbuchwald
Hmm so it looks like there are two different architectures that we could take here.

1. ValidatorSetManager + Owner PoS/PoA (as suggested in this PR):

   • Create the ValidatorSetManager contract to be listed as the owner of a Subnet

   • Create an arbitrary owner contract that could be PoS/PoA to call the ValidatorSetManager

2. xL1StakingLibrary + Reference PoS/PoA:

   • Create a library that provides high level functions to send and parse the required messages from the P-Chain
   • Create reference implementations using the library for PoS/PoA

I'm not sure which implementation would be cleaner or a more standard way to handle things in Solidity, curious for others thoughts on the best way to handle this as well.

I think a lot of this will depend on how much functionality is easily shared between the different reference implementations.

IMO going for option 1. has 2 benefits:

• As I mentioned in the PR, this allows auditing the reference implementation contract that xL1 builders can instantly reuse securely and at no cost
• This minimizes the number of times that the SetSubnetValidatorManagerTx has to be issued on the P-Chain

That being said, it doesn't prevent splitting up the ValidatorSetManager contract into 2 pieces, one being the library described above.

[minghinmatthewlam]

Thanks for this initial draft ACP @Nuttymoon , overall looks great! Here's our WIP branch for validator manager, looking forward to sync on design considerations for this ACP.

[Nuttymoon]

@minghinmatthewlam
Thanks for this initial draft ACP @Nuttymoon , overall looks great! Here's our WIP branch for validator manager, looking forward to sync on design considerations for this ACP.

Tbh I spotted the branch and have been stalking it for a few weeks hehe. Here is the implementation of ACP-99 I have been working on as well: ACP-99 contracts library

I will update the ACP proposal to reflect the implementation. I could only go that far without implementing anything.

[michaelkaplan13]

Hey @Nuttymoon, thanks again for putting together this ACP a few months back.

The ACP-77 spec is very close to being implementable now. After these changes we hope to have a functional reference implementation ready in AvalancheGo. There are a few small tweaks there that require changes to this contract interfaces, mostly the RemainingBalanceOwner and DisableOwner being set in the RegisterSubnetValidatorMessage.

After that, and now that we've also had some time to both work through implementations of the validator manager contract, it would be great to revisit ACP-99 together in earnest. The few small tweaks I think are all that's necessary to get the PR merged, and would be great to have a community call to discuss a couple aspects of it like supporting delegation, tracking historical validations in active state, and possibly making it general enough for the manager and security module pieces to be combined via either composition (as the ACP is written currently) or inheritance (the approach we took here).

Either way looking forward to discussing it!

[michaelkaplan13]

I think there may be a few inconsistencies here:

• Both Validation and ValidationPeriod have startTime and endTime. As is, it seems like they likely belong only in ValidationPeriod.
• The comment mentions the index being the nonce for the validator, but that's not included in any of the struct fields. This likely needs to be tracked per validation period.
• uptimeSeconds seems to fit better in ValidationPeriod than Validation, since it can only be tracked for a specific period while the validator is active.

[michaelkaplan13]

This is just for discussion after this ACP is merged, but curious what your thoughts are on leaving historic validation tracking to indexers/archive nodes, and only keeping the active validator set around in state.

I think this could make sense to prevent large state growth overtime, etc.

[Nuttymoon]

• Yes, it is possible to use validation.periods[0].startTime instead of validation.startTime to avoid duplicating data. For endTime it would be a little bit trickier: validation.periods[validation.periods.length - 1].endTime (but irrelevant to store endTime anyway if we choose to remove history)
• I thought it was enough to use the array index as the place to store the nonce since it is incremented by 1 at each weight change
• I chose to track uptimeSeconds at the Validation level because I don't think it is useful to track it at the ValidationPeriod level and thought it might be difficult to differentiate the uptime from a specific period than from the whole validation. The latter depends on how the uptime is tracked at the VM level. Do we have more information about how this will be handled btw?

About the validation historic tracking, I agree that it is most likely not needed. Thus my open question at the end.

[Nuttymoon]

Hey @Nuttymoon, thanks again for putting together this ACP a few months back.

...

Thanks @michaelkaplan13 for reviewing! I'd be more than happy to discuss this ACP ofc! Maybe it would be nice to dedicate an upcoming Developer Community Call to it @meaghanfitzgerald?

I'll make sure to update the ACP when changes to ACP-77 are finalized and merged.

[Nuttymoon]

Hi, I just updated the ACP to reflect the latest changed made to ACP-77

[michaelkaplan13]

A few minor suggestions, but otherwise makes sense to me!

Noting a few questions/considerations here for the official discussion thread/community call (but these by no means need to be discussed or answered to merge the ACP)..

• For a PoS security module to support delegation, do would the interface methods included here be internal, with other external functions added that support the additional parameters?
• Which pieces are state are tracked by the security module vs manager contracts? They don't need to be included in the ACP since it just defines the interfaces of course, but think it would be helpful to talk through to ensure the interfaces feed the needs of feasible use cases.
• What would it looks like for a manager to upgrade from a PoA security module to a PoS security module? One complexity we tried to work through was still allowing for the removal of existing PoA validators but allowing anyone to remove them after the conversion.

Looking forward to continue the discussion now that ACP-77 is more finalized and this PR can also be merged!

[Nuttymoon]

I think we are ready to merge @michaelkaplan13.

The latest commit 664c893 implements your suggestion about PChainOwner, refactors the structs declaration layout and adds a function that I missed when updating the interface (updateUptime).