Tests: Use self.chain instead of 'regtest' in all current tests #16681

jtimon · 2019-08-22T14:16:26Z

Simply avoiding the hardcoded string in more places for consistency.
It can also allow for more easily reusing tests for other chains other than regtest.

Separated from #8994 .
Continues #16509 .

It is still not complete (ie to be complete, we need the -chain parameter in #16680 and make whether acceptnonstdtxs is allowed for that chain or not customizable for regtest [or for custom chains like in #8994 ] ). But while being incomplete like #16509 , it's quite simple to review and another step forward IMO.

Sjors · 2019-08-22T18:03:19Z

Concept ACK

DrahtBot · 2019-08-22T21:17:08Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16895 (External signer multisig support by Sjors)
#16549 ([WIP] UI external signer support (e.g. hardware wallet) by Sjors)
#16546 ([WIP] External signer support - Wallet Box edition by Sjors)
#16528 (Native Descriptor Wallets using DescriptorScriptPubKeyMan by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

jtimon · 2019-09-06T21:20:19Z

Now also in newly included wallet_reorgsrestore.py

laanwj · 2019-09-10T13:56:55Z

I don't understand the rationale behind this. How much chance do these tests have of working with a non-regtest chain?

instagibbs · 2019-09-10T17:59:22Z

@laanwj with a custom testchain, most tests should work out of the box. Only ones that rely on specific genesis block information will break(there are a couple).

By itself it isn't greatly motivated, but it's also not a burden to maintain, so for me concept ACK.

laanwj · 2019-09-10T18:42:58Z

Many of the functional tests depend on specific parameters of the regtest chain to be able to test things: reward schedule, mining difficulty, even address format.

Anyhow, conecpt ACK on this change, I was just wondering.

jtimon · 2019-09-10T19:12:45Z

I don't understand the rationale behind this. How much chance do these tests have of working with a non-regtest chain?

Well, any custom chain created with #8994 should pass all the tests, like, for example, does -chain=regtest2 (in that PR).
Other chains like signets I guess don't benefit much from this change.
Projects based on bitcoin core that periodically rebase like elements should benefit too (although, yes, they will need further changes, this won't be enough for them).

I guess separating it is mostly about:

Making Testchains: Introduce custom chain whose constructor... #8994 smaller if this get merged first so that it's easier to review and maintain
hopefully new tests will copy the self.chain from existing tests instead of copying the "regtest". That would mean less work when rebasing Testchains: Introduce custom chain whose constructor... #8994

In summary, it's basically about #8994 , so if it doesn't make sense separated, I understand too.

laanwj · 2019-09-26T11:34:32Z

Well I think in any case it's better to have the chain configurable instead of hardcoding a magic string "regtest".

ACK 72d822f

maflcko · 2019-10-02T16:19:50Z

+-0 self.chain and regtest are the same for these test and I don't think it will ever change

jtimon · 2019-10-02T16:53:23Z

@MarcoFalke does that imply a nack to #8994 ?
Because that changes there.

jtimon · 2019-10-02T16:57:23Z

Also, wouldn't it make sense alone, if anything, for consistency with #16509 ?

maflcko · 2019-10-02T17:47:48Z

does that imply a nack to #8994 ?

Yeah, I am -0 on changing to regtest2.

Also, wouldn't it make sense alone, if anything, for consistency with #16509 ?

Yeah, +0 on this.

So overall +-0

Sjors · 2019-10-06T08:04:42Z

ACK 72d822f. Tests still pass on macOS 10.14.6.

maflcko · 2019-10-07T17:14:12Z

Also, this is not a complete fix, since it is impossible to protect against re-introduction of regtest in new code. E.g. https://github.com/bitcoin/bitcoin/blob/eeecdfa27a14df83c441cb4b04c0ae78a2ccbd2c/test/functional/feature_loadblock.py#L39

maflcko · 2019-10-07T17:14:35Z

So, I'll change to overall -0.1

laanwj · 2019-10-08T08:23:51Z

Also, this is not a complete fix, since it is impossible to protect against re-introduction of regtest in new code. E.g.

Strictly you could: randomize the test chain name every time.
Not sure it's worth it though …

Yeah, I am -0 on changing to regtest2.

I don't think we should change the default name of the regtest network either. The only chain we'd potentially rename would be testnet3, if the rules change, but an instance of regtest is meant to be short-lived.

jtimon · 2019-10-08T20:15:27Z

Also, this is not a complete fix,...

Rebased on top of master, but I don't see that test in master. It seems that commit doesn't belong in master unless I'm missing something.

I added jtimon@e889b4b#diff-6651d2553d181295a8570f18069e6b2bR28 for completion when doing the grep.
In any case, even if it's not complete (and it's not complete because it lacks something like jtimon@0943ce0#diff-2a344479dbbded9df5d6a4abde2cd48cL44), it makes #16509 more complete.

I don't think we should change the default name of the regtest network either.

I created #17032 and #17037 separated from #8994 that unlike #8994 don't change the default self.chain to regtest2. Neither of them (#17032 and #17037) need this, but I still think it's worth it for consistency and to complete #16509.

And to reiterate @laanwj 's words:

Well I think in any case it's better to have the chain configurable instead of hardcoding a magic string "regtest".

But if those reasons aren't enough and there's little chances of changing the default "regtest" to "regtest2" in the tests, then #8994 is not a justification for this either (by the way, you can go nack it for that reason, I guess). In that case, I guess we should close this.

Or perhaps we can wait for #17032 or #17037 to be merged first for this to be able to be complete.

To be honest, what I care the most about personally now is #17037, not #8994 anymore.

maflcko · 2019-10-08T20:39:22Z

In that case, I guess we should close this.

Ok, closing then

jtimon · 2019-10-25T17:30:35Z

As answered before, is (or was) only not complete for one case whuch is contemplated in other prs. The example you poibted out wasn't ecen part of master. Of course, the more we delay this the more new things there will be to correct, for people will continue to copy "regtest" instead of self.chain from other tests. I will grep again later, but I really hope this time you're not talking about other branches again. Feel free to poibt out which cases make this incomplete according to you.

…

On Fri, Oct 25, 2019, 19:15 MarcoFalke ***@***.***> wrote: So consistency and completion are not good enough reasons to do things and neither is avoiding hardcoding strings? As mentioned previously, this is not complete nor consistent. If you rebase on master, new regtest strings have been introduced. And there is no sane way to prevent that happen in the future, even if this pull was merged. Also, we have a lot more stuff hardcoded to regtest in the functional tests, e.g. the bech32 hrp in ADDRESS_BCRT1_UNSPENDABLE... — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#16681?email_source=notifications&email_token=AAHWGSV6LZ7PC7YALNVC6ULQQMSTVA5CNFSM4IOVRCR2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECI7JZI#issuecomment-546436325>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHWGSSCEHRC5ZGZL64DZB3QQMSTVANCNFSM4IOVRCRQ> .

jtimon · 2019-10-26T11:26:44Z

Rebased, adapted new test feature_loadblock.py

Sjors · 2019-10-27T18:39:14Z

re-ACK 1abcecc. I think it's an improvement even if incomplete and if some PR's might accidentally bring "regtest" back. Subsequent improvements hopefully don't have to touch 16 files.

I'd like to break the circular arguments between this and related pull requests. Those are complicated enough to reason about without "but I don't like this name change in the other PR"; using self.chain instead of "regtest" gives us more optionality.

ryanofsky

Code review ACK 1abcecc.

I think one reason a change like this can get stuck in the review process is not stating a motivation or list of practical advantages in the PR description. When a description begins with "Separated from _. continues _. It is incomplete...", it suggests a rabbit hole few people will want to go down.

On the other hand, even though our review process is intentionally conservative, I think if a change is safe and has had sufficient code review that we shouldn't hold neutral or "+0" feedback against it. If as a reviewer you think drawbacks and risks of a change outweigh its benefits, you should actually state that, so we can avoid uncertainty and open PRs sitting in limbo.

ryanofsky

Code review ACK 1abcecc

jtimon · 2020-02-04T11:46:46Z

I guess if I rebase now there will be more instances, but then I would lose the 2 ACKs.
What should I do?

elichai · 2020-02-04T13:51:59Z

Code review ACK 1abcecc
I do wonder how many of these tests will fail on testnet (even a custom one with difficulty 1).
Anyhow reducing hard coded strings is good IMHO, and I think even though it's incomplete it will help prevent future hard coded strings just by the fact that a lot of people copy-paste that code.

ryanofsky · 2020-02-04T16:12:27Z

I guess if I rebase now there will be more instances, but then I would lose the 2 ACKs.
What should I do?

I'd suggest rebasing, the changes here are straightforward and I'm happy to reack.

maflcko · 2020-02-04T20:55:54Z

This has two ACKs, merging now.

…nt tests 1abcecc Tests: Use self.chain instead of 'regtest' in almost all current tests (Jorge Timón) Pull request description: Simply avoiding the hardcoded string in more places for consistency. It can also allow for more easily reusing tests for other chains other than regtest. Separated from #8994 . Continues #16509 . It is still not complete (ie to be complete, we need the -chain parameter in #16680 and make whether acceptnonstdtxs is allowed for that chain or not customizable for regtest [or for custom chains like in #8994 ] ). But while being incomplete like #16509 , it's quite simple to review and another step forward IMO. ACKs for top commit: Sjors: re-ACK 1abcecc. I think it's an improvement even if incomplete and if some PR's might accidentally bring "regtest" back. Subsequent improvements hopefully don't have to touch 16 files. elichai: Code review ACK 1abcecc ryanofsky: Code review ACK 1abcecc. ryanofsky: Code review ACK 1abcecc Tree-SHA512: 5620de6dab235ca8bd8670d6366c7b9f04f0e3ca9c5e7f87765b38e16ed80c17d7d1630c0d5fd7c5526f070830d94dc74cc2096d8ede87dc7180ed20569509ee

fanquake · 2020-02-05T00:08:05Z

Opened #18068 to track a followup PR.

eca56f8 test: replace 'regtest' leftovers by self.chain (Sebastian Falbesoner) Pull request description: This is a follow-up PR to #16681 (fixes #18068), replacing all remaining hardcoded `"regtest"` strings in functional tests by `self.chain`. Top commit has no ACKs. Tree-SHA512: 96524649b33164938e5a95215991103ed7855ebab55ef788d4816b3fa5cbc03d8f3b0d39f2247a87522f289fd7f4daf25e059900b8462b5127eb154bbee89054

…l current tests 1abcecc Tests: Use self.chain instead of 'regtest' in almost all current tests (Jorge Timón) Pull request description: Simply avoiding the hardcoded string in more places for consistency. It can also allow for more easily reusing tests for other chains other than regtest. Separated from bitcoin#8994 . Continues bitcoin#16509 . It is still not complete (ie to be complete, we need the -chain parameter in bitcoin#16680 and make whether acceptnonstdtxs is allowed for that chain or not customizable for regtest [or for custom chains like in bitcoin#8994 ] ). But while being incomplete like bitcoin#16509 , it's quite simple to review and another step forward IMO. ACKs for top commit: Sjors: re-ACK 1abcecc. I think it's an improvement even if incomplete and if some PR's might accidentally bring "regtest" back. Subsequent improvements hopefully don't have to touch 16 files. elichai: Code review ACK 1abcecc ryanofsky: Code review ACK 1abcecc. ryanofsky: Code review ACK 1abcecc Tree-SHA512: 5620de6dab235ca8bd8670d6366c7b9f04f0e3ca9c5e7f87765b38e16ed80c17d7d1630c0d5fd7c5526f070830d94dc74cc2096d8ede87dc7180ed20569509ee

Summary: This is a backport of Core [[bitcoin/bitcoin#16681 | PR16681]] and [[bitcoin/bitcoin#18069 | PR18069]] I also replaced one occurence of 'regtest' in test_framework.py, that was missed in the backport of [[bitcoin/bitcoin#16509 | PR16509]] (D5942) Test Plan: `ninja && ninja check-functional-extended` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D8738

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

* 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 <[email protected]> Co-authored-by: Jorge Timón <[email protected]>

* Merge #16509: test: Adapt test framework for chains other than "regtest" faf36838bdba7393960fce6ad0c56dc1f93f5870 test: Avoid hardcoding the chain name in combine_logs (MarcoFalke) fa8a1d7ba30040f8c74f93fc41a61276c255a6a6 test: Adapt test framework for chains other than "regtest" (MarcoFalke) 68f546635d5de2ccfedadeabc7bc79e12e5eca6a 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 #8994 * signet #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 faf36838bdba7393960fce6ad0c56dc1f93f5870, 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/bitcoin#16681: Tests: Use self.chain instead of 'regtest' in almost all current tests, revert one TODO while at it Co-authored-by: MarcoFalke <[email protected]> Co-authored-by: Jorge Timón <[email protected]>

jtimon mentioned this pull request Aug 22, 2019

Testchains: Introduce custom chain whose constructor... #8994

Closed

12 tasks

DrahtBot added the Tests label Aug 22, 2019

jtimon force-pushed the b19-testchains-tests branch 2 times, most recently from b1f99ad to 5f50425 Compare August 22, 2019 17:48

jtimon changed the title ~~QA: Adapt BitcoinTestFramework for chains other than "regtest"~~ Tests: Use self.chain instead of 'regtest' in all current tests Aug 22, 2019

jtimon force-pushed the b19-testchains-tests branch from 5f50425 to 897f7ef Compare September 6, 2019 21:18

jtimon mentioned this pull request Sep 7, 2019

Custom signets jtimon/bitcoin#12

Open

4 tasks

jtimon force-pushed the b19-testchains-tests branch 2 times, most recently from 010a0e5 to 72d822f Compare September 12, 2019 18:01

jtimon mentioned this pull request Oct 3, 2019

Tests: Chainparams: Make regtest almost fully customizable #17032

Closed

jtimon force-pushed the b19-testchains-tests branch from 72d822f to 83f08b3 Compare October 8, 2019 20:13

maflcko closed this Oct 8, 2019

Tests: Use self.chain instead of 'regtest' in almost all current tests

1abcecc

jtimon force-pushed the b19-testchains-tests branch from 83f08b3 to 1abcecc Compare October 26, 2019 11:25

ryanofsky approved these changes Oct 30, 2019

View reviewed changes

ryanofsky approved these changes Nov 18, 2019

View reviewed changes

jtimon mentioned this pull request Feb 4, 2020

test: Adapt test framework for chains other than "regtest" #16509

Merged

maflcko merged commit 1abcecc into bitcoin:master Feb 4, 2020

fanquake mentioned this pull request Feb 5, 2020

Followups from #16681 #18068

Closed

theStack mentioned this pull request Feb 5, 2020

test: replace 'regtest' leftovers by self.chain #18069

Merged

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

KolbyML pushed a commit to KolbyML/bitcoin that referenced this pull request Jan 21, 2021

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

16b9d32

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

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Tests: Use self.chain instead of 'regtest' in all current tests #16681

Tests: Use self.chain instead of 'regtest' in all current tests #16681

Uh oh!

Conversation

jtimon commented Aug 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Aug 22, 2019

Uh oh!

DrahtBot commented Aug 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

jtimon commented Sep 6, 2019

Uh oh!

laanwj commented Sep 10, 2019

Uh oh!

instagibbs commented Sep 10, 2019

Uh oh!

laanwj commented Sep 10, 2019

Uh oh!

jtimon commented Sep 10, 2019

Uh oh!

laanwj commented Sep 26, 2019

Uh oh!

maflcko commented Oct 2, 2019

Uh oh!

jtimon commented Oct 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jtimon commented Oct 2, 2019

Uh oh!

maflcko commented Oct 2, 2019

Uh oh!

Sjors commented Oct 6, 2019

Uh oh!

maflcko commented Oct 7, 2019

Uh oh!

maflcko commented Oct 7, 2019

Uh oh!

laanwj commented Oct 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jtimon commented Oct 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Oct 8, 2019

Uh oh!

jtimon commented Oct 25, 2019 via email

Uh oh!

jtimon commented Oct 26, 2019

Uh oh!

Sjors commented Oct 27, 2019

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

jtimon commented Feb 4, 2020

Uh oh!

elichai commented Feb 4, 2020

Uh oh!

ryanofsky commented Feb 4, 2020

Uh oh!

maflcko commented Feb 4, 2020

Uh oh!

fanquake commented Feb 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

jtimon commented Aug 22, 2019 •

edited

Loading

DrahtBot commented Aug 22, 2019 •

edited

Loading

jtimon commented Oct 2, 2019 •

edited

Loading

laanwj commented Oct 8, 2019 •

edited

Loading

jtimon commented Oct 8, 2019 •

edited

Loading