Skip to content
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

backport: bitcoin#16333, #21862, #22385, #22550, #22597, #22632, #22718, #22907 - fire up test chains by first block - 2/n #6189

Merged
merged 9 commits into from
Aug 14, 2024

Conversation

knst
Copy link
Collaborator

@knst knst commented Aug 9, 2024

Issue being fixed or feature implemented

Backports from bitcoin related to hard-fork mechanism and accelerated action

What was done?

see commits for backports.
Also fixed an issue of using from ChainState on RegTest: should be used specified chain, not the "best chain".

How Has This Been Tested?

Run unit & functional tests

Breaking Changes

That's a breaking changes for RegTest

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation
  • I have assigned this pull request to a milestone

MarcoFalke added 3 commits August 9, 2024 14:50
…yments

fa5658e Use DeploymentEnabled to hide VB deployments (MarcoFalke)
fa11fec doc: Move buried deployment doc to the enum that enumerates them (MarcoFalke)

Pull request description:

  Plus a doc commit.

ACKs for top commit:
  jnewbery:
    utACK fa5658e
  ajtowns:
    utACK fa5658e

Tree-SHA512: 2aeceee0674feb603d76656eff40695b7d7305de309f837bbb6a8c1dbb1d0b962b741f06ab7b9a8b1dbd1964c9c0c9aa5dc9588fd8e6d896e620b69e08eedbaa
…tests (speed, prevent timeout)

12f094e test: use constants for CSV/CLTV activation heights in rpc_signrawtransaction (Sebastian Falbesoner)
746f203 test: introduce `generate_to_height` helper, use in rpc_signrawtransaction (Sebastian Falbesoner)
e3237b1 test: check that CSV/CLTV are active in rpc_signrawtransaction (Sebastian Falbesoner)

