Nxt source code analysis (QA)

[Come-from-Beyond]

There does seem to be a lot of excessive locking. For example:

Code:

Transaction transaction = Transaction.getTransaction(buffer);
synchronized (Nxt.transactions) {						
    transaction.index = ++transactionCounter;						
}

Since all you are really doing is setting a transaction number, you should be using AtomicInteger. This would eliminate the lock completely

But, in general, you should consider minimizing the scopes of your locks. For example, here, you're locking the entire transactions array to update a single transaction. It would be better to lock around that specific transaction so as not to block other pieces of code attempting to inspect / edit other transactions.

Also, there are a number of places where reader-writer locks are more appropriate than using synchronized (e.g. when dealing users, which should change relatively infrequently).

This extra locking can help scalability down the road.

Valid points, but there was a special rule set - code must be readable by a novice programmer. Concurrent API is for more experienced programmers. I used Concurrent API in my code a couple of times but I was trying to follow the rule.

[FrictionlessCoin]

My favorite snippet of code:

Code:

catch (Exception e) { }

More than 18 times in the code.

Reminds me of all these coin holders who have their head in the sand.

[FrictionlessCoin]

There does seem to be a lot of excessive locking. For example:

Code:

Transaction transaction = Transaction.getTransaction(buffer);
synchronized (Nxt.transactions) {						
    transaction.index = ++transactionCounter;						
}

Since all you are really doing is setting a transaction number, you should be using AtomicInteger. This would eliminate the lock completely

But, in general, you should consider minimizing the scopes of your locks. For example, here, you're locking the entire transactions array to update a single transaction. It would be better to lock around that specific transaction so as not to block other pieces of code attempting to inspect / edit other transactions.

Also, there are a number of places where reader-writer locks are more appropriate than using synchronized (e.g. when dealing users, which should change relatively infrequently).

This extra locking can help scalability down the road.

Valid points, but there was a special rule set - code must be readable by a novice programmer. Concurrent API is for more experienced programmers. I used Concurrent API in my code a couple of times but I was trying to follow the rule.

Maybe you should rephrase that "code must be writable by a novice programmer who has never understood Java concurrent api."

So you're saying you made up a rule to hide your ignorance.  "Thou shall not use concurrent API because I don't understand it"

[tacotime]

My favorite snippet of code:

Code:

catch (Exception e) { }

More than 18 times in the code.

Reminds me of all these coin holders who have their head in the sand.

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

[Come-from-Beyond]

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.

[FrictionlessCoin]

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!

[User705]

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.

[lwilliams777]

@Frictionless.  I see that you know what you're talking about with regard to programming.  In fact, you may very well be a brilliant programmer, but the problem is, no one cares.  Why?  Because you have taken it upon yourself as a right to criticize but have not contributed anything productive.  You haven't earned that right yet.  In order to earn the right to be heard you should contribute, even in small amount, not with criticism, but with real productive help.  Once you do that then you've earned the right to criticize and others will actually listen to you.  As it stands, people just think you are a jerk, and using Simon Crowell, a known jerk, as an example doesn't help your cause.  Simon Crowell may be rich and be a star power broker, but I believe he has very few "real" deep long lasting friendships, if any.  

You have not gone about this in an effective way.  If you had played this right you would have contributed in a huge way and others would have sung your praises and others would have gladly signed up for your next generation plan.  In fact they would have become your best sales people and you would have had more folks sign up to help with your development and vision.  But as it stands there are only a few willing to help you and your chances of developing a great system are slim to none.

[tacotime]

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 depends where you put it... it's dangerous in most places though because it doesn't class or report the exception and prevent you from seeing any other bugs that might arise while a function is executing.  Basically: "Regardless of the error that arises, keep running and do not report it."

http://stackoverflow.com/questions/2416316/why-is-the-catchexception-almost-always-a-bad-idea

It's what I write when I'm in a rush and don't really care when I get some kind of error so long as the program does more or less what I want it to; ideally you would address it properly, but if you're writing something like a program for something non-critical it doesn't really matter.

