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

vms/platformvm: Move toEngine channel to mempool #2333

Merged
merged 10 commits into from
Nov 28, 2023
Merged

vms/platformvm: Move toEngine channel to mempool #2333

merged 10 commits into from
Nov 28, 2023

Conversation

dhrubabasu
Copy link
Contributor

Why this should be merged

Removes the circular dependency when creating the mempool

How this works

The mempool will be the only interaction with the consensus engine. Consumers, like the block builder, will rely on the mempool reference to send messages to the engine.

How this was tested

CI

@dhrubabasu dhrubabasu added the cleanup Code quality improvement label Nov 17, 2023
@dhrubabasu dhrubabasu added this to the v1.10.16 milestone Nov 17, 2023
@dhrubabasu dhrubabasu self-assigned this Nov 17, 2023
abi87
abi87 reviewed Nov 19, 2023
View reviewed changes
vms/platformvm/txs/mempool/mempool.go
// built. If [emptyBlockPermitted] is true, the notification will be sent
// if there are no transactions in the mempool. Otherwise, a notification
// will only be sent if there is at least one transaction in the mempool.
RequestBuildBlock(emptyBlockPermitted bool)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to get rid of the boolean parameter here. RequestBuildBlock is called with false only if a tx has just been added to the mempool; moreover every time we add a transaction to the mempool, we then request the engine to build the block.
So I'd suggest we:

  1. Turn Add(*txs.Tx) into Add([]*txs.Tx)
  2. Ping the engine directly at the end of Add(, instead of asking the Add client to do it
  3. Drop the boolead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Having the engine be pinged directly at the end of Add is a bit strange since we'll have two ways to request a block to be built (Add or RequestBuildBlock). I'd prefer to be consistent and have the mempool client always be responsible for calling RequestBuildBlock instead of just in the case there are no txs

The boolean override isn't great but I think it's better to be explicit: "I want to build a block and am OK with it being empty"

Copy link
Contributor

Choose a reason for hiding this comment

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

We already have two ways to build a block rn, from mempool txs and from stakers events. No added strangeness.
Every time a client adds a transaction to the mempool. it has to ping the engine to build a block out of it. I think we don't need the client to do the work here, we can automate this action by letting the mempool it itself as soon as it has added the tx. We would remove some client code too.
I don't think it really matters if the block is empty or not. For sure if we automatize the mempool pinging the engine we can have a simpler interface (no boolean) and we only have the block builder really in need to requesto block building outside the mempool.

Copy link
Contributor

Choose a reason for hiding this comment

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

I coded my suggestion here #2346

abi87
abi87 previously requested changes Nov 19, 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.

I dislike the boolean and I think a simple change to Mempool.Add signature may remove it.

@abi87 abi87 mentioned this pull request Nov 21, 2023
Closed
@StephenButtolph StephenButtolph modified the milestones: v1.10.16, v1.10.17 Nov 22, 2023
danlaine
danlaine approved these changes Nov 28, 2023
View reviewed changes
vms/platformvm/block/builder/builder.go
Comment on lines +43 to +45
// ResetBlockTimer schedules a timer to notify the consensus engine once
// there is a block ready to be built. If a block is ready to be built when
// this function is called, the engine will be notified directly.
Copy link

Choose a reason for hiding this comment

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

I know this comment is pasted from existing code, but I find it kind of confusing. "Schedules a timer" makes it sound like the consensus engine will be notified after some duration, but "once there is a block ready to be built" makes it sound like the consensus engine will be notified after some event rather than after a duration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah it is pretty confusing- I think it stemmed from building the block twice. In the follow-up, I clean up the comment + logic (#2380). Will keep as-is in this PR for now

vms/platformvm/block/builder/builder.go Outdated
@@ -192,7 +190,7 @@ func (b *builder) setNextBuildBlockTime() {

if _, err := b.buildBlock(); err == nil {
// We can build a block now
b.notifyBlockReady()
b.Mempool.RequestBuildBlock(true)
Copy link

Choose a reason for hiding this comment

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

fwiw I like commenting "naked" args like this but fine as is too

Suggested change
b.Mempool.RequestBuildBlock(true)
b.Mempool.RequestBuildBlock(true /*emptyBlockPermitted*/)

Copy link

Choose a reason for hiding this comment

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

Also, might be worth commenting why we're willing to build an empty block here but not elsewhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Commented the naked arg, this function has slightly different behavior in the follow-up so will add the comment there: #2380

vms/platformvm/txs/mempool/mempool.go Outdated Show resolved Hide resolved
@dhrubabasu dhrubabasu requested a review from abi87 November 28, 2023 15:35
@StephenButtolph StephenButtolph added this pull request to the merge queue Nov 28, 2023
Merged via the queue into dev with commit 6e52922 Nov 28, 2023
16 checks passed
@StephenButtolph StephenButtolph deleted the move-engine-to-mempool branch November 28, 2023 16:18
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]>
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

@abi87 abi87 Awaiting requested review from abi87

Assignees

@dhrubabasu dhrubabasu

Labels
cleanup Code quality improvement
Projects
Platform Engineering Group
Archived in project
Milestone
v1.10.17
Development

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