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

feat: only require reindexing when the index was on going to off #6203

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

PastaPastaPasta
Copy link
Member

What was done?

It does not seem reasonable that we need to reindex when turning indexes off. this logic was introduced in e078109

How Has This Been Tested?

Hasn't

Breaking Changes

None, basically

Checklist:

Go over all the following points, and put an x in all the boxes that apply.

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation
  • I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

@PastaPastaPasta PastaPastaPasta added this to the 21.2 milestone Aug 12, 2024
@DashCoreAutoGuix
Copy link

Guix Automation has began to build this PR tagged as v21.1.0-devpr6203.c412d905. A new comment will be made when the image is pushed.

@DashCoreAutoGuix
Copy link

Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v21.1.0-devpr6203.c412d905. The image should be on dockerhub soon.

src/init.cpp Outdated
@@ -1917,19 +1917,19 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
}

// Check for changed -addressindex state
if (fAddressIndex != args.GetBoolArg("-addressindex", DEFAULT_ADDRESSINDEX)) {
if (!fAddressIndex && fAddressIndex != args.GetBoolArg("-addressindex", DEFAULT_ADDRESSINDEX)) {
strLoadError = _("You need to rebuild the database using -reindex to change -addressindex");
Copy link

Choose a reason for hiding this comment

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

Makes sense I guess... let's also update the error message maybe?

Suggested change
strLoadError = _("You need to rebuild the database using -reindex to change -addressindex");
strLoadError = _("You need to rebuild the database using -reindex to enable -addressindex");

Same below. Will have to update tests too.

@thephez
Copy link
Collaborator

thephez commented Aug 12, 2024

I think the title supposed to be "on going to off"?

@PastaPastaPasta PastaPastaPasta changed the title feat: only require reindexing when the index was off going to off feat: only require reindexing when the index was on going to off Aug 13, 2024
@PastaPastaPasta
Copy link
Member Author

Anything else needed here?

@UdjinM6
Copy link

UdjinM6 commented Aug 21, 2024

Anything else needed here?

#6203 (comment):

Will have to update tests too.

@PastaPastaPasta
Copy link
Member Author

Anything else needed here?

#6203 (comment):

Will have to update tests too.

Whoops! I swear I already did that locally, just forgot to push it I guess

Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

see below + same for 2 more tests feature_spentindex.py and feature_timestampindex.py

@@ -42,12 +42,12 @@ def setup_network(self):
def run_test(self):
self.log.info("Test that settings can't be changed without -reindex...")
self.stop_node(1)
self.nodes[1].assert_start_raises_init_error(["-addressindex=0"], "You need to rebuild the database using -reindex to change -addressindex", match=ErrorMatch.PARTIAL_REGEX)
self.nodes[1].assert_start_raises_init_error(["-addressindex=0"], "You need to rebuild the database using -reindex to enable -addressindex", match=ErrorMatch.PARTIAL_REGEX)
Copy link

Choose a reason for hiding this comment

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

-addressindex=0 == disable
you don't need to reindex to disable it...

Comment on lines 44 to +45
self.stop_node(1)
self.nodes[1].assert_start_raises_init_error(["-addressindex=0"], "You need to rebuild the database using -reindex to change -addressindex", match=ErrorMatch.PARTIAL_REGEX)
self.start_node(1, ["-addressindex=0", "-reindex"])
self.start_node(1, ["-addressindex=0"])
Copy link
Collaborator

@knst knst Sep 24, 2024

Choose a reason for hiding this comment

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

-self.stop_node(1)
-self.start_node(1, ["-addressindex=0"])
+self.restart_node(1, "[-addressindex=0"])

Copy link
Collaborator

@knst knst left a comment

Choose a reason for hiding this comment

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

let's wait CI, also consider replacement stop+start to restart_nodes which has been refactored in all functional tests long time ago

@UdjinM6
Copy link

UdjinM6 commented Sep 24, 2024

pls consider 7399b7a aa06f33

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.

5 participants