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

[Malleability] Fix root interfaces implementations under stdmap package #7121

Open
wants to merge 43 commits into
base: 6646-mempool-refactoring
Choose a base branch
from

Conversation

UlyanaAndrukhiv
Copy link
Contributor

@UlyanaAndrukhiv UlyanaAndrukhiv commented Mar 6, 2025

Close: #7075

Notes

While working on this refactor, I noticed that some of these memory pools (BlockByCollections, Blocks, ChunkDataPacks, Collections, Results, and Queues) are not currently used. However, I am not certain if they can be removed.

Context

This PR updates the stdmap package to align with the new structure of BackData and Backend for improved memory pool management. The following components have been refactored :

  • Assignments
  • BlockByCollections
  • Blocks
  • ChunkDataPacks
  • ChunkRequests
  • ChunkStatuses
  • Collections
  • Guarantees
  • IdentifierMap
  • IncorporatedResultSeals
  • PendingReceipts
  • Queues
  • Receipts
  • Results
  • Times
  • TransactionTimings
  • Transactions

Additionally, all relevant tests have been updated to reflect these changes and pass successfully.

… github.com:onflow/flow-go into UlianaAndrukhiv/7075-fix-root-interfaces-implementations-under-stdmap
@UlyanaAndrukhiv UlyanaAndrukhiv marked this pull request as ready for review March 6, 2025 15:25
@UlyanaAndrukhiv UlyanaAndrukhiv requested a review from a team as a code owner March 6, 2025 15:25
@UlyanaAndrukhiv UlyanaAndrukhiv requested review from jordanschalm and removed request for a team March 7, 2025 10:12
Copy link
Member

@durkmurder durkmurder left a comment

Choose a reason for hiding this comment

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

Finished first round, will take another pass after addressing the comments

Copy link
Member

@durkmurder durkmurder left a comment

Choose a reason for hiding this comment

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

Good job ⭐

Co-authored-by: Yurii Oleksyshyn <[email protected]>
Base automatically changed from andron/updte-backend-to-work-with-generics-backdata to 6646-mempool-refactoring March 11, 2025 09:53
Copy link
Member

@jordanschalm jordanschalm left a comment

Choose a reason for hiding this comment

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

Very nice work on this 👏

Feedback falls mainly into two categories:

  1. updating documentation and naming to avoid confusion about what key is used in a mempool
  2. removing type-specific methods which are no longer needed with generics

The second category is largely additive to what you have already done, so feel free to create a separate PR if it's easier. Also, if addressing these comments is more complicated than removing code and renaming methods, and is taking substantial time, feel free to skip them.

@@ -7,13 +7,13 @@ import (

// Assignments implements the chunk assignment memory pool.
type Assignments struct {
Copy link
Member

Choose a reason for hiding this comment

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

This Assignments type largely existed to create a type-safe wrapper (for flow.Assignment) around the general-purpose mempool which operated on Entity-typed values. Now that we have made the Backend generic, I believe we can remove the majority of the boilerplate code in the type-specific mempools.

Simplifying type-specific mempool implementations (using Assignments as example)

We can redefine Assignments minimally as:

package stdmap

// Assignments implements the chunk assignment memory pool.
type Assignments struct {
	*Backend[flow.Identifier, *chunkmodels.Assignment]
}

// NewAssignments creates a new memory pool for Assignments.
func NewAssignments(limit uint) (*Assignments, error) {
	return &Assignments{NewBackend(WithLimit[flow.Identifier, *chunkmodels.Assignment](limit))}, nil
}

Assignments has no other specific functionality. All of its existing methods simply delegate to the Backend. Since Backend is embedded, we can remove all methods defined on Assignments (complete implementation is the code snippet above).

Updating interfaces

Currently stdmap.Assignments implements the mempool.Assignments interface. That interface has a few inconsistencies compared to the generic Backend:

  • Getter is named ByID rather than Get (1 usage per my IDE)
  • All method returns a list rather than a map (0 usages per my IDE)

We can create a generic interface to correspond to the generic implementation:

package mempool

// documentation omitted but should be copied from Backend
type Mempool[K comparable, V any] interface {
	Has(K) bool
	Get(K) (V, bool)
	Add(K, V) bool
	Size() uint
	All() map[K]V
}

Then define the type-specific interface:

package mempool

type Assignments Mempool[flow.Identifier, *chunkmodels.Assignment]

Summary and Suggested Changes

For type-specific mempools where the implementation has few or no differences compared to the generic Backend, I think it is worthwhile to remove the boilerplate code as suggested above. I suspect most mempools will be like Assignments a basically be type-conversion wrappers around Backend.

@@ -16,23 +16,15 @@ import (
// wraps the ChunkDataPackRequests around an internal ChunkRequestStatus data object, and maintains the wrapped
// version in memory.
type ChunkRequests struct {
Copy link
Member

Choose a reason for hiding this comment

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

There is a lot of custom logic here, but we can remove the pass-through methods (eg. Remove, Size)

@@ -10,47 +9,31 @@ import (
// Times implements the times memory pool used to store time.Times for an idetifier to track transaction metrics in
// access nodes
type Times struct {
Copy link
Member

Choose a reason for hiding this comment

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

Similar case to stdmap.Approvals.

@@ -7,63 +7,51 @@ import (
// TransactionTimings implements the transaction timings memory pool of access nodes,
// used to store transaction timings to report the timing of individual transactions
type TransactionTimings struct {
Copy link
Member

Choose a reason for hiding this comment

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

Similar case to stdmap.Approvals.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Malleability] Fix root interfaces implementations under stdmap package
3 participants