Nxt source code analysis (QA)

[Jean-Luc]

All of those synchronized(this) can be replaced with AtomicInteger.addAndGet

How about this:

Code:

            synchronized (this) {
                this.balance += amount;
                this.unconfirmedBalance += amount;
            }

Each of them being Atomic by itself will not ensure balance and unconfirmedBalance are both updated atomically, a thread could still see balance incremented but unconfirmedBalance not yet updated.

[Jean-Luc]

There was a lot of code that looked like:

synchronized (account) {
   1. Check balance
   2. Do something
   3. Adjust balance
}

Is it safe to get rid of synchronized in the quoted code?

It is not, and I haven't.

Code:

			synchronized (account) {
								
				if (account.getUnconfirmedBalance() < amount * 100L) {
									
						doubleSpendingTransaction = true;
									
				} else {
									
						doubleSpendingTransaction = false;
									
						account.addToUnconfirmedBalance(- amount * 100L);
						
...

[jl777]

For the people who don't code, this is essentially, "if you see some kind of error, just ignore it."

Actually not. This is a paradigm heavily used in Erlang. If u tried to recover after an exception or even logged it, u would just consume resources without real profit.

No,  it's more like,  if I see some kind of error,  let me keep quiet about it!

Can you explain how that would effect the actual functionality of the system.  Isn't it the same as someone submitting improperly signed transactions so those should be ignored?  You keep bringing up issues with the code but without actual explanations of how those issues are supposed to screw things up.

It's in over 18 places in the code.

Anyway,  it is bad practice because if an unexpected error happens (which it always does), you do want to know that something happened.   To assume that you can ignore this because you think you know what you are doing is just bad practice.

Folks,  I don't need to explain to you guys who to write code properly!

Do you understand what beta release means? NXT just made first beta release with released source code. You are clearly the worlds best java programmer (no offense jean-luc) and due to your fluency in all things java you are able to see logic flaws much clearer than C programmers like me.

Why not show off how smart you really are, just post some actual logic errors that lead to people losing money? My guess is that you tried to and couldn't find any, so you decided to make a copy of it and call it NGC.

Prove me wrong. Prove that you are able to actually find flaws. That is what this thread is about. Notice how in spite of your almost total uselessness to this thread, you haven't been banned. Most people haven't even ignored you based on the hope that you will use your talents to help. If NXT is the sum of all people who contribute to NXT and you contribute too, how is that a ponzi scheme?

Isn't a ponzi scheme something that has no intrinsic value. Are you saying that NXT has no intrinsic value? Are you saying that NXT doesn't work at all? Are you saying that a refactoring can't fix critical problems that you haven't even identified yet?

What exactly are the flaws in NXT logic. Treat it as a whitepaper not a final release by perfect software engineering team staffed by clones of yourself. NXT is in BETA TEST. Not very smart to criticize things that are accepted during beta tests. I thought you were a smart guy, maybe that makes me stupid?

James

[Jaguar0625]

All of those synchronized(this) can be replaced with AtomicInteger.addAndGet

There was a lot of code that looked like:

synchronized (account) {
   1. Check balance
   2. Do something
   3. Adjust balance
}

Is it safe to get rid of synchronized in the quoted code?

I was specifically referring to code like this:

Code:

synchronized (this) {
    this.balance += amount;
}

If you just need to check the balance and conditionally update it, you can use compareAndSet.

For anything more complicated, you're better off sticking with synchronized.

[Jaguar0625]

All of those synchronized(this) can be replaced with AtomicInteger.addAndGet

How about this:

Code:

            synchronized (this) {
                this.balance += amount;
                this.unconfirmedBalance += amount;
            }

Each of them being Atomic by itself will not ensure balance and unconfirmedBalance are both updated atomically, a thread could still see balance incremented but unconfirmedBalance not yet updated.

It depends. You're correct that a thread could see balance updated and unconfirmedBalance not updated. But, that exists in the code that was released since there are places where balance and unconfirmedBalance are read outside of locks on the account, e.g.:

Code:

						if (accumulatedAmount + amount <= accounts.get(sender).balance && transaction.validateAttachment()) {
							
							accumulatedAmounts.put(sender, accumulatedAmount + amount);
							
							newTransactions.put(transaction.getId(), transaction);
							payloadLength += transactionLength;
							
						}

If you need to ensure they are always consistent, you need to lock around all the reads and writes.

[Jaguar0625]

FWIW, it's kind of annoying to be reviewing outdated code while the developers are working on a much newer copy. I understand why an older snapshot has been released, but it is still annoying to keep finding things that have already been fixed :/. But, I guess it's good that the working code is in a better state than the released code.

[Come-from-Beyond]

Code:

						if (accumulatedAmount + amount <= accounts.get(sender).balance && transaction.validateAttachment()) {
							
							accumulatedAmounts.put(sender, accumulatedAmount + amount);
							
							newTransactions.put(transaction.getId(), transaction);
							payloadLength += transactionLength;
							
						}

Outcome of this code is validated by the peers. Usage of synchronized here would be an overkill in a system with eventual consistency...

[FrictionlessCoin]

Why not show off how smart you really are, just post some actual logic errors that lead to people losing money? My guess is that you tried to and couldn't find any, so you decided to make a copy of it and call it NGC.

That indeed is an interesting idea. 

So I wait till every gets fixed and mature.

Then I reboot NXT.

How do you like that plan?

[Jean-Luc]

If you need to ensure they are always consistent, you need to lock around all the reads and writes.

I know, I have done that. All direct reads of Account.balance are replaced with getBalance() which is synchronized. Account.balance is now private (doesn't make a difference now when in the same class, but it will be moved out of Nxt.java one day).

[Jean-Luc]

FWIW, it's kind of annoying to be reviewing outdated code while the developers are working on a much newer copy. I understand why an older snapshot has been released, but it is still annoying to keep finding things that have already been fixed :/. But, I guess it's good that the working code is in a better state than the released code.

Thanks for your reviews, it could be that you find things I haven't thought of yet. In fact you did in a previous post, now I have added extra checks for Transaction.amount and Transaction.fee in a place where it could overflow. The code is not that outdated actually, it is as of Dec 25 when I first got access to it and we were running it until about Jan 01.

[utopianfuture]

Why not show off how smart you really are, just post some actual logic errors that lead to people losing money? My guess is that you tried to and couldn't find any, so you decided to make a copy of it and call it NGC.

That indeed is an interesting idea.  

So I wait till every gets fixed and mature.

Then I reboot NXT.

How do you like that plan?

LOL at your naive sense of how to build a successful business. Even if you can replicate even a technically better version of NXT in 6 months, the best you can do is to create something akin to Quark or Doge compared to Bitcoin. Even that would be unlikely with current situation. Every decent coder could replicate Twitter in a week still Twitter is worth billion of $ with none of competitor in that space.
In 6 months, I couldn't care less that NXT's codes are 100 percent open. Imitation is the best praise someone said.

[jl777]

Why not show off how smart you really are, just post some actual logic errors that lead to people losing money? My guess is that you tried to and couldn't find any, so you decided to make a copy of it and call it NGC.

That indeed is an interesting idea. 

So I wait till every gets fixed and mature.

Then I reboot NXT.

How do you like that plan?

You already said you were going to copy NXT and call it NGC. That is the reason you are here, hoping to find some tidbits that will help your NXT clone.

Good luck with rebooting NXT as when everything is fixed and mature it will have LTC marketcap, while your new coin will have you and your followers.

If you are not going to contribute when you can, then lurk, steal and clone NXT. Whatever. I was hoping you had some pride in your skills. I guess if you don't have skills you can't have pride in them.

Good luck with your plan, if you are going to post, please try to post an actual bug however trivial. Please show some respect to those of us who are trying to improve NXT. I somehow get the feeling that you are able to, but choose not to as you view NXT as competition to your own clone. Why not join the original instead of trying to clone?

James

[FrictionlessCoin]

Why not show off how smart you really are, just post some actual logic errors that lead to people losing money? My guess is that you tried to and couldn't find any, so you decided to make a copy of it and call it NGC.

That indeed is an interesting idea. 

So I wait till every gets fixed and mature.

Then I reboot NXT.

How do you like that plan?

You already said you were going to copy NXT and call it NGC. That is the reason you are here, hoping to find some tidbits that will help your NXT clone.

Not a copy,  a better version.

As it is NXT holders a losing money left and right.  Wallets getting hacked, DGEX not returning coins, negative balances etc.   

 

[MrBTC]

wow this is very interesting.
thanks for sharing,keep updating guys.

[salsacz]

at least we finally know who is that troll panama01 from nextcoin forum

[jl777]

Why not show off how smart you really are, just post some actual logic errors that lead to people losing money? My guess is that you tried to and couldn't find any, so you decided to make a copy of it and call it NGC.

That indeed is an interesting idea. 

So I wait till every gets fixed and mature.

Then I reboot NXT.

How do you like that plan?

You already said you were going to copy NXT and call it NGC. That is the reason you are here, hoping to find some tidbits that will help your NXT clone.

Not a copy,  a better version.

As it is NXT holders a losing money left and right.  Wallets getting hacked, DGEX not returning coins, negative balances etc.   

 

OK, so you admit you are here just trying to figure out how to make a NXT copycat and have no intention of helping. Thanks for confirming my hunch. I am not sure what the right name for someone like you is. I won't miss anything valuable by ignoring you.

You do know the next generation enterprise got totally destroyed. May the force be with you.

James

[FrictionlessCoin]

OK, so you admit you are here just trying to figure out how to make a NXT copycat and have no intention of helping. Thanks for confirming my hunch. I am not sure what the right name for someone like you is. I won't miss anything valuable by ignoring you.

You do know the next generation enterprise got totally destroyed. May the force be with you.

James

Absolutely nothing wrong with a copy, if you can improve on the original.

However,  I am not even sure that there's anything worth copying here.

I haven't seen the spec.  and have no idea that it even works.

[Meizirkki]

Folks,  I don't need to explain to you guys who to write code properly!

Yea, please don't. GTFO.

[User705]

Why not show off how smart you really are, just post some actual logic errors that lead to people losing money? My guess is that you tried to and couldn't find any, so you decided to make a copy of it and call it NGC.

That indeed is an interesting idea. 

So I wait till every gets fixed and mature.

Then I reboot NXT.

How do you like that plan?

You already said you were going to copy NXT and call it NGC. That is the reason you are here, hoping to find some tidbits that will help your NXT clone.

Not a copy,  a better version.

As it is NXT holders a losing money left and right.  Wallets getting hacked, DGEX not returning coins, negative balances etc.   

 

What does picking a weak password or someone not returning coins have to do with bad coding?  I guess bitcoin sucks too since so many bitcoins were never returned and so many wallets keep getting hacked even now five years after bitcoin launch.

[User705]

Only a few GPUs.  Let's do a practical test.  Post or PM the instructions.  How many long should the attempt be?  Hours, days?  If it works then you'll have succeeded in proving Nxt to be worthless.