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

Metrics screen improvements #1735

Merged
merged 10 commits into from Nov 3, 2016

Conversation

Projects
None yet
7 participants
@str4d
Contributor

str4d commented Oct 30, 2016

Closes #1656, #1685, #1688 and #1716

str4d added some commits Oct 30, 2016

@str4d str4d added this to the 1.0.1 milestone Oct 30, 2016

@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Oct 30, 2016

Contributor

Assigning to 1.0.1 because several of the issues this closes are resulting in user confusion, which we should resolve sooner than later.

Contributor

str4d commented Oct 30, 2016

Assigning to 1.0.1 because several of the issues this closes are resulting in user confusion, which we should resolve sooner than later.

@@ -287,6 +289,7 @@ class CRegTestParams : public CTestNetParams {
public:
CRegTestParams() {
strNetworkID = "regtest";
strCurrencyUnits = "REG";

This comment has been minimized.

@str4d

str4d Oct 31, 2016

Contributor

I just picked this as a placeholder; it will probably never actually be used.

@str4d

str4d Oct 31, 2016

Contributor

I just picked this as a placeholder; it will probably never actually be used.

Show outdated Hide outdated src/metrics.cpp
mature += subsidy;
}
} else {
it = u->erase(it);

This comment has been minimized.

@str4d

str4d Oct 31, 2016

Contributor

The implicit assumption here is that if a block hash disappears either from mapBlockIndex (should only happen on restart, so never relevant here) or chainActive (happens on reorgs), it will never re-appear. I think this is a fair assumption for reorgs, inasmuch as it is far less likely that a second reorg would occur that re-includes a block after the local node has already observed one that orphans it. But if we want to remove that assumption, I can just build two parallel lists (one for tracked blocks, one for tracked orphans) and shuttle block hashes between them.

@str4d

str4d Oct 31, 2016

Contributor

The implicit assumption here is that if a block hash disappears either from mapBlockIndex (should only happen on restart, so never relevant here) or chainActive (happens on reorgs), it will never re-appear. I think this is a fair assumption for reorgs, inasmuch as it is far less likely that a second reorg would occur that re-includes a block after the local node has already observed one that orphans it. But if we want to remove that assumption, I can just build two parallel lists (one for tracked blocks, one for tracked orphans) and shuttle block hashes between them.

This comment has been minimized.

@bitcartel
@bitcartel

This comment has been minimized.

@str4d

str4d Nov 1, 2016

Contributor

According to the specification for std::list::erase, this is fine.

@str4d

str4d Nov 1, 2016

Contributor

According to the specification for std::list::erase, this is fine.

This comment has been minimized.

@bitcartel

bitcartel Nov 2, 2016

Contributor

http://en.cppreference.com/w/cpp/language/for

The iterator is incremented at end of the for loop, will that be an issue?

@bitcartel

bitcartel Nov 2, 2016

Contributor

http://en.cppreference.com/w/cpp/language/for

The iterator is incremented at end of the for loop, will that be an issue?

This comment has been minimized.

@str4d

str4d Nov 2, 2016

Contributor

Ah, yes you're right. Switching to a while loop and manually iterating in the non-erasing branch will fix this.

@str4d

str4d Nov 2, 2016

Contributor

Ah, yes you're right. Switching to a while loop and manually iterating in the non-erasing branch will fix this.

@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Oct 31, 2016

Contributor

Here's what the metrics screen looks like in this branch:

2016-10-31-zcashd-metrics-screen

Contributor

str4d commented Oct 31, 2016

Here's what the metrics screen looks like in this branch:

2016-10-31-zcashd-metrics-screen

@zookozcash

This comment has been minimized.

Show comment
Hide comment
@zookozcash

zookozcash Oct 31, 2016

Beautiful! ☺❤ⓩ

zookozcash commented Oct 31, 2016

Beautiful! ☺❤ⓩ

Show outdated Hide outdated src/metrics.cpp
boost::synchronized_value<std::list<std::string>> messageBox;
boost::synchronized_value<std::string> initMessage;
bool loaded = false;
void TrackMinedBlock(uint256 hash)
{
minedBlocks.increment();

This comment has been minimized.

@bitcartel

bitcartel Nov 1, 2016

Contributor

Make method synchronized for both of these operations.

@bitcartel

bitcartel Nov 1, 2016

Contributor

Make method synchronized for both of these operations.

Show outdated Hide outdated src/metrics.cpp
@@ -121,8 +131,43 @@ int printMetrics(size_t cols, int64_t nStart, bool mining)
int mined = minedBlocks.get();
if (mined > 0) {
LOCK(cs_main);

This comment has been minimized.

@bitcartel

bitcartel Nov 1, 2016

Contributor

LOCK2...

@bitcartel

bitcartel Nov 1, 2016

Contributor

LOCK2...

str4d added some commits Nov 1, 2016

@arcalinea

This comment has been minimized.

Show comment
Hide comment
@arcalinea

arcalinea Nov 2, 2016

Contributor

Looks good, but when I accidentally hit "enter" in the window running zcashd, it duplicated the block height line. That line duplicates as many times as you hit enter

metrics-screen-blockheight

Contributor

arcalinea commented Nov 2, 2016

Looks good, but when I accidentally hit "enter" in the window running zcashd, it duplicated the block height line. That line duplicates as many times as you hit enter

metrics-screen-blockheight

@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Nov 2, 2016

Contributor

Yep, that's an artifact of the way the UI is handled (plain console escape codes). I manually move the cursor up the right number of lines and only reprint the info that is refreshed, which means that spurious newlines cause changes like that. The fix would be to either print out the entire console view each time, or use a proper ncurses library. I'm leaning towards the latter, which should hopefully also improve cross-platform support.

Contributor

str4d commented Nov 2, 2016

Yep, that's an artifact of the way the UI is handled (plain console escape codes). I manually move the cursor up the right number of lines and only reprint the info that is refreshed, which means that spurious newlines cause changes like that. The fix would be to either print out the entire console view each time, or use a proper ncurses library. I'm leaning towards the latter, which should hopefully also improve cross-platform support.

@bitcartel

This comment has been minimized.

Show comment
Hide comment
@bitcartel

bitcartel Nov 2, 2016

Contributor

The counters could end up negative when they overflow.

struct AtomicCounter {
    std::atomic<int> value;

extern AtomicCounter transactionsValidated;
extern AtomicCounter ehSolverRuns;
extern AtomicCounter solutionTargetChecks;

How about using uint64_t ?

Contributor

bitcartel commented Nov 2, 2016

The counters could end up negative when they overflow.

struct AtomicCounter {
    std::atomic<int> value;

extern AtomicCounter transactionsValidated;
extern AtomicCounter ehSolverRuns;
extern AtomicCounter solutionTargetChecks;

How about using uint64_t ?

@bitcartel

This comment has been minimized.

Show comment
Hide comment
@bitcartel

bitcartel Nov 2, 2016

Contributor

utACK

Contributor

bitcartel commented Nov 2, 2016

utACK

@ebfull

This comment has been minimized.

Show comment
Hide comment
@ebfull

ebfull Nov 2, 2016

Contributor

utACK. Cool!

@zkbot r+ 3bddaf6

Contributor

ebfull commented Nov 2, 2016

utACK. Cool!

@zkbot r+ 3bddaf6

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Nov 2, 2016

Contributor

⌛️ Testing commit 3bddaf6 with merge 1fb99a3...

Contributor

zkbot commented Nov 2, 2016

⌛️ Testing commit 3bddaf6 with merge 1fb99a3...

zkbot pushed a commit that referenced this pull request Nov 2, 2016

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Nov 2, 2016

Contributor

💔 Test failed - zcash

Contributor

zkbot commented Nov 2, 2016

💔 Test failed - zcash

@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Nov 2, 2016

Contributor

Test failures are unrelated to this PR:


[----------] 3 tests from wallet_zkeys_tests
[ RUN      ] wallet_zkeys_tests.store_and_load_zkeys
[       OK ] wallet_zkeys_tests.store_and_load_zkeys (0 ms)
[ RUN      ] wallet_zkeys_tests.write_zkey_direct_to_db
gtest/test_wallet_zkeys.cpp:92: Failure
Value of: addrs.size()
  Actual: 2
Expected: 0
[  FAILED  ] wallet_zkeys_tests.write_zkey_direct_to_db (12 ms)
[ RUN      ] wallet_zkeys_tests.write_cryptedzkey_direct_to_db
gtest/test_wallet_zkeys.cpp:167: Failure
Value of: addrs.size()
  Actual: 2
Expected: 0
[  FAILED  ] wallet_zkeys_tests.write_cryptedzkey_direct_to_db (3 ms)
[----------] 3 tests from wallet_zkeys_tests (15 ms total)

@bitcartel this feels like the weird gtest ordering bug again.

Contributor

str4d commented Nov 2, 2016

Test failures are unrelated to this PR:


[----------] 3 tests from wallet_zkeys_tests
[ RUN      ] wallet_zkeys_tests.store_and_load_zkeys
[       OK ] wallet_zkeys_tests.store_and_load_zkeys (0 ms)
[ RUN      ] wallet_zkeys_tests.write_zkey_direct_to_db
gtest/test_wallet_zkeys.cpp:92: Failure
Value of: addrs.size()
  Actual: 2
Expected: 0
[  FAILED  ] wallet_zkeys_tests.write_zkey_direct_to_db (12 ms)
[ RUN      ] wallet_zkeys_tests.write_cryptedzkey_direct_to_db
gtest/test_wallet_zkeys.cpp:167: Failure
Value of: addrs.size()
  Actual: 2
Expected: 0
[  FAILED  ] wallet_zkeys_tests.write_cryptedzkey_direct_to_db (3 ms)
[----------] 3 tests from wallet_zkeys_tests (15 ms total)

@bitcartel this feels like the weird gtest ordering bug again.

@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Nov 2, 2016

Contributor

Moved test_checkblock below test_checktransaction.

@zkbot r+

Contributor

str4d commented Nov 2, 2016

Moved test_checkblock below test_checktransaction.

@zkbot r+

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Nov 2, 2016

Contributor

📌 Commit e80490f has been approved by str4d

Contributor

zkbot commented Nov 2, 2016

📌 Commit e80490f has been approved by str4d

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Nov 2, 2016

Contributor

⌛️ Testing commit e80490f with merge f6b5176...

Contributor

zkbot commented Nov 2, 2016

⌛️ Testing commit e80490f with merge f6b5176...

zkbot pushed a commit that referenced this pull request Nov 2, 2016

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Nov 2, 2016

Contributor

💔 Test failed - zcash

Contributor

zkbot commented Nov 2, 2016

💔 Test failed - zcash

@bitcartel

This comment has been minimized.

Show comment
Hide comment
@bitcartel

bitcartel Nov 2, 2016

Contributor

Fix the whitespace:

        gtest/json_test_vectors.cpp \
-        gtest/json_test_vectors.h \
+    gtest/json_test_vectors.h \
        gtest/test_foundersreward.cpp \
Contributor

bitcartel commented Nov 2, 2016

Fix the whitespace:

        gtest/json_test_vectors.cpp \
-        gtest/json_test_vectors.h \
+    gtest/json_test_vectors.h \
        gtest/test_foundersreward.cpp \
@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Nov 2, 2016

Contributor

I really dislike the fact that whitespace issues in build scripts can break tests. I just built this and ran zcash-gtest, and the failing tests passed for me.

@zkbot r+

Contributor

str4d commented Nov 2, 2016

I really dislike the fact that whitespace issues in build scripts can break tests. I just built this and ran zcash-gtest, and the failing tests passed for me.

@zkbot r+

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Nov 2, 2016

Contributor

📌 Commit 6b82011 has been approved by str4d

Contributor

zkbot commented Nov 2, 2016

📌 Commit 6b82011 has been approved by str4d

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Nov 2, 2016

Contributor

⌛️ Testing commit 6b82011 with merge ae369c4...

Contributor

zkbot commented Nov 2, 2016

⌛️ Testing commit 6b82011 with merge ae369c4...

zkbot pushed a commit that referenced this pull request Nov 2, 2016

@bitcartel

This comment has been minimized.

Show comment
Hide comment
@bitcartel

bitcartel Nov 2, 2016

Contributor

If you delete zcash-gtest and make it again, it will fail again (at least that is what I am seeing). After I moveed test_checkblock to the end of the list of tests did it stop crashing.

Contributor

bitcartel commented Nov 2, 2016

If you delete zcash-gtest and make it again, it will fail again (at least that is what I am seeing). After I moveed test_checkblock to the end of the list of tests did it stop crashing.

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Nov 2, 2016

Contributor

💔 Test failed - zcash

Contributor

zkbot commented Nov 2, 2016

💔 Test failed - zcash

@bitcartel

This comment has been minimized.

Show comment
Hide comment
@bitcartel

bitcartel Nov 3, 2016

Contributor
Contributor

bitcartel commented Nov 3, 2016

@ebfull

This comment has been minimized.

Show comment
Hide comment
@ebfull

ebfull Nov 3, 2016

Contributor

The latent slave is still up, @str4d's recommendation was to wait for it.

Contributor

ebfull commented Nov 3, 2016

The latent slave is still up, @str4d's recommendation was to wait for it.

@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Nov 3, 2016

Contributor

#1718 passed the problematic gtests after letting the latent server be destroyed, implying there is some weird caching error going on. But for now:

@zkbot retry

Contributor

str4d commented Nov 3, 2016

#1718 passed the problematic gtests after letting the latent server be destroyed, implying there is some weird caching error going on. But for now:

@zkbot retry

zkbot pushed a commit that referenced this pull request Nov 3, 2016

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Nov 3, 2016

Contributor

⌛️ Testing commit 6b82011 with merge 0a1130d...

Contributor

zkbot commented Nov 3, 2016

⌛️ Testing commit 6b82011 with merge 0a1130d...

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Nov 3, 2016

Contributor

💔 Test failed - zcash

Contributor

zkbot commented Nov 3, 2016

💔 Test failed - zcash

@ebfull

This comment has been minimized.

Show comment
Hide comment
@ebfull

ebfull Nov 3, 2016

Contributor

@zkbot retry

Contributor

ebfull commented Nov 3, 2016

@zkbot retry

zkbot pushed a commit that referenced this pull request Nov 3, 2016

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Nov 3, 2016

Contributor

⌛️ Testing commit 6b82011 with merge 11c9be9...

Contributor

zkbot commented Nov 3, 2016

⌛️ Testing commit 6b82011 with merge 11c9be9...

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Nov 3, 2016

Contributor

☀️ Test successful - zcash

Contributor

zkbot commented Nov 3, 2016

☀️ Test successful - zcash

@zkbot zkbot merged commit 6b82011 into zcash:master Nov 3, 2016

1 check passed

homu Test successful
Details

@str4d str4d deleted the str4d:metrics-screen-improvements branch Nov 3, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment