Skip to content

test: set segwit height back to 0 on regtest#24527

Merged
maflcko merged 1 commit intobitcoin:masterfrom
mzumsande:202203_regtest_segwitheight
Mar 13, 2022
Merged

test: set segwit height back to 0 on regtest#24527
maflcko merged 1 commit intobitcoin:masterfrom
mzumsande:202203_regtest_segwitheight

Conversation

@mzumsande
Copy link
Contributor

@mzumsande mzumsande commented Mar 10, 2022

The change of consensus.SegwitHeight from 0 to 1 for regtest in #22818 had the effect that if I create a regtest enviroment with current master (or 23.x), and then try to load this chain with an older version (22.x), I get an InitError
Witness data for blocks after height 0 requires validation. Please restart with -reindex
and have to reindex because BLOCK_OPT_WITNESS is no longer set for the Genesis block and NeedsRedownload() in validation returns true with an older version.
That might be a bit annoying for tests that use a shared regtest dir with different versions.

If people think this is enough of an issue to be worth fixing, I think it should also make it into 23.x

@mzumsande
test: set segwit height back to 0 on regtest
5ce3057 
This was changed in bitcoin#22818 from 0 to 1. Since it changes
BLOCK_OPT_WIT of the genesis block, older versions of bitcoin
core would not read regtest directories created with newer versions
without a reindex.
@mzumsande mzumsande mentioned this pull request Mar 10, 2022
Merged
@maflcko maflcko added the Tests label Mar 10, 2022
@maflcko
Copy link
Member

maflcko commented Mar 10, 2022

The workaround in the meantime would be to write the genesis block with a previous version (or call -reindex to re-write it with a previous version).

@maflcko
Copy link
Member

maflcko commented Mar 10, 2022 •
edited
Loading

I haven't looked, but an alternative code patch would be to not check the genesis block for the witness flag. (Though, such a patch might be harder to backport)

Unrelated: It is still a problem that the genesis block is passed around too much in validation. For example it is passed to ContextualCheckBlock, which will then lead to assertion failures if other deployments are backdated to it:

$ ./src/qt/bitcoin-qt -datadir=/tmp -regtest -reindex
bitcoin-qt: validation.cpp:3432: bool ContextualCheckBlock(const CBlock &, BlockValidationState &, const Consensus::Params &, const CBlockIndex *): Assertion `pindexPrev != nullptr' failed.
Aborted (core dumped)

theStack
theStack approved these changes Mar 11, 2022
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.

Concept and code-review ACK 5ce3057

In #22818 I suggested setting the segwit height to 1 only for consistency reasons (see #22818 (comment)), but given the problem you describe it seems reasonable to set it back to 0.

@maflcko maflcko added this to the 23.0 milestone Mar 13, 2022
@maflcko maflcko merged commit 2860a91 into bitcoin:master Mar 13, 2022
jonatack pushed a commit to jonatack/bitcoin that referenced this pull request Mar 13, 2022
@mzumsande @jonatack
test: set segwit height back to 0 on regtest
b1646f1 
This was changed in bitcoin#22818 from 0 to 1. Since it changes
BLOCK_OPT_WIT of the genesis block, older versions of bitcoin
core would not read regtest directories created with newer versions
without a reindex.

Github-Pull: bitcoin#24527
Rebased-From: 5ce3057
@jonatack jonatack mentioned this pull request Mar 13, 2022
Merged
@jonatack
Copy link
Member

jonatack commented Mar 13, 2022

Included in #24512 for backport to v23.

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Mar 13, 2022
Merge bitcoin#24527: test: set segwit height back to 0 on regtest
9464ddb 
5ce3057 test: set segwit height back to 0 on regtest (Martin Zumsande)

Pull request description:

  The change of `consensus.SegwitHeight` from 0 to 1 for regtest in bitcoin#22818 had the effect that if I create a regtest enviroment with  current master (or 23.x), and then try to load this chain with an older version (22.x), I get an InitError
  `Witness data for blocks after height 0 requires validation. Please restart with -reindex`
  and have to reindex because `BLOCK_OPT_WITNESS` is no longer set for the Genesis block and `NeedsRedownload()` in validation returns `true` with an older version.
  That might be a bit annoying for tests that use a shared regtest dir with different versions.

  If people think this is enough of an issue to be worth fixing, I think it should also make it into 23.x

ACKs for top commit:
  theStack:
    Concept and code-review ACK 5ce3057

Tree-SHA512: b0e89ff7fc953bc0ae929d2da44cde7149321d987fb4763934f6c9635d00d807129a50b459cc5e69e86bb1819e4b063b969486e8016a1cb8db8f905fa315653d
fanquake added a commit that referenced this pull request Mar 31, 2022
@fanquake
Merge #24512: 23.x backports
c243e08 
174af33 util: Add inotify_rm_watch to syscall sandbox (AllowFileSystem) (Hennadii Stepanov)
ded10fe build: Fix Boost.Process test for Boost 1.78 (Hennadii Stepanov)
26c2f23 build: Fix Boost.Process detection on macOS arm64 (Hennadii Stepanov)
85f85c7 util: add linkat to syscall sandbox (AllowFileSystem) (fanquake)
eaa0419 contrib: fix signet miner (sighash mismatch) (Sebastian Falbesoner)
235b042 rpc: Exclude descriptor when address is excluded (MarcoFalke)
b05a59b ci: Temporarily use clang-13 to work around clang-14 TSan bug (MarcoFalke)
65b9667 doc, init: add links to doc/cjdns.md (Jon Atack)
7a553d4 doc: update i2p.md with cjdns, improve local addresses section (Jon Atack)
4148396 doc: update tor.md with cjdns and getnodeaddresses, fix tor grep, (Jon Atack)
4690e8a doc: create initial doc/cjdns.md for cjdns how-to documentation (Jon Atack)
5d24f61 Clarify in -maxtimeadjustment that only outbound peers influence time data (Jon Atack)
b1646f1 test: set segwit height back to 0 on regtest (Martin Zumsande)
ef6a37b rpc: rename getdeploymentinfo status-next to status_next (Jon Atack)
2a6fcf9 init, doc: improve -onlynet help and tor/i2p documentation (Jon Atack)

Pull request description:

  Backport the following to 23.x:

  - #24468
  - #24528
  - #24527
  - #24609
  - #24555
  - #24663
  - #24572
  - #24636
  - #24553
  - #24659
  - #24521
  - #24523
  - #24690
  - #24710

  Possibly also:
  - #24579
  - #24691

ACKs for top commit:
  laanwj:
    List-of-commits ACK 174af33, I think we should merge this and move forward with rc3..
  hebasto:
    ACK 174af33

Tree-SHA512: 5a493e1652b780b527767d6ca9e67012abd2fa5573496e85e0d8aa4bed3eb332bfcd72610b8dfb954ff274d42450623233c96c479de2085b9c8344ba5abf1935
@mzumsande mzumsande deleted the 202203_regtest_segwitheight branch April 6, 2022 13:03
@bitcoin bitcoin locked and limited conversation to collaborators Apr 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@theStack theStack theStack approved these changes

@maflcko maflcko maflcko approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Tests

Projects

None yet

Milestone

23.0

Development

Successfully merging this pull request may close these issues.

5 participants

@mzumsande @maflcko @jonatack @theStack @fanquake