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

util: Diagnose Lib Version #1 #2573

Merged
merged 22 commits into from
Oct 31, 2022

Conversation

MinaFarhan
Copy link
Contributor

@MinaFarhan MinaFarhan commented Sep 9, 2022

Closes #2477.

@MinaFarhan
Copy link
Contributor Author

Create a library for Diagnose so it can be called by the RPC.

@Pythonix
Copy link
Contributor

Pythonix commented Sep 9, 2022

Looks like a great addition! I will pay a bounty of 1000 GRC to @MinaFarhan once this gets merged =)
Once the RPC commands are added it probably closes #2477?

@iFoggz
Copy link
Member

iFoggz commented Sep 10, 2022

Great to have a RPC of diagnostics,

there is some coding changes that should be done to comply with: coding.txt as per the CONTRIBUTING.md.

Once it's cleaned up I'll have a gander through as well as the other devs.

Look forward to this feature to be added.

@iFoggz
Copy link
Member

iFoggz commented Sep 10, 2022

Also should note that I'd like to see RPC diagnostics available for daemon as well.

@MinaFarhan MinaFarhan marked this pull request as draft September 10, 2022 15:21
@div72 div72 added enhancement refactor This is for refactoring (if also an enhancement, use that label too). labels Sep 10, 2022
@jamescowens jamescowens changed the title Diagnose Lib Version #1 util: Diagnose Lib Version #1 Sep 11, 2022
@jamescowens
Copy link
Member

@iFoggz He has been looking to volunteer and asked me a while back what would be a good way to get started. I suggested a smaller scale task at first, but one which would introduce him to parts of the code and not be too difficult. We settled on a split-out of the diagnostics into a service API as his first contribution. He looks to build the rpc calls as the next step.

@jamescowens jamescowens added this to the LaVerne milestone Sep 11, 2022
Copy link
Member

@jamescowens jamescowens left a comment

Choose a reason for hiding this comment

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

This is a nice starting point. @iFoggz is right about some of the style elements. Please use the Bitcoin Core coding guide as our own contribution guide is a little out of date. Note that the VerifyTCPPort and VerifyClock functions are still using Qt plumbing. This will not work for the daemon, because Qt dependencies should not be used there. (I warned you about this, and this will be the hardest part, because you are going to have to use basic networking without the benefit of the Qt superstructure.)

@jamescowens
Copy link
Member

Also, I have authorized your PR to do CI. Please review which build is failing and correct the cause.

@MinaFarhan
Copy link
Contributor Author

Looks like a great addition! I will pay a bounty of 1000 GRC to @MinaFarhan once this gets merged =) Once the RPC commands are added it probably closes #2477?

Thank you.

@MinaFarhan
Copy link
Contributor Author

This is a nice starting point. @iFoggz is right about some of the style elements. Please use the Bitcoin Core coding guide as our own contribution guide is a little out of date. Note that the VerifyTCPPort and VerifyClock functions are still using Qt plumbing. This will not work for the daemon, because Qt dependencies should not be used there. (I warned you about this, and this will be the hardest part, because you are going to have to use basic networking without the benefit of the Qt superstructure.)

I am thinking of calling the QApplication() from the library. This way even if the library is called from the daemon, we can initialte the Qapplication and call the QT functions , like QTimer(). What do you think?

@jamescowens
Copy link
Member

jamescowens commented Sep 13, 2022

I don't think it is going to work, because Qt is not linked into the daemon, and it is not a good idea to try and call Qt stuff in the core. The native core has scheduling functionality. The existing global scheduler (static CScheduler scheduler) can be used to initiate a "timeout" interval in the core instead of the QTimer for example. See schedule() or scheduleFromNow() in scheduler.h/cpp

@MinaFarhan
Copy link
Contributor Author

Sounds Good!
I will change the QT functions with May be boost libraries. QTimer for example can be changed with boost deadline_timer.

@MinaFarhan
Copy link
Contributor Author

  1. Changed file names to use lower cases
  2. Replaced the QT functions (ex QTimer) with boost library functions
  3. Fixed the compilation error.
    Next, I need to fix the code comments to match the Bitcoin standard.

