Wiring for Custom Datadir

[D33r-Gee]

This pull request builds upon previous issues (#390, #392, and #397) by re-introducing the functionality for users to specify a custom data directory (datadir) during the onboarding process.

The majority of changes in qml/bitcoin.cpp are mainly splitting the onboarding and node creation so that the user can set their preferred datatdir it's inspired by the qt/bitcoin.cpp however it uses the anonymous namespace instead of using a class. The intention came from feedback on #390

In the QML pages, the node_model context property is no longer been initialized during onboarding. We're now using options_model instead. This means the code has been updated to use the options_model for settings that were previously set with the node_model context property.

Support for the Android version is being added to this PR.

[johnny9]

https://github.com/bitcoin-core/gui-qml/actions/runs/9523738145/job/26255610402?pr=408

qml/models/options_model.cpp:232:9: error: use of undeclared identifier 'settings'
        settings.setValue("strDataDir", path);
        ^

Android builds are failing. Will need that fixed.

My testing on desktop seemed to work well. I had one odd thing happening. After onboarding, I tried again with using -resetguisettings and the onboarding page had a strange Custom path as the initial selection. I imagine this is maybe just resetguisettings not clearing out the datadir in QSettings but haven't dug in yet.

[D33r-Gee]

Thanks @johnny9 for testing

Android builds are failing. Will need that fixed.

addressed it for now with the latest push 679818a

My testing on desktop seemed to work well. I had one odd thing happening. After onboarding, I tried again with using -> > resetguisettings and the onboarding page had a strange Custom path as the initial selection. I imagine this is maybe just resetguisettings not clearing out the datadir in QSettings but haven't dug in yet.

Addressed the above for now... although there are a few issues when using the -resetguisettings flag, so a question I have is is it crucial to address them here or can they be addressed in later PRs that specifically tackle them?

[MarnixCroes]

679818a

• during onboarding, when clicking Custom datadir it gives this output in terminal

Model size of -106 is less than 0
Model size of -6 is less than 0

• startNode and startOnboarding have a lot of duplicate code. maybe consider extracting common functionality to avoid duplication?

[D33r-Gee]

679818a

Thanks testing and reviewing @MarnixCroes

• during onboarding, when clicking Custom datadir it gives this output in terminal

Model size of -106 is less than 0
Model size of -6 is less than 0

Hmmm... not sure what that is... will look into it

• startNode and startOnboarding have a lot of duplicate code. maybe consider extracting common functionality to avoid duplication?

Yes some refinement can be done there... will look into how best approach it

[D33r-Gee]

Currently tackling a strange beahvior involving the custom datadir and reading the settings files:

when going through first setup it up it works great and the settings.json file is created in the the proper place...

However on restart it seems like it doesn't read the settings.json file from the new datadir and only looks in the default directory.

Right now trying to see how that can be addressed... will update soon with what I find

[D33r-Gee]

with aebfc89 addressed the settings.json file not being read on restart by moving the gArgs.ReadSettingsFile right before node creation in qml/bitcoin.cpp

Now will tackle the -resetguisettings not updating the datadir location right away

[D33r-Gee]

with the latest update 5ef5d19 addressed the -resetguisettings discrepencies.

It now resets to defaults and allows for changing the datadir

Next will tackle code optimization...

[D33r-Gee]

This update 6d4c111 is the first pass at optimizing the qml/bitcoin.cpp code.

There are no major functionality updates, mainly code consolidation.

[D33r-Gee]

• during onboarding, when clicking Custom datadir it gives this output in terminal

Model size of -106 is less than 0
Model size of -6 is less than 0

@MarnixCroes are you still seeing the above pop-up with 6d4c111?

[pablomartin4btc]

tACK 6d4c111

Normal use case, running bitcoin-qt and setting the custom data dir in one go, works as expected.

A use case that has an issue I think, which is selecting a custom datadir, going back to the default, I can't manage to set a custom datadir back, the default remains selected and this is working in main.

I got this crash (not always but quite common) when I click on the "Custom" component:

./src/qt/bitcoin-qt -signet
Warning: Ignoring XDG_SESSION_TYPE=wayland on Gnome. Use QT_QPA_PLATFORM=wayland to run on Wayland anyway.
**
Gtk:ERROR:../../../../gtk/gtkpathbar.c:1342:set_button_image_get_info_cb: assertion failed: (cancellable == data->button_data->cancellable)
Bail out! Gtk:ERROR:../../../../gtk/gtkpathbar.c:1342:set_button_image_get_info_cb: assertion failed: (cancellable == data->button_data->cancellable)
Aborted (core dumped)

To reproduce it delete any settings.json and any "BitcoinCore-App*.conf" in home/.config/BitcoinCore.

Code-wise I need more time to check the changes, especially the last commit, cannot be split? It would be useful to add a brief description of the intention, what's the intention of the changes in the protocol of bitcoin.cpp (re-structuring it for clarity? something was missing/ needed?).

On the first commit, the commit name doesn't explain much either (eg , it seems there's a refactoring within, what's the motivation behind making node a pointer instead of a reference? If you check the code in core/ qt it's always defined as &. Is there an issue you are trying to solve or just syntax or semantic?).

