Page MenuHomePhabricator

[consensus] Pin P2SH activation to block 173805 on mainnet
ClosedPublic

Authored by markblundeberg on Jun 19 2019, 03:54.

Details

Summary

Partial backport of PR11389:
https://github.com/bitcoin/bitcoin/pull/11389/commits/18e071841e83044b47aa45c3e98c0796a407d445

Reviewers please also confirm that the rest of the PR is not worth
backporting (it has to do with segwit activation / versionbits), so we
can mark it 'done'.

This is not a totally faithful backport, due to divergence (D1262).

Depends on D3365 due to how the miner_tests build on a weird chain.

Test Plan

make check
test_runner.py --extended

  • IBD with -checkpoints=0 and -assumevalid=0 on mainnet: OK up to at least height 216000 (2013 January)
  • IBD with -checkpoints=0 and -assumevalid=0 on testnet: OK up to at least height 230000 (2014 May)

Diff Detail

Repository
rABC Bitcoin ABC
Branch
patch_bip16height
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 6398
Build 10843: Bitcoin ABC Buildbot (legacy)
Build 10842: arc lint + arc unit

Event Timeline

src/test/miner_tests.cpp
508

This was causing a segfault (null pointer deref) without parent D3365, due to the call path TestBlockValidity / ConnectBlock / CheckInputs / GetSpendHeight / LookupBlockIndex.

Basically, the way the 210000-long chain is built above is very funky, and doesn't populate mapBlockIndex. So, LookupBlockIndex on that tip returns a nullptr, and GetSpendHeight barfs.

src/consensus/consensus.h
36

Note that this is the MTP for block 173804.

So, for both the old and new code the first block with P2SH flag is 173805.

markblundeberg added inline comments.
src/test/miner_tests.cpp
448

^^^ This bit removed (came from D1262).

470

vvv ditto for the bit below this line; otherwise this test is just copied down

Fabien added inline comments.
src/test/miner_tests.cpp
508

Nice ! This is the segfault I was after, good to see you found the missing backport !

src/validation.cpp
1565

Nit: remove extra parenthesis:
(pindex->nHeight + 1) => pindex->nHeight + 1

This revision is now accepted and ready to land.Jun 19 2019, 08:20
src/validation.cpp
1565

I'm going to keep it like this since it's styled the same as the other height-based activations, just below this if-statement.