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

Make witness v0 outputs non-standard #8381

Merged
merged 2 commits into from Jul 26, 2016

Conversation

Projects
None yet
@jl2012
Member

jl2012 commented Jul 20, 2016

Witness v0 outputs are unsafe before segwit is activated and should remain non-standard

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Jul 20, 2016

Contributor

Did you try spending one? They can't be spent due to the CLEANSTACK rule.

Contributor

petertodd commented Jul 20, 2016

Did you try spending one? They can't be spent due to the CLEANSTACK rule.

@jl2012

This comment has been minimized.

Show comment
Hide comment
@jl2012

jl2012 Jul 20, 2016

Member

@petertodd : yes, when spending it is non-standard for <= 0.12. The issue here is transactions with such outputs are relayed and mined by 0.13, even before activation of segwit

Member

jl2012 commented Jul 20, 2016

@petertodd : yes, when spending it is non-standard for <= 0.12. The issue here is transactions with such outputs are relayed and mined by 0.13, even before activation of segwit

@instagibbs

This comment has been minimized.

Show comment
Hide comment
@instagibbs

instagibbs Jul 20, 2016

Member

concept ACK

Member

instagibbs commented Jul 20, 2016

concept ACK

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Jul 20, 2016

Contributor

@jl2012 Ah sorry, misread that. Still there's no harm to those outputs being created.

Contributor

petertodd commented Jul 20, 2016

@jl2012 Ah sorry, misread that. Still there's no harm to those outputs being created.

@instagibbs

This comment has been minimized.

Show comment
Hide comment
@instagibbs

instagibbs Jul 20, 2016

Member

@petertodd miner confiscation?

Member

instagibbs commented Jul 20, 2016

@petertodd miner confiscation?

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Jul 20, 2016

Contributor

@instagibbs There's lots of ways you can screw up and lose your coins you know... :)

Contributor

petertodd commented Jul 20, 2016

@instagibbs There's lots of ways you can screw up and lose your coins you know... :)

@jl2012

This comment has been minimized.

Show comment
Hide comment
@jl2012

jl2012 Jul 20, 2016

Member

@petertodd why do we have IsStandard check for outputs at all? To prevent UTXO spam or to prevent people losing money by doing stupid things? This patch is for the latter reason.

Member

jl2012 commented Jul 20, 2016

@petertodd why do we have IsStandard check for outputs at all? To prevent UTXO spam or to prevent people losing money by doing stupid things? This patch is for the latter reason.

@btcdrak

This comment has been minimized.

Show comment
Hide comment
@btcdrak

btcdrak Jul 20, 2016

Member

Concept ACK

Member

btcdrak commented Jul 20, 2016

Concept ACK

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Jul 20, 2016

Contributor

eh, whatever, harmless to do this too.

Concept ACK

Contributor

petertodd commented Jul 20, 2016

eh, whatever, harmless to do this too.

Concept ACK

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jul 20, 2016

Member

utACK, though we should have an issue to revisit this for 0.13.1

Member

sipa commented Jul 20, 2016

utACK, though we should have an issue to revisit this for 0.13.1

@NicolasDorier

This comment has been minimized.

Show comment
Hide comment
@NicolasDorier

NicolasDorier Jul 21, 2016

Member

This will not be in 0.13, and 0.13.1 will include segwit, and we don't want to require people to update to 0.13.2 for starting to relay segwit transaction, so I don't understand the reason of this PR.

Member

NicolasDorier commented Jul 21, 2016

This will not be in 0.13, and 0.13.1 will include segwit, and we don't want to require people to update to 0.13.2 for starting to relay segwit transaction, so I don't understand the reason of this PR.

@NicolasDorier

This comment has been minimized.

Show comment
Hide comment
@NicolasDorier

NicolasDorier Jul 21, 2016

Member

And if it is meant for 0.13, then it means that once segwit is activated, the relay of segwit outputs will be penalized because of old 0.13 nodes.

Member

NicolasDorier commented Jul 21, 2016

And if it is meant for 0.13, then it means that once segwit is activated, the relay of segwit outputs will be penalized because of old 0.13 nodes.

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jul 21, 2016

Member

Should we make this conditional on !testnet?

@NicolasDorier This makes 0.13 equivalent to 0.12 in that regard.

Member

luke-jr commented Jul 21, 2016

Should we make this conditional on !testnet?

@NicolasDorier This makes 0.13 equivalent to 0.12 in that regard.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jul 21, 2016

Member

I'd prefer to make this based on IsWitnessEnabled, so we don't need to touch that code again.

Member

sipa commented Jul 21, 2016

I'd prefer to make this based on IsWitnessEnabled, so we don't need to touch that code again.

@NicolasDorier

This comment has been minimized.

Show comment
Hide comment
@NicolasDorier

NicolasDorier Jul 21, 2016

Member

@luke-jr yes, but I feel it is kind of a waste, would be nice that when segwit get activated that it can be broadly relayed. Agree on using IsWitnessEnabled.

Member

NicolasDorier commented Jul 21, 2016

@luke-jr yes, but I feel it is kind of a waste, would be nice that when segwit get activated that it can be broadly relayed. Agree on using IsWitnessEnabled.

@instagibbs

This comment has been minimized.

Show comment
Hide comment
@instagibbs

instagibbs Jul 21, 2016

Member

ok, concept NACK this in favor of IsWitnessEnabled

Member

instagibbs commented Jul 21, 2016

ok, concept NACK this in favor of IsWitnessEnabled

@jl2012

This comment has been minimized.

Show comment
Hide comment
@jl2012

jl2012 Jul 21, 2016

Member

updated to using IsWitnessEnabled

Member

jl2012 commented Jul 21, 2016

updated to using IsWitnessEnabled

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Jul 21, 2016

Member

Concept ACK, will test.

Member

sdaftuar commented Jul 21, 2016

Concept ACK, will test.

Show outdated Hide outdated src/main.cpp
@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Jul 22, 2016

Contributor

utACK 1ffaff2

Contributor

dcousens commented Jul 22, 2016

utACK 1ffaff2

@NicolasDorier

This comment has been minimized.

Show comment
Hide comment
@NicolasDorier
Member

NicolasDorier commented Jul 22, 2016

utACK 1ffaff2

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Jul 22, 2016

Member

Tested ACK 1ffaff2

FYI I added a test to p2p-segwit.py to cover this case, feel free to include if you like:
sdaftuar@21c59ea

Member

sdaftuar commented Jul 22, 2016

Tested ACK 1ffaff2

FYI I added a test to p2p-segwit.py to cover this case, feel free to include if you like:
sdaftuar@21c59ea

@jl2012

This comment has been minimized.

Show comment
Hide comment
@jl2012

jl2012 Jul 22, 2016

Member

@sdaftuar I have added your test. Thank you!

Member

jl2012 commented Jul 22, 2016

@sdaftuar I have added your test. Thank you!

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jul 26, 2016

Member

utACK c59c434, thanks @sdaftuar for adding the test

Member

laanwj commented Jul 26, 2016

utACK c59c434, thanks @sdaftuar for adding the test

@laanwj laanwj merged commit c59c434 into bitcoin:master Jul 26, 2016

1 check passed

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

laanwj added a commit that referenced this pull request Jul 26, 2016

Merge #8381: Make witness v0 outputs non-standard
c59c434 qa: Add test for standardness of segwit v0 outputs (Suhas Daftuar)
1ffaff2 Make witness v0 outputs non-standard before segwit activation (Johnson Lau)

laanwj added a commit that referenced this pull request Jul 26, 2016

laanwj added a commit that referenced this pull request Jul 26, 2016

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jul 26, 2016

Member

Backported to 0.13 as f84ee3d 4f7f531 (used two commits instead of squashing because of separate authors)

Member

laanwj commented Jul 26, 2016

Backported to 0.13 as f84ee3d 4f7f531 (used two commits instead of squashing because of separate authors)

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Jul 31, 2016

Member

@laanwj Removed the label as well.

Member

MarcoFalke commented Jul 31, 2016

@laanwj Removed the label as well.

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Jul 31, 2016

Contributor

Heh, title should probably read "Make witness v0 outputs non-standard until activation".

Contributor

dcousens commented Jul 31, 2016

Heh, title should probably read "Make witness v0 outputs non-standard until activation".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment