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

vms/platformvm: Remove EnableAdding and DisableAdding from Mempool interface #2463

Merged
merged 3 commits into from
Dec 11, 2023

Conversation

dhrubabasu
Copy link
Contributor

@dhrubabasu dhrubabasu commented Dec 11, 2023

Why this should be merged

These methods were added to prevent an infinite recursion bug. This recursion no longer exists -> cleanup 🧹

How this works

Remove methods

How this was tested

CI

@dhrubabasu dhrubabasu changed the title vms/platformvm: Remove EnableAdding and DisableAdding vms/platformvm: Remove EnableAdding and DisableAdding from Mempool interface Dec 11, 2023
@dhrubabasu dhrubabasu self-assigned this Dec 11, 2023
@dhrubabasu dhrubabasu added the cleanup Code quality improvement label Dec 11, 2023
@dhrubabasu dhrubabasu added this to the v1.10.18 milestone Dec 11, 2023
@@ -192,9 +192,7 @@ func (b *builder) ShutdownBlockTimer() {
// This method removes the transactions from the returned
// blocks from the mempool.
func (b *builder) BuildBlock(context.Context) (snowman.Block, error) {
b.Mempool.DisableAdding()
Copy link
Contributor

@patrick-ogrady patrick-ogrady Dec 11, 2023

Choose a reason for hiding this comment

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

Do we need to add some locking to the mempool now? I think there are a number of internal fields (like bytesAvailable) modified on the call to Add, for example?

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, the context lock is held when the Add function is called

Copy link
Contributor

@StephenButtolph StephenButtolph left a comment

Choose a reason for hiding this comment

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

long outstayed its welcome

@StephenButtolph StephenButtolph added this pull request to the merge queue Dec 11, 2023
Merged via the queue into dev with commit 4e5b20e Dec 11, 2023
16 checks passed
@StephenButtolph StephenButtolph deleted the slim-mempool-inft branch December 11, 2023 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Code quality improvement
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants