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

1. 
   mzumsande commented at 7:48 pm on March 10, 2022: member

   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

2. test: set segwit height back to 0 on regtest

   This was changed in #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.

   5ce3057c8e
3. MarcoFalke added the label Tests on Mar 10, 2022
4. 
   MarcoFalke commented at 7:53 pm on March 10, 2022: member

   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).
5. 
   MarcoFalke commented at 7:57 pm on March 10, 2022: member

   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:

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

6. MarcoFalke approved
7. theStack approved
8. 
   theStack commented at 11:07 am on March 11, 2022: member

   Concept and code-review ACK 5ce3057c8e8f192921fd5e4bdb95bb15e3f7dbad

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

9. MarcoFalke added the label Needs backport (23.x) on Mar 13, 2022
10. MarcoFalke added this to the milestone 23.0 on Mar 13, 2022
11. MarcoFalke merged this on Mar 13, 2022
12. MarcoFalke closed this on Mar 13, 2022

    ---

13. jonatack referenced this in commit b1646f1bb5 on Mar 13, 2022
14. 
    jonatack commented at 4:46 pm on March 13, 2022: member

    Included in #24512 for backport to v23.
15. sidhujag referenced this in commit 9464ddbfe4 on Mar 13, 2022
16. fanquake removed the label Needs backport (23.x) on Mar 16, 2022
17. hebasto referenced this in commit af3ef5b386 on Mar 31, 2022
18. fanquake referenced this in commit c243e08351 on Mar 31, 2022
19. mzumsande deleted the branch on Apr 6, 2022
20. DrahtBot locked this on Apr 6, 2023

Contributors
mzumsande MarcoFalke theStack jonatack

Labels
Tests

Milestone
23.0