piotr_n (OP)
Legendary
Offline
Activity: 2053
Merit: 1356
aka tonikt
|
![](https://bitcointalk.org/Themes/custom1/images/post/xx.gif) |
July 23, 2013, 12:53:59 PM |
|
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: 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: 315ac7d4c26d69668129cc352851d9389b4a6868f1509c6c8b66bead11e2619f dbf38261224ebff0c455c405e2435cfc69adb6b8a42d7b10674d9a4eb0464dca de744408e4198c0a39310c8106d1830206e8d8a5392bcf715c9b5ec97d784edd
|
Check out gocoin - my original project of full bitcoin node & cold wallet written in Go.PGP fingerprint: AB9E A551 E262 A87A 13BB 9059 1BE7 B545 CDF3 FD0E
|
|
|
|
jackjack
Legendary
Offline
Activity: 1176
Merit: 1257
May Bitcoin be touched by his Noodly Appendage
|
![](https://bitcointalk.org/Themes/custom1/images/post/xx.gif) |
July 23, 2013, 01:06:12 PM Last edit: July 23, 2013, 01:21:40 PM by jackjack |
|
Broken?
Why would SIGHASH_SINGLE require nIn==nOut? I misread It only require nIn<=nOut
|
Own address: 19QkqAza7BHFTuoz9N8UQkryP4E9jHo4N3 - Pywallet support: 1AQDfx22pKGgXnUZFL1e4UKos3QqvRzNh5 - Bitcointalk++ script support: 1Pxeccscj1ygseTdSV1qUqQCanp2B2NMM2 Pywallet: instructions. Encrypted wallet support, export/import keys/addresses, backup wallets, export/import CSV data from/into wallet, merge wallets, delete/import addresses and transactions, recover altcoins sent to bitcoin addresses, sign/verify messages and files with Bitcoin addresses, recover deleted wallets, etc.
|
|
|
piotr_n (OP)
Legendary
Offline
Activity: 2053
Merit: 1356
aka tonikt
|
![](https://bitcointalk.org/Themes/custom1/images/post/xx.gif) |
July 23, 2013, 01:10:43 PM |
|
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!
|
Check out gocoin - my original project of full bitcoin node & cold wallet written in Go.PGP fingerprint: AB9E A551 E262 A87A 13BB 9059 1BE7 B545 CDF3 FD0E
|
|
|
jackjack
Legendary
Offline
Activity: 1176
Merit: 1257
May Bitcoin be touched by his Noodly Appendage
|
![](https://bitcointalk.org/Themes/custom1/images/post/xx.gif) |
July 23, 2013, 01:21:05 PM |
|
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?
|
Own address: 19QkqAza7BHFTuoz9N8UQkryP4E9jHo4N3 - Pywallet support: 1AQDfx22pKGgXnUZFL1e4UKos3QqvRzNh5 - Bitcointalk++ script support: 1Pxeccscj1ygseTdSV1qUqQCanp2B2NMM2 Pywallet: instructions. Encrypted wallet support, export/import keys/addresses, backup wallets, export/import CSV data from/into wallet, merge wallets, delete/import addresses and transactions, recover altcoins sent to bitcoin addresses, sign/verify messages and files with Bitcoin addresses, recover deleted wallets, etc.
|
|
|
piotr_n (OP)
Legendary
Offline
Activity: 2053
Merit: 1356
aka tonikt
|
![](https://bitcointalk.org/Themes/custom1/images/post/xx.gif) |
July 23, 2013, 01:22:45 PM |
|
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 ![Smiley](https://bitcointalk.org/Smileys/default/smiley.gif)
|
Check out gocoin - my original project of full bitcoin node & cold wallet written in Go.PGP fingerprint: AB9E A551 E262 A87A 13BB 9059 1BE7 B545 CDF3 FD0E
|
|
|
jackjack
Legendary
Offline
Activity: 1176
Merit: 1257
May Bitcoin be touched by his Noodly Appendage
|
![](https://bitcointalk.org/Themes/custom1/images/post/xx.gif) |
July 23, 2013, 01:25:57 PM |
|
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?
|
Own address: 19QkqAza7BHFTuoz9N8UQkryP4E9jHo4N3 - Pywallet support: 1AQDfx22pKGgXnUZFL1e4UKos3QqvRzNh5 - Bitcointalk++ script support: 1Pxeccscj1ygseTdSV1qUqQCanp2B2NMM2 Pywallet: instructions. Encrypted wallet support, export/import keys/addresses, backup wallets, export/import CSV data from/into wallet, merge wallets, delete/import addresses and transactions, recover altcoins sent to bitcoin addresses, sign/verify messages and files with Bitcoin addresses, recover deleted wallets, etc.
|
|
|
piotr_n (OP)
Legendary
Offline
Activity: 2053
Merit: 1356
aka tonikt
|
![](https://bitcointalk.org/Themes/custom1/images/post/xx.gif) |
July 23, 2013, 01:28:28 PM Last edit: July 23, 2013, 01:45:13 PM by 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.
|
Check out gocoin - my original project of full bitcoin node & cold wallet written in Go.PGP fingerprint: AB9E A551 E262 A87A 13BB 9059 1BE7 B545 CDF3 FD0E
|
|
|
Mike Hearn
Legendary
Offline
Activity: 1526
Merit: 1129
|
![](https://bitcointalk.org/Themes/custom1/images/post/xx.gif) |
July 23, 2013, 02:00:32 PM |
|
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
Legendary
Offline
Activity: 1596
Merit: 1099
|
![](https://bitcointalk.org/Themes/custom1/images/post/xx.gif) |
July 23, 2013, 02:04:59 PM |
|
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 ![Smiley](https://bitcointalk.org/Smileys/default/smiley.gif) 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.pyCan you spot the problem? ![Smiley](https://bitcointalk.org/Smileys/default/smiley.gif)
|
Jeff Garzik, Bloq CEO, former bitcoin core dev team; opinions are my own. Visit bloq.com / metronome.io Donations / tip jar: 1BrufViLKnSWtuWGkryPsKsxonV2NQ7Tcj
|
|
|
piotr_n (OP)
Legendary
Offline
Activity: 2053
Merit: 1356
aka tonikt
|
![](https://bitcointalk.org/Themes/custom1/images/post/xx.gif) |
July 23, 2013, 02:05:43 PM Last edit: July 23, 2013, 02:20:41 PM by 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 ![Smiley](https://bitcointalk.org/Smileys/default/smiley.gif)
|
Check out gocoin - my original project of full bitcoin node & cold wallet written in Go.PGP fingerprint: AB9E A551 E262 A87A 13BB 9059 1BE7 B545 CDF3 FD0E
|
|
|
piotr_n (OP)
Legendary
Offline
Activity: 2053
Merit: 1356
aka tonikt
|
![](https://bitcointalk.org/Themes/custom1/images/post/xx.gif) |
July 23, 2013, 02:08:13 PM Last edit: July 23, 2013, 02:22:29 PM by 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 ![Smiley](https://bitcointalk.org/Smileys/default/smiley.gif) 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.pyCan you spot the problem? ![Smiley](https://bitcointalk.org/Smileys/default/smiley.gif) 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. ![Smiley](https://bitcointalk.org/Smileys/default/smiley.gif) Then its probably a good strategy, to not make such an info public.
|
Check out gocoin - my original project of full bitcoin node & cold wallet written in Go.PGP fingerprint: AB9E A551 E262 A87A 13BB 9059 1BE7 B545 CDF3 FD0E
|
|
|
piotr_n (OP)
Legendary
Offline
Activity: 2053
Merit: 1356
aka tonikt
|
![](https://bitcointalk.org/Themes/custom1/images/post/xx.gif) |
July 23, 2013, 03:53:20 PM Last edit: July 23, 2013, 04:58:04 PM by 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.
|
Check out gocoin - my original project of full bitcoin node & cold wallet written in Go.PGP fingerprint: AB9E A551 E262 A87A 13BB 9059 1BE7 B545 CDF3 FD0E
|
|
|
jackjack
Legendary
Offline
Activity: 1176
Merit: 1257
May Bitcoin be touched by his Noodly Appendage
|
![](https://bitcointalk.org/Themes/custom1/images/post/xx.gif) |
July 23, 2013, 09:13:57 PM |
|
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
|
Own address: 19QkqAza7BHFTuoz9N8UQkryP4E9jHo4N3 - Pywallet support: 1AQDfx22pKGgXnUZFL1e4UKos3QqvRzNh5 - Bitcointalk++ script support: 1Pxeccscj1ygseTdSV1qUqQCanp2B2NMM2 Pywallet: instructions. Encrypted wallet support, export/import keys/addresses, backup wallets, export/import CSV data from/into wallet, merge wallets, delete/import addresses and transactions, recover altcoins sent to bitcoin addresses, sign/verify messages and files with Bitcoin addresses, recover deleted wallets, etc.
|
|
|
Peter Todd
Legendary
Offline
Activity: 1120
Merit: 1152
|
![](https://bitcointalk.org/Themes/custom1/images/post/xx.gif) |
July 23, 2013, 09:24:55 PM |
|
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-scriptsYou'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 (OP)
Legendary
Offline
Activity: 2053
Merit: 1356
aka tonikt
|
![](https://bitcointalk.org/Themes/custom1/images/post/xx.gif) |
July 24, 2013, 05:17:28 PM Last edit: July 24, 2013, 05:46:42 PM by piotr_n |
|
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 ![Smiley](https://bitcointalk.org/Smileys/default/smiley.gif) BTW, it would be nice to a have a test tx that triggers this 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));
|
Check out gocoin - my original project of full bitcoin node & cold wallet written in Go.PGP fingerprint: AB9E A551 E262 A87A 13BB 9059 1BE7 B545 CDF3 FD0E
|
|
|
Peter Todd
Legendary
Offline
Activity: 1120
Merit: 1152
|
![](https://bitcointalk.org/Themes/custom1/images/post/xx.gif) |
July 24, 2013, 05:53:27 PM |
|
BTW, it would be nice to a have a test tx that triggers this 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 (OP)
Legendary
Offline
Activity: 2053
Merit: 1356
aka tonikt
|
![](https://bitcointalk.org/Themes/custom1/images/post/xx.gif) |
July 24, 2013, 06:01:48 PM |
|
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 ![Wink](https://bitcointalk.org/Smileys/default/wink.gif)
|
Check out gocoin - my original project of full bitcoin node & cold wallet written in Go.PGP fingerprint: AB9E A551 E262 A87A 13BB 9059 1BE7 B545 CDF3 FD0E
|
|
|
genjix
Legendary
Offline
Activity: 1232
Merit: 1076
|
![](https://bitcointalk.org/Themes/custom1/images/post/xx.gif) |
July 24, 2013, 07:13:44 PM |
|
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
Legendary
Offline
Activity: 1120
Merit: 1152
|
![](https://bitcointalk.org/Themes/custom1/images/post/xx.gif) |
July 24, 2013, 07:25:37 PM |
|
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 ![Wink](https://bitcointalk.org/Smileys/default/wink.gif) 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.
|
|
|
|
|