[pablomartin4btc]

Every commit should pass compilation and all tests so if the failure occurs on the previous commit you should include it there (we are missing the "CI / test each commit (pull_request)" that exists on the other repos and I think @hebasto has it pending with other things he wanted to merge here into this repo).

[MarnixCroes]

• during onboarding, when clicking Custom datadir it gives this output in terminal

Model size of -106 is less than 0
Model size of -6 is less than 0

@MarnixCroes are you still seeing the above pop-up with 6d4c111?

I do not

[D33r-Gee]

Thanks @pablomartin4btc for testing and for your questions.

what's the intention of the changes in the protocol of bitcoin.cpp (re-structuring it for clarity?

The majority of changes in qml/bitcoin.cpp are mainly splitting the onboarding and node creation so that the user can set their preferred datatdir it's inspired by the qt/bitcoin.cpp however it uses the anonymous namespace instead of using a class. The intention came from feedback on #390

what's the motivation behind making node a pointer instead of a reference?

I'm assuming you are referencing:

gui-qml/src/qml/models/options_model.cpp

Line 27 in f3b8c5a

OptionsQmlModel::OptionsQmlModel(interfaces::Node* node, bool is_onboarded)

if so the logic there came from having to initiate the options_model in qml/bitcoin.cpp with the node as a nullptr. And this was a way for it to work with the least amount of changes.

If you check the code in core/ qt it's always defined as &. Is there an issue you are trying to solve or just syntax or semantic?).

Yes in qt/bitcoin.cpp the OptionsModel is initiated through a reference because node creation doesn't happen until the splashscreen/intro have gone through and no settings are being set with the OptionsModel.

On the other hand in qml/bitcoin.cpp the options_model is needed for onboarding so initiating it with a nullptr is allowing the QML context property to be set without Node initiation.

[MarnixCroes]

7a34b0d
did some manual testing, so far works ok

some commits could be squashed imo, also as Pablo mentioned here #408 (comment)

[D33r-Gee]

Thanks @MarnixCroes for testing and @hebasto for the comments.

Will update once more with cleaner and clearer commits...

Also working on the Android version... Found a workaround for QSettings not working with Android, will post Android related commits soon...

[D33r-Gee]

This update 2cf64dd introduces Android functionality by creating qml/androidcustomdatadir files. This allows the app to retrieve the custom data directory path, if previously set by the user.

[johnny9]

statically defining all of these pointers is concerning. Eventhough it is redundant it feels safer to keep a lot of these withing the function scopes.

[D33r-Gee]

Ok thanks for looking into it... will refactor further to use arguments instead of pointers

[D33r-Gee]

After our online chat, let's keep things as is with the caveat that if any issues arise due to the static pointers we'll have to refactor further. Resolving for now...

[hebasto]

After our online chat...

Could the main points from that discussion be posted here?

[D33r-Gee]

To summarize our convo: We talked about the safety of static pointers in the code. We thought about adding a class like in #390, however decided to leave them for now, since there might be issues raised by @jarold that we need to consider first. That said we both agreed that having a clear owner for pointers is usually a good idea for better code management. Is there anything I missed @johnny9 ?

[johnny9]

tACK 2cf64dd

Onboarded with a custom directory, started IBD, then restarted the application to make sure the application continued to use the custom directory. Worked on both Linux Desktop and Android.

[hebasto]

The https://github.com/bitcoin-core/gui-qml/pull/408/files#r1658350670 still needs to be addressed.

[hebasto]

After our online chat...

Could the main points from that discussion be posted here?

[MarnixCroes]

ok when you run dpkg -l "*gtk*" | grep ii do you get this : ii qt5-gtk-platformtheme:amd64 5.15.3+dfsg-2ubuntu0.2 amd64 Qt 5 GTK+ 3 platform theme ?

yes

[hebasto]

Please rebase to resolve conflicts.

[D33r-Gee]

rebased over main with 8d53c44

tested out on WSL Ubuntu 22.04 however didn't tested out on Android...

[MarnixCroes]

8d53c44

apart from the know crash issue (which happens occasionally) the previous issues are fixed and it works as expected.

commit history could still be improved (commits like c1067d1) which was also previously mentioned c1067d1#r1658350670

[D33r-Gee]

rebased over main with 370bc64

Addressed @MarnixCroes comment.

Tested on both Ubuntu 22.04 and Android

[johnny9]

ACK 370bc64

tested the flow on Desktop and Android and both worked as expected without any crashing.

Desktop

Android:

[pablomartin4btc]

tested d76761d on Ubuntu 22.04.

I found a few inconsistencies regarding the commits structure:

• As I noticed earlier, currently 4rd and 5th commits should be squashed together.
• Same for 2nd and 3rd commits, the 2nd commit fails as you removed fullClientVersion from the node model but you didn't update the reference until the next commit.
• I'd put all the android changes together at the end and separate from non-android stuff (in fact, going forward, if possible, I'd add them as a follow-up PR to make the review and testing easier and faster), I don't see the reason of splitting them in so many, this also would avoid other mistakes: in commit *refactoring to allow..." you are including src/qml/androidcustomdatadir.cpp that is being added 2 commits later. Commit "android: permissions for custom datadir" is not for custom datadir specific but for android "external" storage in general.

I think the PR should have 4 commits in total (maybe 5 if you want to have the android storage permission one separate).

Regarding the UI and the wiring/ persistence of the datadir configuration, it works as expected. As I mentioned in the UI only PR earlier, we need to think about the different chain-nets or when user specify -datadir, perhaps this is more related with the onboarding, do we need to trigger the onboarding if it's the first time on those situations?

Somehow I got different numbers in the storage estimation:

As it can be seen as well, regarding the Gtk error, this time didn't crash but it appears after the 6th time of bitcoin-qt execution.

(bitcoin-qt:276616): Gtk-CRITICAL **: 23:44:35.670: gtk_image_set_from_gicon: assertion 'GTK_IS_IMAGE (image)' failed

This is happening when the file dialog pops up, started to happen in the UI only PR #397.

Please try to re-org the commits, let me know if you need any help on this, so I can do another code review ASAP and we can merge this as I also need to touch the options model, onboarding and bitcoin.cpp.

[D33r-Gee]

Thanks @johnny9, @MarnixCroes, @pablomartin4btc and @hebasto for testing and reviewing this PR.

@pablomartin4btc , thanks for pointing the commit inconsistencies I will keep your feedback in mind as I publish future PRs.

After speaking with @jarolrod it is now clear that the code is not up to par (to put it politely).

I am closing it, so I can focus on AssumeUTXO integration, however I opened issue #426 so whoever takes it on can learn from my mistakes.