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

Disallow extended encoding for non-witness transactions #14039

Merged
merged 1 commit into from Apr 25, 2019

Conversation

Projects
None yet
10 participants
@sipa
Copy link
Member

commented Aug 24, 2018

BIP144 specifies that transactions without witness should use the legacy encoding, which is currently not enforced.

This rule was present in the original SegWit implementation (#8149), but was subsequently dropped (#8589).

As all hashes, txids, and weights are always computed over a reserialized version of a transaction, it is mostly harmless to permit extended encoding for non-segwit transactions, but I'd rather strictly follow the BIP.

@sipa sipa force-pushed the sipa:201808_no_superfluous_witness branch to bb530ef Aug 24, 2018

@practicalswift

This comment has been minimized.

Copy link
Member

commented Aug 24, 2018

Concept ACK

Stricter is better (in this case)!

@instagibbs

This comment has been minimized.

Copy link
Member

commented Aug 24, 2018

concept ACK, I discovered this a few months ago, reported it, then forgot to follow up.

@instagibbs

This comment has been minimized.

Copy link
Member

commented Aug 24, 2018

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Aug 24, 2018

To fix the test failure you could try rebasing on fae0400

I have no opinion on this change itself. Is there any evidence of other software using this on the p2p or rpc interface? Also note that Bitcoin Core will already normalize the transaction encoding when relaying txs.

@laanwj

This comment has been minimized.

Copy link
Member

commented Aug 29, 2018

concept ACK

@laanwj laanwj added the RPC/REST/ZMQ label Aug 31, 2018

@Sjors

This comment has been minimized.

Copy link
Member

commented Sep 7, 2018

Maybe add a test?

@gmaxwell

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

utACK

@instagibbs

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

utACK bb530ef

@instagibbs

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

upgrading to tACK, please @sipa take this test:

instagibbs@7c1ad7c

Fails for me on master, passes with your commit.

@stevenroose

This comment has been minimized.

Copy link
Contributor

commented Mar 22, 2019

utACK bb530ef
We backported this to Elements already as well: ElementsProject/elements#525

instagibbs added a commit to ElementsProject/elements that referenced this pull request Mar 25, 2019

Merge #402: Disallow extended encoding for non-witness transactions
8719c94 Disallow extended encoding for non-witness transactions (Pieter Wuille)

Pull request description:

  Backport from upstream bitcoin/bitcoin#14039

Tree-SHA512: a91810e0afe4d631e2e08cd1bc919e83e3dbdac050f1a66ddc0c6210b222e5813f5b36b27111976e83a4c7683507e4ed06ef475467e1c0e572ccb3d0abc631e4
@sdaftuar

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

utACK, I think this is ready for merge?

@instagibbs

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

I can PR my test right after merge

@MarcoFalke MarcoFalke merged commit bb530ef into bitcoin:master Apr 25, 2019

2 checks passed

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

MarcoFalke added a commit that referenced this pull request Apr 25, 2019

Merge #14039: Disallow extended encoding for non-witness transactions
bb530ef Disallow extended encoding for non-witness transactions (Pieter Wuille)

Pull request description:

  BIP144 specifies that transactions without witness should use the legacy encoding, which is currently not enforced.

  This rule was present in the original SegWit implementation (#8149), but was subsequently dropped (#8589).

  As all hashes, txids, and weights are always computed over a reserialized version of a transaction, it is mostly harmless to permit extended encoding for non-segwit transactions, but I'd rather strictly follow the BIP.

ACKs for commit bb530e:
  instagibbs:
    utACK bb530ef
  stevenroose:
    utACK bb530ef

Tree-SHA512: 1aeccd6a555f43784fefb076ce2e8ad2f5ba7be49840544a50050d0390f82373f87201bf56cf8bb30841b4f9cd893b382261a080da875d4e11ab7051f8640dbe

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Apr 26, 2019

Merge bitcoin#15893: Add test for superfluous witness record in deser…
…ialization

cc556e4 Add test for superfluous witness record in deserialization (Gregory Sanders)
25b0786 Fix missing input template by making minimal tx (Gregory Sanders)

Pull request description:

  Adds coverage for changed behavior in bitcoin#14039

ACKs for commit cc556e:
  MarcoFalke:
    utACK cc556e4

Tree-SHA512: 3404c8f75e87503983fac5ae27d877309eb3b902f2ec993762911c71610ca449bef0ed98bd17e029414828025b2713e1bd012e63b2a06497e34f1056acaa6321

@MarcoFalke MarcoFalke added this to the 0.18.1 milestone Apr 26, 2019

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 27, 2019

Merge bitcoin#14039: Disallow extended encoding for non-witness trans…
…actions

bb530ef Disallow extended encoding for non-witness transactions (Pieter Wuille)

Pull request description:

  BIP144 specifies that transactions without witness should use the legacy encoding, which is currently not enforced.

  This rule was present in the original SegWit implementation (bitcoin#8149), but was subsequently dropped (bitcoin#8589).

  As all hashes, txids, and weights are always computed over a reserialized version of a transaction, it is mostly harmless to permit extended encoding for non-segwit transactions, but I'd rather strictly follow the BIP.

ACKs for commit bb530e:
  instagibbs:
    utACK bitcoin@bb530ef
  stevenroose:
    utACK bb530ef

Tree-SHA512: 1aeccd6a555f43784fefb076ce2e8ad2f5ba7be49840544a50050d0390f82373f87201bf56cf8bb30841b4f9cd893b382261a080da875d4e11ab7051f8640dbe

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 27, 2019

Merge bitcoin#15893: Add test for superfluous witness record in deser…
…ialization

cc556e4 Add test for superfluous witness record in deserialization (Gregory Sanders)
25b0786 Fix missing input template by making minimal tx (Gregory Sanders)

Pull request description:

  Adds coverage for changed behavior in bitcoin#14039

ACKs for commit cc556e:
  MarcoFalke:
    utACK cc556e4

Tree-SHA512: 3404c8f75e87503983fac5ae27d877309eb3b902f2ec993762911c71610ca449bef0ed98bd17e029414828025b2713e1bd012e63b2a06497e34f1056acaa6321
@moneyball

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2019

Out of curiosity do we have any sense for whether such misformatted transactions occur and if so with what frequency?

@sipa

This comment has been minimized.

Copy link
Member Author

commented Apr 29, 2019

@moneyball This is just about the encoding; it's not a properry of the transaction at all. Before this PR, If someone were to use the invalid encoding on a particular P2P link, it would still be propagated in the correct form on further links.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

Though, it wouldn't be propagated on p2p after this pull. And also rejected by any RPC call.

laanwj added a commit that referenced this pull request May 20, 2019

Merge #16021: p2p: Avoid logging transaction decode errors to stderr
fa2b52a Disallow extended encoding for non-witness transactions (take 3) (MarcoFalke)

Pull request description:

  (previous title "p2p: Disallow extended encoding for non-witness transactions (take 3)")

  Remote peers can send us illegally encoded txs and thus have us write to stderr. Fix that by not writing to stderr.

  This is a follow up to the previous (incomplete) attempts at this:

  *  Disallow extended encoding for non-witness transactions #14039
  *  Add test for superfluous witness record in deserialization #15893

ACKs for commit fa2b52:
  laanwj:
    utACK fa2b52a
  ryanofsky:
    utACK fa2b52a. Would change title to something like "Avoid logging transaction decode errors to stderr" instead of "Disallow extended encoding for non-witness transactions." The current title is confusing because this PR isn't really allowing or disallowing anything, just logging the condition differently. "Disallow" also seems to contradict the "Allow exceptions from..." comments in the actual code.

Tree-SHA512: c66990e69b432d00dc1c5510bf976a1188664d0890a32d1e5c6459094e7e27da82a5d227627afcbc203676f5540eec74b7d9b1d71d2c62d3b2069e1781824b4d

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request May 20, 2019

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 20, 2019

Merge bitcoin#16021: p2p: Avoid logging transaction decode errors to …
…stderr

fa2b52a Disallow extended encoding for non-witness transactions (take 3) (MarcoFalke)

Pull request description:

  (previous title "p2p: Disallow extended encoding for non-witness transactions (take 3)")

  Remote peers can send us illegally encoded txs and thus have us write to stderr. Fix that by not writing to stderr.

  This is a follow up to the previous (incomplete) attempts at this:

  *  Disallow extended encoding for non-witness transactions bitcoin#14039
  *  Add test for superfluous witness record in deserialization bitcoin#15893

ACKs for commit fa2b52:
  laanwj:
    utACK fa2b52a
  ryanofsky:
    utACK fa2b52a. Would change title to something like "Avoid logging transaction decode errors to stderr" instead of "Disallow extended encoding for non-witness transactions." The current title is confusing because this PR isn't really allowing or disallowing anything, just logging the condition differently. "Disallow" also seems to contradict the "Allow exceptions from..." comments in the actual code.

Tree-SHA512: c66990e69b432d00dc1c5510bf976a1188664d0890a32d1e5c6459094e7e27da82a5d227627afcbc203676f5540eec74b7d9b1d71d2c62d3b2069e1781824b4d
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.