Nxt source code analysis (QA)

[ImmortAlex]

While lurking for injected bugs, I found that some parts of Nxt code makes my eyes bleed. With hope that I have enough programming skills (probably better, than my runglish ), I'm starting this thread. Not for bounty, but for better quality (but donations welcome, as usual ).

First of all, thanks to IntelliJ IDEA code analysis, I see a lot of syncronized with potentially wrong usage.
Static variables like peers, blocks, accounts, users are not final. They are initialized at start, so it is not a big flaw right now, but who knows about future.
When I look more deeply in syncronizations, I see a lot of places, where author get content of such global collection and then sync on it, but not sync on collection itself. Example:

Code:

Block.analyse() method:

Account generatorAccount = accounts.get(Account.getId(generatorPublicKey));
synchronized (generatorAccount) {
    generatorAccount.setBalance(generatorAccount.balance + totalFee * 100L);
    generatorAccount.setUnconfirmedBalance(generatorAccount.unconfirmedBalance + totalFee * 100L);
}

While in some places such usage may be safe, I think it's very bad practice anyway.

More to come...

[1715309623]

1715309623

[1715309623]

1715309623

[1715309623]

1715309623

[Come-from-Beyond]

Watching

[ImmortAlex]

Small suggestion on usage of Collection.toArray().
I see a lot of calls like

Code:

peers.toArray(new Peer[0])

1. It is completely useless and equal to simple toArray() call.
2. It is better in terms of speed and memory usage to code like this:

Code:

peers.toArray(new Peer[peers.size()])

[ImmortAlex]

One more thanks to IDEA: line 6533, peer can be null. At least there's check for null above and below, but not here.

[Jean-Luc]

One more thanks to IDEA: line 6533, peer can be null. At least there's check for null above and below, but not here.

Relax, I am also using IntelliJ. And the above has been fixed in 0.4.9e.

[Come-from-Beyond]

One more thanks to IDEA: line 6533, peer can be null. At least there's check for null above and below, but not here.

Hey, who is checking the code, u or robot?  

[Jean-Luc]

Static variables like peers, blocks, accounts, users are not final. They are initialized at start, so it is not a big flaw right now, but who knows about future.

In 0.4.9e, they are. :-)

When I look more deeply in syncronizations, I see a lot of places, where author get content of such global collection and then sync on it, but not sync on collection itself. Example:

Code:

Block.analyse() method:

Account generatorAccount = accounts.get(Account.getId(generatorPublicKey));
synchronized (generatorAccount) {
    generatorAccount.setBalance(generatorAccount.balance + totalFee * 100L);
    generatorAccount.setUnconfirmedBalance(generatorAccount.unconfirmedBalance + totalFee * 100L);
}

I have fixed, I hope, the synchronization issues related to Blocks and Transactions in 0.4.9e. The synchronization in Accounts is on my TODO list for the next version.

[ImmortAlex]

Okey, let me rewrite something

Code:

        static Peer getAnyPeer(int state, boolean applyPullThreshold) {

            synchronized (peers) {

                Collection<Peer> peers = ((HashMap<String, Peer>)Nxt.peers.clone()).values();
                Iterator<Peer> iterator = peers.iterator();
                while (iterator.hasNext()) {

                    Peer peer = iterator.next();
                    if (peer.blacklistingTime > 0 || peer.state != state || peer.announcedAddress.length() == 0 || (applyPullThreshold && enableHallmarkProtection && peer.getWeight() < pullThreshold)) {

                        iterator.remove();

                    }

                }

                if (peers.size() > 0) {

                    Peer[] selectedPeers = peers.toArray(new Peer[0]);
...

For memory usage I prefer to code like this:

Code:

                List<Peer> selectedPeers = new ArrayList<>();
                for (Map.Entry<String, Peer> entry : peers.entrySet()) {
                    Peer peer = entry.getValue();
                    if (peer.blacklistingTime <= 0 && peer.state == state && peer.announcedAddress.length() != 0 && ((!applyPullThreshold || !enableHallmarkProtection || peer.getWeight() >= pullThreshold))) {
                        selectedPeers.add(peer);
                    }
                }

And even more, List selectedPeers can be reused, if used only in this method.

[ImmortAlex]

Relax, I am also using IntelliJ. And the above has been fixed in 0.4.9e.

I have fixed, I hope, the synchronization issues related to Blocks and Transactions in 0.4.9e. The synchronization in Accounts is on my TODO list for the next version.

Good news!
Okey, let me find something really interesting...

[Jean-Luc]

Small suggestion on usage of Collection.toArray().
I see a lot of calls like

Code:

peers.toArray(new Peer[0])

1. It is completely useless and equal to simple toArray() call.
2. It is better in terms of speed and memory usage to code like this:

Code:

peers.toArray(new Peer[peers.size()])

A search for toArray in 0.4.9e finds only two instances in old, commented out code. Keep digging, I am still a step ahead.

[Come-from-Beyond]

Okey, let me rewrite something

Code:

        static Peer getAnyPeer(int state, boolean applyPullThreshold) {

            synchronized (peers) {

                Collection<Peer> peers = ((HashMap<String, Peer>)Nxt.peers.clone()).values();
                Iterator<Peer> iterator = peers.iterator();
                while (iterator.hasNext()) {

                    Peer peer = iterator.next();
                    if (peer.blacklistingTime > 0 || peer.state != state || peer.announcedAddress.length() == 0 || (applyPullThreshold && enableHallmarkProtection && peer.getWeight() < pullThreshold)) {

                        iterator.remove();

                    }

                }

                if (peers.size() > 0) {

                    Peer[] selectedPeers = peers.toArray(new Peer[0]);
...

For memory usage I prefer to code like this:

Code:

                List<Peer> selectedPeers = new ArrayList<>();
                for (Map.Entry<String, Peer> entry : peers.entrySet()) {
                    Peer peer = entry.getValue();
                    if (peer.blacklistingTime <= 0 && peer.state == state && peer.announcedAddress.length() != 0 && ((!applyPullThreshold || !enableHallmarkProtection || peer.getWeight() >= pullThreshold))) {
                        selectedPeers.add(peer);
                    }
                }

And even more, List selectedPeers can be reused, if used only in this method.

This proves u r not BCNext. Ur and his logic r completely different!

(Or u did this intentionally )

[ImmortAlex]

Keep digging, I am still a step ahead.

Two step ahead (0.4.8 and 0.4.9), and I hope you start to do third step already

[Jean-Luc]

Okey, let me rewrite something

For memory usage I prefer to code like this:

Code:

                List<Peer> selectedPeers = new ArrayList<>();
                for (Map.Entry<String, Peer> entry : peers.entrySet()) {
                    Peer peer = entry.getValue();
                    if (peer.blacklistingTime <= 0 && peer.state == state && peer.announcedAddress.length() != 0 && ((!applyPullThreshold || !enableHallmarkProtection || peer.getWeight() >= pullThreshold))) {
                        selectedPeers.add(peer);
                    }
                }

And even more, List selectedPeers can be reused, if used only in this method.

You are reading my mind. My tinfoil hat has been leaking!
My replacement of the same lines in getAnyPeer, already in 0.4.9e:

Code:

            List<Peer> selectedPeers = new ArrayList<Peer>();

            for (Peer peer : Nxt.peers.values()) {

                if (peer.blacklistingTime <= 0 && peer.state == state && peer.announcedAddress.length() > 0
                        && (!applyPullThreshold || !enableHallmarkProtection || peer.getWeight() >= pullThreshold)) {

                    selectedPeers.add(peer);

                }

            }

[bitcoinpaul]

You little nerds. 

[ImmortAlex]

This proves u r not BCNext. Ur and his logic r completely different!

I am from generation "640k is enough" So I dislike such memory copies.

[ImmortAlex]

My replacement of the same lines in getAnyPeer, already in 0.4.9e:

Code:

            List<Peer> selectedPeers = new ArrayList<Peer>();

            for (Peer peer : Nxt.peers.values()) {

                if (peer.blacklistingTime <= 0 && peer.state == state && peer.announcedAddress.length() > 0
                        && (!applyPullThreshold || !enableHallmarkProtection || peer.getWeight() >= pullThreshold)) {

                    selectedPeers.add(peer);

                }

            }

AFAIR, HashMap.values() make a copy, while HashMap.entrySet() definitely not.
So my variant is even better. You sync in peers anyway, so it is safe.

[ImmortAlex]

I hope you already remove "double copy"

Code:

peers = ((HashMap<String, Peer>)Nxt.peers.clone()).values();

from thread, which is start every second

[Jaguar0625]

There are also a lot of places where the base Exception is being caught and suppressed. It's generally best to only catch exceptions that you can handle, otherwise, you're just continuing without knowing what went wrong and your code could be in a bad state.

Two examples:
(1) Around all the parseInts where you're loading config, you should be catching the more specific NumberFormatException (which you can handle) than Exception (which you probably can't).
(2) It does seem odd that if saveTransactions fails by throwing an exception (which it can) the code just ignores it and keeps going. I would expect it to do something in that case.

I'm unsure what you're seeding transactions.nxt with when it's not found? Are those the genesis block transactions? Shouldn't they be getting downloaded from somewhere?

It's kind of awkward to have a private class (e.g. Blocks) populate a member of the containing type directly (e.g. Blocks.loadBlocks assigns Nxt.blocks directly). It looks like you're using classes primarily for namespacing (e.g. grouping related functions) instead of creating standalone classes. For example, the Blocks class cannot be used anywhere else because it has hard dependencies on Nxt (which is also doing server stuff). Might not be a big deal in your case, but it's usually better to minimize class dependencies by creating small, reusable classes with a single purpose (in other words, following the SOLID principle).

The file handles on error conditions:

Code:

		static void loadBlocks(String fileName) throws Exception {
			
			FileInputStream fileInputStream = new FileInputStream(fileName);
			ObjectInputStream objectInputStream = new ObjectInputStream(fileInputStream);
			blockCounter = objectInputStream.readInt();
			blocks = (HashMap<Long, Block>)objectInputStream.readObject();
			lastBlock = objectInputStream.readLong();
			objectInputStream.close();
			fileInputStream.close();

If read object throws here, neither stream will be closed. You should consider moving the closes to a finally block.

That's all I have for now, but I'll probably look the code more later .
         
      }

[Jean-Luc]

AFAIR, HashMap.values() make a copy, while HashMap.entrySet() definitely not.
So my variant is even better. You sync in peers anyway, so it is safe.

I didn't know that, are you sure? From the java source of ConcurrentHashMap, which Nxt.peers already is in 0.4.9e:

Code:

    /**
     * Returns a {@link Collection} view of the values contained in this map.
     * The collection is backed by the map, so changes to the map are
     * reflected in the collection, and vice-versa.  The collection
     * supports element removal, which removes the corresponding
     * mapping from this map, via the <tt>Iterator.remove</tt>,
     * <tt>Collection.remove</tt>, <tt>removeAll</tt>,
     * <tt>retainAll</tt>, and <tt>clear</tt> operations.  It does not
     * support the <tt>add</tt> or <tt>addAll</tt> operations.
     *
     * <p>The view's <tt>iterator</tt> is a "weakly consistent" iterator
     * that will never throw {@link ConcurrentModificationException},
     * and guarantees to traverse elements as they existed upon
     * construction of the iterator, and may (but is not guaranteed to)
     * reflect any modifications subsequent to construction.
     */
    public Collection<V> values() {
        Collection<V> vs = values;
        return (vs != null) ? vs : (values = new Values());
    }

[ImmortAlex]

Just make sure you don't give him/them the whole 0.4.9e code in this topic!   

I have no time to be full-time Nxt core developer