What is up with this SIGHASH_SINGLE and nOut out of range?

[piotr_n]

Since yesterday there have been like a few txs, committed into the blockchain, which IMO are literally broken.
This broke my client BTW, but I fixed it already, and I know this cannot be reverted anymore, so I only wonder..

From what I understand using SIGHASH_SINGLE that has more outputs than inputs does not make any sense - so the code even prints an error then:

Code:

unsigned int nOut = nIn;                                            
if (nOut >= txTmp.vout.size())                                      
{                                                                   
    printf("ERROR: SignatureHash() : nOut=%d out of range\n", nOut);
    return 1;                                                       
}                                                                   

Normally when there is an error somewhere when executing a script, it just fails - but here, despite of printing the error, it still accepts the transaction, using a hash value of 1 and then verifying the signature for such a hash.

So what is up with it?
Is it like someone just overlooked this piece of code - or is it "error => hash=1" by design?

EDIT:
If you don't know what I am talking about, checks these three txs:

Code:

315ac7d4c26d69668129cc352851d9389b4a6868f1509c6c8b66bead11e2619f
dbf38261224ebff0c455c405e2435cfc69adb6b8a42d7b10674d9a4eb0464dca
de744408e4198c0a39310c8106d1830206e8d8a5392bcf715c9b5ec97d784edd

[blueadept]

https://bitcointalk.org/index.php?topic=260595.0

[jackjack]

Broken?

Why would SIGHASH_SINGLE require nIn==nOut? I misread
It only require nIn<=nOut

[piotr_n]

https://bitcointalk.org/index.php?topic=260595.0

Thx.
And sorry for double post.

It only require nIn<=nOut

That's right.
But what if nIn=2 and nOut=1 - like here?
Well, now I know: use the hash value of 1, but don't dare to fail the script!

[jackjack]

https://bitcointalk.org/index.php?topic=260595.0

Thx.
And sorry for double post.

It only require nIn<=nOut

That's right.
But what if nIn=2 and nOut=1 - like here?
Well, now I know: use the hash value of 1, but don't dare to fail the script!

Wow I didn't know...
Is it allowed by bitcoin-qt? Yes (I thought retep was talking about webbtc in his OP)
By the protocol?

[piotr_n]

https://bitcointalk.org/index.php?topic=260595.0

Thx.
And sorry for double post.

It only require nIn<=nOut

That's right.
But what if nIn=2 and nOut=1 - like here?
Well, now I know: use the hash value of 1, but don't dare to fail the script!

Wow I didn't know...
Is it allowed by bitcoin-qt? By the protocol?
I don't find anything in the linked thread

These three transaction I referred to my OP have been mined, so obviously it is allowed.
And now we all need to live with it - that's another argument on why a documentation other than satoshi's code is useless

[jackjack]

https://bitcointalk.org/index.php?topic=260595.0

Thx.
And sorry for double post.

It only require nIn<=nOut

That's right.
But what if nIn=2 and nOut=1 - like here?
Well, now I know: use the hash value of 1, but don't dare to fail the script!

Wow I didn't know...
Is it allowed by bitcoin-qt? By the protocol?
I don't find anything in the linked thread

These three transaction I referred to my OP have been mined, so obviously it is allowed.
And now we all need to live with it.

Live with it ok, but we need to know what is the good behavior for future implementations!
Doesn't this look like a crazy hack/bug?

The only piece of protocol we have is this:

Procedure for Hashtype SIGHASH_SINGLE

    The output of txCopy is resized to the size of the current input index+1.
    All other txCopy outputs aside from the output that is the same as the current input index are set to a blank script and a value of (long) -1.
    All other txCopy inputs aside from the current input are set to have an nSequence index of zero.

That requires adding empty outputs until len=index+1, then processing
Is it what bitcoin-qt does?

[piotr_n]

Live with it ok, but we need to know what is the good behavior!
Doesn't this look like a crazy hack/bug?

Thus my question.

For me it seems like someone has overlooked it at some point; "return 1" was supposed to indicate an error, but it was somehow overlooked and now it implicitly gets cast to (uint256)1 which is a pretty valid hash value, so C++ compiler does not complain and so nobody has noticed..

Don't you have the same problem in Armory?
I cannot imagine reproducing the same behavior in python, just by repeating such a mistake.

[Mike Hearn]

I believe Peter created these transactions, and I think that was a good idea. Go Peter!

Speaking to all readers of this thread (not specifically piotr_n) - if your implementation broke because of these transactions, two things to consider:

1) Your implementation almost certainly contains other chain splitting bugs beyond this one.

2) You should be using Matt's blocktester which would have revealed the issue, as Matt discovered this bug >1 year ago when he did the bitcoinj full verification mode.

[jgarzik]

Live with it ok, but we need to know what is the good behavior!
Doesn't this look like a crazy hack/bug?

Thus my question.

For me it seems like someone has overlooked it at some point; "return 1" was supposed to indicate an error, but it was somehow overlooked and now it implicitly gets cast to (uint256)1 which is a pretty valid hash value, so C++ compiler does not complain and so nobody has noticed..

Don't you have the same problem in Armory?
I cannot imagine reproducing the same behavior in python, just by repeating such a mistake.

This was noticed a long time ago.  It is just now being re-noticed by others

I discovered this over a year ago while rewriting everything into python:

https://github.com/jgarzik/python-bitcoinlib/
https://github.com/jgarzik/python-bitcoinlib/blob/master/bitcoin/scripteval.py

Can you spot the problem? 

[piotr_n]

I believe Peter created these transactions, and I think that was a good idea. Go Peter!

Yes.
As much as I don't get along with him in general, thank you @Peter for bringing these txs to the network!

I was always worried about different implementation/language related technicals that could be exploited to create a hard fork, so I was designing my node to be less strict in any checking, for such cases. Since it is not a mining node, it should not matter much, but Peter has actually managed to find a case where my implementation was more strict..
And now I have at least one less to go

[piotr_n]

Live with it ok, but we need to know what is the good behavior!
Doesn't this look like a crazy hack/bug?

Thus my question.

For me it seems like someone has overlooked it at some point; "return 1" was supposed to indicate an error, but it was somehow overlooked and now it implicitly gets cast to (uint256)1 which is a pretty valid hash value, so C++ compiler does not complain and so nobody has noticed..

Don't you have the same problem in Armory?
I cannot imagine reproducing the same behavior in python, just by repeating such a mistake.

This was noticed a long time ago.  It is just now being re-noticed by others

I discovered this over a year ago while rewriting everything into python:

https://github.com/jgarzik/python-bitcoinlib/
https://github.com/jgarzik/python-bitcoinlib/blob/master/bitcoin/scripteval.py

Can you spot the problem? 

Man, you should have put it on the wiki then, or at least as some comment in the source code.
Unless your goal is to make sure that the clients you develop keep the monopoly.
Then its probably a good strategy, to not make such an info public.

[piotr_n]

Just as a general comment, to anyone reading this: please always, whenever you discover such a case, do whatever you can to public any possible sources of hard forks that can be a consequence of an existing implementation, that is not an obvious design decision, but rather a hard to spot, implementation specific whatever (a bug or a feature).

As they say it: knowledge is power.
And knowledge of how you can attack a 1+bn USD worth of a currency is a power worth exploiting.
So if you are honest people, and I think you are, please do not withhold such an important info, but rather advertise it as much as you can - embedding test cases in the block chain is the perfect ultimate solution.

[jackjack]

Just as a general comment, to anyone reading this: please always, whenever you discover such a case, do whatever you can to public any possible sources of hard forks that can be a consequence of an existing implementation, that is not an obvious design decision, but rather a hard to spot, implementation specific whatever (a bug or a feature).

+1


I think such transactions must be treated as non-standard

[Peter Todd]

Just as a general comment, to anyone reading this: please always, whenever you discover such a case, do whatever you can to public any possible sources of hard forks that can be a consequence of an existing implementation, that is not an obvious design decision, but rather a hard to spot, implementation specific whatever (a bug or a feature).

We do that already. This issue with SIGHASH_SINGLE was documented in the unittests here: https://github.com/bitcoin/bitcoin/blob/master/src/test/data/tx_valid.json#L39 (I did not find it first)

The other main source of test cases is Matt Corallo's block tester: https://github.com/TheBlueMatt/test-scripts

You're welcome to go through those test cases and summarize the more subtle ones, but frankly I don't see the point: any change, no matter how small, triggers a hard fork. Perfect compliance with those tests is a necessary but far from sufficient requirement.

[piotr_n]

We do that already. This issue with SIGHASH_SINGLE was documented in the unittests here: https://github.com/bitcoin/bitcoin/blob/master/src/test/data/tx_valid.json#L39 (I did not find it first)

The other main source of test cases is Matt Corallo's block tester: https://github.com/TheBlueMatt/test-scripts

Thanks! The vectors from the json file helped me a lot today. And the tx_invalid.json also turned out to be useful.
Who would have thought that cutting off the signature from a message that gets hashed is such a complex operation (talking about the length prefix here).

Though, the Matt's block tester - I did not even try to build it, not too mention figuring out how to use it.. Maybe some day.

You're welcome to go through those test cases and summarize the more subtle ones, but frankly I don't see the point: any change, no matter how small, triggers a hard fork. Perfect compliance with those tests is a necessary but far from sufficient requirement.

Sure - but at least this many less to go


BTW, it would be nice to a have a test tx that triggers this code:

Code:

// In case concatenating two scripts ends up with two codeseparators,             
// or an extra one at the end, this prevents all those possible incompatibilities.
scriptCode.FindAndDelete(CScript(OP_CODESEPARATOR));

[Peter Todd]

BTW, it would be nice to a have a test tx that triggers this code:

Code:

// In case concatenating two scripts ends up with two codeseparators,             
// or an extra one at the end, this prevents all those possible incompatibilities.
scriptCode.FindAndDelete(CScript(OP_CODESEPARATOR));

Working on it.


FWIW I'd suggest you consider removing script evaluation from your library entirely for now. Keep the rest of the scripting code - you need it to create transactions after all - but make it clear that the only way you know that a transaction is valid is if a miner mines it. This is more in-line with the guarantees SPV clients can provide anyway, and will save you an enormous amount of work that is probably wasted until we have better testing infrastructure for third party clients to use.

[piotr_n]

FWIW I'd suggest you consider removing script evaluation from your library entirely for now. Keep the rest of the scripting code - you need it to create transactions after all - but make it clear that the only way you know that a transaction is valid is if a miner mines it. This is more in-line with the guarantees SPV clients can provide anyway, and will save you an enormous amount of work that is probably wasted until we have better testing infrastructure for third party clients to use.

Oh, no!
Thanks for the advise to make my life easy, but switching the validation off, after I had spent so much effort on validating the txs, is like admitting that I failed on the most important part.
I know that there are still things that will make my code fail, eventually somewhere in a future, but that is exactly where I need people like you, who try to break it now, while I seem to be the only user, thus with no big consequences.
So please keep up the good job, and let me handle whatever you break

[genjix]

yeah it returns one, but the tx is still valid in the blockchain. i know it's a big wtf.

the actual hash looks like 00 00 00 ... 00 01 (last byte is 0x01 but all the rest is 0x00).

[Peter Todd]

Oh, no!
Thanks for the advise to make my life easy, but switching the validation off, after I had spent so much effort on validating the txs, is like admitting that I failed on the most important part.
I know that there are still things that will make my code fail, eventually somewhere in a future, but that is exactly where I need people like you, who try to break it now, while I seem to be the only user, thus with no big consequences.
So please keep up the good job, and let me handle whatever you break

Successful people are willing to cut their losses and refocus their efforts. You can always retrieve the code from revision control history later when the testing infrastructure improves; without good testing capabilities you're wasting your time.

The general who advances without coveting fame and retreats without fearing disgrace, whose only thought is to protect his country and do good service for his sovereign, is the jewel of the kingdom.