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

Put BlockClock into an error state if node init fails #383

Merged
merged 1 commit into from
Sep 6, 2024

Conversation

johnny9
Copy link
Contributor

@johnny9 johnny9 commented Dec 20, 2023

Initial step for adding error states to the NodeRunner

Link to github actions build artifacts.

Build Artifacts

@johnny9
Copy link
Contributor Author

johnny9 commented Dec 20, 2023

Tested by starting up a pruned testnet node and then changing the prune settings

Screenshot from 2023-12-19 20-37-07

Copy link
Contributor

@MarnixCroes MarnixCroes left a comment

Choose a reason for hiding this comment

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

tested: error is displayed when reindex is need, instead of infinite loading on master.

nit: would it be easy to remove HandCursor when there is the error?
as there is nothing to click on

@johnny9
Copy link
Contributor Author

johnny9 commented Dec 21, 2023

nit: would it be easy to remove HandCursor wh

Yeah I noticed that as well. Should probably do that until the pop up view with the details is sorted out.

Copy link
Contributor

@pablomartin4btc pablomartin4btc left a comment

Choose a reason for hiding this comment

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

Concept ACK

Is it possible to add some small text/ description within the BlockClock? Like what kind of error... "init"? With a tooltip perhaps to show an extended description. As in the brainstorming issue #348 where you can see "No Wifi" within the "Paused" BlockClock state.

@hebasto hebasto changed the title qml: put BlockClock into an error state if node init fails Put BlockClock into an error state if node init fails Jan 1, 2024
@GBKS
Copy link
Contributor

GBKS commented Jan 10, 2024

We have some mock-ups for different error types in the design docs (half-way down the page). Ideally, we can always know what the problem was and show a respective icon & message. Maybe not exactly, but the general error source is already helpful.

For errors that cannot be recognized, the mock-ups show "Error" and "Tap for details". If we want to go this route, we need to figure out what is shown when tapping. A full screen with a proper explanation, error log, tips to resolve, etc, like here?

@johnny9
Copy link
Contributor Author

johnny9 commented Jan 10, 2024

Concept ACK

Is it possible to add some small text/ description within the BlockClock? Like what kind of error... "init"? With a tooltip perhaps to show an extended description. As in the brainstorming issue #348 where you can see "No Wifi" within the "Paused" BlockClock state.

This is ultimately the goal. In this PR, I've just implemented the portion that I'm certain about and would likely want to follow up afterwards with the description/details portion.

We have some mock-ups for different error types in the design docs (half-way down the page). Ideally, we can always know what the problem was and show a respective icon & message. Maybe not exactly, but the general error source is already helpful.

For errors that cannot be recognized, the mock-ups show "Error" and "Tap for details". If we want to go this route, we need to figure out what is shown when tapping. A full screen with a proper explanation, error log, tips to resolve, etc, like here?

What to show when tapping is what I'm uncertain about at the moment and was feeling like it might be a chance for me to pretend to be a designer a bit and play with figma. The mobile design looked good and just need to get a basic design for the Desktop.

Copy link
Contributor

@pablomartin4btc pablomartin4btc left a comment

Choose a reason for hiding this comment

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

tACK 03a8725

I've 2 issues running bitcoin-qt with this change:

1. As soon the app starts up I got these 2 errors regarding qml components
QVariant::load: unknown user type with name BitcoinUnits::Unit.
Running initialization in thread
qrc:/qml/components/BlockClock.qml:33:5: Unable to assign [undefined] to QString
2. After the above, since I got an instance on a pruned node with some corrupted index (due to some break during loadtxoutset) and bitcoin.conf still referencing blockfilterindex=1 and coinstatsindex=1. So, when I remove those indexing settings I got what @MarnixCroes described above -> "infinite loading on master". Should I not get the error icon also in this case? How I can validate this change? @johnny9 could you please add repro instructions on the description?
Error: basic block filter index best block of the index goes beyond pruned data. Please disable the index or reindex (which will download the whole blockchain again)
Error: Failed to start indexes, shutting down.

Complete terminal error logs
~/workspace/bitcoin-core-gui-qml$ ./src/qt/bitcoin-qt 
Warning: Ignoring XDG_SESSION_TYPE=wayland on Gnome. Use QT_QPA_PLATFORM=wayland to run on Wayland anyway.
QVariant::load: unknown user type with name BitcoinUnits::Unit.
Running initialization in thread
qrc:/qml/components/BlockClock.qml:33:5: Unable to assign [undefined] to QString
Error: basic block filter index best block of the index goes beyond pruned data. Please disable the index or reindex (which will download the whole blockchain again)
Error: Failed to start indexes, shutting down.