src/qt/diagnose.h Outdated Show resolved Hide resolved
src/qt/diagnose.h Outdated Show resolved Hide resolved
src/qt/diagnose.h Outdated Show resolved Hide resolved
src/qt/diagnose.h Outdated Show resolved Hide resolved
src/qt/diagnose.h Outdated Show resolved Hide resolved
src/qt/diagnosticsdialog.cpp Outdated Show resolved Hide resolved
@MinaFarhan
Copy link
Contributor Author

I have committed new code

  1. changed the QT functions to the equivalent boost library so the library can be called outside UI from RPC
  2. Cleaned the code and removed commented out functions
  3. fixed typos in the code comment

@jamescowens
Copy link
Member

@MinaFarhan please review the logs for CI and correct issues and get the code to compile cleanly on all architectures and pass the linter.

@MinaFarhan
Copy link
Contributor Author

@MinaFarhan please review the logs for CI and correct issues and get the code to compile cleanly on all architectures and pass the linter.

Yes, I am actually working on adding the RPC to the seam on as well. This requires some changes . Will submit all fixes together

@MinaFarhan
Copy link
Contributor Author

I have added rpc command "walletdiagnose",
I also fixed some compiling warnings and errors.

Copy link
Member

@jamescowens jamescowens left a comment

Choose a reason for hiding this comment

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

Nitpicking stuff, but you need to correct the code style stuff so the linter passes. There are also still some misspellings too. Please go through with a fine tooth comb. I may not have caught them all in the review. Once the code passes the CI and linter, then I will go through a more detailed pass.

src/Makefile.am Outdated Show resolved Hide resolved
src/Makefile.am Outdated Show resolved Hide resolved
src/qt/diagnosticsdialog.cpp Outdated Show resolved Hide resolved
src/qt/diagnosticsdialog.cpp Outdated Show resolved Hide resolved
@jamescowens
Copy link
Member

@MinaFarhan don't get discouraged by the "nitpicking" reviews. You will quickly get used to writing code that passes the spelling and code style stuff. The linter and CI is your friend in many ways in this regard. After your first PR, the CI will run automatically when you push a branch to github to allow you to check. You can also run the linter locally on your development workstation if you have all of the required packages installed. (This is a superset of the normal build packages.)

@MinaFarhan
Copy link
Contributor Author

No problem :)
I will make the corrections and resubmit.

This commit should be removed before merge to development.
@jamescowens
Copy link
Member

@MinaFarhan I did a PR to your branch. Please take a look at the commits. Note that while it passes compile now, There are some problems in operation that you need to troubleshoot related to the boost:asio.

src/wallet/rpcwallet.cpp Show resolved Hide resolved
src/wallet/diagnose.h Outdated Show resolved Hide resolved
src/wallet/diagnose.h Show resolved Hide resolved
@jamescowens
Copy link
Member

@MinaFarhan after you merge my PR to your branch, you should rebase your branch on top of the head of development before pushing back here, because the root of your PR is behind development a bit at this point. Thanks.

@jamescowens
Copy link
Member

@MinaFarhan you should really do a rebase on top of development with your branch rather than a merge, because a merge will generate an extra merge commit that I have to filter out later in the change log.

Something like
git checkout DiagnoseLib
git reset --hard caaf952 (This is the merge of my PR to your branch.)
git checkout development
git pull
git checkout DiagnoseLib
git rebase development
git push

In general the rule is that all branches that are the source of PR's (which means the PR's themselves) should always be rebased to "advance" them to include any new commits on the base branch, while PRs should always be merged by the maintainer onto the target branch.

@jamescowens
Copy link
Member

We still need to troubleshoot the crashes on that bus error in boost asio.

@MinaFarhan
Copy link
Contributor Author

We still need to troubleshoot the crashes on that bus error in boost asio.

Yes, working on it.

@MinaFarhan
Copy link
Contributor Author

@MinaFarhan you should really do a rebase on top of development with your branch rather than a merge, because a merge will generate an extra merge commit that I have to filter out later in the change log.

Something like git checkout DiagnoseLib git reset --hard caaf952 (This is the merge of my PR to your branch.) git checkout development git pull git checkout DiagnoseLib git rebase development git push

In general the rule is that all branches that are the source of PR's (which means the PR's themselves) should always be rebased to "advance" them to include any new commits on the base branch, while PRs should always be merged by the maintainer onto the target branch.

Ohh, sure I can do that next time. Do I still need to rebase instead of merge or is it too late?

@MinaFarhan
Copy link
Contributor Author

We still need to troubleshoot the crashes on that bus error in boost asio.

Yes, working on it.

Hello
L
I can not reproduce the issue on my PC. What version of Linux you are using? And is it a random issue or always appear when diagnostics are run?
thank you

@jamescowens
Copy link
Member

Always. openSuSE Leap 15.4, which has boost 1.66.0.

@jamescowens
Copy link
Member

Note that from my particular public IP, the default time service we use does not respond in a timely manner. You should simulate a non-responsive URL for the time server and see if you can reproduce the crash that way. That is the likely culprit.

@MinaFarhan
Copy link
Contributor Author

I was not able to reproduce the issue. I have tried different situations. I turned off the internet before the test and while doing the test.
Anyway, I have added error catching for the boost asio. Can you please try again and check if it still crashes from your end?
thanks

@jamescowens
Copy link
Member

@MinaFarhan I have some more fixes for your branch (PR) here that solves the issue I was having and some other stuff, like the Bionic problem above. The error catching that you put in does not work in Bionic, because boost 1.65 doesn't have results_type. I made some changes and I think everything works now.

@jamescowens
Copy link
Member

@MinaFarhan please merge my PR and let it flow here. Then this should be ready to merge.

Copy link
Member

@jamescowens jamescowens left a comment

Choose a reason for hiding this comment

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

tACK.

@jamescowens
Copy link
Member

This is ready to merge. The next thing on this is an rpc command to do diagnostics using this core library.

@jamescowens jamescowens merged commit b0b8f1a into gridcoin-community:development Oct 31, 2022
@MinaFarhan
Copy link
Contributor Author

I have added rpc command "walletdiagnose",

Thank you,
the RPC command is actually there. I have added rpc command "walletdiagnose". It is submitted in "ee0fda0e4e1b44b2eb9fc26e638faf19f6adef12"

Thanks
Mina

@jamescowens
Copy link
Member

There is still an issue about crashing on the assert. I will track this down. assert(m_name_to_test_map.size() < Diagnose::TestSize);

@jamescowens
Copy link
Member

This is the problem...

2022-10-31T03:31:56Z Using data directory /home/jowens/.GridcoinResearch2/testnet

2022-10-31T03:31:56Z Gridcoin version v5.4.0.2-ae6b4bc35718-dirty (release build)
2022-10-31T03:31:59Z INFO: Diagnose: m_test_name = 144
2022-10-31T03:31:59Z INFO: registerTest: test->m_test_name enum 144 registered, test map size = 1, Diagnose::TestSize = 12
2022-10-31T03:31:59Z INFO: VerifyWalletIsSynced: m_test_name = 2
2022-10-31T03:31:59Z INFO: Diagnose: m_test_name = 2
2022-10-31T03:31:59Z INFO: registerTest: test->m_test_name enum 2 registered, test map size = 2, Diagnose::TestSize = 12
2022-10-31T03:31:59Z INFO: CheckOutboundConnectionCount: m_test_name = 1
2022-10-31T03:31:59Z INFO: Diagnose: m_test_name = -150333728
2022-10-31T03:31:59Z INFO: registerTest: test->m_test_name enum -150333728 registered, test map size = 3, Diagnose::TestSize = 12
2022-10-31T03:31:59Z INFO: CheckConnectionCount: m_test_name = 0
2022-10-31T03:31:59Z INFO: Diagnose: m_test_name = 2
2022-10-31T03:31:59Z INFO: registerTest: test->m_test_name enum 2 registered, test map size = 3, Diagnose::TestSize = 12
2022-10-31T03:31:59Z INFO: VerifyClock: m_test_name = 8
2022-10-31T03:31:59Z INFO: Diagnose: m_test_name = 1451884496
2022-10-31T03:31:59Z INFO: registerTest: test->m_test_name enum 1451884496 registered, test map size = 4, Diagnose::TestSize = 12
2022-10-31T03:31:59Z INFO: CheckClientVersion: m_test_name = 3
2022-10-31T03:31:59Z INFO: Diagnose: m_test_name = 2610
2022-10-31T03:31:59Z INFO: registerTest: test->m_test_name enum 2610 registered, test map size = 5, Diagnose::TestSize = 12
2022-10-31T03:31:59Z INFO: VerifyBoincPath: m_test_name = 4
2022-10-31T03:31:59Z INFO: Diagnose: m_test_name = 144
2022-10-31T03:31:59Z INFO: registerTest: test->m_test_name enum 144 registered, test map size = 5, Diagnose::TestSize = 12
2022-10-31T03:31:59Z INFO: VerifyCPIDValid: m_test_name = 7
2022-10-31T03:31:59Z INFO: Diagnose: m_test_name = 1449514896
2022-10-31T03:31:59Z INFO: registerTest: test->m_test_name enum 1449514896 registered, test map size = 6, Diagnose::TestSize = 12
2022-10-31T03:31:59Z INFO: VerifyCPIDHasRAC: m_test_name = 5
2022-10-31T03:31:59Z INFO: Diagnose: m_test_name = 1442843186
2022-10-31T03:31:59Z INFO: registerTest: test->m_test_name enum 1442843186 registered, test map size = 7, Diagnose::TestSize = 12
2022-10-31T03:31:59Z INFO: VerifyCPIDIsActive: m_test_name = 6
2022-10-31T03:31:59Z INFO: Diagnose: m_test_name = 7471220
2022-10-31T03:31:59Z INFO: registerTest: test->m_test_name enum 7471220 registered, test map size = 8, Diagnose::TestSize = 12
2022-10-31T03:31:59Z INFO: VerifyTCPPort: m_test_name = 9
2022-10-31T03:31:59Z INFO: Diagnose: m_test_name = 2610
2022-10-31T03:31:59Z INFO: registerTest: test->m_test_name enum 2610 registered, test map size = 8, Diagnose::TestSize = 12
2022-10-31T03:31:59Z INFO: CheckDifficulty: m_test_name = 10
2022-10-31T03:31:59Z INFO: Diagnose: m_test_name = 1449260224
2022-10-31T03:31:59Z INFO: registerTest: test->m_test_name enum 1449260224 registered, test map size = 9, Diagnose::TestSize = 12
2022-10-31T03:31:59Z INFO: CheckETTS: m_test_name = 11

The constructor for Dialog is trying to register the test before the m_test_name is set.

@jamescowens
Copy link
Member

Ok... this is better...

2022-10-31T03:43:35Z Using data directory /home/jowens/.GridcoinResearch2/testnet

