test: remove confusing `MAX_BLOCK_BASE_SIZE` #22378

pull theStack wants to merge 3 commits into bitcoin:master from theStack:202106-test-remove-max_block_base_size changing 6 files +57 −62

theStack commented at 12:21 AM on July 1, 2021: member

This is a very late follow-up PR to #10618, which removed the constant MAX_BLOCK_BASE_SIZE from the core implementation about four years ago (see also #10608 in why it was considered confusing and superfluous). Since there is also no point in still keeping it in the functional test framework, the PR switches to weight-based accounting on the relevant test code parts and use MAX_BLOCK_WEIGHT instead for the block limit checks. To prepare that, the first two commits introduce get_weight() helpers for the classes CTransaction and CBlock, respectively.
fanquake added the label Tests on Jul 1, 2021
test: introduce `get_weight()` helper for CTransaction a084ebe133
test: introduce `get_weight()` helper for CBlock 4af97c74ed
theStack force-pushed on Jul 1, 2021

in test/functional/feature_block.py:505 in e26ad3b39c outdated

 501 | @@ -502,7 +502,7 @@ def run_test(self):
 502 |          # Make sure we didn't accidentally make too big a block. Note that the
 503 |          # size of the block has non-determinism due to the ECDSA signature in
 504 |          # the first transaction.
 505 | -        while (len(b39.serialize()) >= MAX_BLOCK_BASE_SIZE):
 506 | +        while (b39.get_weight() >= MAX_BLOCK_WEIGHT):

MarcoFalke commented at 12:55 PM on July 2, 2021:

e26ad3b39cf7afcf5588bb55ceaf3c3902f7b5d3: Can remove ()

theStack commented at 3:39 PM on July 3, 2021:

Thanks, done. Also identified another pair of unneeded parantheses in an if condition to remove.

in test/functional/feature_block.py:336 in e26ad3b39c outdated
```
 339 | +        script_length = (MAX_BLOCK_WEIGHT - b24.get_weight() - 276) // 4
 340 |          script_output = CScript([b'\x00' * (script_length + 1)])
 341 |          tx.vout = [CTxOut(0, script_output)]
 342 |          b24 = self.update_block(24, [tx])
 343 | -        assert_equal(len(b24.serialize()), MAX_BLOCK_BASE_SIZE + 1)
 344 | +        assert_equal(b24.get_weight(), MAX_BLOCK_WEIGHT + 1 * 4)
```
brunoerg commented at 2:52 PM on July 2, 2021:

Does this 1 * 4 make sense? If you wanna add 1 to MAX_BLOCK_WEIGHT I think you have to use (MAX_BLOCK_WEIGHT + 1) * 4.

MarcoFalke commented at 3:25 PM on July 2, 2021:

bytes in the scriptSig count for 4 weight units.

Not sure, but it might be possible to pad the scriptWitness to get exactly +1 weight unit

theStack commented at 3:54 PM on July 3, 2021:

@brunoerg: MAX_BLOCK_WEIGHT + 4 is the smallest over-weight block you can reach if you only change non-witness data (as in this test scenario, the scriptPubKey), as every byte added increases the block weight by 4 WU (What you probably meant was (MAX_BLOCK_BASE_SIZE + 1) * 4 but that constant doesn't exist anymore in this PR :)) @MarcoFalke: yes I guess it's possible to reach exactly MAX_BLOCK_WEIGHT+1 by tweaking the witness. Didn't want to change too much of the surrounding logic though, can be done in a follow-up I guess. E.g. adding test coverage for the rejection state bad-blk-weight is on my TODO list, it could be appropriate to include it in such a PR.

test: remove confusing `MAX_BLOCK_BASE_SIZE`

The constant `MAX_BLOCK_BASE_SIZE` has been removed from the
core implementation years ago due to being confusing and
superfluous, as it is implied by the block weight limit (see
PRs #10618 and #10608). Since there is also no point in
still keeping it in the functional test framework, we switch
to weight-based accounting on the relevant test code parts
and use `MAX_BLOCK_WEIGHT` instead for the block limit
checks.

607076d01b

theStack force-pushed on Jul 3, 2021
sriramdvt referenced this in commit 70a15a8b46 on Jul 21, 2021
sriramdvt referenced this in commit 80008d0bee on Jul 21, 2021
sriramdvt referenced this in commit 48e55a83c3 on Jul 21, 2021
DrahtBot commented at 12:44 PM on July 21, 2021: member


The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #22509 by sriramdvt
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
ShubhamPalriwala commented at 1:17 PM on July 24, 2021: contributor

Concept Ack!

Since the core codebase does not use MAX_BLOCK_BASE_SIZE anymore, it makes sense to update our test implementations too accordingly. Replacing them with MAX_BLOCK_WEIGHT just like we did in the codebase is the appropriate approach to make our tests more relevant to the core

Reviewed the codebase too! Merging this PR will remove the existence of MAX_BLOCK_BASE_SIZE entirely from the codebase.