Add test for superfluous witness record in deserialization #15893
Conversation
This file contains hidden or 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
instagibbs
force-pushed
the
test_super_witness
branch
from
91dcbe1 to
a49893c
Compare
instagibbs
changed the title
Member
|
Tests are failing with |
Member
Author
|
wait, what? I didn't touch this test: |
Member
|
Could you include the following diff: diff --git a/test/functional/data/invalid_txs.py b/test/functional/data/invalid_txs.py
index 02deae92f3..80d8d66f24 100644
--- a/test/functional/data/invalid_txs.py
+++ b/test/functional/data/invalid_txs.py
@@ -68,7 +68,7 @@ class OutputMissing(BadTxTemplate):
class InputMissing(BadTxTemplate):
- reject_reason = "bad-txns-vin-empty"
+ reject_reason = "Superfluous witness record"
expect_disconnect = False
def get_tx(self): |
instagibbs
force-pushed
the
test_super_witness
branch
from
a49893c to
59e81b1
Compare
Member
Author
|
@MarcoFalke I tried a slightly different track by using the fact that 0-vin, 0-vout have input sizes checked first, and kept that test case, then also kept my new test for the actual test we're checking for. It seemed weird to me to simply hope that that type of transaction would trigger this case. |
maflcko
reviewed
Apr 25, 2019
test/functional/feature_block.py
Outdated
Member
There was a problem hiding this comment.
Please move this comment to data/invalid_txs.py, as it is not clear why the tx there is completely "empty"
Member
Author
There was a problem hiding this comment.
I wrote something else in its place. I found the previous text a little baffling to be honest. Willing to take heavy edits on it :)
instagibbs
force-pushed
the
test_super_witness
branch
from
59e81b1 to
cc556e4
Compare
Member
|
utACK cc556e4 |
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
…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
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#15893 Rebased-From: 25b0786
maflcko
pushed a commit
to maflcko/bitcoin-core
that referenced
this pull request
May 20, 2019
Github-Pull: bitcoin#15893 Rebased-From: cc556e4
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#15893 Rebased-From: 25b0786
HashUnlimited
pushed a commit
to HashUnlimited/chaincoin
that referenced
this pull request
Aug 23, 2019
Github-Pull: bitcoin#15893 Rebased-From: cc556e4
Bushstar
pushed a commit
to Bushstar/omnicore
that referenced
this pull request
Aug 24, 2019
Github-Pull: bitcoin#15893 Rebased-From: 25b0786
Bushstar
pushed a commit
to Bushstar/omnicore
that referenced
this pull request
Aug 24, 2019
Github-Pull: bitcoin#15893 Rebased-From: cc556e4
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.
3 participants
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.
Adds coverage for changed behavior in #14039