p2p: Avoid logging transaction decode errors to stderr #16021
Merged
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
fa14c82 to
fa2b52a
Compare
|
Marked for backport |
ryanofsky
approved these changes
May 15, 2019
There was a problem hiding this comment.
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.
| def serialize_with_bogus_witness(tx): | ||
| flags = 1 | ||
| flags = 3 |
There was a problem hiding this comment.
Why's it important that the flag is 3 and not 1? Maybe add a code comment.
There was a problem hiding this comment.
The test fails if it is set to 1
There was a problem hiding this comment.
yes, I also hate these magic numbers.
There was a problem hiding this comment.
@thijstriemstra Mind adding a comment to explain the magic number? It should be clear what it does if you try to set it to 0, 1, or 2 and run the test (or review the test case)
There was a problem hiding this comment.
I can try.. also noticed a lot of if statement with parentheses that are not needed, e.g. if (len(tx.wit.vtxinwit) != len(tx.vin)):. Why not if len(tx.wit.vtxinwit) != len(tx.vin):? Or is this preferred coding style for Python in this project?
There was a problem hiding this comment.
We don't have a python coding style (except for the ./test/lint/lint-python.sh), but without parens is indeed preferred.
There was a problem hiding this comment.
searched a while for any python linter that can detect this but no luck yet. Possibly needs to have a custom flake8 plugin that can find this.
|
Changed title |
|
utACK fa2b52a |
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#16021 Rebased-From: fa2b52a
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#16021 Rebased-From: fa2b52a
Bushstar
pushed a commit
to Bushstar/omnicore
that referenced
this pull request
Aug 24, 2019
Github-Pull: bitcoin#16021 Rebased-From: fa2b52a
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.
(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: