Nxt source code flaw reports - page 62.

Come-from-Beyond

legendary

Activity: 2142

Merit: 1009

Newbie

Quote from: ?? on ??

Code:

Account senderAccount = accounts.get(Account.getId(transaction.senderPublicKey));
							synchronized (senderAccount) {
								
								senderAccount.setBalance(senderAccount.balance + (transaction.amount + transaction.fee) * 100L);
								
							}
							
							Account recipientAccount = accounts.get(transaction.recipient);
							synchronized (recipientAccount) {
								
								recipientAccount.setBalance(recipientAccount.balance - transaction.amount * 100L);
								recipientAccount.setUnconfirmedBalance(recipientAccount.unconfirmedBalance - transaction.amount * 100L);
								
							}

Shouldn't the + and - be the other way around?

Shouldn't the sender's account balance decrease?

Shouldn't the recipient's account balance increase?

No. This code works when last block becomes orphaned.

GröBkAz

hero member

Activity: 854

Merit: 500

It seem to be a personal crusade of FrictionlessCoin against nxt Grin

xibeijan

legendary

Activity: 1232

Merit: 1001

Quote from: FrictionlessCoin on January 03, 2014, 04:26:03 PM

Quote from: xibeijan on January 03, 2014, 04:22:30 PM

Quote from: FrictionlessCoin on January 03, 2014, 02:36:51 PM

Talk about amateur hour!!

So let's get this right, the source code is released and we find it all in a single .java file.

One class with a lot of inner classes.

Does the developer ever know what a java package is for?

Then he declares all the member variables of the Nxt class as static. Does he know the difference between static and instance variable?

To place matters worse, the Nxt class happens to be a servlet. Does he not know that a new servlet is instantiated per thread??!

Well, to be honest, 21 BTC original invested may have been too generous!

You are nitpicking over insignificant details.

This is not a beauty contest. NXT's beauty is in the algorithms and maths that have brought forward the state-of-the-art. I seriously doubt the creator cares about keeping tidy with the code. In fact, I know quite a few genius computer scientists who produce really ugly, stupid code. However, that doesn't matter one iota when the algorithms are groundbreakingly better than anything that's come before.

Dude... where are the unit tests? If you don't care for style, you should care that it is at least tested.

This code is so bad, that I'm just itching to fork it!

It has obviously undergone significant testing to get where it has, but whether unit tests have been included in the source you see is another question.

As I said, some really great computer scientists do not follow great software engineering principles, but their code does things no one else's can. And that, my friend, is the point.

Go ahead fork the code, but I doubt people will invest in cleaner prettier code. Our investment is in the extremely clever people behind the NXT project. Computer science and software engineering, though related, are different things. I am glad the NXT has a computer scientist like BCNext behind it who actually has new ideas and can bring them to life, no matter how messy the code may be. New capabilities are something that has been missing from every other alt coin that has come before NXT.

Jean-Luc is obviously an experienced software engineer and will be improving the software engineering practices over the coming days. The priority, however, is in the core functionality and keeping up with the market. This my interpretation, as I'm not a NXT project member.

I think the NXT developers are doing a fantastic job!

FrictionlessCoin

legendary

Activity: 868

Merit: 1000

Cryptotalk.org - Get paid for every post!

Quote from: xibeijan on January 03, 2014, 04:22:30 PM

Quote from: FrictionlessCoin on January 03, 2014, 02:36:51 PM

Talk about amateur hour!!

So let's get this right, the source code is released and we find it all in a single .java file.

One class with a lot of inner classes.

Does the developer ever know what a java package is for?

Then he declares all the member variables of the Nxt class as static. Does he know the difference between static and instance variable?

To place matters worse, the Nxt class happens to be a servlet. Does he not know that a new servlet is instantiated per thread??!

Well, to be honest, 21 BTC original invested may have been too generous!

You are nitpicking over insignificant details.

This is not a beauty contest. NXT's beauty is in the algorithms and maths that have brought forward the state-of-the-art. I seriously doubt the creator cares about keeping tidy with the code. In fact, I know quite a few genius computer scientists who produce really ugly, stupid code. However, that doesn't matter one iota when the algorithms are groundbreakingly better than anything that's come before.

Dude... where are the unit tests? If you don't care for style, you should care that it is at least tested.

This code is so bad, that I'm just itching to fork it!

xibeijan

legendary

Activity: 1232

Merit: 1001

Quote from: FrictionlessCoin on January 03, 2014, 02:36:51 PM

Talk about amateur hour!!

So let's get this right, the source code is released and we find it all in a single .java file.

One class with a lot of inner classes.

Does the developer ever know what a java package is for?

Then he declares all the member variables of the Nxt class as static. Does he know the difference between static and instance variable?

To place matters worse, the Nxt class happens to be a servlet. Does he not know that a new servlet is instantiated per thread??!

Well, to be honest, 21 BTC original invested may have been too generous!

You are nitpicking over insignificant details.

This is not a beauty contest. NXT's beauty is in the algorithms and maths that have brought forward the state-of-the-art. I seriously doubt the creator cares about keeping tidy with the code. In fact, I know quite a few genius computer scientists who produce really ugly, stupid code. However, that doesn't matter one iota when the algorithms are groundbreakingly better than anything that's come before.

Come-from-Beyond

legendary

Activity: 2142

Merit: 1009

Newbie

Quote from: singula on January 03, 2014, 03:10:57 PM

Quote

         int amount = 0;
         for (long transactionId : Block.getLastBlock().transactions) {

            Transaction transaction = transactions.get(transactionId);
            if (transaction.recipient == id) {

               amount += transaction.amount;

            }

         }

         return (int)(balance / 100) - amount;

Wait ... shouldn't the last line be "return (int)(balance / 100) + amount;" ?

How it is written now, any transactions incoming to your account will decrease the balance instead of increasing it ....

The logic is:

If this account at least 1440 blocks old then its effective balance is current balance minus money received in the last block.

Come-from-Beyond

legendary

Activity: 2142

Merit: 1009

Newbie

Quote from: Malooka on January 03, 2014, 03:03:49 PM

Thrilled that my small investment in NXT is being represented by someone who uses "u" instead of "you."

Undecided

Why do ppl always think I represent Nxt...

Come-from-Beyond

legendary

Activity: 2142

Merit: 1009

Newbie

Quote from: BloodyRookie on January 03, 2014, 03:03:43 PM

I somehow doubt it is the original 0.4.7e version minus the alias/Colorcoins stuff:

In original 0.4.7e:

Code:

