Bitcoin Forum
February 20, 2019, 08:30:40 AM *
News: Latest Bitcoin Core release: 0.17.1 [Torrent]
 
   Home   Help Search Login Register More  
Pages: [1] 2 3 4 5 6 7 8 9 10 »  All
  Print  
Author Topic: Nxt source code analysis (QA)  (Read 13897 times)
ImmortAlex
Hero Member
*****
Offline Offline

Activity: 784
Merit: 501


View Profile
January 03, 2014, 01:01:42 PM
 #1

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 Smiley ), I'm starting this thread. Not for bounty, but for better quality (but donations welcome, as usual Smiley ).

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...
1550651440
Hero Member
*
Offline Offline

Posts: 1550651440

View Profile Personal Message (Offline)

Ignore
1550651440
Reply with quote  #2

1550651440
Report to moderator
1550651440
Hero Member
*
Offline Offline

Posts: 1550651440

View Profile Personal Message (Offline)

Ignore
1550651440
Reply with quote  #2

1550651440
Report to moderator
Your Bitcoin transactions
The Ultimate Bitcoin mixer
made truly anonymous.
with an advanced technology.
Mix coins
Advertised sites are not endorsed by the Bitcoin Forum. They may be unsafe, untrustworthy, or illegal in your jurisdiction. Advertise here.
1550651440
Hero Member
*
Offline Offline

Posts: 1550651440

View Profile Personal Message (Offline)

Ignore
1550651440
Reply with quote  #2

1550651440
Report to moderator
1550651440
Hero Member
*
Offline Offline

Posts: 1550651440

View Profile Personal Message (Offline)

Ignore
1550651440
Reply with quote  #2

1550651440
Report to moderator
1550651440
Hero Member
*
Offline Offline

Posts: 1550651440

View Profile Personal Message (Offline)

Ignore
1550651440
Reply with quote  #2

1550651440
Report to moderator
Come-from-Beyond
Legendary
*
Offline Offline

Activity: 2072
Merit: 1007

Newbie


View Profile
January 03, 2014, 01:04:48 PM
 #2

Watching
ImmortAlex
Hero Member
*****
Offline Offline

Activity: 784
Merit: 501


View Profile
January 03, 2014, 01:14:05 PM
 #3

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
Hero Member
*****
Offline Offline

Activity: 784
Merit: 501


View Profile
January 03, 2014, 01:22:21 PM
 #4

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
Sr. Member
****
Offline Offline

Activity: 392
Merit: 250



View Profile WWW
January 03, 2014, 01:26:42 PM
 #5

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.

lead Nxt developer, gpg key id: 0x811D6940E1E4240C
Nxt blockchain platform | Ardor blockchain platform | Ignis ICO
Come-from-Beyond
Legendary
*
Offline Offline

Activity: 2072
Merit: 1007

Newbie


View Profile
January 03, 2014, 01:26:54 PM
 #6

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?  Grin
Jean-Luc
Sr. Member
****
Offline Offline

Activity: 392
Merit: 250



View Profile WWW
January 03, 2014, 01:33:49 PM
 #7

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. :-)
Quote
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.

lead Nxt developer, gpg key id: 0x811D6940E1E4240C
Nxt blockchain platform | Ardor blockchain platform | Ignis ICO
ImmortAlex
Hero Member
*****
Offline Offline

Activity: 784
Merit: 501


View Profile
January 03, 2014, 01:37:00 PM
 #8

Okey, let me rewrite something Smiley

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
Hero Member
*****
Offline Offline

Activity: 784
Merit: 501


View Profile
January 03, 2014, 01:38:15 PM
 #9

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
Sr. Member
****
Offline Offline

Activity: 392
Merit: 250



View Profile WWW
January 03, 2014, 01:38:44 PM
 #10

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

lead Nxt developer, gpg key id: 0x811D6940E1E4240C
Nxt blockchain platform | Ardor blockchain platform | Ignis ICO
Come-from-Beyond
Legendary
*
Offline Offline

Activity: 2072
Merit: 1007

Newbie


View Profile
January 03, 2014, 01:39:58 PM
 #11

Okey, let me rewrite something Smiley

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 Smiley)
ImmortAlex
Hero Member
*****
Offline Offline

Activity: 784
Merit: 501


View Profile
January 03, 2014, 01:42:34 PM
 #12

Keep digging, I am still a step ahead. Smiley
Two step ahead (0.4.8 and 0.4.9), and I hope you start to do third step already Smiley
Jean-Luc
Sr. Member
****
Offline Offline

Activity: 392
Merit: 250



View Profile WWW
January 03, 2014, 01:43:43 PM
 #13

Okey, let me rewrite something Smiley

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);

                }

            }


lead Nxt developer, gpg key id: 0x811D6940E1E4240C
Nxt blockchain platform | Ardor blockchain platform | Ignis ICO
bitcoinpaul
Hero Member
*****
Offline Offline

Activity: 924
Merit: 1000



View Profile
January 03, 2014, 01:44:27 PM
 #14

You little nerds.  Kiss
ImmortAlex
Hero Member
*****
Offline Offline

Activity: 784
Merit: 501


View Profile
January 03, 2014, 01:44:49 PM
 #15

This proves u r not BCNext. Ur and his logic r completely different!
I am from generation "640k is enough" Smiley So I dislike such memory copies.
ImmortAlex
Hero Member
*****
Offline Offline

Activity: 784
Merit: 501


View Profile
January 03, 2014, 01:47:19 PM
 #16

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
Hero Member
*****
Offline Offline

Activity: 784
Merit: 501


View Profile
January 03, 2014, 01:49:14 PM
 #17

I hope you already remove "double copy"
Code:
peers = ((HashMap<String, Peer>)Nxt.peers.clone()).values();
from thread, which is start every second Smiley
Jaguar0625
Sr. Member
****
Offline Offline

Activity: 299
Merit: 250


View Profile
January 03, 2014, 01:50:37 PM
 #18

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

NEM - nem.io
Jean-Luc
Sr. Member
****
Offline Offline

Activity: 392
Merit: 250



View Profile WWW
January 03, 2014, 01:52:02 PM
 #19

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());
    }

lead Nxt developer, gpg key id: 0x811D6940E1E4240C
Nxt blockchain platform | Ardor blockchain platform | Ignis ICO
ImmortAlex
Hero Member
*****
Offline Offline

Activity: 784
Merit: 501


View Profile
January 03, 2014, 01:53:15 PM
 #20

Just make sure you don't give him/them the whole 0.4.9e code in this topic!   Wink
I have no time to be full-time Nxt core developer Sad
Pages: [1] 2 3 4 5 6 7 8 9 10 »  All
  Print  
 
Jump to:  

Bitcointalk.org is not available or authorized for sale. Do not believe any fake listings.
Sponsored by , a Bitcoin-accepting VPN.
Powered by MySQL Powered by PHP Powered by SMF 1.1.19 | SMF © 2006-2009, Simple Machines Valid XHTML 1.0! Valid CSS!