backport bitcoin#16509 and add devnet test #3946

KolbyML · 2021-01-17T17:27:04Z

Allows tests to use networks other then regtest so we can add tests for devnet and solve issue #3550 Which I will probably make a PR for later today

This PR also adds a test to solve #3550

… "regtest" faf3683 test: Avoid hardcoding the chain name in combine_logs (MarcoFalke) fa8a1d7 test: Adapt test framework for chains other than "regtest" (MarcoFalke) 68f5466 test: Fix “local variable 'e' is assigned to but never used” (Ben Woosley) Pull request description: This is required for various work in progress: * testchains bitcoin#8994 * signet bitcoin#16411 * some of my locally written tests While it will be unused in the master branch as of now, it will make all of those pull requests shorter. Thus review for non-regtest tests can focus on the actual changes and not some test framework changes. ACKs for top commit: jonatack: ACK faf3683, ran tests and reviewed the code. Tree-SHA512: 35add66c12cab68f2fac8f7c7d47c604d3f24eae9336ff78f83e2c92b3dc08a25e7f4217199bac5393dd3fb72f945bba9c001d6fbb8efd298c88858075fcb3d6

KolbyML · 2021-01-18T00:42:40Z

I am gonna close the PR as I wanna change the code to work with devnets

UdjinM6 · 2021-01-18T00:54:06Z

If you are going to submit another one, pls add Dash specific code as a separate commit on top of the backport to make it easier to review (reopening this PR with a changed title and marking it as "draft' for now would be ok too btw).

PastaPastaPasta · 2021-01-18T00:55:33Z

Agree with Udjin, would be good to mark this as a draft if you want to do the devnet stuff before we review, then add that as a separate commit

KolbyML · 2021-01-18T02:51:28Z

Ok I will reopen it and add draft to the title, I will probably work on it for a few days then do like git fixup and will rename the "Dash specific code" commit

edit: I didn't know you could change it to a draft xD, just did that

PastaPastaPasta

Tested, seems to work. See comments

test/functional/p2p_connect_to_devnet.py

UdjinM6 · 2021-01-21T13:42:42Z

Looks good but incomplete :) Please see 9a5d7fc and 7834086

KolbyML · 2021-01-21T14:32:13Z

So you wanted me to add that? I did see that PR and did see it would make the original backport I wanted more complete but I didn't add it because I didn't need it for what I wanted to do.

I will keep that in mind for the future, as I guess that PR does make the one I was backporting more whole. So next time I will keep that in mind well backporting, thanks for the insight.

UdjinM6 · 2021-01-21T18:02:35Z

because I didn't need it for what I wanted to do.

Ah, I see. It makes sense I guess for some complicated set of backports with lots of things toughed at once but this one is relatively straightforward imo and we shouldn't leave tests in some inconsistent state in general.

UdjinM6

utACK

KolbyML · 2021-01-21T18:56:06Z

because I didn't need it for what I wanted to do.

Ah, I see. It makes sense I guess for some complicated set of backports with lots of things toughed at once but this one is relatively straightforward imo and we shouldn't leave tests in some inconsistent state in general.

Oh ok. I will keep that in mind.

PastaPastaPasta

utACK

PastaPastaPasta

Actually, please squash 0f6f62e into 84b4721

…t' in almost all current tests, revert one TODO while at it

KolbyML · 2021-01-21T20:56:46Z

Actually, please squash 0f6f62e into 84b4721

Ok I squashed the commits, I wanted to actually do that so awesome.

PastaPastaPasta

utACK, force-push clean

UdjinM6

utACK

xdustinface

utACK

This reverts commit e197f97 Signed-off-by: pasta <pasta@dashboost.org>

* Merge bitcoin#16509: test: Adapt test framework for chains other than "regtest" faf3683 test: Avoid hardcoding the chain name in combine_logs (MarcoFalke) fa8a1d7 test: Adapt test framework for chains other than "regtest" (MarcoFalke) 68f5466 test: Fix “local variable 'e' is assigned to but never used” (Ben Woosley) Pull request description: This is required for various work in progress: * testchains bitcoin#8994 * signet bitcoin#16411 * some of my locally written tests While it will be unused in the master branch as of now, it will make all of those pull requests shorter. Thus review for non-regtest tests can focus on the actual changes and not some test framework changes. ACKs for top commit: jonatack: ACK faf3683, ran tests and reviewed the code. Tree-SHA512: 35add66c12cab68f2fac8f7c7d47c604d3f24eae9336ff78f83e2c92b3dc08a25e7f4217199bac5393dd3fb72f945bba9c001d6fbb8efd298c88858075fcb3d6 * Add devnet support for tests * test: make sure devnet can connect to each other and start * Partial merge bitcoin#16681: Tests: Use self.chain instead of 'regtest' in almost all current tests, revert one TODO while at it Co-authored-by: MarcoFalke <falke.marco@gmail.com> Co-authored-by: Jorge Timón <jtimon@jtimon.cc>

