Skip to content

test: Set regtest.BIP65Height = 111 to speed up tests #21862

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 26, 2021
Merged

test: Set regtest.BIP65Height = 111 to speed up tests #21862

merged 1 commit into from
Aug 26, 2021

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented May 5, 2021 •
edited
Loading

No need to waste time by forcing creation of more than 1000 blocks to get the benefits of being able to test BIP 65. Also, reducing the height makes it more likely that (third-party) tests are conforming to BIP 65, which is enforced on mainnet for all new blocks.

@fanquake fanquake added the Tests label May 5, 2021
@maflcko maflcko force-pushed the 2105-testFasterBip65 branch from ce16c75 to fa91563 Compare May 5, 2021 07:26
@maflcko maflcko marked this pull request as draft May 5, 2021 07:29
@practicalswift
Copy link
Contributor

Concept ACK

@DrahtBot
Copy link
Contributor

DrahtBot commented May 5, 2021 •
edited
Loading

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #22788 (scripted-diff: Use generate* from TestFramework by MarcoFalke)
  • #22567 (test: Implicitly sync after generate* to preempt races and intermittent test failures by MarcoFalke)

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.

@theStack
Copy link
Contributor

theStack commented May 9, 2021

Concept ACK

This was referenced Jul 27, 2021
Merged
Closed
@maflcko maflcko force-pushed the 2105-testFasterBip65 branch from fa91563 to 13106f4 Compare August 5, 2021 09:32
@DrahtBot DrahtBot mentioned this pull request Aug 5, 2021
Merged
@maflcko maflcko force-pushed the 2105-testFasterBip65 branch 2 times, most recently from fe1e0ae to 94a14ed Compare August 17, 2021 13:15
@maflcko maflcko changed the title test: Set regtest.BIP65Height = 112 to speed up tests test: Set regtest.BIP65Height = 111 to speed up tests Aug 17, 2021
@Zero-1729
Copy link
Contributor

Concept ACK

@DrahtBot DrahtBot mentioned this pull request Aug 19, 2021
Merged
@maflcko maflcko marked this pull request as ready for review August 23, 2021 09:58
@maflcko maflcko force-pushed the 2105-testFasterBip65 branch from 94a14ed to faa5228 Compare August 23, 2021 09:59
@maflcko
Copy link
Member Author

maflcko commented Aug 23, 2021

Rebased and ready for review

theStack
theStack approved these changes Aug 23, 2021
View reviewed changes
Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

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

Tested ACK faa5228 🚀

Good to see that the burden of generating large numbers of blocks is more and more relieved. This enables nice speed-ups for the affected tests (>3x for feature_cltv.py, 2x for rpc_signrawtransaction.py on my machine).

Zero-1729
Zero-1729 approved these changes Aug 23, 2021
View reviewed changes
Copy link
Contributor

@Zero-1729 Zero-1729 left a comment

Choose a reason for hiding this comment

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

tACK faa5228 🧪

Tested on macOS v11.5.2

Clean changes, affected tests aren't broken. More importantly, saw the following times on master and this patch respectively:

Master

File User System
test/functional/feature_cltv.py 1.35s 0.36s
test/functional/rpc_blockchain.py 1.00s 0.31s
test/functional/rpc_signrawtransaction.py 3.44s 0.68s

After Patch

File User System
test/functional/feature_cltv.py 0.33s 0.11s
test/functional/rpc_blockchain.py 1.00s 0.31s
test/functional/rpc_signrawtransaction.py 1.52s 0.41s

@maflcko maflcko force-pushed the 2105-testFasterBip65 branch from faa5228 to faf7e48 Compare August 26, 2021 09:08
@maflcko
Copy link
Member Author

maflcko commented Aug 26, 2021

Rebased (trivial)

theStack
theStack approved these changes Aug 26, 2021
View reviewed changes
Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

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

re-ACK faf7e48 📍

Checked via git range-diff faa5228f...faf7e485 that changes since my last ACK are only rebase-related.

Zero-1729
Zero-1729 approved these changes Aug 26, 2021
View reviewed changes
Copy link
Contributor

@Zero-1729 Zero-1729 left a comment

Choose a reason for hiding this comment

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

re-ACK faf7e48 🧋

LGTM, clean rebase since last review 🧼

kristapsk
kristapsk approved these changes Aug 26, 2021
View reviewed changes
Copy link
Contributor

@kristapsk kristapsk left a comment

Choose a reason for hiding this comment

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

ACK faf7e48

@DrahtBot DrahtBot mentioned this pull request Aug 26, 2021
Merged
@fanquake fanquake merged commit adccbb3 into bitcoin:master Aug 26, 2021
@maflcko maflcko deleted the 2105-testFasterBip65 branch August 27, 2021 09:57
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 28, 2021
@fanquake
Merge bitcoin#21862: test: Set regtest.BIP65Height = 111 to speed up …
42c98c4 
…tests

faf7e48 Set regtest.BIP65Height = 111 to speed up tests (MarcoFalke)

Pull request description:

  No need to waste time by forcing creation of more than 1000 blocks to get the benefits of being able to test BIP 65. Also, reducing the height makes it more likely that (third-party) tests are conforming to BIP 65, which is enforced on mainnet for all new blocks.

ACKs for top commit:
  theStack:
    re-ACK faf7e48 📍
  Zero-1729:
    re-ACK faf7e48
  kristapsk:
    ACK faf7e48

Tree-SHA512: 79a8263e7233838666b9b636b496a8b9eb12398c779f9434677e1d62816732c0a7c7b3e73965be1fb0038d35e05e5a90e665bd74e9610104127dfc4ea38169bf
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
3 more reviewers

@theStack theStack theStack approved these changes

@kristapsk kristapsk kristapsk approved these changes

@Zero-1729 Zero-1729 Zero-1729 approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@maflcko @practicalswift @DrahtBot @theStack @Zero-1729 @kristapsk @fanquake