Make witness v0 outputs non-standard #8381

jl2012 commented at 12:14 PM on July 20, 2016: contributor

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

petertodd commented at 12:39 PM on July 20, 2016: contributor

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

jl2012 commented at 12:44 PM on July 20, 2016: contributor

@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 commented at 12:56 PM on July 20, 2016: member

concept ACK

petertodd commented at 1:06 PM on July 20, 2016: contributor

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

instagibbs commented at 1:09 PM on July 20, 2016: member

@petertodd miner confiscation?

petertodd commented at 1:12 PM on July 20, 2016: contributor

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

jl2012 commented at 1:18 PM on July 20, 2016: contributor

@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 commented at 1:20 PM on July 20, 2016: contributor

Concept ACK

petertodd commented at 2:53 PM on July 20, 2016: contributor

eh, whatever, harmless to do this too.

Concept ACK

MarcoFalke added the label TX fees and policy on Jul 20, 2016

MarcoFalke added the label Needs backport on Jul 20, 2016

sipa commented at 4:19 PM on July 20, 2016: member

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

NicolasDorier commented at 2:56 PM on July 21, 2016: contributor

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 commented at 2:59 PM on July 21, 2016: contributor

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 commented at 3:03 PM on July 21, 2016: member

Should we make this conditional on !testnet? @NicolasDorier This makes 0.13 equivalent to 0.12 in that regard.

sipa commented at 3:06 PM on July 21, 2016: member

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

NicolasDorier commented at 3:33 PM on July 21, 2016: contributor

@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 commented at 3:35 PM on July 21, 2016: member

ok, concept NACK this in favor of IsWitnessEnabled

jl2012 force-pushed on Jul 21, 2016

jl2012 commented at 5:25 PM on July 21, 2016: contributor

updated to using IsWitnessEnabled

jl2012 force-pushed on Jul 21, 2016

sdaftuar commented at 6:45 PM on July 21, 2016: member

Concept ACK, will test.

Make witness v0 outputs non-standard before segwit activation 1ffaff2f74

in src/main.cpp:None in 3f67f310ce outdated

1143 | @@ -1144,13 +1144,14 @@ bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState& state, const C
1144 |      }
1145 |  
1146 |      // Reject transactions with witness before segregated witness activates (override with -prematurewitness)
1147 | -    if (!GetBoolArg("-prematurewitness",false) && !tx.wit.IsNull() && !IsWitnessEnabled(chainActive.Tip(), Params().GetConsensus())) {
1148 | +    bool witnessenabled = IsWitnessEnabled(chainActive.Tip(), Params().GetConsensus());

dcousens commented at 6:18 AM on July 22, 2016:

nit: s/witnessenabled/witnessEnabled would be more consistent with all other naming, no?

jl2012 commented at 6:36 AM on July 22, 2016:

fixed

jl2012 force-pushed on Jul 22, 2016

dcousens commented at 6:40 AM on July 22, 2016: contributor

utACK 1ffaff2

NicolasDorier commented at 2:51 PM on July 22, 2016: contributor

utACK 1ffaff2f747af683513d6d74a7241d41e3f6e051

sdaftuar commented at 3:18 PM on July 22, 2016: member

Tested ACK 1ffaff2f747af683513d6d74a7241d41e3f6e051

FYI I added a test to p2p-segwit.py to cover this case, feel free to include if you like: https://github.com/sdaftuar/bitcoin/commit/21c59eac187af8a35abc9aec87bfbf51d812a449

qa: Add test for standardness of segwit v0 outputs c59c434b7d

jl2012 commented at 4:03 PM on July 22, 2016: contributor

@sdaftuar I have added your test. Thank you!

laanwj commented at 12:24 PM on July 26, 2016: member

utACK c59c434, thanks @sdaftuar for adding the test

laanwj merged this on Jul 26, 2016

laanwj closed this on Jul 26, 2016

laanwj referenced this in commit 4b1a4d8810 on Jul 26, 2016

laanwj referenced this in commit f84ee3dab6 on Jul 26, 2016

laanwj referenced this in commit 4f7f531af6 on Jul 26, 2016

laanwj commented at 12:26 PM on July 26, 2016: member

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

MarcoFalke removed the label Needs backport on Jul 31, 2016

MarcoFalke commented at 8:10 PM on July 31, 2016: member

@laanwj Removed the label as well.

dcousens commented at 11:11 PM on July 31, 2016: contributor

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

DrahtBot locked this on Sep 8, 2021