2022-10-31T03:43:35Z Gridcoin version v5.4.0.2-ae6b4bc35718-dirty (release build)
2022-10-31T03:43:37Z INFO: Diagnose: m_test_name = 2
2022-10-31T03:43:37Z INFO: registerTest: test->m_test_name enum 2 registered, test map size = 1, Diagnose::TestSize = 12
2022-10-31T03:43:37Z INFO: Diagnose: m_test_name = 1
2022-10-31T03:43:37Z INFO: registerTest: test->m_test_name enum 1 registered, test map size = 2, Diagnose::TestSize = 12
2022-10-31T03:43:37Z INFO: CheckOutboundConnectionCount: m_test_name = 1
2022-10-31T03:43:37Z INFO: Diagnose: m_test_name = 0
2022-10-31T03:43:37Z INFO: registerTest: test->m_test_name enum 0 registered, test map size = 3, Diagnose::TestSize = 12
2022-10-31T03:43:37Z INFO: CheckConnectionCount: m_test_name = 0
2022-10-31T03:43:37Z INFO: Diagnose: m_test_name = 8
2022-10-31T03:43:37Z INFO: registerTest: test->m_test_name enum 8 registered, test map size = 4, Diagnose::TestSize = 12
2022-10-31T03:43:37Z INFO: VerifyClock: m_test_name = 8
2022-10-31T03:43:37Z INFO: Diagnose: m_test_name = 3
2022-10-31T03:43:37Z INFO: registerTest: test->m_test_name enum 3 registered, test map size = 5, Diagnose::TestSize = 12
2022-10-31T03:43:37Z INFO: CheckClientVersion: m_test_name = 3
2022-10-31T03:43:37Z INFO: Diagnose: m_test_name = 4
2022-10-31T03:43:37Z INFO: registerTest: test->m_test_name enum 4 registered, test map size = 6, Diagnose::TestSize = 12
2022-10-31T03:43:37Z INFO: VerifyBoincPath: m_test_name = 4
2022-10-31T03:43:37Z INFO: Diagnose: m_test_name = 7
2022-10-31T03:43:37Z INFO: registerTest: test->m_test_name enum 7 registered, test map size = 7, Diagnose::TestSize = 12
2022-10-31T03:43:37Z INFO: VerifyCPIDValid: m_test_name = 7
2022-10-31T03:43:37Z INFO: Diagnose: m_test_name = 5
2022-10-31T03:43:37Z INFO: registerTest: test->m_test_name enum 5 registered, test map size = 8, Diagnose::TestSize = 12
2022-10-31T03:43:37Z INFO: VerifyCPIDHasRAC: m_test_name = 5
2022-10-31T03:43:37Z INFO: Diagnose: m_test_name = 6
2022-10-31T03:43:37Z INFO: registerTest: test->m_test_name enum 6 registered, test map size = 9, Diagnose::TestSize = 12
2022-10-31T03:43:37Z INFO: VerifyCPIDIsActive: m_test_name = 6
2022-10-31T03:43:37Z INFO: Diagnose: m_test_name = 9
2022-10-31T03:43:37Z INFO: registerTest: test->m_test_name enum 9 registered, test map size = 10, Diagnose::TestSize = 12
2022-10-31T03:43:37Z INFO: VerifyTCPPort: m_test_name = 9
2022-10-31T03:43:37Z INFO: Diagnose: m_test_name = 10
2022-10-31T03:43:37Z INFO: registerTest: test->m_test_name enum 10 registered, test map size = 11, Diagnose::TestSize = 12
2022-10-31T03:43:37Z INFO: CheckDifficulty: m_test_name = 10
2022-10-31T03:43:37Z INFO: Diagnose: m_test_name = 11
2022-10-31T03:43:37Z INFO: registerTest: test->m_test_name enum 11 registered, test map size = 12, Diagnose::TestSize = 12

@jamescowens
Copy link
Member

I just pushed two additional commits directly to development to fix this issue.
ae6b4bc
4be2469

@jamescowens
Copy link
Member

A few more cleanup commits directly to development branch that are followons to this PR.
da333ca
1239ac0
1239ac0
e325567

I think there is still something not quite right with the clock test, because even with more than 5 connections the clock test does not return immediately all of the time. I will do some more investigation of that.

jamescowens added a commit to jamescowens/Gridcoin-Research that referenced this pull request Nov 27, 2022
Added
 - net: Add and document network messages in protocol.h (backport) gridcoin-community#2533 (@Pythonix)
 - Define MAX_DIGITS_BTC for magic number in BitcoinUnits::format gridcoin-community#2555 (@barton2526)
 - rpc: Implementation of getmrcinfo gridcoin-community#2570 (@jamescowens)
 - init: Add init error message if -printtoconsole and -daemon specified simultaneously gridcoin-community#2571 (@jamescowens)
 - rpc: getmrcinfo part 2 - add calculated minimum fees and fee boosting and by CPID reporting gridcoin-community#2575 (@jamescowens)
 - fs: fully initialize `_OVERLAPPED` for win32 gridcoin-community#2587 (@div72)
 - util: Diagnose Lib Version #1 gridcoin-community#2573 (@MinaFarhan)
 - util: Implement core diagnostics #2 (@jamescowens)
 - util: modify Win32LockedPageAllocator to query windows for limit. gridcoin-community#2536 (@div72)
 - gui, voting: Implement information for wallet holder's votes on poll info cards gridcoin-community#2605 (@jamescowens)

Changed
 - scripted-diff: Drop Darwin version for better maintainability gridcoin-community#2557 (@barton2526)
 - build: Require gcc8 on Ubuntu Bionic to enable C++17 features gridcoin-community#2579 (@barton2526)
 - util: Replace use of locale dependent atoi(…) with locale-independent std::from_chars(…) (C++17) gridcoin-community#2564 (@barton2526)
 - translation: Translation updates gridcoin-community#2581 (@jamescowens)
 - depends: update urls for dmg tools gridcoin-community#2583 (@div72)
 - Use ReadLE64 in uint256::GetUint64 instead of duplicating logic gridcoin-community#2586 (@div72)
 - util: Make Parse{Int,UInt}{32,64} use locale independent std::from_chars(…) (C++17) instead of locale dependent strto{l,ll,ul,ull} gridcoin-community#2592 (@barton2526)
 - build: don't set PORT=no in config.site gridcoin-community#2593 (@barton2526)
 - build: Replace `which` command with `command -v` gridcoin-community#2595 (@barton2526)
 - build: update ax_cxx_compile_stdcxx to serial 14 gridcoin-community#2596 (@barton2526)
 - gui: Changed the unlocked for staking only icons to green gridcoin-community#2598 (@delta1513)
 - gui: Translation updates gridcoin-community#2599 (@jamescowens)
 - build: update CI for linter and actions version gridcoin-community#2606 (@jamescowens)
 - gui: Update translations gridcoin-community#2608 (@jamescowens)

Removed
 - refactor: remove unused c-string variant of atoi64() gridcoin-community#2562 (@barton2526)
 - refactor: Remove unused CDataStream::rdbuf method gridcoin-community#2585 (@div72)

Fixed
 - net: Fix some benign races (backport) gridcoin-community#2532 (@Pythonix)
 - rpc: fix invalid parameter error codes for {sign,verify}message RPCs gridcoin-community#2556 (@barton2526)
 - build: Fix x86_64 <-> arm64 cross-compiling for macOS gridcoin-community#2560 (@barton2526)
 - rpc, mrc: Fix field name and initialization of mrc_fees_to_staker gridcoin-community#2567 (@jamescowens)
 - gui: Add missing resizeTableColumns to fix send address book column widths gridcoin-community#2569 (@jamescowens)
 - accrual: rebuild snapshot registry on corruption instead of crashing gridcoin-community#2577 (@div72)
 - doc: Fix link to MurmurHash3.cpp (moved from Google Code to Github) gridcoin-community#2584 (@div72)
 - fix help text for `revokebeacon` command gridcoin-community#2591 (@Pythonix)
 - util: Fix spelling error in gridcoinresearchd.cpp gridcoin-community#2590 (@jamescowens)
 - depends: always use correct ar for win qt build gridcoin-community#2588 (@div72)
 - util: Fix some bugs due to new implementation and change in BOINC dir handling (@jamescowens)
 - util: Diagnose lib - Implement changes to solve crash on some Boost 1.66 machines gridcoin-community#2597 (@jamescowens)
 - contrib: Check for `patch` command, Check for `wget` command gridcoin-community#2594 (@barton2526)
 - build: Check std::system for -[alert|block|wallet]notify gridcoin-community#2582 (@barton2526)
 - gui: Changed the wording on the tooltip for the address book gridcoin-community#2602 (@delta1513)
 - build: pass win32-dll to LT_INIT() gridcoin-community#2601 (@barton2526)
 - build: minor cleanups to native_clang package gridcoin-community#2600 (@barton2526)
 - util: restore translations to diagnostics gridcoin-community#2603 (@jamescowens)
 - refactor: Fix problems found by valgrind gridcoin-community#2607 (@jamescowens)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement refactor This is for refactoring (if also an enhancement, use that label too).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create RPC Command to Run Diagnostics
6 participants