p2p: Avoid logging transaction decode errors to stderr #16021

1. 
   MarcoFalke commented at 10:20 PM on May 13, 2019: member

   (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
2. DrahtBot added the label P2P on May 13, 2019
3. DrahtBot added the label Tests on May 13, 2019
4. fanquake removed the label Tests on May 13, 2019
5. Disallow extended encoding for non-witness transactions (take 3) fa2b52af32
6. MarcoFalke force-pushed on May 14, 2019
7. MarcoFalke added the label Needs backport on May 14, 2019
8. MarcoFalke added this to the milestone 0.18.1 on May 14, 2019
9. 
   MarcoFalke commented at 1:40 PM on May 14, 2019: member

   Marked for backport

10. in test/functional/p2p_segwit.py:2043 in fa2b52af32

    2037 | @@ -2038,9 +2038,9 @@ def test_witness_sigops(self):
    2038 |          # TODO: test p2sh sigop counting
    2039 |  
    2040 |      def test_superfluous_witness(self):
    2041 | -        # Serialization of tx that puts witness flag to 1 always
    2042 | +        # Serialization of tx that puts witness flag to 3 always
    2043 |          def serialize_with_bogus_witness(tx):
    2044 | -            flags = 1
    2045 | +            flags = 3
    

    ---

    ---

    

    ryanofsky commented at 8:17 PM on May 15, 2019:

    Why's it important that the flag is 3 and not 1? Maybe add a code comment.

    ---

    

    MarcoFalke commented at 8:29 PM on May 15, 2019:

    The test fails if it is set to 1

    ---

    

    thijstriemstra commented at 11:59 PM on May 21, 2019:

    yes, I also hate these magic numbers.

    ---

    

    MarcoFalke commented at 11:46 AM on May 22, 2019:

    @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)

    ---

    

    thijstriemstra commented at 12:09 PM on May 22, 2019:

    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?

    ---

    

    MarcoFalke commented at 12:16 PM on May 22, 2019:

    We don't have a python coding style (except for the ./test/lint/lint-python.sh), but without parens is indeed preferred.

    ---

    

    thijstriemstra commented at 1:06 PM on May 22, 2019:

    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.

11. ryanofsky approved
12. 
    ryanofsky commented at 8:24 PM on May 15, 2019: member

    utACK fa2b52af32f6a4b9c22c270f36e92960c29ef364. 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.

13. MarcoFalke renamed this:
    p2p: Disallow extended encoding for non-witness transactions (take 3)
    p2p: Avoid logging transaction decode errors to stderr
    on May 15, 2019
14. 
    MarcoFalke commented at 8:30 PM on May 15, 2019: member

    Changed title

15. 
    laanwj commented at 3:27 PM on May 20, 2019: member

    utACK fa2b52af32f6a4b9c22c270f36e92960c29ef364

16. laanwj merged this on May 20, 2019
17. laanwj closed this on May 20, 2019

    ---

18. laanwj referenced this in commit bb291b50f2 on May 20, 2019
19. MarcoFalke deleted the branch on May 20, 2019
20. MarcoFalke referenced this in commit b6c1f9478f on May 20, 2019
21. MarcoFalke removed the label Needs backport on May 20, 2019
22. sidhujag referenced this in commit 9f749c185f on May 20, 2019
23. HashUnlimited referenced this in commit e6d96c9d61 on Aug 23, 2019
24. Bushstar referenced this in commit 6c4483c27f on Aug 24, 2019
25. Munkybooty referenced this in commit 4df2763b77 on Oct 17, 2021
26. Munkybooty referenced this in commit d6cefa9ce1 on Oct 22, 2021
27. Munkybooty referenced this in commit c6c032be5f on Oct 22, 2021
28. Munkybooty referenced this in commit ca421845a7 on Oct 23, 2021
29. Munkybooty referenced this in commit c7e4472b83 on Oct 26, 2021
30. Munkybooty referenced this in commit f63e287c06 on Oct 28, 2021
31. Munkybooty referenced this in commit 211cf5bb25 on Oct 28, 2021
32. Munkybooty referenced this in commit 740f21e8a1 on Nov 12, 2021
33. Munkybooty referenced this in commit 466d006e5d on Nov 13, 2021
34. Munkybooty referenced this in commit 6a652a4a27 on Nov 13, 2021
35. Munkybooty referenced this in commit 1ab9ba86ef on Nov 14, 2021
36. Munkybooty referenced this in commit 2749dbc651 on Nov 16, 2021
37. Munkybooty referenced this in commit d21e550b0e on Nov 18, 2021
38. MarcoFalke locked this on Dec 16, 2021

Contributors

MarcoFalkeryanofskythijstriemstralaanwj

Milestone
0.18.1