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.

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

Drop Pending Stakers 1 - introduced ScheduledStaker txs #2323

Merged
merged 41 commits into from
Dec 6, 2023

Conversation

abi87
Copy link
Contributor

@abi87 abi87 commented Nov 16, 2023

Why this should be merged

Following Durango activation, stakers start time won't be the StartTime specified in the AddStakerTx. Also stakers won't have a PendingPriority anymore.
We won't change any tx format, but we should discourage calling tx.StartTime in most places, because that won't likely be the actual time when staking started.
Moveover we don't want to force any future AddStakerTx type to specify StartTime and PendingPriority, since they won't really need them.
To achieve this, we do the folllowing in this PR:

  1. We rename current txs.Staker interface to txs.ScheduledStaker interface. This interface will keep StartTime() and PendingPriority() methods
  2. We introduce a txs.Staker interface without StartTime and PendingPriority.

Once all the Drop pending stakers code will be merged, txs.Staker will be the interface generally called and it will be as minimal as possible. On the contrary txs.ScheduledStaker will be used only in specific places (e.g. upon loading genesis validators and to create pending stakers during bootstrap)

How this works

Slicing #2175 up.
New tx type, mock regeneration, small nits to reduce diffs in the future

How this was tested

CI

@@ -144,11 +144,11 @@ func TestNewCurrentStaker(t *testing.T) {
subnetID := ids.GenerateTestID()
weight := uint64(12345)
startTime := time.Now()
endTime := time.Now()
endTime := startTime.Add(time.Hour)
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 a nit, not strictly relevant to this PR, but I'd rather take it out of the way

@@ -1324,7 +1324,7 @@ func (s *state) syncGenesis(genesisBlk block.Block, genesis *genesis.Genesis) er
return fmt.Errorf("expected tx type *txs.AddValidatorTx but got %T", vdrTx.Unsigned)
}

stakeAmount := tx.Validator.Wght
stakeAmount := tx.Weight()
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 a nit, not strictly relevant to this PR, but I'd rather take it out of the way

Comment on lines +22 to +23
_ DelegatorTx = (*AddDelegatorTx)(nil)
_ ScheduledStaker = (*AddDelegatorTx)(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.

no change to existing tx format

txID := e.Tx.ID()
newStaker, err := state.NewPendingStaker(txID, tx)
if err != nil {
if err := e.addStakerFromStakerTx(tx); 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.

scaffolding, but it's going to be very helpful as we handle non-scheduled stakers in later PRs

EndTime() time.Time
Weight() uint64
PendingPriority() Priority
CurrentPriority() Priority
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This could probably could be renamed Priority() instead of current priority. But I'd wait for that to keep PR scope small

Comment on lines +56 to +60
type ScheduledStaker interface {
Staker
StartTime() time.Time
PendingPriority() Priority
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the main change of the PR, the rest is just unlocking compilation + small nits

@abi87 abi87 self-assigned this Nov 16, 2023
@@ -83,7 +83,7 @@ func (s *Staker) Less(than *Staker) bool {
return bytes.Compare(s.TxID[:], than.TxID[:]) == -1
}

func NewCurrentStaker(txID ids.ID, staker txs.Staker, potentialReward uint64) (*Staker, error) {
func NewCurrentStaker(txID ids.ID, staker txs.ScheduledStaker, potentialReward uint64) (*Staker, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

in next PR we'll change the signature of this function to use txs.Staker instead of txs.ScheduledStaker.

@abi87 abi87 changed the title Drop Pending Stakers 0 - introduced ScheduledStaker Drop Pending Stakers 0 - introduced ScheduledStaker txs Nov 16, 2023
@abi87 abi87 force-pushed the post_durango_staker_txs_0 branch from d38da76 to c6736b6 Compare November 16, 2023 13:41
@abi87 abi87 marked this pull request as ready for review November 16, 2023 13:53
@abi87 abi87 requested a review from dhrubabasu November 16, 2023 13:53
Comment on lines 168 to 173
// Make sure the tx doesn't start too far in the future. This is done last
// to allow the verifier visitor to explicitly check for this error.
maxStartTime := currentTimestamp.Add(MaxFutureStartTime)
if startTime.After(maxStartTime) {
return nil, ErrFutureStakeTime
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved to its own little function to reduce diffs down the line

Comment on lines 127 to 134
var (
currentTimestamp = chainState.GetTimestamp()
startTime = tx.StartTime()
)

// Ensure the proposed validator starts after the current time
startTime := tx.StartTime()
if !currentTimestamp.Before(startTime) {
return nil, fmt.Errorf(
"%w: %s >= %s",
ErrTimestampNotBeforeStartTime,
currentTimestamp,
startTime,
)
if err := checkStakerStartTime(currentTimestamp, startTime); err != nil {
return nil, err
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved to its own little function to reduce diffs down the line

@abi87 abi87 changed the title Drop Pending Stakers 0 - introduced ScheduledStaker txs Drop Pending Stakers 1 - introduced ScheduledStaker txs Nov 19, 2023
@abi87 abi87 requested review from gyuho and marun as code owners November 19, 2023 13:04
@abi87 abi87 changed the base branch from dev to post_durango_nits_0 November 19, 2023 13:05
Base automatically changed from post_durango_nits_0 to dev December 4, 2023 16:38
@abi87 abi87 requested a review from StephenButtolph December 5, 2023 10:31
@StephenButtolph StephenButtolph added this to the v1.10.18 milestone Dec 5, 2023
vms/platformvm/state/state.go Show resolved Hide resolved
vms/platformvm/state/state.go Outdated Show resolved Hide resolved
@abi87 abi87 requested a review from StephenButtolph December 6, 2023 10:13
@StephenButtolph StephenButtolph added this pull request to the merge queue Dec 6, 2023
Merged via the queue into dev with commit b36416d Dec 6, 2023
16 checks passed
@StephenButtolph StephenButtolph deleted the post_durango_staker_txs_0 branch December 6, 2023 19:35
joshua-kim added a commit that referenced this pull request Dec 6, 2023
commit b36416d
Author: Alberto Benegiamo <[email protected]>
Date:   Wed Dec 6 12:12:11 2023 -0700

    Drop Pending Stakers 1 - introduced ScheduledStaker txs (#2323)

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

commit 7df1f3a
Author: Stephen Buttolph <[email protected]>
Date:   Wed Dec 6 13:46:56 2023 -0500

    Restrict GOPROXY (#2434)

commit 21b7ab8
Author: Stephen Buttolph <[email protected]>
Date:   Tue Dec 5 19:43:00 2023 -0500

    Fix platformvm.SetPreference (#2429)

commit ada692a
Author: Stephen Buttolph <[email protected]>
Date:   Tue Dec 5 17:40:03 2023 -0500

    Update minimum golang version to v1.20.12 (#2427)

commit 004a23e
Author: Dhruba Basu <[email protected]>
Date:   Tue Dec 5 12:56:25 2023 -0500

    `vms/platformvm`: Add `decisionTxs` parameter to `NewBanffProposalBlock` (#2411)

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

commit 439dc1e
Author: Alberto Benegiamo <[email protected]>
Date:   Tue Dec 5 10:46:13 2023 -0700

    ProposerVM Extend windows 0 - Cleanup (#2404)

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

commit 477157d
Author: Dhruba Basu <[email protected]>
Date:   Tue Dec 5 11:10:56 2023 -0500

    `vms/platformvm`: Cleanup some block tests (#2422)

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

commit b6700c9
Author: Stephen Buttolph <[email protected]>
Date:   Mon Dec 4 22:24:34 2023 -0500

    Update slices dependency to use Compare (#2424)

    Co-authored-by: James Walker <[email protected]>

commit 5d9e482
Author: Dan Laine <[email protected]>
Date:   Mon Dec 4 19:27:29 2023 -0500

    allow user of `build_fuzz.sh` to specify a directory to fuzz in (#2414)

commit 2e32281
Author: Stephen Buttolph <[email protected]>
Date:   Mon Dec 4 18:30:41 2023 -0500

    Fix duplicated bootstrapper engine termination (#2334)

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

commit b741c19
Author: Dhruba Basu <[email protected]>
Date:   Mon Dec 4 17:49:15 2023 -0500

    `vms/platformvm`: Move `VerifyUniqueInputs` from `verifier` to `backend` (#2410)

commit 6aa20fc
Author: Dhruba Basu <[email protected]>
Date:   Mon Dec 4 17:32:07 2023 -0500

    `vms/platformvm`: Initialize txs in `Transactions` field for `BanffProposalBlock` (#2419)

commit c11accd
Author: Alberto Benegiamo <[email protected]>
Date:   Mon Dec 4 09:16:01 2023 -0700

    Drop Pending Stakers 0 - De-duplicate staking tx verification (#2335)

commit 05ce366
Author: marun <[email protected]>
Date:   Sat Dec 2 13:33:18 2023 -0800

    testing: Update to latest version of ginkgo (#2390)

commit 04af33e
Author: Dhruba Basu <[email protected]>
Date:   Sat Dec 2 16:33:07 2023 -0500

    `vms/platformvm`: Cleanup block builder tests (#2406)

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

commit 7623ffd
Author: Stephen Buttolph <[email protected]>
Date:   Fri Dec 1 19:39:15 2023 -0500

    Update versions for v1.10.17 (#2394)

commit be1a2ad
Author: aaronbuchwald <[email protected]>
Date:   Fri Dec 1 13:31:49 2023 -0500

    Add more descriptive formatted error (#2403)

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

commit 9b85141
Author: Dhruba Basu <[email protected]>
Date:   Thu Nov 30 12:42:36 2023 -0800

    `vms/platformvm`: Move `GetRewardUTXOs`, `GetSubnets`, and `GetChains` to `State` interface (#2402)

commit de3b16c
Author: Joshua Kim <[email protected]>
Date:   Thu Nov 30 12:49:53 2023 -0500

    Add `p2p.Network` component (#2283)

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

commit 0ab2046
Author: marun <[email protected]>
Date:   Thu Nov 30 09:18:59 2023 -0800

    Rename `testnet` fixture to `tmpnet` (#2307)

commit 907b34c
Author: Stephen Buttolph <[email protected]>
Date:   Wed Nov 29 21:32:43 2023 -0500

    Update bootstrap IPs (#2396)

commit 96d451d
Author: Stephen Buttolph <[email protected]>
Date:   Wed Nov 29 18:15:47 2023 -0500

    Periodically PullGossip only from connected validators (#2399)

commit 0da5bcc
Author: Dan Laine <[email protected]>
Date:   Wed Nov 29 16:32:16 2023 -0500

    Remove method `CappedList` from `set.Set` (#2395)

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
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Drop Pending Stakers
4 participants