, bitcoin#19972, bitcoin#20724, bitcoin#19884, bitcoin#21165, bitcoin#20721, bitcoin#21254, partial bitcoin#19829 (networking backports) 0d90465 merge bitcoin#21254: Avoid connecting to real network when running tests (Kittywhiskers Van Gogh) 2f672bd merge bitcoin#20721: Move ping data to net_processing (Kittywhiskers Van Gogh) 0d46acb merge bitcoin#21165: Use mocktime in test_seed_peers (Kittywhiskers Van Gogh) 8f40769 merge bitcoin#19884: No delay in adding fixed seeds if -dnsseed=0 and peers.dat is empty (Kittywhiskers Van Gogh) 446076d test: add missing `dnsseed=0` in configuration (Kittywhiskers Van Gogh) 081d8db mempool: remove stray boost::optional usage (Kittywhiskers Van Gogh) bcd383c merge bitcoin#20724: Cleanup of -debug=net log messages (Kittywhiskers Van Gogh) 8c63868 merge bitcoin#19972: Add fuzzing harness for node eviction logic (Kittywhiskers Van Gogh) 18f2dc0 partial bitcoin#19829: Move block inventory state to net_processing (Kittywhiskers Van Gogh) ec77bd3 net: move nLast{Block,TX}Time to match upstream location (Kittywhiskers Van Gogh) f635e4a merge bitcoin#20624: Remove nStartingHeight check from block relay (Kittywhiskers Van Gogh) 9f1a3e5 merge bitcoin#20477: Add unit testing of node eviction logic (Kittywhiskers Van Gogh) 2d838a6 merge bitcoin#20146: Send post-verack handshake messages at most once (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * [bitcoin#19884](bitcoin#19884) doesn't seem to play nice on its own and necessitated two more backports (namely [bitcoin#21165](bitcoin#21165) and [bitcoin#21254](bitcoin#21254)) before it did. * Trying to find why that is has been time consuming but there doesn't seem to be a concrete answer. If running the daemon normally (`dashd --regtest --dnsseed=1`), the functionality behaves as expected but the test still fails. * The closest explanation is that our OOO backports with relation to mocking time could explain why it isn't working as expected due to debug statements I added that always shown the time delta between each "enough time has passed" check was 0 seconds even when the log was advancing forward in time. * The usage of `dnsseed=0` stems from [bitcoin#16551](bitcoin#16551) (link to diff in commit comment), a backport that was skipped due to complexity. Though, some aspects of the PR have made it with [dash#3946](#3946). * [bitcoin#19829](bitcoin#19829) does away with `CConnman::ForEachNode` usage in `PeerManagerImpl::UpdatedBlockTip` ([source](https://github.com/bitcoin/bitcoin/pull/19829/files#diff-6875de769e90cec84d2e8a9c1b962cdbcda44d870d42e4215827e599e11e90e3R1318-R1328)). Dash cannot do the same and continues to use `ForEachNode` as we use the `CNode::CanRelay` check, which is not accessible through the `Peer` struct. * It would be valuable to find values that are Dash-specific (like `m_masternode_connection`) and migrate them to `Peer` to avoid Dash-specific patches and closer alignment with upstream. ## Breaking Changes Potential change in behaviour in the GUI and RPC. In RPC, `getpeerinfo` will now display `pingwait` and `startingheight` if `fStateStats` is true (earlier behaviour was unconditional). `startingheight` has been placed below `banscore` (earlier behaviour placed it above). In the GUI (Qt), `peerHeight` and `peerPingWait` are subject to similar conditionality as mentioned earlier. No changes in protocol or consensus. Changes are primarily related to refactoring, cleaning up, improving networking code and adding a new flag (`-fixedseeds`). ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: PastaPastaPasta: re-utACK 0d90465 Tree-SHA512: 79eedf47387a8715fc9f20c6bc051d4eae832266454445043e1478dc36daafc1679e002623917af43cf923735217622e3985f664123a1de23fadfdfece7e9b6b

KolbyML force-pushed the patch-9 branch from 1332296 to ebf48e9 Compare January 17, 2021 17:30

KolbyML closed this Jan 18, 2021

KolbyML reopened this Jan 18, 2021

KolbyML changed the title ~~Backport 16509~~ Draft: Backport 16509 Jan 18, 2021

KolbyML marked this pull request as draft January 18, 2021 02:54

KolbyML force-pushed the patch-9 branch from 2b4ad58 to 7c471f6 Compare January 19, 2021 23:21

KolbyML changed the title ~~Draft: Backport 16509~~ backport 16509 and add devnet test Jan 19, 2021

KolbyML force-pushed the patch-9 branch 3 times, most recently from ba1e1d4 to 6f5c130 Compare January 20, 2021 01:04

KolbyML marked this pull request as ready for review January 20, 2021 01:52

PastaPastaPasta requested a review from UdjinM6 January 20, 2021 16:38

PastaPastaPasta reviewed Jan 20, 2021

View reviewed changes

test/functional/p2p_connect_to_devnet.py Outdated Show resolved Hide resolved

test/functional/p2p_connect_to_devnet.py Outdated Show resolved Hide resolved

KolbyML requested a review from PastaPastaPasta January 20, 2021 18:54

Add devnet support for tests

9b98d0d

KolbyML force-pushed the patch-9 branch 2 times, most recently from 2ef9b45 to 0f6f62e Compare January 21, 2021 02:40

KolbyML force-pushed the patch-9 branch 2 times, most recently from c76a0de to ef37c2f Compare January 21, 2021 17:26

UdjinM6 added this to the 17 milestone Jan 21, 2021

UdjinM6 previously approved these changes Jan 21, 2021

View reviewed changes

PastaPastaPasta approved these changes Jan 21, 2021

View reviewed changes

PastaPastaPasta changed the title ~~backport 16509 and add devnet test~~ backport bitcoin#16509 and add devnet test Jan 21, 2021

PastaPastaPasta requested changes Jan 21, 2021

View reviewed changes

KolbyML dismissed UdjinM6’s stale review via 16b9d32 January 21, 2021 20:55

KolbyML force-pushed the patch-9 branch from ef37c2f to 16b9d32 Compare January 21, 2021 20:55

KolbyML and others added 2 commits January 21, 2021 13:55

test: make sure devnet can connect to each other and start

9ef6d27

Partial merge bitcoin#16681: Tests: Use self.chain instead of 'regtes…

16b9d32

…t' in almost all current tests, revert one TODO while at it

KolbyML requested review from PastaPastaPasta and UdjinM6 January 21, 2021 22:47

PastaPastaPasta approved these changes Jan 22, 2021

View reviewed changes

UdjinM6 approved these changes Jan 22, 2021

View reviewed changes

xdustinface approved these changes Jan 22, 2021

View reviewed changes

PastaPastaPasta merged commit e197f97 into dashpay:develop Jan 22, 2021

PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Jan 22, 2021

Revert "backport bitcoin#16509 and add devnet test (dashpay#3946)"

176f780

This reverts commit e197f97 Signed-off-by: pasta <pasta@dashboost.org>

kwvg mentioned this pull request Mar 23, 2024

backport: merge bitcoin#20146, #20477, #20624, #19972, #20724, #19884, #21165, #20721, #21254, partial bitcoin#19829 (networking backports) #5952

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

backport bitcoin#16509 and add devnet test #3946

backport bitcoin#16509 and add devnet test #3946

KolbyML commented Jan 17, 2021 •

edited

Loading

KolbyML commented Jan 18, 2021

UdjinM6 commented Jan 18, 2021

PastaPastaPasta commented Jan 18, 2021

KolbyML commented Jan 18, 2021 •

edited

Loading

PastaPastaPasta left a comment

UdjinM6 commented Jan 21, 2021

KolbyML commented Jan 21, 2021 •

edited

Loading

UdjinM6 commented Jan 21, 2021

UdjinM6 left a comment

KolbyML commented Jan 21, 2021

PastaPastaPasta left a comment

PastaPastaPasta left a comment

KolbyML commented Jan 21, 2021

PastaPastaPasta left a comment

UdjinM6 left a comment

xdustinface left a comment

backport bitcoin#16509 and add devnet test #3946

backport bitcoin#16509 and add devnet test #3946

Conversation

KolbyML commented Jan 17, 2021 • edited Loading

KolbyML commented Jan 18, 2021

UdjinM6 commented Jan 18, 2021

PastaPastaPasta commented Jan 18, 2021

KolbyML commented Jan 18, 2021 • edited Loading

PastaPastaPasta left a comment

Choose a reason for hiding this comment

UdjinM6 commented Jan 21, 2021

KolbyML commented Jan 21, 2021 • edited Loading

UdjinM6 commented Jan 21, 2021

UdjinM6 left a comment

Choose a reason for hiding this comment

KolbyML commented Jan 21, 2021

PastaPastaPasta left a comment

Choose a reason for hiding this comment

PastaPastaPasta left a comment

Choose a reason for hiding this comment

KolbyML commented Jan 21, 2021

PastaPastaPasta left a comment

Choose a reason for hiding this comment

UdjinM6 left a comment

Choose a reason for hiding this comment

xdustinface left a comment

Choose a reason for hiding this comment

KolbyML commented Jan 17, 2021 •

edited

Loading

KolbyML commented Jan 18, 2021 •

edited

Loading

KolbyML commented Jan 21, 2021 •

edited

Loading