MarcoFalke
commented at 1:43 pm on July 3, 2019:
member
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
fanquake added the label
Tests
on Jul 3, 2019
in
test/functional/test_framework/blocktools.py:99
in
faa2e0d668outdated
108- return r
109+
110+def script_BIP34_coinbase_height(height):
111+ if height <= 16:
112+ res = CScriptOp.encode_op_n(height)
113+ # Append dummy to increase scriptSig size above 2
clang format will put them in separate lines, which is wasteful. So I ran the “paragraph formatter” in vim for extra compaction. I don’t think this needs to be human-readable, so should be fine as is.
MarcoFalke
commented at 12:36 pm on August 5, 2019:
Done. Added // clang-format {off,on}
MarcoFalke force-pushed
on Jul 3, 2019
MarcoFalke closed this
on Jul 3, 2019
MarcoFalke reopened this
on Jul 3, 2019
DrahtBot
commented at 5:57 pm on July 3, 2019:
member
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Conflicts
No conflicts as of last run.
promag
commented at 3:38 pm on July 4, 2019:
member
Fails with Assertion failed: lock cs_main not held in ./validation.h:411
fanquake added the label
Waiting for author
on Jul 5, 2019
ajtowns
commented at 1:51 am on July 5, 2019:
member
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 didn’t quite make sense to me, but I think it works. I think the two behaviours that warrant testing are:
ajtowns
commented at 6:25 am on July 16, 2019:
member
Based on the comments in #14633 our implementation doesn’t match BIP34 for blocks at heights lower than 17 (since we do OP_1..OP_16 instead of non-minimal 0x01 [0x01...0x10] as per BIP34); maybe it might be better to only test BIP34 from block 17 rather than block 2? FWIW, tests look like they work unchanged with BIP34Height = 17 instead of 2.
If so, could be worth adding assert(globalChainParams->GetConsensus().BIP34Height > 16); to SelectParams() along with a comment about divergence from BIP34 if that assertion were violated.
Alternatively, seems like BIP34 probably should be updated to at least note the divergence to the original specification for bitcoind’s regtest (and potentially signet or “tapnet” or similar, which might actually require interoperability with other clients).
(Edited to add: ACK once the above is dealt with one way or another, fwiw)
MarcoFalke referenced this in commit
62117f9f36
on Aug 5, 2019
MarcoFalke removed the label
Waiting for author
on Aug 5, 2019
MarcoFalke
commented at 12:27 pm on August 5, 2019:
member
maybe it might be better to only test BIP34 from block 17 rather than block 2
If so, could be worth adding assert(globalChainParams->GetConsensus().BIP34Height > 16);
Adding this assert would break existing test networks such as
sidhujag referenced this in commit
f44d5064d6
on Aug 10, 2019
MarcoFalke force-pushed
on Aug 15, 2019
DrahtBot added the label
Needs rebase
on Sep 17, 2019
MarcoFalke force-pushed
on Sep 17, 2019
DrahtBot removed the label
Needs rebase
on Sep 17, 2019
jtimon
commented at 10:12 pm on October 10, 2019:
contributor
Concept ACK
Just a question, the second commit won’t pass the tests until the next commit is applied too, will it?
If that’s the case, perhaps we should consider a squash for “git bisect” purposes.
If all commits pass all tests independently, I said nothing.
MarcoFalke
commented at 6:37 pm on October 11, 2019:
member
Huh, the second commit changes nothing other than that the tests mine version 4 blocks instead of version 1 blocks. If version 1 blocks were accepted, then version 4 blocks are accepted as well under our current consensus rules. So the second commit should pass. And I am pretty sure I did check that it passed.
DrahtBot added the label
Needs rebase
on Oct 30, 2019
MarcoFalke force-pushed
on Oct 30, 2019
DrahtBot removed the label
Needs rebase
on Oct 30, 2019
MarcoFalke force-pushed
on Jan 2, 2020
DrahtBot added the label
Needs rebase
on Mar 5, 2020
MarcoFalke force-pushed
on Apr 30, 2020
DrahtBot removed the label
Needs rebase
on Apr 30, 2020
DrahtBot added the label
Needs rebase
on May 23, 2020
MarcoFalke force-pushed
on May 23, 2020
DrahtBot removed the label
Needs rebase
on May 23, 2020
DrahtBot added the label
Needs rebase
on Jul 4, 2020
MarcoFalke force-pushed
on Jul 4, 2020
DrahtBot removed the label
Needs rebase
on Jul 4, 2020
ShengguangXiao referenced this in commit
efa1e6e6cc
on Aug 28, 2020
DrahtBot added the label
Needs rebase
on Oct 16, 2020
MarcoFalke force-pushed
on Oct 27, 2020
DrahtBot removed the label
Needs rebase
on Oct 27, 2020
DrahtBot added the label
Needs rebase
on Feb 1, 2021
MarcoFalke force-pushed
on Jun 18, 2021
DrahtBot removed the label
Needs rebase
on Jun 18, 2021
MarcoFalke force-pushed
on Jun 18, 2021
test: Create all blocks with version 4 or higherfac90c55be
MarcoFalke force-pushed
on Jun 18, 2021
MarcoFalke force-pushed
on Jun 18, 2021
test: Set BIP34Height = 2 for regtest222290f543
MarcoFalke force-pushed
on Jun 18, 2021
theStack
commented at 11:35 pm on July 25, 2021:
member
Concept ACK
in
test/functional/test_framework/messages.py:618
in
222290f543
fac90c55be478f0323eafa1d560e it seems that using from test_framework.blocktools import VERSIONBITS_LAST_OLD_BLOCK_VERSION in messages.py would be a circular import, but perhaps
0 self.nVersion =4# VERSIONBITS_LAST_OLD_BLOCK_VERSION in test_framework.blocktools
in
src/test/validation_block_tests.cpp:101
in
222290f543
98 }
99100+ // submit block header, so that miner can get the block height from the
101+ // global state and the node has the topology of the chain
102+ BlockValidationState ignored;
103+ BOOST_CHECK(Assert(m_node.chainman)->ProcessNewBlockHeaders({pblock->GetBlockHeader()}, ignored, Params()));
I don’t think named args are useful for args whose meaning is already clear. Otherwise we could add the comment to every arg. So leaving as-is.
jonatack
commented at 1:56 pm on July 28, 2021:
member
ACK222290f54388270937cb6c174195717e2214ec0d tested and reviewed rebased to current master 5e213822f86d
Feel free to ignore the comments.
theStack
commented at 1:16 am on August 1, 2021:
member
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 from where it activated. Just bump it up to at least 4.
This confused me a little. The block version required after BIP34 activation is fixed and hard-coded to >= 2, I don’t think it depends on anything related to what version was used prior the activation (maybe I misunderstood your explanation though?):
All tests still pass with nVersion=2 instead of 4 in the first commit. That said, bumping already to 4 for future similar PRs to activate soft-forks in regtest earlier can’t hurt. Will code-review tomorrow, just wanted to get sure that I understand the motivation for choosing 4 in the first commit.
MarcoFalke
commented at 3:05 pm on August 1, 2021:
member
This confused me a little.
Edited OP comment
theStack approved
theStack
commented at 5:21 pm on August 1, 2021:
member
ajtowns
commented at 2:13 pm on August 2, 2021:
member
ACK222290f54388270937cb6c174195717e2214ec0d
Still seems sad this isn’t documented accurately outside the source, but bip34’s final and a new bip just for the first ~16 blocks on signet or regtest seems weak too.
MarcoFalke merged this
on Aug 3, 2021
MarcoFalke closed this
on Aug 3, 2021
MarcoFalke deleted the branch
on Aug 3, 2021
sidhujag referenced this in commit
c0cf3a4c0b
on Aug 4, 2021
MarcoFalke referenced this in commit
3a36ec83d0
on Nov 22, 2021
Munkybooty referenced this in commit
2c0929fa2f
on Nov 25, 2021
Munkybooty referenced this in commit
0b1215a5fc
on Nov 25, 2021
Munkybooty referenced this in commit
d0f1663305
on Nov 30, 2021
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2024-10-04 13:12 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me