test: Correct ineffectual WithOrVersion from transactions_tests #14855

pull Empact wants to merge 1 commits into bitcoin:master from Empact:with-or-version-test changing 2 files +4 −10

Empact commented at 10:19 AM on December 2, 2018: member

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 ab48c5e72156b34300db4a6521cb3c9969be3937 Last updated 81e3228fcb33e8ed32d8b9fbe917444ba080073a
Empact commented at 10:20 AM on December 2, 2018: member

/cc #8524
fanquake added the label Tests on Dec 2, 2018
laanwj commented at 1:13 PM on January 2, 2019: member

Re-ping @NicolasDorier @sipa
sipa commented at 1:29 PM on January 2, 2019: member

Looks like you can drop vstream entirely here, and directly (de)serialize to/from ssout?
Empact force-pushed on Jan 2, 2019
Empact commented at 10:21 PM on January 2, 2019: member

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. https://github.com/bitcoin/bitcoin/commit/ab48c5e72156b34300db4a6521cb3c9969be3937#diff-4f6d60b6976522cec2974a0aea9a5ab3R467

Thoughts? /cc @NicolasDorier
Empact force-pushed on Jan 2, 2019
Empact commented at 10:25 PM on January 2, 2019: member

Also removed WithOrVersion as this was the only use and shows the function's pitfalls. The other uses of OverrideStream are direct.
NicolasDorier commented at 2:14 PM on January 3, 2019: contributor

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 ab48c5e72156b34300db4a6521cb3c9969be3937
Last updated 81e3228fcb33e8ed32d8b9fbe917444ba080073a

75778a0724

Empact force-pushed on Jan 3, 2019
Empact commented at 5:14 PM on January 3, 2019: member

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 commented at 5:38 AM on January 4, 2019: contributor

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 commented at 5:39 AM on January 4, 2019: contributor

ACK
MarcoFalke commented at 11:15 AM on January 4, 2019: member

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 75778a0724b73a0cecd99bd2e1c49fbfdb90e194
sipa commented at 11:16 AM on January 4, 2019: member

utACK
MarcoFalke referenced this in commit 5b6b371c77 on Jan 4, 2019
MarcoFalke merged this on Jan 4, 2019
MarcoFalke closed this on Jan 4, 2019
NicolasDorier commented at 1:57 PM on January 4, 2019: contributor

@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 commented at 4:02 PM on January 4, 2019: member

Oh, I see. Thanks for clarifying.
Empact deleted the branch on Jan 7, 2019
DrahtBot locked this on Dec 16, 2021

Contributors

Labels