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

test: add unit test for non-standard txs w/ too large tx size #17570

Closed
wants to merge 2 commits into from

Conversation

KaanKC
Copy link

@KaanKC KaanKC commented Nov 23, 2019

Adds one of the missing tests of issue #17394: the function IsStandardTx() returns rejection reason tx-size if the transaction's size is larger than MAX_STANDARD_TX_WEIGHT, which is as of now defined as 400000.

Simply increasing the size of vin seemed the easiest way to test this case.

Heavily inspired by #17480

The function IsStandardTx() returns rejection reason tx-size if the transaction's size is larger than MAX_STANDARD_TX_WEIGHT, which is as of now defined as 400000.
@DrahtBot DrahtBot added the Tests label Nov 23, 2019
@DrahtBot
Copy link
Contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #17502 (test: add unit test for non-standard bare multisig txs by theStack)

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.

Using numbers to close to the MAX_STANARD_TX_WEIGHT, might cause problems down the road. Since the actual tx weight depends on more than just the size of the input array, the test might fail if it is moved above or under other tests, which mutate the tx object. Was also the reason why the initial commit failed the test
@instagibbs
Copy link
Member

Would it be easier to just make a new transaction, make the vin size large enough to just break the size, rather than using an arbitrarily large number?

@KaanKC
Copy link
Author

KaanKC commented Dec 4, 2019

Sure. Completely new transaction object or reset the existing one to the default values?

@DrahtBot
Copy link
Contributor

Needs rebase

@theStack
Copy link
Contributor

Hi @KaanKC, thanks for your pull request!

Sure. Completely new transaction object or reset the existing one to the default values?

There is no need for a completely new object, you can just reuse the existing one (CMutableTransaction t) which also used for all other thests in the same function-- first reset it, then resize t.vin to a size where it passes and then fails. Your second commit is not needed then.

@maflcko
Copy link
Member

maflcko commented Jan 16, 2020

Is anyone still working on this?

@theStack
Copy link
Contributor

Is anyone still working on this?

Since this PR's author doesn't seem to be working on the unit test anymore, I created a new PR: #17947.

@fanquake
Copy link
Member

Going to close in favour of #17947.

@fanquake fanquake closed this Jan 17, 2020
@KaanKC KaanKC deleted the test_unit_IsStandardTx_tx-size branch January 17, 2020 12:41
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants