Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[POLICY] Make sending to future native witness outputs standard #15846

Merged
merged 1 commit into from Apr 27, 2019

Conversation

Projects
None yet
10 participants
@sipa
Copy link
Member

commented Apr 18, 2019

As discussed in the April 18 2019 IRC meeting.

This makes sending to future Segwit versions via native outputs (bech32) standard for relay, mempool acceptance, and mining. The reasons are:

  • This may interfere with smooth adoption of future segwit versions, if they're defined (by the sender wallet/node).
  • It violates BIP173 ("Version 0 witness addresses are always 42 or 62 characters, but implementations MUST allow the use of any version."), though admittedly this code was written before BIP173.
  • It doesn't protect much, as P2SH-embedded segwit cannot be filtered in this way.
  • As a general policy, the sender shouldn't care what the receiver likes his outputs to be.

Note that spending such outputs (including P2SH-embedded ones) remains nonstandard, as that is actually required for softfork safety.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

utACK c634b1e

@MarcoFalke MarcoFalke added this to the 0.19.0 milestone Apr 18, 2019

@jnewbery

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

IRC discussion here: http://www.erisian.com.au/meetbot/bitcoin-core-dev/2019/bitcoin-core-dev.2019-04-18-19.00.log.html#l-77

Concept ACK this change. I agree that the mempool should accept and relay txs with outputs to any segwit version.

It violates BIP173 ("Version 0 witness addresses are always 42 or 62 characters, but implementations MUST allow the use of any version."), though admittedly this code was written before BIP173.

This isn't relevant to the change here. BIP173 describes an address format and how to decode that into a scriptPubKey. The node/mempool has no concept of addresses.

I think your last three points are actually arguments to not stop the wallet from sending to segwit v1+ addresses, rather than arguments to allow the mempool to accept/relay those txs.

I'd prefer to prevent the Bitcoin Core wallet from sending to v1+ addresses, or at least to warn. We can't prevent all classes of sending insecure or unspendable addresses, but I think it makes sense to do so when we can.

I think the main concern that people have with that is that it might slow down segwit v1 adoption. I disagree - bech32 adoption has mostly been slowed down by large exchanges or other services not implementing send-to-bech32. That's something that is outside our control.

@luke-jr

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

utACK

1 similar comment
@gmaxwell

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

utACK

@harding

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

Tested ACK c634b1e

Separately tested a bc1p... v1 address with sendrawtransaction and the wallet GUI on mainnet. Transactions were accepted to the local mempool, relayed to a remote peer I controlled, and entered that peer's mempool (both peers running this PR).

(Hint for anyone else testing, if you want to empty your mempool so you can run abandontransaction, you need to restart with -persistmempool=0 -walletbroadcast=0, run the savemempool RPC, abandon your transaction either via the RPC or the GUI, and then restart again. If you don't savemempool, your node will restore the mempool from the run before you set persistmempool=0 and that mempool will contain your wallet transaction.)

@gmaxwell

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

utACK

@sdaftuar

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

utACK

1 similar comment
@instagibbs

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

utACK

@meshcollider

This comment has been minimized.

Copy link
Member

commented Apr 27, 2019

utACK c634b1e

@meshcollider meshcollider merged commit c634b1e into bitcoin:master Apr 27, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

meshcollider added a commit that referenced this pull request Apr 27, 2019

Merge #15846: [POLICY] Make sending to future native witness outputs …
…standard

c634b1e [POLICY] Make sending to future native witness outputs standard (Pieter Wuille)

Pull request description:

  As discussed in the April 18 2019 IRC meeting.

  This makes sending to future Segwit versions via native outputs (bech32) standard for relay, mempool acceptance, and mining. The reasons are:
  * This may interfere with smooth adoption of future segwit versions, if they're defined (by the sender wallet/node).
  * It violates BIP173 ("Version 0 witness addresses are always 42 or 62 characters, but implementations MUST allow the use of any version."), though admittedly this code was written before BIP173.
  * It doesn't protect much, as P2SH-embedded segwit cannot be filtered in this way.
  * As a general policy, the sender shouldn't care what the receiver likes his outputs to be.

  Note that _spending_ such outputs (including P2SH-embedded ones) remains nonstandard, as that is actually required for softfork safety.

ACKs for commit c634b1:
  MarcoFalke:
    utACK c634b1e
  harding:
    Tested ACK c634b1e
  meshcollider:
    utACK c634b1e

Tree-SHA512: e37168a1be9f445a04d4280593f0a92bdae33eee00ecd803d5eb16acb5c9cfc0f1f0a1dfbd5a0cc73da2c9928ec11cbdac7911513a78f85b789ae0d00e1b5962

sidhujag added a commit to syscoin/syscoin that referenced this pull request May 1, 2019

Merge bitcoin#15846: [POLICY] Make sending to future native witness o…
…utputs standard

c634b1e [POLICY] Make sending to future native witness outputs standard (Pieter Wuille)

Pull request description:

  As discussed in the April 18 2019 IRC meeting.

  This makes sending to future Segwit versions via native outputs (bech32) standard for relay, mempool acceptance, and mining. The reasons are:
  * This may interfere with smooth adoption of future segwit versions, if they're defined (by the sender wallet/node).
  * It violates BIP173 ("Version 0 witness addresses are always 42 or 62 characters, but implementations MUST allow the use of any version."), though admittedly this code was written before BIP173.
  * It doesn't protect much, as P2SH-embedded segwit cannot be filtered in this way.
  * As a general policy, the sender shouldn't care what the receiver likes his outputs to be.

  Note that _spending_ such outputs (including P2SH-embedded ones) remains nonstandard, as that is actually required for softfork safety.

ACKs for commit c634b1:
  MarcoFalke:
    utACK c634b1e
  harding:
    Tested ACK c634b1e
  meshcollider:
    utACK bitcoin@c634b1e

Tree-SHA512: e37168a1be9f445a04d4280593f0a92bdae33eee00ecd803d5eb16acb5c9cfc0f1f0a1dfbd5a0cc73da2c9928ec11cbdac7911513a78f85b789ae0d00e1b5962
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.