boolean validateAttachment()
{
    if ((this.amount > 1000000000) || (this.fee > 1000000000)) {
      return false;
 }

In 0.4.8:

Code:

boolean validateAttachment()
{
	if (this.fee > 1000000000){
		return false;
	}

Now the code release by Jean-Luc is the same as 0.4.8.
If he really had taken 0.4.7e it would still look like 0.4.7e.

Maybe u r close to an injected flaw? Wink

singula

sr. member

Activity: 462

Merit: 251

Quote

         int amount = 0;
         for (long transactionId : Block.getLastBlock().transactions) {

            Transaction transaction = transactions.get(transactionId);
            if (transaction.recipient == id) {

               amount += transaction.amount;

            }

         }

         return (int)(balance / 100) - amount;

Wait ... shouldn't the last line be "return (int)(balance / 100) + amount;" ?

How it is written now, any transactions incoming to your account will decrease the balance instead of increasing it ....

mr_random

legendary

Activity: 1344

Merit: 1001

Quote from: FrictionlessCoin on January 03, 2014, 03:02:48 PM

Quote from: Come-from-Beyond on January 03, 2014, 02:55:07 PM

Quote from: FrictionlessCoin on January 03, 2014, 02:51:07 PM

Man, the B.S. is unbelievable.

Well, I think u shouldn't bother with talking to me anymore, unless someone quotes ur post. Welcome to my collection of trolls.

You have a fiduciary duty to tell every one who invested in this system.

(1) How much experience you have developing Java programs.
(2) How many hours you spent writing this code and testing it.

If you think what you are doing is legal, then you have a lot to learn.

Cool story bro. Go back to Bitcoin if you don't like it.

Malooka

copper member

Activity: 289

Merit: 254

Thrilled that my small investment in NXT is being represented by someone who uses "u" instead of "you."

Undecided

BloodyRookie

hero member

Activity: 687

Merit: 500

I somehow doubt it is the original 0.4.7e version minus the alias/Colorcoins stuff:

In original 0.4.7e:

Code:

boolean validateAttachment()
{
    if ((this.amount > 1000000000) || (this.fee > 1000000000)) {
      return false;
 }

In 0.4.8:

Code:

boolean validateAttachment()
{
	if (this.fee > 1000000000){
		return false;
	}

Now the code release by Jean-Luc is the same as 0.4.8.
If he really had taken 0.4.7e it would still look like 0.4.7e.

FrictionlessCoin

legendary

Activity: 868

Merit: 1000

Cryptotalk.org - Get paid for every post!

Quote from: Come-from-Beyond on January 03, 2014, 02:55:07 PM

Quote from: FrictionlessCoin on January 03, 2014, 02:51:07 PM

Man, the B.S. is unbelievable.

Well, I think u shouldn't bother with talking to me anymore, unless someone quotes ur post. Welcome to my collection of trolls.

You have a fiduciary duty to tell every one who invested in this system.

(1) How much experience you have developing Java programs.
(2) How many hours you spent writing this code and testing it.

If you think what you are doing is legal, then you have a lot to learn.

Come-from-Beyond

legendary

Activity: 2142

Merit: 1009

Newbie

Quote from: ricot on January 03, 2014, 02:54:51 PM

First question:
getEffectiveBalance returns 0 if Block.getLastBlock().height - height < 1440.
If I understood that correctly, it means that a new account will return 0 until the first transfer to it was 1440 blocks ago? Why???

To prevent cheating with block generation chances.

Quote from: ricot on January 03, 2014, 02:54:51 PM

FYI: Transactions and Blocks only contain full NXT, no cents, so the Wiki entry about granularity of transactions is wrong.

Cents r used for Asset Exchange only.

FrictionlessCoin

legendary

Activity: 868

Merit: 1000

Cryptotalk.org - Get paid for every post!

Quote from: ricot on January 03, 2014, 02:54:51 PM

I'm a bit disappointed with that source...
Mixing the Servlet with the block generation logic, all with inner classes that act on some static class variables, no capsulation, no comments, NO TESTS!?!?
Amounts in NXT and "Cents" are used interchangeably everywhere...

But anyways, it's early days and I already invested some BTC in the Coin...

First question:
getEffectiveBalance returns 0 if Block.getLastBlock().height - height < 1440.
If I understood that correctly, it means that a new account will return 0 until the first transfer to it was 1440 blocks ago? Why???

FYI: Transactions and Blocks only contain full NXT, no cents, so the Wiki entry about granularity of transactions is wrong.

Disappointed would be an understatement.

A servlet... with all static member variables.

Inner classes with most all member functions defined as static.

Throwing of the Exception class (not something inherited from it).

This is amateur hour.

There's no magic here... its just amateurish stuff.

Don't wait for other folks to discover the extent of incompetence here, just leave it.

BloodyRookie

hero member

Activity: 687

Merit: 500

Quote from: ricot on January 03, 2014, 02:54:51 PM

First question:
getEffectiveBalance returns 0 if Block.getLastBlock().height - height < 1440.
If I understood that correctly, it means that a new account will return 0 until the first transfer to it was 1440 blocks ago? Why???

You need to wait 1 day before you can forge.

Come-from-Beyond

legendary

Activity: 2142

Merit: 1009

Newbie

Quote from: black_swan on January 03, 2014, 02:51:39 PM

Quote

3350: if (account.unconfirmedBalance < amount * 100L) {

Why only account.unconfirmedBalance is checked instead of ~~(account.unconfirmedBalance + account.balance) ?~~

Edit: Actually only balance should be checked

If only balance is checked then an adversary can spam the network with 1000s of double-spending transactions that will never be confirmed.

Come-from-Beyond

legendary

Activity: 2142

Merit: 1009

Newbie

Quote from: FrictionlessCoin on January 03, 2014, 02:51:07 PM

Man, the B.S. is unbelievable.

Well, I think u shouldn't bother with talking to me anymore, unless someone quotes ur post. Welcome to my collection of trolls.

ricot

newbie

Activity: 56

Merit: 0

I'm a bit disappointed with that source...
Mixing the Servlet with the block generation logic, all with inner classes that act on some static class variables, no capsulation, no comments, NO TESTS!?!?
Amounts in NXT and "Cents" are used interchangeably everywhere...

But anyways, it's early days and I already invested some BTC in the Coin...

First question:
getEffectiveBalance returns 0 if Block.getLastBlock().height - height < 1440.
If I understood that correctly, it means that a new account will return 0 until the first transfer to it was 1440 blocks ago? Why???

FYI: Transactions and Blocks only contain full NXT, no cents, so the Wiki entry about granularity of transactions is wrong.

black_swan

newbie

Activity: 42

Merit: 0

Quote

3350: if (account.unconfirmedBalance < amount * 100L) {

Why only account.unconfirmedBalance is checked instead of ~~(account.unconfirmedBalance + account.balance) ?~~

Edit: Actually only balance should be checked

Topic: Nxt source code flaw reports - page 62. (Read 113359 times)