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

Update node initiation to handle Persistent DB #61

Merged
merged 1 commit into from
May 13, 2022
Merged

Update node initiation to handle Persistent DB #61

merged 1 commit into from
May 13, 2022

Conversation

rajarshimaitra
Copy link
Contributor

In usage of recently added #52 , the expected behaviour for Persistent type datadir is, the new instance of core will inherent all the existing data. Which is working as expected for other data (blocks, chainstate etc), except the wallet directory.

The current behavior tries to always create a new default wallet at initiation. In case of persistent DB that gives rpc error "wallet file already exists".

This PR updates the init process to handle persistent type database properly..

A test to check the behavior is also added.

@rajarshimaitra
Copy link
Contributor Author

@RCasatta tried to fix the test failure.. But now its failing in some other tests.. Not the newly added one..
Any idea why only two tests failed?? Can't reproduce them in local..

@RCasatta
Copy link
Collaborator

Hi @raj, sorry for not having a look in a timely manner. I see there are multiple issues:

  1. you should drop 0c47bd7 and related merge commit since they are now on master
  2. one of the CI error (the one failing assert on amounts) is unfortunately some flakiness we have in a test that I have to examine better but for the time being I usually resolve by re-kicking (so don't bother if you see this)
  3. we can't really do 995e0f7 to fix the test, because test runs in parallel you cannot alter the global state. It should not be a big problem in this case, but I prefer not to. Please temporarily comment out the CI configuration line where we set TEMPDIR instead.

Once ready I will check again.

@rajarshimaitra rajarshimaitra force-pushed the persistency-fix branch 2 times, most recently from 747b61a to d1aeecf Compare May 11, 2022 17:26
@rajarshimaitra
Copy link
Contributor Author

Rebased on current master and cleared commit history.. Not really sure how those previous commits got added.. Removed the TEMPDIR_ROOT from CI env..

@rajarshimaitra rajarshimaitra mentioned this pull request May 12, 2022
Closed
RCasatta
RCasatta reviewed May 12, 2022
View reviewed changes
.github/workflows/test.yml Outdated Show resolved Hide resolved
src/lib.rs Outdated
Comment on lines 307 to 310
client_base
.create_wallet("default", None, None, None, None)
.unwrap();
break Client::new(&node_url_default, Auth::CookieFile(cookie_file.clone()))
Copy link
Collaborator

@RCasatta RCasatta May 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

did you consider a more "ask-for-forgiveness" approach? I suspect it would be shorter/cleaner

like matching on client_base.create_wallet("default", None, None, None, None) and in case of error doing the staticdir case checks

Copy link
Contributor Author

@rajarshimaitra rajarshimaitra May 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Ah.. I think that makes more sense and I probably took your simplification idea to extreme, 😅 and removed almost all of the code.. But I think the logic still stands and we don't necessarily need to find out the existing wallet file to try to load it up.. And yes its much simpler than what I was trying..

src/lib.rs Outdated Show resolved Hide resolved
@rajarshimaitra
Copy link
Contributor Author

Thanks Ricardo for the suggestion.. Updated with the same.. Kept the commit separate for easier review.. Let me know if you need a squash..

@RCasatta
Copy link
Collaborator

00bc944 LGTM

yes, please squash since commits touch the same code, I will ack and merge

@rajarshimaitra
Fix behavior to handle persistent db
2d5661d 
In case of persistent db, check if the "default" wallet file already
exists. Load it if it does. Or else create new wallet.

Always create new wallet for temp db.

Remove global temp env variable in CI.
@rajarshimaitra
Copy link
Contributor Author

Done..

@RCasatta
Copy link
Collaborator

Thanks!

ACK 2d5661d

@RCasatta RCasatta merged commit 2d5661d into rust-bitcoin:master May 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RCasatta RCasatta RCasatta left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rajarshimaitra @RCasatta