Increase test coverage for Dash specific code #1608

the-schnibble · 2017-09-04T13:53:36Z

added CTestLogger patch to enable test log
added masternode functionality to the test_framework library
added masternode rate check and proposal propagation tests
...

UdjinM6 · 2017-09-04T20:36:49Z

src/netaddress.cpp

@@ -221,7 +222,7 @@ bool CNetAddr::IsValid() const

 bool CNetAddr::IsRoutable() const
 {
-    return IsValid() && !(IsRFC1918() || IsRFC2544() || IsRFC3927() || IsRFC4862() || IsRFC6598() || IsRFC5737() || (IsRFC4193() && !IsTor()) || IsRFC4843() || IsLocal());
+    return IsValid() && (Params().NetworkIDString() == CBaseChainParams::REGTEST || !(IsRFC1918() || IsRFC2544() || IsRFC3927() || IsRFC4862() || IsRFC6598() || IsRFC5737() || (IsRFC4193() && !IsTor()) || IsRFC4843() || IsLocal()));


I don't think it's a good idea. We are trying to (slowly) get rid of the logic like that actually. I would rather see it somewhere where IsRoutable() is avoided specifically, not canceling function logic from the inside. One (preferable) way to do so would be to add smth like protected fRequireRoutable to CChainParams, specifying different values for mainnet/testnet/regtest, wrapping access into public function and using it via Params(). See fMiningRequiresPeers as as example. But again, this class is a base one and should be kept network agnostic, so rather use Params() where you want IsRoutable() to be avoided specifically.

Agree. Actually we can explicitly specify -externalip in tests and then IsRoutable should be skipped only at initialization phase: https://github.com/dashpay/dash/pull/1608/files#diff-9a82240fe7dfe86564178691cc57f2f1L207
Is it acceptable solution, what do you think?

Hmmm... let's take a step back actually - what issue exactly are you trying to solve by that?

The issue is that masternodes need to be accessible from the Internet, and local network addresses (like 127..., 192.168..., 172... etc) aren't considered as valid external addresses. So we need to make an exception and allow these addresses for our automatic tests.

Then maybe it's CMasternode::IsValidNetAddr that have to be fixed/adjusted instead?

It seems this method is already adjusted and always returns true for regtest. The issue with masternodes is here:

dash/src/activemasternode.cpp

Line 175 in 91ae0b7

nState = ACTIVE_MASTERNODE_NOT_CAPABLE;

BTW, this line also seems like workaround for explicitly specified addresses: https://github.com/dashpay/dash/blob/fe491f97868ff36a31646cbee7ad09a284b8beee/src/net.cpp#L210

UdjinM6 · 2017-09-06T15:33:26Z

build fails, looks like testlogger.h is missing in src/Makefile.am

the-schnibble · 2017-09-06T15:52:45Z

I have the same idea, but can't reproduce the issue on my local machine in order to verify.

UdjinM6

Looks better and passes tests now 👍

Found only few small issues so far, see comments

UdjinM6 · 2017-09-09T02:18:26Z

src/netaddress.cpp

@@ -11,6 +11,7 @@
 #include "hash.h"
 #include "utilstrencodings.h"
 #include "tinyformat.h"
+#include "chainparams.h"


should be removed

UdjinM6 · 2017-09-09T02:23:16Z

src/net.cpp

@@ -204,7 +204,8 @@ void AdvertiseLocal(CNode *pnode)
 // learn a new local address
 bool AddLocal(const CService& addr, int nScore)
 {
-    if (!addr.IsRoutable())
+    // TODO: skip IsRoutable only in regtest mode
+    if (!addr.IsRoutable() && nScore < LOCAL_MANUAL)


Should probably use Params().RequireRoutableExternalIP() here as well if this is still needed for tests to pass.

Surely! I forgot to commit net.cpp... Thanks.

Hmm.. shouldn't nScore < LOCAL_MANUAL go away here then? Also, // TODO: above is not a TODO anymore I guess ;)

Yes, I will remove TODO when tests finished. Actually, with nScore < LOCAL_MANUAL logic is unchanged even in regtest mode (for other tests without -expernalip parameter). But I think it can be removed safely...

Hmm... Untouched tests failed. Seems like port 11211 is busy for some reason. Same problem with this port was noticed yesterday.

…ization script

…ation tests added

…nalip=)

… the condition "explicitly specified address" in `AddLocal` method; other minor changes

OlegGirko · 2017-09-12T18:42:08Z

Could you please write longer and more descriptive commit messages, not one-liners?

It would be helpful if commit messages contain more detailed description of what is done here, as well as rationale for the change (why it's needed).

OlegGirko · 2017-09-12T18:44:24Z

Also, if you add a new file and then fix a bug in it in subsequent commit. please squash these commits together, to make it look like you've added a new file without any bugs originally. :-)

willwray · 2017-09-22T20:37:56Z

qa/rpc-tests/lib/base58.py

@@ -0,0 +1,124 @@
+'''


There's already a very similar version of this in contrib/testgen/base58.py.
The differences look like mostly format

Yes. Actually, entire lib folder is taken from sentinel. We have already discussed this issue with @OlegGirko. Presumably, the best way is to make this dash-specific code as python library with modules (like dash_hash, for example) in order to use it in the sentinel as well as in rpc-tests.
So far I'm preparing the wrapper for dash-specific functionality to add it to the BitcoinTestFramework.

nmarley · 2018-05-11T20:52:23Z

Also, if you add a new file and then fix a bug in it in subsequent commit. please squash these commits together, to make it look like you've added a new file without any bugs originally. :-)

@OlegGirko I don't agree w/this. It hides what is done on the PR and makes inline comments on the PR not make sense if those get superceded by changes. We already are squash-merging via Github, so I think it is ok to push a new commit w/o squashing and changing history on the PR.

OlegGirko · 2018-05-11T21:36:48Z

@nmarley:
OK, I'm not a big fan of squashing anyway, so my idea is following: if you find that something was wrong in one of your commits, fix this commit, not add a new commit on top of it. And it commit message became outdated for fixed commit, fix commit message as well. Once you've force-pushed fixed branch, online comments become outdated anyway, because you've fixed issues already.

Of course, this applies only for a branch being reviewed, but not merged yet. Once it's merged, it should not be amended.

And, BTW, squashing commits when merging PRs is wrong on many levels.

It makes finding what commit caused some particular change more difficult than necessary. Instead of finding commit with git blame, you essentially find a squashed commit corresponding some PR, and then need to preform many additional steps to consult Github and find the needed commit. Sometimes it's impossible (if you're working in disconnected mode). And it may become impossible at all in future, once Github goes bankrupt or its service discontinued. Relying on centralised service for essential tasks like this is wrong for decentralised project.
It makes rebasing of local branches unnecessary difficult. This was especially prominent when I was doing backports of selected Bitcoin PRs. I already had a long branch with changes, and every new PR was some first commits from that branch. But once that PR was merged, it was not that current development branch just moved few commits further in my branch (as it would be if PR was merged normal way), and I had to explicitly rebase part of my branch (without commits that were squash-merged) to move forward. This is unnecessary manual and error-prone step.

Hence, I vote for normal merging, as other projects do.

nmarley · 2018-05-11T21:50:35Z

if you find that something was wrong in one of your commits, fix this commit, not add a new commit on top of it

This is changing history, and requires "squashing" or "rebasing" or whatever you want to call it. It's a history change and after a PR has started to get reviewed by other eyes. Now previous commits aren't the same, and the reviewers have to start over completely.

it may become impossible at all in future, once Github goes bankrupt or its service discontinued

I don't think this is a valid argument, as PRs are temporary and main master/develop branches are all that matters. The commit history in these branches should be clean at any rate.

And, BTW, squashing commits when merging PRs is wrong on many levels.
Hence, I vote for normal merging, as other projects do.

I assume by normal merging, you mean a merge commit. Github added the "Squash and Merge" and "Rebase and Merge" as options, so there must be something right with those as well.

It's up to the developers to commit code, and the repo maintainers to decide how these get merged in. The maintainers have the ability to update the main commit message when they merge it in, thus "clean up" the commit messages. And AFAIK the way that changes get merged into the mainline repo is not up for a vote.

UdjinM6 · 2018-05-12T00:42:28Z

Since this PR had no reviews/changes for almost 8 months, if @the-schnibble (or anyone willing to take the job) is still interested in merging it, he'd need to address few points IMO:

This needs rebase to fix merge conflicts first;
Given the abandonment time, I think it would be ok to squash/reorganize commits in a bit more meaningful/easier_to_review set of changes while rebasing it and then start the review from scratch;
Watchdogs are no longer a thing, some RPC have changed too I believe, this should be fixed;
Not sure how this PR is going to be affected by DIP2/3 but I guess it still could be useful probably (whoever is going to submit changes needs to sync on that with @codablock).

UdjinM6 · 2018-10-15T15:56:54Z

Some changes were partially merged in other PRs, others are pretty much outdated now. Closing.

UdjinM6 reviewed Sep 4, 2017

View reviewed changes

the-schnibble force-pushed the increase_test_coverage branch 8 times, most recently from 5bf61b2 to 3ab24b6 Compare September 8, 2017 11:27

UdjinM6 reviewed Sep 9, 2017

View reviewed changes

the-schnibble force-pushed the increase_test_coverage branch 2 times, most recently from 19b2148 to b39de49 Compare September 11, 2017 14:48

the-schnibble added 15 commits September 11, 2017 18:33

patch to enable test log

c98dd0a

added an exception for local address restrictions in regtest mode

711ccd9

added the initial version of the dash regression test network initial…

a484338

…ization script

fixed masternode initialization

ad52323

chain initialization for masternodes functionality added

64df670

test_framework: update_ports fixed; nm_rate_check and proposal_propag…

b188fab

…ation tests added

skip IsRoutable check only for explicitly specified addresses (-exter…

a1faf1d

…nalip=)

mn_rate_check test integrated with rpc-tests

baab954

fix build

e78b393

added sentinel's dependencies to .travis.yml

e2e74c6

proposal_propagation test added

40e9022

fix mingw build

2c7e822

adjust AddLocal method only in regtest mode

f1bcf47

remove #include "chainparams.h" from netaddress.cpp

893bdb7

reduced number of masternodes in the mn_rate_check.py test; removed…

fc00a88

… the condition "explicitly specified address" in `AddLocal` method; other minor changes

the-schnibble force-pushed the increase_test_coverage branch from b39de49 to e1fb094 Compare September 11, 2017 15:35

fixed an issue with occasional proposal_propagation test fail

9e4e259

the-schnibble force-pushed the increase_test_coverage branch from e1fb094 to 9e4e259 Compare September 12, 2017 21:17

the-schnibble changed the title ~~[WIP] Increase test coverage for Dash specific code~~ Increase test coverage for Dash specific code Sep 21, 2017

base p2p port changed to 13000; comments removed

c1ac7dd

the-schnibble force-pushed the increase_test_coverage branch from 4a13a7d to c1ac7dd Compare September 21, 2017 14:55

willwray reviewed Sep 22, 2017

View reviewed changes

code refactoring

6b87f02

UdjinM6 changed the base branch from v0.12.2.x to develop May 11, 2018 20:14

UdjinM6 closed this Oct 15, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Increase test coverage for Dash specific code #1608

Increase test coverage for Dash specific code #1608

the-schnibble commented Sep 4, 2017 •

edited

UdjinM6 Sep 4, 2017

the-schnibble Sep 6, 2017

UdjinM6 Sep 6, 2017

the-schnibble Sep 6, 2017

UdjinM6 Sep 6, 2017 •

edited

the-schnibble Sep 6, 2017 •

edited

the-schnibble Sep 6, 2017 •

edited

UdjinM6 commented Sep 6, 2017

the-schnibble commented Sep 6, 2017

UdjinM6 left a comment

UdjinM6 Sep 9, 2017

the-schnibble Sep 9, 2017

UdjinM6 Sep 9, 2017

the-schnibble Sep 9, 2017

UdjinM6 Sep 9, 2017

the-schnibble Sep 9, 2017

the-schnibble Sep 9, 2017

OlegGirko commented Sep 12, 2017

OlegGirko commented Sep 12, 2017

willwray Sep 22, 2017

the-schnibble Sep 25, 2017 •

edited

nmarley commented May 11, 2018

OlegGirko commented May 11, 2018

nmarley commented May 11, 2018

UdjinM6 commented May 12, 2018

UdjinM6 commented Oct 15, 2018

Increase test coverage for Dash specific code #1608

Increase test coverage for Dash specific code #1608

Conversation

the-schnibble commented Sep 4, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

UdjinM6 Sep 6, 2017 • edited

Choose a reason for hiding this comment

the-schnibble Sep 6, 2017 • edited

Choose a reason for hiding this comment

the-schnibble Sep 6, 2017 • edited

Choose a reason for hiding this comment

UdjinM6 commented Sep 6, 2017

the-schnibble commented Sep 6, 2017

UdjinM6 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OlegGirko commented Sep 12, 2017

OlegGirko commented Sep 12, 2017

Choose a reason for hiding this comment

the-schnibble Sep 25, 2017 • edited

Choose a reason for hiding this comment

nmarley commented May 11, 2018

OlegGirko commented May 11, 2018

nmarley commented May 11, 2018

UdjinM6 commented May 12, 2018

UdjinM6 commented Oct 15, 2018

the-schnibble commented Sep 4, 2017 •

edited

UdjinM6 Sep 6, 2017 •

edited

the-schnibble Sep 6, 2017 •

edited

the-schnibble Sep 6, 2017 •

edited

the-schnibble Sep 25, 2017 •

edited