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: Correct ineffectual WithOrVersion from transactions_tests #14855

Merged
merged 1 commit into from Jan 4, 2019

Conversation

Projects
None yet
6 participants
@Empact
Copy link
Member

commented Dec 2, 2018

WithOrVersion uses | to combine the versions, and | with 0 is a no-op.

NicolasDorier / sipa do you recall why the version is being overridden here?

Introduced in ab48c5e
Last updated 81e3228

@Empact

This comment has been minimized.

Copy link
Member Author

commented Dec 2, 2018

/cc #8524

@fanquake fanquake added the Tests label Dec 2, 2018

@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 2, 2019

@sipa

This comment has been minimized.

Copy link
Member

commented Jan 2, 2019

Looks like you can drop vstream entirely here, and directly (de)serialize to/from ssout?

@Empact Empact force-pushed the Empact:with-or-version-test branch Jan 2, 2019

@Empact

This comment has been minimized.

Copy link
Member Author

commented Jan 2, 2019

Updated to remove the overrides, and to separately test against PROTOCOL_VERSION and version 0. Because it seemed the original intended to test against 0 but in fact tested against PROTOCOL_VERSION. ab48c5e#diff-4f6d60b6976522cec2974a0aea9a5ab3R467

Thoughts? /cc @NicolasDorier

@Empact Empact force-pushed the Empact:with-or-version-test branch Jan 2, 2019

@Empact

This comment has been minimized.

Copy link
Member Author

commented Jan 2, 2019

Also removed WithOrVersion as this was the only use and shows the function's pitfalls. The other uses of OverrideStream are direct.

@NicolasDorier

This comment has been minimized.

Copy link
Member

commented Jan 3, 2019

I don't remember this function at all, ACK to remove it.

Only point to consider: test_big_witness_transaction was quite a slow test... which you are now executing twice. Fine for me though, just mentioning it.

test: Correct ineffectual WithOrVersion from transactions_tests
WithOrVersion uses | to combine the versions, and | with 0 is a no-op.

Instead I run it with PROTOCOL_VERSION and 0 separately, as the original
code only tested PROTOCOL_VERSION but apparently only intended to test
version 0.

Introduced in ab48c5e
Last updated 81e3228

@Empact Empact force-pushed the Empact:with-or-version-test branch to 75778a0 Jan 3, 2019

@Empact

This comment has been minimized.

Copy link
Member Author

commented Jan 3, 2019

Dropped the 0-version test as there is no version-specific behavior in transaction serialization, and if there were, does not seem this would be the place to test it.

Maybe you can weigh in on: is it necessary to do this serialization round-trip at all? Could instantiate the tx from the mtx directly.

@NicolasDorier

This comment has been minimized.

Copy link
Member

commented Jan 4, 2019

The version is used on https://github.com/bitcoin/bitcoin/blob/master/src/primitives/transaction.h#L200

I speculate that the intention of the WithOrVersion was to add segwit support to the serialization, because before PROTOCOL_VERSION was setting the flag fAllowWitness to false. Now, PROTOCOL_VERSION implies we support segwit serialization, so it became useless.

The roundtrip itself is just used to convert a CMutableTransaction into a CTransaction.

It is safe to drop the 0-version test.

@NicolasDorier

This comment has been minimized.

Copy link
Member

commented Jan 4, 2019

ACK

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Jan 4, 2019

PROTOCOL_VERSION was setting the flag fAllowWitness to false

Are you sure on that? To me it seems that fAllowWitness is always true unless SERIALIZE_TRANSACTION_NO_WITNESS is flagged in the version.

utACK 75778a0

@sipa

This comment has been minimized.

Copy link
Member

commented Jan 4, 2019

utACK

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Jan 4, 2019

Merge bitcoin#14855: test: Correct ineffectual WithOrVersion from tra…
…nsactions_tests

75778a0 test: Correct ineffectual WithOrVersion from transactions_tests (Ben Woosley)

Pull request description:

  `WithOrVersion` uses `|` to combine the versions, and `|` with 0 is a no-op.

  NicolasDorier / sipa do you recall why the version is being overridden here?

  Introduced in ab48c5e
  Last updated 81e3228

Tree-SHA512: 2aea925497bab2da973f17752410a6759d67181a57c3b12a685d184fbfcca2984c45b702ab0bd641d75e086696a0424f1bf77c5578ca765d6882dc03b42d5f9a

@MarcoFalke MarcoFalke merged commit 75778a0 into bitcoin:master Jan 4, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@NicolasDorier

This comment has been minimized.

Copy link
Member

commented Jan 4, 2019

@MarcoFalke yes, that's why I said was. Before we had PROTOCOL_VERSION + a flag to add witness.

Then it was refactored for having witness in PROTOCOL_VERSION + a flag to disable witness. Unsure if it was before the merge or after the merge that this was refactored, but I am now almost sure it was the initial reason behind this weird code.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Jan 4, 2019

Oh, I see. Thanks for clarifying.

@Empact Empact deleted the Empact:with-or-version-test branch Jan 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.