Nxt source code flaw reports

[black_swan]

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

[1715064454]

1715064454

[1715064454]

1715064454

[1715064454]

1715064454

[ricot]

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.

[Come-from-Beyond]

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.

[Come-from-Beyond]

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.

[BloodyRookie]

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.

[FrictionlessCoin]

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.

[Come-from-Beyond]

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.

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]

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.

[BloodyRookie]

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.

[Malooka]

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

 

[mr_random]

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.

[singula]

         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 ....

[Come-from-Beyond]

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?

[Come-from-Beyond]

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

 

Why do ppl always think I represent Nxt...

[Come-from-Beyond]

         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.

[xibeijan]

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.

[FrictionlessCoin]

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]

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!

[GröBkAz]

It seem to be a personal crusade of FrictionlessCoin against nxt 

[Come-from-Beyond]

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.