[Jean-Luc]

Since setBalance can throw, where you do the following, balance and unconfirmed balance can get out of sync:

Code:

generatorAccount.setBalance(generatorAccount.balance + totalFee * 100L);
generatorAccount.setUnconfirmedBalance(generatorAccount.unconfirmedBalance + totalFee * 100L);

Code:

void setBalance(long balance) throws Exception {
    this.balance = balance;
    for (Peer peer : peers.values()) {
    if (peer.accountId == id && peer.adjustedWeight > 0) {
        peer.updateWeight();
    }
}

Granted, this is unlikely since you are suppressing all exceptions around the network calls. But it might be more robust to calculate balance from unconfirmedbalance or vice-versa.

There is no setBalance() in 0.5.0:

Code:

        synchronized long getBalance() {
            return balance;
        }

        synchronized long getUnconfirmedBalance() {
            return unconfirmedBalance;
        }

        void addToBalance(long amount) throws Exception {

            synchronized (this) {

                this.balance += amount;

            }

            updatePeerWeights();

        }
        
        void addToUnconfirmedBalance(long amount) throws Exception {

            synchronized (this) {

                this.unconfirmedBalance += amount;

            }

            updateUserUnconfirmedBalance();

        }

        void addToBalanceAndUnconfirmedBalance(long amount) throws Exception {

            synchronized (this) {

                this.balance += amount;
                this.unconfirmedBalance += amount;

            }

            updatePeerWeights();
            updateUserUnconfirmedBalance();

        }

[FrictionlessCoin]

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!

[FrictionlessCoin]

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 depends where you put it... it's dangerous in most places though because it doesn't class or report the exception and prevent you from seeing any other bugs that might arise while a function is executing.  Basically: "Regardless of the error that arises, keep running and do not report it."

http://stackoverflow.com/questions/2416316/why-is-the-catchexception-almost-always-a-bad-idea

It's what I write when I'm in a rush and don't really care when I get some kind of error so long as the program does more or less what I want it to; ideally you would address it properly, but if you're writing something like a program for something non-critical it doesn't really matter.

Yes, maybe NXT handling money is a non-critical application.

[Jean-Luc]

There does seem to be a lot of excessive locking. For example:

Code:

Transaction transaction = Transaction.getTransaction(buffer);
synchronized (Nxt.transactions) {						
    transaction.index = ++transactionCounter;						
}

In 0.5.0 this looks like:

Code:

Transaction transaction = Transaction.getTransaction(buffer);
transaction.index = transactionCounter.incrementAndGet();

I am afraid CfB and BCNext may get mad at me, but 0.5.0 relies heavily on the Concurrent API...

[FrictionlessCoin]

Code:

        synchronized long getBalance() {
            return balance;
        }

        synchronized long getUnconfirmedBalance() {
            return unconfirmedBalance;
        }

        void addToBalance(long amount) throws Exception {

            synchronized (this) {

                this.balance += amount;

            }

            updatePeerWeights();

        }
        
        void addToUnconfirmedBalance(long amount) throws Exception {

            synchronized (this) {

                this.unconfirmedBalance += amount;

            }

            updateUserUnconfirmedBalance();

        }

        void addToBalanceAndUnconfirmedBalance(long amount) throws Exception {

            synchronized (this) {

                this.balance += amount;
                this.unconfirmedBalance += amount;

            }

            updatePeerWeights();
            updateUserUnconfirmedBalance();

        }

sigh... synchronized (this) ... how ignorant can you get!

[FrictionlessCoin]

There does seem to be a lot of excessive locking. For example:

Code:

Transaction transaction = Transaction.getTransaction(buffer);
synchronized (Nxt.transactions) {						
    transaction.index = ++transactionCounter;						
}

In 0.5.0 this looks like:

Code:

Transaction transaction = Transaction.getTransaction(buffer);
transaction.index = transactionCounter.incrementAndGet();

I am afraid CfB and BCNext may get mad at me, but 0.5.0 relies heavily on the Concurrent API...

in your dreams.

[FrictionlessCoin]

@Frictionless.  I see that you know what you're talking about with regard to programming.  In fact, you may very well be a brilliant programmer, but the problem is, no one cares.  Why?  Because you have taken it upon yourself as a right to criticize but have not contributed anything productive.  You haven't earned that right yet.  In order to earn the right to be heard you should contribute, even in small amount, not with criticism, but with real productive help.  Once you do that then you've earned the right to criticize and others will actually listen to you.  As it stands, people just think you are a jerk, and using Simon Crowell, a known jerk, as an example doesn't help your cause.  Simon Crowell may be rich and be a star power broker, but I believe he has very few "real" deep long lasting friendships, if any.  

You have not gone about this in an effective way.  If you had played this right you would have contributed in a huge way and others would have sung your praises and others would have gladly signed up for your next generation plan.  In fact they would have become your best sales people and you would have had more folks sign up to help with your development and vision.  But as it stands there are only a few willing to help you and your chances of developing a great system are slim to none.

Why would I contribute to what obviously looks like a Ponzi scheme?

You all first explain to me why this entire shenanigan is any different from a Ponzi scheme?

[Jaguar0625]

Code:

        synchronized long getBalance() {
            return balance;
        }

        synchronized long getUnconfirmedBalance() {
            return unconfirmedBalance;
        }

        void addToBalance(long amount) throws Exception {

            synchronized (this) {

                this.balance += amount;

            }

            updatePeerWeights();

        }
        
        void addToUnconfirmedBalance(long amount) throws Exception {

            synchronized (this) {

                this.unconfirmedBalance += amount;

            }

            updateUserUnconfirmedBalance();

        }

        void addToBalanceAndUnconfirmedBalance(long amount) throws Exception {

            synchronized (this) {

                this.balance += amount;
                this.unconfirmedBalance += amount;

            }

            updatePeerWeights();
            updateUserUnconfirmedBalance();

        }

sigh... synchronized (this) ... how ignorant can you get!

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

[Come-from-Beyond]

I am afraid CfB and BCNext may get mad at me, but 0.5.0 relies heavily on the Concurrent API...

It's ok. Released source code is the whitepaper written in Java. Now we don't need that rule.

[lwilliams777]

@Frictionless.  I see that you know what you're talking about with regard to programming.  In fact, you may very well be a brilliant programmer, but the problem is, no one cares.  Why?  Because you have taken it upon yourself as a right to criticize but have not contributed anything productive.  You haven't earned that right yet.  In order to earn the right to be heard you should contribute, even in small amount, not with criticism, but with real productive help.  Once you do that then you've earned the right to criticize and others will actually listen to you.  As it stands, people just think you are a jerk, and using Simon Crowell, a known jerk, as an example doesn't help your cause.  Simon Crowell may be rich and be a star power broker, but I believe he has very few "real" deep long lasting friendships, if any.  

You have not gone about this in an effective way.  If you had played this right you would have contributed in a huge way and others would have sung your praises and others would have gladly signed up for your next generation plan.  In fact they would have become your best sales people and you would have had more folks sign up to help with your development and vision.  But as it stands there are only a few willing to help you and your chances of developing a great system are slim to none.

Why would I contribute to what obviously looks like a Ponzi scheme?

You all first explain to me why this entire shenanigan is any different from a Ponzi scheme?

Ponzi scheme or no ponzi scheme, what you're doing here doesn't really help.  If you think you are the defender of the naive who are investing then I would ask "Who appointed you the defender".  I think you know the answer.  If you are against the concept of pre-mining I can understand your opposition.  But do you think we can't figure that out for ourselves whether or not to trust pre-mined concepts or not?  Whether or not this is a risky venture or not?  In fact most people in the financial and investment business would consider all virtual systems scams and ponzi schemes.  In any case it is up to the investors to weigh the risks for themselves.  

[Come-from-Beyond]

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?