Enable spending Segwit outputs

[erikarvstedt]

Previously, transactions spending Segwit outputs were only accepted if they were already included in a block. With this patch, new Segwit transactions are accepted and relayed.

[grzegorzszczecin]

@jc23424 @erikarvstedt This work appears to be already included in #147

[erikarvstedt]

Sorry, I hadn't noticed #147. Nonetheless, #147 will still reject Segwit txs as nonstandard because ScriptSigArgsExpected in AreInputsStandard will return -1 for Segwit txs. This hunk from my PR is a possible fix.

I'd very much prefer a small, clean patch to enable Segwit outputs; wallet support can follow later.

[jc23424]

utACK - will do a bit more testing

[BlueSilver22]

@erikarvstedt I would like to invite you to discuss this in our discord. Please send an email to opportunities@btcprivate.org so we can give you an invite.

Please be sure the email matches the email on your GitHub profile.

[jc23424]

ACK (almost, i think, see below)

actually - reading this a little more - do you also need to handle whichtype == TX_WITNESS_V0_SCRIPTHASH in main.cpp - AreInputsStandard() ? As well as the case where a normal P2SH wraps a P2WSH?

[jc23424]

It looks that they've eliminated the ScriptSigArgsExpected check in bitcoin - writing the code to count expected stack items in a P2SH-P2WSH will get pretty hairy - maybe just eliminate that check from IsStandard entirely?

[grzegorzszczecin]

@jc23424 are you waiting for a response from @erikarvstedt ?

The change you're referring to is https://github.com/bitcoin/bitcoin/commit/5d743099b5fe77ba423110bea4f5dfd854fef3b2

However I'm afraid it is not easy to cherry-pick this commit because of the ongoing refactoring in the bitcoin repository.

[jc23424]

Yeah - with these changes as is I don't think it will work right now for P2WSH? (i've removed the ArgsExpected check in my larger pr https://github.com/BTCPrivate/BitcoinPrivate/pull/147/files)

I think it would be good to merge this in the meantime - would be nice to get it to work for P2WSH though and not just P2WPKH

[Ayms]

Why don't you just merge for now the minimum, corresponding to @erikarvstedt PR or the equivalent changes in yours if you are sure about it? (I don't see why it would not work for P2WSH)

147 as a whole maybe looks a bit ambitious

[jc23424]

P2WSH doesn't work here because the args expected check would fail i believe not sure where @erikarvstedt is - but #158 takes this commit along with https://github.com/bitcoin/bitcoin/commit/5d743099b5fe77ba423110bea4f5dfd854fef3b2 to remove the args expected check entirely

[Ayms]

@jc23424 I don't get why you are not sure, cant't you just test it (and make it work if not)?

[jc23424]

yes - i did can you look at the other pr i referenced which is a minimal fix to make P2WSH work (its not the long one) #158

[Ayms]

I am not an expert with bitcoin core code, as far as I understand #158 first commit makes segwit-like txs standard, so merging it and the second commit should be ok

Now should not a check be added in VerifyScript where you have commented scriptSig != 0 and scriptSig != CScript() like scriptSig==0 / scriptSig==CScript() --> error so a tx without [sig(s)][pubkey or redeem] does not become standard (ie could be spent by anybody by only using the segwit address 00xyz)?

Probably it's not difficult to test the different cases to be sure

[Ayms]

Any progress? People are asking for it

[jc23424]

Merged here! Sorry for delay - wanted to have a bunch of eyes for review - https://github.com/BTCPrivate/BitcoinPrivate/pull/147/

[Ayms]

Great, what is your plan to upgrade the network?

[jc23424]

Its a policy only change (non-consensus) so network upgrades can happen at miner leisure - miners have been informed that theres a new release. I imagine if you broadcast spending transactions to a large number of nodes you'll find some mempools that will accept these

[Ayms]

OK but I imagine that btcprivate.org (node/seed+pool+explorer node) did upgrade, no? Because this is not working for now

[Ayms]

This is working (tested with an upgraded node) but apparently nobody did upgrade, ccing @ocminer

[ocminer]

Hmm.. I'm sorry but wasn't there a major flaw with segwit outputs enabling "stealing" of coins ? I've disabled it on the pool for that reason.. I'll check if I find the post again.

[Ayms]

You are probably referring to #140

147 is fixing it and allows to spend signed segwit outputs

[ocminer]

Okay, I'll check and update to segwit enabled payments as well asap

[Ayms]

Thanks, please let us know when it is done

[interbiznw]

@ayms actually yes we did upgrade our core nodes and explorers. Not sure the issue your having. Can you provide more details

[Ayms]

I tried again, now sending from the explorer seems to work (but not yesterday), at least it accepts the tx and advertises a successful broadcast, I don't know what are your core nodes but dnsseed.btcprivate.org still rejects the tx, and the one I just broadcasted from the explorer does not appear in the explorer... let's wait a bit and see if the explorer finds it and if it is mined by your pool or someone else...

[ocminer]

all updated now

[Ayms]

Thanks, same tx sent to suprnova, seems to be working, now we can see it in the explorer too, unconfirmed, let's see when it is mined

[ocminer]

A block was just mined, should include your tx as well.

[Ayms]

Yes! First spending from a segwit output mined probably :-)

[ocminer]

Yay :)

[BlueSilver22]

@ayms dnsseed.btcprivate.org is actually a dns seeder. I do not believe it needs to be upgraded but we might be able to tweak it to only look for 1.0.11 nodes.