Co-authored-by: thebagmaster <thebagmaster>
@johnny9
Copy link
Contributor Author

johnny9 commented Jan 29, 2024

Update from 03a8725 to 2b61d22

  • Removed invalid property from BlockClock.qml. this was leftover from previous work on this.

@johnny9
Copy link
Contributor Author

johnny9 commented Jan 29, 2024

I've 2 issues running bitcoin-qt with this change:

  1. As soon the app starts up I got these 2 errors regarding qml components
QVariant::load: unknown user type with name BitcoinUnits::Unit.
Running initialization in thread
qrc:/qml/components/BlockClock.qml:33:5: Unable to assign [undefined] to QString

I removed the invalid property in the latest update which resolved that error

  1. After the above, since I got an instance on a pruned node with some corrupted index (due to some break during loadtxoutset) and bitcoin.conf still referencing blockfilterindex=1 and coinstatsindex=1. So, when I remove those indexing settings I got what @MarnixCroes described above -> "infinite loading on master". Should I not get the error icon also in this case? How I can validate this change? @johnny9 could you please add repro instructions on the description?
    Complete terminal error logs

This won't catch all errors and there will need to be an effort to categorize all of these possible error states as well as render a description of what happened to the user.

This change triggers when void NodeModel::initializeResult(bool success, interfaces::BlockAndHeaderTipInfo tip_info) callback has success = false as a parameter. The specific case I use to test is to start up a signet node with prune=1907 (the default you get with our onboarding flow). Then after it syncs a bit, I remove the prune option in my settings.json and restart the app.

Copy link
Contributor

@pablomartin4btc pablomartin4btc left a comment

Choose a reason for hiding this comment

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

tACK 2b61d22

I've managed to reproduce the errored BlockClock picture when there's another instance of Bitcoin Core running:

image

Also the error reported in previous review regarding the error on BlockClock.qml component doesn't show anymore:

./src/qt/bitcoin-qt -signet
Warning: Ignoring XDG_SESSION_TYPE=wayland on Gnome. Use QT_QPA_PLATFORM=wayland to run on Wayland anyway.
Running initialization in thread
Error: Unable to bind to 0.0.0.0:38333 on this computer. Bitcoin Core is probably already running.
Error: Failed to listen on any port. Use -listen=0 if you want this

@GBKS
Copy link
Contributor

GBKS commented Feb 1, 2024

Related, but probably not essential to this PR, here's an idea for handling errors. So on the left, we have this old mock-up for a tooltip that appears when hovering the peers dots, to give you slightly more info about connectivity. Clicking would take you to the peers page.

Now, we can take a similar approach to errors. Based on the type of error, a click can either show the tooltip or take you to the page relevant to this error (the page then needs to show appropriate information).

What do you make of this pattern?

image

Copy link
Contributor

@D33r-Gee D33r-Gee left a comment

Choose a reason for hiding this comment

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

tested on Ubuntu 22.04 and Android (Galaxy A13)

Was able to reproduce error on Ubuntu 22.04 by deleting the index directory located at .bitcoin/signet/block

For android used Android Studio IDE and deleted the index folder using the device explorer

Suggestion (echoing @GBKS): it would be nice if there was an error message in the GUI similar to what is displayed in terminal, i.e.:
: Error initializing block database. Please restart with -reindex or -reindex-chainstate to recover.

@GBKS
Copy link
Contributor

GBKS commented Mar 12, 2024

Please restart with -reindex or -reindex-chainstate to recover.

Is there a way to automatically restart with these options so the user can just click a button?

@MarnixCroes
Copy link
Contributor

Please restart with -reindex or -reindex-chainstate to recover.

Is there a way to automatically restart with these options so the user can just click a button?

it's possible, best would be to do it as a follow up imo

Copy link
Contributor

@MarnixCroes MarnixCroes left a comment

Choose a reason for hiding this comment

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

tACK 2b61d22

@hebasto hebasto merged commit b979bdc into bitcoin-core:main Sep 6, 2024
7 of 9 checks passed
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.

6 participants