Disallow extended encoding for non-witness transactions #14039
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sipa
force-pushed
the
201808_no_superfluous_witness
branch
from
cfb10d3
to
bb530ef
Compare
|
Concept ACK Stricter is better (in this case)! |
|
concept ACK, I discovered this a few months ago, reported it, then forgot to follow up. |
|
failing decoderaw test: https://travis-ci.org/bitcoin/bitcoin/jobs/419906176#L2871 |
|
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. |
|
concept ACK |
|
Maybe add a test? |
|
utACK |
|
utACK bb530ef |
|
upgrading to tACK, please @sipa take this test: Fails for me on master, passes with your commit. |
|
utACK bb530ef |
instagibbs
added a commit
to ElementsProject/elements
that referenced
this pull request
Mar 25, 2019
8719c94 Disallow extended encoding for non-witness transactions (Pieter Wuille) Pull request description: Backport from upstream bitcoin/bitcoin#14039 Tree-SHA512: a91810e0afe4d631e2e08cd1bc919e83e3dbdac050f1a66ddc0c6210b222e5813f5b36b27111976e83a4c7683507e4ed06ef475467e1c0e572ccb3d0abc631e4
|
utACK, I think this is ready for merge? |
|
I can PR my test right after merge |
maflcko
pushed a commit
that referenced
this pull request
Apr 25, 2019
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
maflcko
pushed a commit
to maflcko/bitcoin-core
that referenced
this pull request
Apr 26, 2019
…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
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Apr 27, 2019
…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
…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
|
Out of curiosity do we have any sense for whether such misformatted transactions occur and if so with what frequency? |
|
@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. |
|
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
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
maflcko
pushed a commit
to maflcko/bitcoin-core
that referenced
this pull request
May 20, 2019
Github-Pull: bitcoin#14039 Rebased-From: bb530ef
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
May 20, 2019
…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
HashUnlimited
pushed a commit
to HashUnlimited/chaincoin
that referenced
this pull request
Aug 23, 2019
Github-Pull: bitcoin#14039 Rebased-From: bb530ef
Bushstar
pushed a commit
to Bushstar/omnicore
that referenced
this pull request
Aug 24, 2019
Github-Pull: bitcoin#14039 Rebased-From: bb530ef
Munkybooty
pushed a commit
to Munkybooty/dash
that referenced
this pull request
Oct 17, 2021
…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
Munkybooty
pushed a commit
to Munkybooty/dash
that referenced
this pull request
Oct 22, 2021
…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
Munkybooty
pushed a commit
to Munkybooty/dash
that referenced
this pull request
Oct 22, 2021
…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
Munkybooty
pushed a commit
to Munkybooty/dash
that referenced
this pull request
Oct 23, 2021
…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
Munkybooty
pushed a commit
to Munkybooty/dash
that referenced
this pull request
Oct 26, 2021
…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
Munkybooty
pushed a commit
to Munkybooty/dash
that referenced
this pull request
Oct 28, 2021
…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
Munkybooty
pushed a commit
to Munkybooty/dash
that referenced
this pull request
Oct 28, 2021
…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
Munkybooty
pushed a commit
to Munkybooty/dash
that referenced
this pull request
Nov 12, 2021
…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
Munkybooty
pushed a commit
to Munkybooty/dash
that referenced
this pull request
Nov 13, 2021
…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
Munkybooty
pushed a commit
to Munkybooty/dash
that referenced
this pull request
Nov 13, 2021
…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
Munkybooty
pushed a commit
to Munkybooty/dash
that referenced
this pull request
Nov 14, 2021
…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
Munkybooty
pushed a commit
to Munkybooty/dash
that referenced
this pull request
Nov 16, 2021
…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
Munkybooty
pushed a commit
to Munkybooty/dash
that referenced
this pull request
Nov 18, 2021
…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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.