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

validation: Remove useless call to mempool->clear() #20030

Closed
wants to merge 1 commit into from
Closed

validation: Remove useless call to mempool->clear() #20030

wants to merge 1 commit into from

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Sep 28, 2020

The mempool.clear() in UnloadBlockIndex has been added in commit 51598b2
to clear global state between unit tests. Now that there is no global
mempool anymore, this it not needed anymore. Also, the clear isn't used
for anything else in the current code.

@practicalswift
Copy link
Contributor

Concept ACK

Thanks for cleaning up old cruft! :)

@maflcko maflcko mentioned this pull request Sep 28, 2020
Closed
sipa
sipa reviewed Sep 28, 2020
View reviewed changes
src/init.cpp Outdated
@@ -1563,7 +1563,7 @@ bool AppInitMain(const util::Ref& context, NodeContext& node, interfaces::BlockA
chainman.m_total_coinstip_cache = nCoinCacheUsage;
chainman.m_total_coinsdb_cache = nCoinDBCache;

UnloadBlockIndex(node.mempool.get(), chainman);
UnloadBlockIndex(chainman);
Copy link
Member

Choose a reason for hiding this comment

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

Are we sure that there is no need to reset the mempool here? Could anything have been added to it further in this loop?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, otherwise I wouldn't have opened the pull request. You probably wanted to say that the review effort is not worth it?

Copy link
Member

Choose a reason for hiding this comment

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

No, I just wanted to make sure you hadn't missed the fact that there is a loop here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah ok. I might reopen some other day, unless someone beats me to it.

@maflcko maflcko closed this Sep 28, 2020
@maflcko maflcko deleted the 2009-valMemClear branch September 28, 2020 19:29
@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 28, 2020 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #24595 (deploymentstatus: move g_versionbitscache global to ChainstateManager by ajtowns)
  • #24232 (assumeutxo: add init and completion logic by jamesob)
  • #24008 (assumeutxo: net_processing changes by jamesob)
  • #22564 (refactor: Move mutable globals cleared in ::UnloadBlockIndex to BlockManager by dongcarl)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@fanquake
Copy link
Member

Ah ok. I might reopen some other day, unless someone beats me to it.

Concept ACK. If these calls really are no-longer doing anything I can't see why they shouldn't be removed and UnloadBlockIndex simplified.

@maflcko maflcko mentioned this pull request Jan 26, 2022
Merged
@maflcko maflcko restored the 2009-valMemClear branch January 26, 2022 17:59
@maflcko maflcko reopened this Jan 26, 2022
@DrahtBot DrahtBot mentioned this pull request Jan 27, 2022
Closed
@maflcko maflcko mentioned this pull request Jan 28, 2022
Closed
This was referenced Feb 2, 2022
Closed
Closed
This was referenced Feb 9, 2022
Merged
Merged
This was referenced Feb 24, 2022
Merged
Closed
@maflcko maflcko force-pushed the 2009-valMemClear branch from fa6350e to fabe5f6 Compare March 7, 2022 14:19
@DrahtBot DrahtBot mentioned this pull request Mar 19, 2022
Merged
@DrahtBot DrahtBot mentioned this pull request Apr 19, 2022
Merged
validation: Remove useless call to mempool->clear()
1721021 
The mempool.clear() in UnloadBlockIndex has been added in commit 51598b2
to clear global state between unit tests. Now that there is no global
mempool anymore, this it not needed anymore. Also, the clear isn't used
for anything else in the current code.
@maflcko
Copy link
Member Author

maflcko commented Apr 20, 2022

Probably fixed in #22564 (refactor: Move mutable globals cleared in ::UnloadBlockIndex to BlockManager by dongcarl), so please review that first.

@maflcko
Copy link
Member Author

maflcko commented Apr 20, 2022

Leaving open for now due to #24145 (comment)

@DrahtBot
Copy link
Contributor

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".

@maflcko maflcko closed this Apr 28, 2022
@maflcko maflcko deleted the 2009-valMemClear branch April 28, 2022 14:49
@bitcoin bitcoin locked and limited conversation to collaborators Apr 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sipa sipa sipa left review comments

Assignees
No one assigned
Labels
Needs rebase Refactoring Validation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@maflcko @practicalswift @DrahtBot @fanquake @sipa