Skip to content

Conversation

theStack
Copy link
Contributor

@theStack theStack commented Nov 15, 2021

The helper create_block offers two parameters version and txlist which set the nVersion field / extend the vtx array of the block, respectively. By taking use of those, we can remove a lot of code, including the recalculation of the merkle root. Both passing txs in string and CTransaction format is supported, i.e. we also save potential calls to tx_from_hex.
The PR also contains another commit which replaces magic numbers for OP_TRUE/OP_1 (0x51) with the proper constant from the script module.

Instances setting the block version of 4 explicitely after calling create_block are removed, as this is the default since #16333 got merged (see #23521 (comment)).

@theStack theStack force-pushed the 202111-test-refactor-use_createblock_parameters branch from 4121983 to 06cb276 Compare November 15, 2021 18:12
@DrahtBot DrahtBot added the Tests label Nov 15, 2021
@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 16, 2021

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

Conflicts

No conflicts as of last run.

@theStack
Copy link
Contributor Author

Rebased on master.

@theStack theStack force-pushed the 202111-test-refactor-use_createblock_parameters branch from 66e97bd to 397f6cb Compare November 17, 2021 13:17
Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other, too?

Passing a list of transactions `txlist` to `create_block` appends
them to the block, hence we don't need to do that manually anymore.
The merkle root calculation can also be removed, since this is done
in the end of the helper.
@theStack
Copy link
Contributor Author

As suggested by MarcoFalke, I removed now all instances of explicitely setting block version to 4. FWIW as historical context, blocks are created with version 4 by default since #16333 (commit fac90c5 was created in 2019, but the PR was merged only a few months ago).

@theStack theStack force-pushed the 202111-test-refactor-use_createblock_parameters branch from 397f6cb to e57c0eb Compare November 17, 2021 15:01
Copy link
Contributor

@stratospher stratospher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tested ACK e57c0eb.

@maflcko maflcko merged commit 3a36ec8 into bitcoin:master Nov 22, 2021
@theStack theStack deleted the 202111-test-refactor-use_createblock_parameters branch November 22, 2021 10:19
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 22, 2021
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 23, 2021
@bitcoin bitcoin locked and limited conversation to collaborators Nov 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants