Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tests: Speedup feature_pruning test and refactor big transaction logic #14691

Closed
wants to merge 2 commits into from

Conversation

conscott
Copy link
Contributor

@conscott conscott commented Nov 8, 2018

Description

Extended tests like feature_pruning.py create large blocks by making many large transactions with a lot of OP_RETURNS in a single tx. Under this model roughly 14 of these transactions make a full-ish block.

These transactions are already non-standard, so I have just modified the logic to instead just create 1 large output, making a single large transaction, mining a block of roughly the same size (947k). The reduction in tx creation / number of outputs seems to significantly improve performance.

Another optimization is to splice in the OP_RETURN output after transaction signing, since it reduces the copying of the large OP_RETURN portion during signing. All signatures are SIGHHASH_NONE, so splicing after is okay.

feature_pruning.py has about a 1.5x speedup on my machine (~12 minutes down to ~8 minutes), and mempool_limit, and feature_maxuploadtarget seem to benefit as well.

Refactoring

  • The original OP_RETURN generating function is now just a string. If there is a better place to put this, I will move it. At the very least I just wanted to document the format because the previous function had little explanation for all the hex values.
  • create_lots_of_big_transactions no longer requires providing the OP_RETURN splice, and uses the default from before, with an optional op_return_txout= keyword args for using the giant OP_RETURN mentioned above.

@conscott conscott changed the title Speedup feature_pruning test and refactor big transaction logic [Tests] - Speedup feature_pruning test and refactor big transaction logic Nov 8, 2018
@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 8, 2018

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #15282 (test: Replace hard-coded hex tx with class in test framework by stevenroose)
  • #13541 (wallet/rpc: sendrawtransaction maxfeerate by kallewoof)

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.

Coverage

Coverage Change (pull 14691) Reference (master)
Lines +0.0395 % 87.0730 %
Functions +0.0153 % 84.3717 %
Branches +0.0151 % 51.5638 %

Updated at: 2018-11-08T22:02:58.107718.

@fanquake fanquake changed the title [Tests] - Speedup feature_pruning test and refactor big transaction logic tests: Speedup feature_pruning test and refactor big transaction logic Nov 9, 2018
@fanquake fanquake requested a review from maflcko November 9, 2018 09:51
@fanquake
Copy link
Member

Concept ACK

Haven't looked at the changes, but I'm seeing a ~1.4x speedup on my machine between feature_pruning.py in master (e70a19e) and c8ef522:

feature_pruning.py | ✓ Passed  | 819 s
ALL                | ✓ Passed  | 819 s (accumulated) 
Runtime: 819 s
feature_pruning.py | ✓ Passed  | 577 s
ALL                | ✓ Passed  | 577 s (accumulated) 
Runtime: 577 s

@DrahtBot
Copy link
Contributor

Needs rebase

@fanquake
Copy link
Member

@conscott I realise this hasn't seen much traction / concept ACKs, however are you interested in rebasing?

@conscott
Copy link
Contributor Author

I almost forgot this was still alive :)

I will rebase when I get the chance, hopefully this week!

@sipa
Copy link
Member

sipa commented Jun 19, 2019

Concept ACK

Copy link
Member

@promag promag left a comment

Choose a reason for hiding this comment

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

Concept ACK.

# Create a spend of each passed-in utxo, splicing in "txout" to each raw
# transaction to make it large. By default, it will splice in
# 128 OP_RETURN outputs of 512 data bytes (see MULTI_OP_RETURN)
def create_lots_of_big_transactions(node, utxos, num, fee, op_return_txout=MULTI_OP_RETURN):
Copy link
Member

Choose a reason for hiding this comment

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

Could simplify by ditching num parameter? This way below could be

return [create_big_transaction(node, utxo, fee, op_return_txout) for utxo in utxos]

@jamesob
Copy link
Member

jamesob commented Jun 26, 2019

Concept ACK. Will review on rebase.

@jnewbery
Copy link
Contributor

jnewbery commented Jul 8, 2019

Is this still required after #15686?

@conscott
Copy link
Contributor Author

conscott commented Jul 9, 2019

@jnewbery - Looks like you got it covered more elegantly with OP_NOPs :) It was a fun learning experience anyhow.

I will go ahead and close this.

@conscott conscott closed this Jul 9, 2019
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
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.

None yet

9 participants