Disallow extended encoding for non-witness transactions #14039

pull sipa wants to merge 1 commits into bitcoin:master from sipa:201808_no_superfluous_witness changing 1 files +4 −0
  1. sipa commented at 12:08 AM on August 24, 2018: member

    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 (https://github.com/bitcoin/bitcoin/pull/8149), but was subsequently dropped (https://github.com/bitcoin/bitcoin/pull/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.

  2. Disallow extended encoding for non-witness transactions bb530efa18
  3. sipa force-pushed on Aug 24, 2018
  4. apoelstra cross-referenced this on Aug 24, 2018 from issue Various serialization fixes by apoelstra
  5. practicalswift commented at 9:16 AM on August 24, 2018: contributor

    Concept ACK

    Stricter is better (in this case)!

  6. instagibbs commented at 12:50 PM on August 24, 2018: member

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

  7. instagibbs commented at 4:28 PM on August 24, 2018: member

    failing decoderaw test: https://travis-ci.org/bitcoin/bitcoin/jobs/419906176#L2871

  8. instagibbs cross-referenced this on Aug 24, 2018 from issue Disallow extended encoding for non-witness transactions by apoelstra
  9. MarcoFalke commented at 6:06 PM on August 24, 2018: member

    To fix the test failure you could try rebasing on fae040010deda9404b15b214cec2a099fb831253

    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.

  10. MarcoFalke cross-referenced this on Aug 27, 2018 from issue qa: Add some actual witness in rpc_rawtransaction by MarcoFalke
  11. laanwj commented at 12:21 PM on August 29, 2018: member

    concept ACK

  12. laanwj added the label RPC/REST/ZMQ on Aug 31, 2018
  13. Sjors commented at 6:22 PM on September 7, 2018: member

    Maybe add a test?

  14. gmaxwell commented at 8:59 PM on February 14, 2019: contributor

    utACK

  16. instagibbs commented at 5:48 PM on March 21, 2019: member

    upgrading to tACK, please @sipa take this test:

    https://github.com/instagibbs/bitcoin/commit/7c1ad7c0367e94748e724d6a39d056b20ecc3975

    Fails for me on master, passes with your commit.

  17. stevenroose commented at 5:19 PM on March 22, 2019: contributor

    utACK bb530efa1872ec963417f61da9a95185c7a7a7d6 We backported this to Elements already as well: https://github.com/ElementsProject/elements/pull/525

  18. instagibbs referenced this in commit aaee9dde6f on Mar 25, 2019
  19. sdaftuar commented at 3:53 PM on April 25, 2019: member

    utACK, I think this is ready for merge?

  20. instagibbs commented at 3:57 PM on April 25, 2019: member

    I can PR my test right after merge

  21. MarcoFalke merged this on Apr 25, 2019
  22. MarcoFalke closed this on Apr 25, 2019
  23. MarcoFalke referenced this in commit c65c77c721 on Apr 25, 2019
  24. instagibbs cross-referenced this on Apr 25, 2019 from issue Add test for superfluous witness record in deserialization by instagibbs
  25. MarcoFalke referenced this in commit 653b2b4426 on Apr 26, 2019
  26. MarcoFalke added the label Needs backport on Apr 26, 2019
  27. MarcoFalke removed the label Needs backport on Apr 26, 2019
  28. MarcoFalke added the label Needs backport on Apr 26, 2019
  29. MarcoFalke added this to the milestone 0.18.1 on Apr 26, 2019
  30. sidhujag referenced this in commit fd6ff32bc6 on Apr 27, 2019
  31. sidhujag referenced this in commit 4a65581154 on Apr 27, 2019
  32. moneyball commented at 8:24 PM on April 29, 2019: contributor

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

  33. sipa commented at 8:37 PM on April 29, 2019: member

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

  34. MarcoFalke commented at 8:50 PM on April 29, 2019: member

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

  35. MarcoFalke cross-referenced this on May 13, 2019 from issue p2p: Avoid logging transaction decode errors to stderr by MarcoFalke
  36. laanwj referenced this in commit bb291b50f2 on May 20, 2019
  37. MarcoFalke referenced this in commit 206f5ee875 on May 20, 2019
  38. MarcoFalke removed the label Needs backport on May 20, 2019
  39. sidhujag referenced this in commit 9f749c185f on May 20, 2019
  40. HashUnlimited referenced this in commit e30fa22d54 on Aug 23, 2019
  41. Bushstar referenced this in commit 6de72e5d3a on Aug 24, 2019
  42. Munkybooty referenced this in commit 4df2763b77 on Oct 17, 2021
  43. Munkybooty referenced this in commit d6cefa9ce1 on Oct 22, 2021
  44. Munkybooty referenced this in commit c6c032be5f on Oct 22, 2021
  45. Munkybooty referenced this in commit ca421845a7 on Oct 23, 2021
  46. Munkybooty referenced this in commit c7e4472b83 on Oct 26, 2021
  47. Munkybooty referenced this in commit f63e287c06 on Oct 28, 2021
  48. Munkybooty referenced this in commit 211cf5bb25 on Oct 28, 2021
  49. Munkybooty referenced this in commit 740f21e8a1 on Nov 12, 2021
  50. Munkybooty referenced this in commit 466d006e5d on Nov 13, 2021
  51. Munkybooty referenced this in commit 6a652a4a27 on Nov 13, 2021
  52. Munkybooty referenced this in commit 1ab9ba86ef on Nov 14, 2021
  53. Munkybooty referenced this in commit 2749dbc651 on Nov 16, 2021
  54. Munkybooty referenced this in commit d21e550b0e on Nov 18, 2021
  55. bitcoin locked this on Aug 18, 2022
Contributors
sipapracticalswiftinstagibbsMarcoFalkelaanwjSjorsgmaxwellstevenroosesdaftuarmoneyball
Labels
RPC/REST/ZMQ

Milestone
0.18.1

Linked (view graph)
#8149 Segregated witness rebased#8589 Inline CTxInWitness inside CTxIn#14052 qa: Add some actual witness in rpc_rawtransaction#15893 Add test for superfluous witness record in deserialization#16021 p2p: Avoid logging transaction decode errors to stderr
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-19 06:54 UTC