Details

Reviewers

Group Reviewers

Restricted Project

Commits

rABCfe29b3461f5a: QA: blocktools: Accept block template to create_block

Summary

The goal here is to decouple unrelated tests from the details of block versions.
Currently, these tests are forcing specific versions of blocks for no real reason.

This is a backport of core#19401 [1/2]
https://github.com/bitcoin/bitcoin/pull/19401/commits/1df2cd1c8f468bd7a5b1335a46ccea28fbddaacb

Note that the txlist parameter from the original commit is not going to be useful in Bitcoin ABC (as of today in Bitcoin Core it is only used on feature_taproot.py)

Test Plan

ninja check-functional-extended

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

PiRK created this revision.Dec 17 2021, 10:15

Herald added a reviewer: Restricted Project. · View Herald TranscriptDec 17 2021, 10:15

PiRK requested review of this revision.Dec 17 2021, 10:15

teamcity edited the summary of this revision. (Show Details)Dec 17 2021, 10:15

PiRK edited the summary of this revision. (Show Details)Dec 17 2021, 10:16

PiRK added a child revision: D10689: QA: Use getblocktemplate to get block versions correct.Dec 17 2021, 10:29

Harbormaster completed remote builds in B17661: Diff 31443.Dec 17 2021, 10:30

Really this code is terrible. It's very hard to determine what parameter is expected to do what, and which combination is valid. The template is expected to have some parameters, but all are not required and some will cause a crash if missing. The interface for txlist is absurd. There is not even a comment to give a hint.
Can you look if this has been improved in later PRs ?

test/functional/test_framework/blocktools.py
49 ↗	(On Diff #31443)	Just a style preference for `tmpl.get('version', 1)`, not blocking on that
52 ↗	(On Diff #31443)	the first condition is useless
62 ↗	(On Diff #31443)	so tx is expected to be either a CTransaction (in fact any object with this name as an attribute) or a serialized CTransaction ?
64 ↗	(On Diff #31443)	Use FromHex() instead

This revision now requires changes to proceed.Dec 17 2021, 14:14

I agree that the code has a lot of issues. Unfortunately it has not been improved since. I will clean it up as much as I can without breaking anything.

test/functional/test_framework/blocktools.py
62 ↗	(On Diff #31443)	Some people seem to think duck typing and accepting anything as input is the recommended way of coding in python. Fortunately, there are on the decline :) I will look at the callsites and make sure this is called consistently with either a raw hex string or a CTransaction, and adjust the code accordingly.

Use dict.get(<key>, <default>) instead of dict.get(<key>) or <default> when applicable. Remove txlist, as it is confusing code and currently not needed in our tests (it is only used for a taproot test in bitcoin core).

PiRK added inline comments.Dec 17 2021, 17:30

test/functional/test_framework/blocktools.py
51 ↗	(On Diff #31447)	This could maybe be simplified further into `if "bits" in tmpl: ...`, unless there is a possibility that `getblocktemplate` returns a dict that has the "bits" key but with `None` as a value? I'm not sure about this one.

After checking the getblocktemplate RPC, I see that it always returns something for "bits", so this cannot be None. We can replace if tmpl.get("bits") is not None:... with if "bits" in tmpl:... (in case tmpl is not specified, it is set to an empty dict).

Harbormaster completed remote builds in B17665: Diff 31447.Dec 17 2021, 17:42

Harbormaster completed remote builds in B17666: Diff 31448.Dec 17 2021, 17:53

Is the txlist param never used ? It's hard to believe that he went through all that API madness with zero use case

Out of scope for this diff, but it would be much better to have another prototype for create_block that takes a template and calls the other prototype with the template extracted values for hashprev, coinbase, nTime, etc.