Pull request description:

  This PR primarily aims to solve the current RPC timeout problem for test rpc_signrawtransaction.py, as described in bitcoin#22542. In the course of that the test is also improved in other ways (see bitcoin#22542 (review)).

  Reviewers guideline:
  * In `test_signing_with_cltv()`, a comment is fixed -- it wrongly referred to CSV, it should be CLTV.
  * As preparation, assertions are added that ensure that CSV and CLTV have been really activated after generating blocks by checking the 'softforks' output of the getblockchaininfo() RPC. Right now in master, one could remove (or decrease, like in bitcoin#22542) the generate calls and the test would still pass, when it shouldn't.
  * A helper `generate_to_height()` is introduced which improves the previous way of reaching a block height in two ways:
      - instead of blindly generating TH blocks to reach target block height >= TH, the current block height CH is taken into account, and only (TH - CH) are generated in total
      - to avoid potential RPC timeouts, the block generation is split up into multiple generatetoaddress RPC calls ([as suggested by laanwj](bitcoin#22542 (comment))); here chunks of 200 blocks have been chosen
   * The helper is used in the affected sub-tests, which should both speed-up the test (from ~18s to ~12s on my machine) and avoid potential timeouts
   * Finally, the activation constants for CSV and CLTV are used instead of using magic numbers 500 and 1500

  Open questions:
  * Any good naming alternatives for `generate_to_height()`? Not really happy with the name, happy to hear suggestions
  * Where to put the CSV and CLTV activation height constants in the test_framewor folder? I guess importing constants from other tests isn't really the desired way to go

ACKs for top commit:
  laanwj:
    Code review and tested ACK 12f094e
  rajarshimaitra:
    reACK bitcoin@12f094e

Tree-SHA512: 14509f6d3e5a5a05d6a30a3145bb82cd96a29d9d8a589abf1944a8bf34291cde78ce711195f52e9426588dc822b3618ec9b455e057360021ae46152bb7613516
… to avoid gcc warning

0591710 consensus/params: simplify ValidDeployment check to avoid gcc warning (Anthony Towns)

Pull request description:

  Simplifies the ValidDeployment check to only check the upperbound, relying on the lower bound to be trivially true due to enum values starting at the minimum value of the underlying type (which is checked at compile time in deploymentstatus.cpp). Avoids a "comparison always true" warning in some versions of gcc.

  Fixes bitcoin#22587

ACKs for top commit:
  MarcoFalke:
    review ACK 0591710
  tryphe:
    retested ACK 0591710

Tree-SHA512: e53b5d478b46d35ec476d004e3c92803afb874c138dd6ef3848861f4281cc113fe88921bc4ac74fd4decbf318ed776d3f816c3a1185f99dc36a5cfecfff51f7c
@knst knst added this to the 21.2 milestone Aug 9, 2024
@knst knst changed the title backport: bitcoin#16333, #21862, #22385, #22550, #22597, #22632, #22718, #22907 - Fire up RegTest by first block - 2/n backport: bitcoin#16333, #21862, #22385, #22550, #22597, #22632, #22718, #22907 - Fire up test chains by first block - 2/n Aug 9, 2024
@knst knst changed the title backport: bitcoin#16333, #21862, #22385, #22550, #22597, #22632, #22718, #22907 - Fire up test chains by first block - 2/n backport: bitcoin#16333, #21862, #22385, #22550, #22597, #22632, #22718, #22907 - fire up test chains by first block - 2/n Aug 9, 2024
UdjinM6
UdjinM6 previously approved these changes Aug 9, 2024
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

utACK 90e84fd

doc/release-notes-22632.md Outdated Show resolved Hide resolved
laanwj and others added 6 commits August 12, 2024 11:43
…tests

fafe896 test: Set regtest.BIP66Height = 102 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 66. Also, reducing the height makes it more likely that (third-party) tests are conforming to BIP 66, which is enforced on mainnet for all new blocks.

ACKs for top commit:
  GeneFerneau:
    Concept + code review ACK [fafe896](bitcoin@fafe896)
  0xB10C:
    crACK fafe896
  laanwj:
    ACK fafe896
  Zero-1729:
    tACK fafe896
  kristapsk:
    ACK fafe896. Full functional test suite showed few second speed incrase on my laptop (although I didn't do proper benchmarking with multiple runs, just single `time ./test/functional/test_runner.py` on current master vs this PR).
  theStack:
    Tested ACK fafe896
  hg333:
    tACK bitcoin@fafe896

Tree-SHA512: 4bbee3c8587d612e74a59fde49b6439c1296f2fc27d3a7cf59a35e920f729fdd581c930290bd04def618f81412236676ddb99b4ceb4d80dfb9fd610b128a04b1
…_csv_activation.py

fa676db test: pep-8 whitespace (MarcoFalke)
faed284 test: Avoid intermittent test failure in feature_csv_activation.py (MarcoFalke)

Pull request description:

  Otherwise there will be disconnects if the test runs longer than the default peertimeout (60s):

  ```
   node0 2021-09-05T20:28:30.973116Z (mocktime: 2021-09-01T07:17:29Z) [net] [net.cpp:1323] [InactivityCheck] socket receive timeout: 393061s peer=0
  ```

  Fix that by skipping `InactivityCheck` via a large `-peertimeout`.

ACKs for top commit:
  fanquake:
    ACK fa676db

Tree-SHA512: 061c0585a805aa2f8e55c4beedd4b8498a2951f33d60aa3632dda0a284db3a627d14a23dbd57e8a66c69a1612f39418e3a755c8ca97f6ae1105c0d70f0d1a801
222290f test: Set BIP34Height = 2 for regtest (MarcoFalke)
fac90c5 test: Create all blocks with version 4 or higher (MarcoFalke)

Pull request description:

  BIP34 is active on the current tip of mainnet, so all miners must obey it. It would be nice if it also was active in fresh regtest instances from the earliest time possible.

  I changed the BIP34 height to `2`, so that the block at height=1 may be used to mine a duplicate coinbase. (Needed to test mainnet behaviour)

  This pull is done in two commits:

  *  test: Create all blocks with version 4 or higher:
     Now that BIP34 is activated earlier, we need to create blocks with a higher version number. Just bump it to 4 instead of 2 to avoid having to bump it again later.

  *  test: Set BIP34Height = 2 for regtest:
     This fixes the BIP34 implementation in the tests (to match the one of the Core codebase) and updates the tests where needed

ACKs for top commit:
  ajtowns:
    ACK 222290f
  jonatack:
    ACK 222290f tested and reviewed rebased to current master 5e21382
  theStack:
    Tested ACK 222290f

Tree-SHA512: d69c637a62a64b8e87de8c7f0b305823d8f4d115c1852514b923625dbbcf9a4854b5bb3771ff41702ebf47c4c182a4442c6d7c0b9f282c95a34b83e56a73939b
fa76ebd doc: Add missing PR 16333 release note (MarcoFalke)

Pull request description:

  Missed in commit ad0fc45

ACKs for top commit:
  laanwj:
    Documentation review ACK fa76ebd
  Zero-1729:
    ACK fa76ebd
  theStack:
    ACK fa76ebd

Tree-SHA512: 5fc9e3f61d2f8534f537cc14c7518d09babd5ebbedd5f24ad10ca9aff69d499f3daa0c45eb548eec94cdbf25bf53c9f0158ec2ee7aa5c3f653f7670c68d1d24c
…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
Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

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

utACK e4e7c44

@knst
Copy link
Collaborator Author

knst commented Aug 14, 2024

Range-diff:

$ git range-diff origin/develop 2d1f3e2657d698de2b3d3a9cdc8947590edc9bbd e4e7c440f48058dabdc571a0a20626b513efd26c
 1:  fb00431b7c =  1:  fb00431b7c Merge bitcoin/bitcoin#22385: refactor: Use DeploymentEnabled to hide VB deployments
 2:  8928146bfa =  2:  8928146bfa Merge bitcoin/bitcoin#22550: test: improve `test_signing_with_{csv,cltv}` subtests (speed, prevent timeout)
 3:  cbd2be8e18 =  3:  cbd2be8e18 Merge bitcoin/bitcoin#22597: consensus/params: simplify ValidDeployment check to avoid gcc warning
 4:  c2b61f062b !  4:  fc25503cbc Merge bitcoin/bitcoin#22632: test: Set regtest.BIP66Height = 102 to speed up tests
    @@ Commit message
     
         Tree-SHA512: 4bbee3c8587d612e74a59fde49b6439c1296f2fc27d3a7cf59a35e920f729fdd581c930290bd04def618f81412236676ddb99b4ceb4d80dfb9fd610b128a04b1
     
    - ## doc/release-notes-22632.md (new) ##
    + ## doc/release-notes-6189.md (new) ##
     @@
     +Tests
     +-----
     +
     +- For the `regtest` network the BIP 66 (DERSIG) activation height was changed
    -+  from 1251 to 102. (#22632)
    ++  from 1251 to 102. (dash#6189)
     
      ## src/chainparams.cpp ##
     @@ src/chainparams.cpp: public:
 5:  94483a8c2f =  5:  71af8816ef Merge bitcoin/bitcoin#22907: test: Avoid intermittent test failure in feature_csv_activation.py
 6:  8401431a82 =  6:  101a863399 Merge bitcoin/bitcoin#16333: test: Set BIP34Height = 2 for regtest
 7:  a546f13ff3 <  -:  ---------- Merge bitcoin/bitcoin#22718: doc: Add missing PR 16333 release note
 -:  ---------- >  7:  adcf095ab9 Merge bitcoin/bitcoin#22718: doc: Add missing PR 16333 release note
 8:  a584b4da0b !  8:  65b92fa093 Merge bitcoin/bitcoin#21862: test: Set regtest.BIP65Height = 111 to speed up tests
    @@ Commit message
     
         Tree-SHA512: 79a8263e7233838666b9b636b496a8b9eb12398c779f9434677e1d62816732c0a7c7b3e73965be1fb0038d35e05e5a90e665bd74e9610104127dfc4ea38169bf
     
    - ## doc/release-notes-22632.md ##
    -@@ doc/release-notes-22632.md: Tests
    -   changed.
    -   * BIP 34 (blockheight in coinbase) from 500 to 2 (#16333)
    -   * BIP 66 (DERSIG) from 1251 to 102 (#22632)
    -+  * BIP 65 (CLTV) from 1351 to 111 (#21862)
    + ## doc/release-notes-6189.md ##
    +@@ doc/release-notes-6189.md: Tests
    +   changed. (dash#6189)
    +   * BIP 34 (blockheight in coinbase) from 500 to 2
    +   * BIP 66 (DERSIG) from 1251 to 102
    ++  * BIP 65 (CLTV) from 1351 to 111
     
      ## src/chainparams.cpp ##
     @@ src/chainparams.cpp: public:
 9:  90e84fd2c8 =  9:  e4e7c440f4 fix: use proper chain instead using ActiveChain for test framework
10:  2d1f3e2657 <  -:  ---------- Update doc/release-notes-22632.md

@PastaPastaPasta PastaPastaPasta merged commit df07c38 into dashpay:develop Aug 14, 2024
8 of 10 checks passed
@UdjinM6 UdjinM6 modified the milestones: 21.2, 22 Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants