Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Zcoin implementation bug enabled attacker to create over 500K Zcoins

makebitcoingreatagain.wordpress.com

Post reply on HN
Print view View original

61–70 of 223 posts

Re: Zcoin implementation bug enabled attacker to create over 500K Zcoins

#61

Post by gpawl »

WalterBright wrote: post #53
Strom wrote: post #48

Earlier quoted context omitted.

Yes, major improvements could be made by changing the defaults of the tools that beginners use. I edited my original comment to be more clear on this as well.

I think we're going to have to disagree. It isn't just an issue for beginner code. Experts make such mistakes, too. Expecting them to not make mistakes is unrealistic, and sending them back for more training isn't going to work, either. I can't think of a reason any modern language should accept such code. If you're interested, I recommend the series "Air Disasters" on TV. Each episode analyzes one crash, where inves…

I think strom meant that beginners don't know to turn on -Wall -Werror and friends

Re: Zcoin implementation bug enabled attacker to create over 500K Zcoins

#62

Post by WalterBright »

gpawl wrote: post #58
WalterBright wrote: post #42

The error is here: https://github.com/zcoinofficial/zcoin/blob/81a667867b5d8489... and the line of code: zccoinSpend.denomination == libzerocoin::ZQ_LOVELACE; In other words, a statement with no effect. In D, such a line gives an error, not a warning: Error: == has no effect in expression You can force the statement to be accepted by casting it to void. It's really past time for languages to not accept such code any…

How is D holding up against newer upstarts like Go and Rust? It seems that D never really got the love it deserved, perhaps because it didn't have a Google or Mozilla behind it.

Our user base is steadily growing. Coming soon is a nice upgrade to the language designed to eliminate stray pointers into expired stack frames.

I feel it is my duty as a language designer to make it hard for programmers to commit common bugs. It's just too expensive for companies to ship these kinds of things.

Re: Zcoin implementation bug enabled attacker to create over 500K Zcoins

#63

Post by jacquesm »

WalterBright wrote: post #59
jacquesm wrote: post #52

Earlier quoted context omitted.

You're absolutely right (to my surprise), it gives: test.cpp: In function ‘int main()’: test.cpp:13:11: warning: statement has no effect [-Wunused-value] a == b; ^ But only after enabling a warning that is not on by default. -Wall and -Werror should be the default.

It should be an error, not a warning (default or not). There are a number of things like this that should just not be accepted anymore. Here's another one: if (condition); dothis(); No compiler should accept that, regardless of switch settings.

Fully agreed. There is absolutely no way that is something a programmer ever intended. In fact I think that just

    if (condition);
Should already be enough to trigger the error. (Any 'if' statement without a body).

Re: Zcoin implementation bug enabled attacker to create over 500K Zcoins

#64

Post by WalterBright »

gpawl wrote: post #60
WalterBright wrote: post #47

Earlier quoted context omitted.

You're right. My contention is that the language itself should no longer accept such code, it shouldn't be relegated to an ignorable warning or a 3rd party tool that isn't run.

Indeed, the next standard should ban such things, and modern compilers should reject these patterns by default, unless a --legacy_security_unsage flag is set.

Perhaps the switch should be named:

   --accept_my_buggy_stupid_code
so nobody is going to type that in by accident, and nobody is going to want to defend it in the code review process. And the compiler vendor will still get a gold star for being 100% language compliant :-)

Re: Zcoin implementation bug enabled attacker to create over 500K Zcoins

#66

Post by electrum »

WalterBright wrote: post #42

The error is here: https://github.com/zcoinofficial/zcoin/blob/81a667867b5d8489... and the line of code: zccoinSpend.denomination == libzerocoin::ZQ_LOVELACE; In other words, a statement with no effect. In D, such a line gives an error, not a warning: Error: == has no effect in expression You can force the statement to be accepted by casting it to void. It's really past time for languages to not accept such code any…

The commit that fixes it does not change any tests:

https://github.com/zcoinofficial/zcoin/commit/0359bcb2ead7fe...

This indicates that the code in question is not covered by tests. I work on a database, for which correctness is paramount, so it's unnerving to see any code fixes not associated with test additions or changes. I would have thought that cryptocurrency had similar standards.

Re: Zcoin implementation bug enabled attacker to create over 500K Zcoins

#67

Post by pilif »

jacquesm wrote: post #52
WalterBright wrote: post #42

The error is here: https://github.com/zcoinofficial/zcoin/blob/81a667867b5d8489... and the line of code: zccoinSpend.denomination == libzerocoin::ZQ_LOVELACE; In other words, a statement with no effect. In D, such a line gives an error, not a warning: Error: == has no effect in expression You can force the statement to be accepted by casting it to void. It's really past time for languages to not accept such code any…

You're absolutely right (to my surprise), it gives: test.cpp: In function ‘int main()’: test.cpp:13:11: warning: statement has no effect [-Wunused-value] a == b; ^ But only after enabling a warning that is not on by default. -Wall and -Werror should be the default.

-Wall sure (also, it should really enable all warnings, just as the name implies. Currently it doesn't)

-Werror, I'm not sure. During development and on CI systems it makes sense. But if you intend to ship source code, it's probably best left off. Other people might be using different compilers that emit warnings for things yours didn't. You don't want the build to fail for those people.

Re: Zcoin implementation bug enabled attacker to create over 500K Zcoins

#68

Post by JoachimSchipper »

WalterBright wrote: post #42

The error is here: https://github.com/zcoinofficial/zcoin/blob/81a667867b5d8489... and the line of code: zccoinSpend.denomination == libzerocoin::ZQ_LOVELACE; In other words, a statement with no effect. In D, such a line gives an error, not a warning: Error: == has no effect in expression You can force the statement to be accepted by casting it to void. It's really past time for languages to not accept such code any…

gcc doesn't accept that if you enable some warnings and -Werror. I agree that the default is bad, though.

Re: Zcoin implementation bug enabled attacker to create over 500K Zcoins

#69

Post by prance »

I'm not too familiar with cryptocurrency or generally blockchain implementations, so I wonder: shouldn't it be normal that at least the reference clients of any such protocol should be proven correct, and follow a rigorous scheme accepting contributions?

I mean, all the mathematical rigor and proofs of the underlying theory and protocols are basically useless if the rigor isn't carried over to at least the reference implementations. (If people choose to use alternative, non-proven clients, fine, that's their own decision and risk.)

Re: Zcoin implementation bug enabled attacker to create over 500K Zcoins

#70

Post by WalterBright »

electrum wrote: post #66
WalterBright wrote: post #42

The error is here: https://github.com/zcoinofficial/zcoin/blob/81a667867b5d8489... and the line of code: zccoinSpend.denomination == libzerocoin::ZQ_LOVELACE; In other words, a statement with no effect. In D, such a line gives an error, not a warning: Error: == has no effect in expression You can force the statement to be accepted by casting it to void. It's really past time for languages to not accept such code any…

The commit that fixes it does not change any tests: https://github.com/zcoinofficial/zcoin/commit/0359bcb2ead7fe... This indicates that the code in question is not covered by tests. I work on a database, for which correctness is paramount, so it's unnerving to see any code fixes not associated with test additions or changes. I would have thought that cryptocurrency had similar standards.

We've modified the dlang Github tester to run coverage tests over Pull Requests, and uncovered changes get marked with a scarlet bar. While some changes can't be tested with a test suite, the result has been a marked improvement in the quality of pull requests (including mine) because it is embarrassing to submit untested code.
Post reply on HN