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 linkedhashmap for P-Chain mempool #1536

Merged
merged 72 commits into from
Nov 22, 2023
Merged

Use linkedhashmap for P-Chain mempool #1536

merged 72 commits into from
Nov 22, 2023

Conversation

gyuho
Copy link
Contributor

@gyuho gyuho commented May 22, 2023

Why this should be merged

More consistent mempool implementation with recent AVM changes.

Replace #1455.

How this works

Replace txheap by age with linked hashmap

How this was tested

UT

@gyuho gyuho self-assigned this May 22, 2023
StephenButtolph
StephenButtolph reviewed May 25, 2023
View reviewed changes
vms/platformvm/txs/mempool/mempool.go Show resolved Hide resolved
vms/platformvm/txs/mempool/mempool.go Show resolved Hide resolved
vms/platformvm/txs/mempool/mempool.go Outdated Show resolved Hide resolved
vms/platformvm/txs/mempool/mempool.go Outdated Show resolved Hide resolved
vms/platformvm/txs/mempool/mempool.go Outdated Show resolved Hide resolved
@gyuho gyuho added the DO NOT MERGE This PR must not be merged in its current state label May 26, 2023
@gyuho gyuho removed the DO NOT MERGE This PR must not be merged in its current state label May 28, 2023
abi87
abi87 reviewed May 29, 2023
View reviewed changes
vms/platformvm/txs/txheap/by_age.go
// Copyright (C) 2019-2023, Ava Labs, Inc. All rights reserved.
// See the file LICENSE for licensing terms.

package txheap
Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC we could drop the txheap package completelly, once the mempool sorts transactions in a FIFO order. Please cleanup all you can here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Found out BuildGenesis P-Chain API still uses it :)

abi87
abi87 reviewed May 29, 2023
View reviewed changes
vms/platformvm/blocks/builder/builder.go Outdated
Comment on lines 249 to 256
for iter.Next() {
tx := iter.Value()
stakerTx, ok := tx.Unsigned.(txs.Staker)
if !ok {
continue
}

startTime := stakerTx.StartTime()
Copy link
Contributor

@abi87 abi87 May 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This is probably slower than the pre-sorted version with mempool double tx container (decision and stakers). It may be not be a big deal.
Note that following continuous staking fork activation, we won't care anymore about startTime (stakers will start staking as soon as they'll be accepted), hence this all check will be gone

abi87
abi87 suggested changes May 29, 2023
View reviewed changes
Copy link
Contributor

@abi87 abi87 left a comment

Choose a reason for hiding this comment

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

Some questions about mempool cleanup with the new FIFO policy.
I am generally in favour of the change.
Note also that with continuous staking we plan to drop pending stakers and the startTime concept altogether. Hence, post continuous staking fork activation, we could carry out a cleanup pretty similar to what you propose here!

@gyuho gyuho added the DO NOT MERGE This PR must not be merged in its current state label Jun 2, 2023
@gyuho
remove PeekTx
4c93854 
Signed-off-by: Gyuho Lee <[email protected]>
@gyuho gyuho removed the DO NOT MERGE This PR must not be merged in its current state label Jun 2, 2023
@dhrubabasu dhrubabasu marked this pull request as ready for review November 16, 2023 22:17
@dhrubabasu dhrubabasu changed the base branch from remove-add-unverified-tx to dev November 16, 2023 22:19
@dhrubabasu dhrubabasu added this to the v1.10.16 milestone Nov 16, 2023
dhrubabasu
dhrubabasu reviewed Nov 17, 2023
View reviewed changes
vms/platformvm/txs/mempool/mempool.go
// Drops all [txs.Staker] transactions whose [StartTime] is before
// [minStartTime] from [mempool]. The dropped tx ids are returned.
//
// TODO: Remove once [StartTime] field is ignored in staker txs
func DropExpiredStakerTxs(mempool Mempool, minStartTime time.Time) []ids.ID {
func (m *mempool) DropExpiredStakerTxs(minStartTime time.Time) []ids.ID {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not efficient, we have to iterate through the entire mempool. The fix for this is to move verification of the StartTime to the block builder- this will be done in a follow-up.

@StephenButtolph StephenButtolph merged commit 4ce0d67 into dev Nov 22, 2023
16 checks passed
@StephenButtolph StephenButtolph deleted the gyuho-p-mempool-like-avm branch November 22, 2023 17:39
joshua-kim added a commit that referenced this pull request Nov 29, 2023
@joshua-kim
Squashed commit of the following:
738bd2e 
commit c5169a3
Author: David Boehm <[email protected]>
Date:   Wed Nov 29 15:34:57 2023 -0500

    MerkleDB Remove ID from Node to reduce size and removal channel creation. (#2324)

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

commit 56c2ad9
Author: Stephen Buttolph <[email protected]>
Date:   Wed Nov 29 14:35:33 2023 -0500

    Replace periodic push accepted gossip with pull preference gossip for block discovery (#2367)

commit 1dddf30
Author: Dhruba Basu <[email protected]>
Date:   Wed Nov 29 07:16:27 2023 -0800

    Rename `D` to `Durango` (#2389)

commit 6ed238c
Author: Stephen Buttolph <[email protected]>
Date:   Wed Nov 29 01:05:16 2023 -0500

    Add block source metrics to monitor gossip (#2386)

commit 21c14b1
Author: Stephen Buttolph <[email protected]>
Date:   Tue Nov 28 23:59:51 2023 -0500

    Add metric to track the stake weight of block providers (#2376)

commit 02ae8d9
Author: Stephen Buttolph <[email protected]>
Date:   Tue Nov 28 15:10:25 2023 -0500

    Unexport RequestID from snowman engine (#2384)

commit be422a0
Author: Stephen Buttolph <[email protected]>
Date:   Tue Nov 28 13:58:45 2023 -0500

    Implement generic bimap (#2383)

commit d7e7ff5
Author: Dhruba Basu <[email protected]>
Date:   Tue Nov 28 10:10:49 2023 -0800

    `vms/avm`: Rename `states` pkg to `state` (#2381)

commit 6e52922
Author: Dhruba Basu <[email protected]>
Date:   Tue Nov 28 07:55:27 2023 -0800

    `vms/platformvm`: Move `toEngine` channel to mempool (#2333)

commit 590ad12
Author: Stephen Buttolph <[email protected]>
Date:   Mon Nov 27 16:57:15 2023 -0500

    Remove common bootstrapper (#2297)

commit 9ad213c
Author: Dhruba Basu <[email protected]>
Date:   Mon Nov 27 11:45:03 2023 -0800

    `vms/platformvm`: Consolidate `state` pkg mocks (#2370)

commit 3a424d0
Author: Stephen Buttolph <[email protected]>
Date:   Mon Nov 27 13:36:03 2023 -0500

    Remove `engine.GetVM` (#2374)

commit b9ab41a
Author: felipemadero <[email protected]>
Date:   Mon Nov 27 14:44:54 2023 -0300

    Add option to provide BLS keys to validators in the genesis (#2371)

commit 42161aa
Author: Dhruba Basu <[email protected]>
Date:   Mon Nov 27 09:07:48 2023 -0800

    `vms/platformvm`: Adjust `Diff.Apply` signature (#2368)

commit 79e572d
Author: felipemadero <[email protected]>
Date:   Mon Nov 27 13:43:32 2023 -0300

    Avoid closing stdout and stderr during log close (#2372)

commit e04dad8
Author: Joshua Kim <[email protected]>
Date:   Sun Nov 26 13:13:56 2023 -0500

    Move peerTracker from x/sync to network/p2p (#2356)

commit b1b051a
Author: Dhruba Basu <[email protected]>
Date:   Sat Nov 25 16:32:05 2023 -0800

    `vms/platformvm`: Remove unused `withMetrics` txheap (#2373)

commit 8d9b93c
Author: Stephen Buttolph <[email protected]>
Date:   Fri Nov 24 12:45:51 2023 -0500

    Add metric for duration between block timestamp and block acceptance time (#2366)

commit 9353569
Author: Stephen Buttolph <[email protected]>
Date:   Fri Nov 24 11:23:36 2023 -0500

    Document storage growth in readme (#2364)

commit 6ad31d6
Author: Dhruba Basu <[email protected]>
Date:   Wed Nov 22 17:40:43 2023 -0800

    Remove Banff check from mempool verifier (#2360)

commit 62df19c
Author: Stephen Buttolph <[email protected]>
Date:   Wed Nov 22 19:47:37 2023 -0500

    Update versions for v1.10.16 (#2353)

commit 48c541c
Author: Stephen Buttolph <[email protected]>
Date:   Wed Nov 22 17:46:14 2023 -0500

    Fix P-chain mempool tx count metric (#2361)

commit 4ce0d67
Author: Gyuho Lee <[email protected]>
Date:   Thu Nov 23 01:39:53 2023 +0800

    Use linkedhashmap for P-Chain mempool (#1536)

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

commit f2f6d0a
Author: Patrick O'Grady <[email protected]>
Date:   Tue Nov 21 22:07:24 2023 -0600

    [vms/proposervm] Update Build Heuristic (#2348)

    Co-authored-by: Alberto Benegiamo <[email protected]>

commit 459f8ba
Author: Stephen Buttolph <[email protected]>
Date:   Tue Nov 21 19:24:02 2023 -0500

    Reduce the size of the OracleBlock interface (#2355)

commit f7cc69b
Author: Stephen Buttolph <[email protected]>
Date:   Tue Nov 21 17:13:17 2023 -0500

    Expand consensus health check (#2354)

commit 2b737d5
Author: Stephen Buttolph <[email protected]>
Date:   Tue Nov 21 13:26:55 2023 -0500

    Cleanup snowman consensus metrics (#2349)

Signed-off-by: Joshua Kim <[email protected]>
@dhrubabasu dhrubabasu mentioned this pull request Dec 24, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@StephenButtolph StephenButtolph StephenButtolph approved these changes

@danlaine danlaine danlaine approved these changes

@dhrubabasu dhrubabasu dhrubabasu approved these changes

@abi87 abi87 Awaiting requested review from abi87

Assignees

@dhrubabasu dhrubabasu

Labels
cleanup Code quality improvement
Projects
Platform Engineering Group
Archived in project
Ski Patrol
Status: In Review 👀
Milestone
v1.10.16
Development

Successfully merging this pull request may close these issues.
5 participants
@gyuho @dhrubabasu @danlaine @abi87 @StephenButtolph