Nxt source code analysis (QA) - page 8.

gsan1

newbie

Activity: 50

Merit: 0

Doing some black box tests right now. The code compiles & starts fine, so at a first glance there seems to be no flaw concerning the init()/destroy() or account unlocking/locking.

Come-from-Beyond

legendary

Activity: 2142

Merit: 1010

Newbie

Quote from: ImmortAlex on January 04, 2014, 01:02:58 AM

Seems like Peer.getAnyPeer() can return even non-halmarked peer in rare case?
Is it ok? What is the reason?

The reason is to give non-hallmarked peers to take part in the show.

ImmortAlex

hero member

Activity: 784

Merit: 501

Seems like Peer.getAnyPeer() can return even non-halmarked peer in rare case?
Is it ok? What is the reason?

ImmortAlex

hero member

Activity: 784

Merit: 501

What is interesting for me... It appears that even if some transaction with big fee doesn't fit to block because block is almost full, some cheaper transaction can be put to block if it is very small. I always thought that deadline is only fee-based, and cheaper transactions always need to wait next block.
It's not a bug, it's a feature Wink

FrictionlessCoin

legendary

Activity: 868

Merit: 1000

Cryptotalk.org - Get paid for every post!

Quote from: ImmortAlex on January 03, 2014, 11:27:42 PM

Being good Android device, I need to have deep sleep at night. Now I woke up and continue to read the code.

Note to those who complain about one big file or JUnit absence. This is very young project. There're different approaches to start project. You can write a lot of tests, interfaces, abstract implementations... and have no working code for years (I personally have such expirience). Or you can write quick and dirty, and have something to show to your customers since the beginning. Both approach have its good and bad sides.

If you want - you can start rewrite Nxt right now. You can even fork it. With blackjack and hookers. There are just 6800 lines of code, and a lot of that lines are just empty

I am still trying to get to the gist of this implementation.

Nobody can seem to give me some straight answers on how exactly it should work.

Well that 6800 lines of code also has some embedded elliptic cryptographic stuff, so the actual real code is even less.

bitcool

legendary

Activity: 1441

Merit: 1000

Live and enjoy experiments

Quote from: theKnownUnknown on January 03, 2014, 04:02:08 PM

Now we know why this source code was not published before. This is a ticking time bomb. If you try to refactor this, it will explode. Do not take it personally

. Just my opininion and experience.

You have to give the creator credit for implementing the first version bitcoin-era crytocurrency in Java. I am sure a lot of time and effort were invested in this.

From a brief glance of it, it's definitely a very interesting and unorthodox way using JEE, and Java in general:

1) Application running as one HUGE servlet
2) One file, one class, many inner classes
3) All (almost) static methods
4) No test harness
...

With only 6800 lines of code, it's amazing this thing actually works! It just shows how powerful java libraries are. but.... the more layers you have (JVM, Jetty, 3rd party Jars), the more vulnerable it becomes to security threats.

It's probably fine (and fun) for a play project, but for a currency? I agree, I think I hear the ticking sound.

User705

legendary

Activity: 896

Merit: 1006

First 100% Liquid Stablecoin Backed by Gold

Forking with hookers. I'm sure there's a GIF somewhere on the net that would be appropriate but I'm lazy.

ImmortAlex

hero member

Activity: 784

Merit: 501

Being good Android device, I need to have deep sleep at night. Now I woke up and continue to read the code.

Note to those who complain about one big file or JUnit absence. This is very young project. There're different approaches to start project. You can write a lot of tests, interfaces, abstract implementations... and have no working code for years (I personally have such expirience). Or you can write quick and dirty, and have something to show to your customers since the beginning. Both approach have its good and bad sides.

If you want - you can start rewrite Nxt right now. You can even fork it. With blackjack and hookers. There are just 6800 lines of code, and a lot of that lines are just empty

NxtChoice

full member

Activity: 238

Merit: 100

Watching.

I care much more about the core of Nxt than the code style. We can think that BCNext may be not a great Java programmer(maybe he intended to use Java instead of C++ just for some reasons), but he is a great guy who invented Nxt, especially the thought, theory of Nxt. If the theory works ok and it is secure in theory, all of you guys can implement it in a reliable time frame and get better efficiency of the code. Of course, I want to thank all you guys taking time to advance the project.

EvilDave

hero member

Activity: 854

Merit: 1001

Quote from: FrictionlessCoin on January 03, 2014, 03:31:17 PM

Quote from: Revelations86 on January 03, 2014, 03:18:25 PM

For us common folks, what does this mean in plan english? Grin

in plain english... the code has not been tested exhaustively.

So its like flying an airplane that hasn't been tested to fly.

That coders/devs are like cats...one on it's own is fine, but the chances of a massive fight increase exponentially with the number of coders (or cats) in the same space.

Is it possible for a coder to look at someone elses code and not start criticizing?

Keep it up, guys, u're doing good work, even the trolls are useful to provide some extra motivation.

newsilike

sr. member

Activity: 630

Merit: 262

This account was hacked. just recently got it back

Quote from: FrictionlessCoin on January 03, 2014, 07:51:55 PM

Quote from: pinarello on January 03, 2014, 07:14:50 PM

Quote from: FrictionlessCoin on December 09, 2013, 03:07:45 PM

Always going to be a trade off between high liquidity and high growth.

I own BTC (30%) ... high liquidity, can easily go on margin, can trade futures. most flexible trading vehicle out there.
I own LTC (20%) high liquidity only in BTC-E, however potential BTCChina and mtgox listing.
I own NMC (40%) decent liquidity, normally follows BTC, but higher potential because of low valuation. 2nd highest hash rate among coins, however needs more aggressive dev support. I like it because not a lot of folks have big holdings here.

and for my purely speculative play ... don't day trade this coin... liquidity isn't great at the moment.
I own IXC (10%)

Anyway, do be careful with a lot of the "pump and dump" coins. You know, the ones with a high mint rate and with hundreds of millions of coins.

busted, your agenda is clear... can't get over the NXT alias system is it?

dont get emotional when trading, you don't have to do this you know Wink

My agenda is clear? By having a very conservative crypto-currency portfolio?

My only agenda is in the case of NXT is to figure out if it is legit or not. So far, I am very unimpressed.

Every investor should do their due diligence, I am doing mine. So far, it hasn't passed the sniff test.

I'm sorry if my standards are much higher than most NXT investors.

Then you might consider joining in later if your standards cant be matched yet. The pricle level will grow accordingly tho Sad

FrictionlessCoin

legendary

Activity: 868

Merit: 1000

Cryptotalk.org - Get paid for every post!

Quote from: pinarello on January 03, 2014, 07:14:50 PM

Quote from: FrictionlessCoin on December 09, 2013, 03:07:45 PM

Always going to be a trade off between high liquidity and high growth.

I own BTC (30%) ... high liquidity, can easily go on margin, can trade futures. most flexible trading vehicle out there.
I own LTC (20%) high liquidity only in BTC-E, however potential BTCChina and mtgox listing.
I own NMC (40%) decent liquidity, normally follows BTC, but higher potential because of low valuation. 2nd highest hash rate among coins, however needs more aggressive dev support. I like it because not a lot of folks have big holdings here.

and for my purely speculative play ... don't day trade this coin... liquidity isn't great at the moment.
I own IXC (10%)

Anyway, do be careful with a lot of the "pump and dump" coins. You know, the ones with a high mint rate and with hundreds of millions of coins.

busted, your agenda is clear... can't get over the NXT alias system is it?

dont get emotional when trading, you don't have to do this you know Wink

My agenda is clear? By having a very conservative crypto-currency portfolio?

My only agenda is in the case of NXT is to figure out if it is legit or not. So far, I am very unimpressed.

Every investor should do their due diligence, I am doing mine. So far, it hasn't passed the sniff test.

I'm sorry if my standards are much higher than most NXT investors.

pinarello

full member

Activity: 266

Merit: 100

NXT is the future

Quote from: FrictionlessCoin on January 03, 2014, 03:34:57 PM

Quote from: ImmortAlex on January 03, 2014, 08:49:14 AM

I hope you already remove "double copy"

Code:

peers = ((HashMap)Nxt.peers.clone()).values();

from thread, which is start every second

Which is dumb as hell because concurrent collections handles this kind of stuff.

So why is this poorly designed code out in the wild securing a currency?

It is only a matter of time before some guy drains out all the money out of all accounts.

This is a ticking time bomb!

Don't tell me I did not warn you folks.

Quote from: FrictionlessCoin on December 09, 2013, 03:07:45 PM

Always going to be a trade off between high liquidity and high growth.

I own BTC (30%) ... high liquidity, can easily go on margin, can trade futures. most flexible trading vehicle out there.
I own LTC (20%) high liquidity only in BTC-E, however potential BTCChina and mtgox listing.
I own NMC (40%) decent liquidity, normally follows BTC, but higher potential because of low valuation. 2nd highest hash rate among coins, however needs more aggressive dev support. I like it because not a lot of folks have big holdings here.

and for my purely speculative play ... don't day trade this coin... liquidity isn't great at the moment.
I own IXC (10%)

Anyway, do be careful with a lot of the "pump and dump" coins. You know, the ones with a high mint rate and with hundreds of millions of coins.

busted, your agenda is clear... can't get over the NXT alias system is it?

dont get emotional when trading, you don't have to do this you know Wink

Jaguar0625

sr. member

Activity: 299

Merit: 250

Code:

Here are a few more thoughts:

(1) This is a minor thing, but i don't think you're checking for overflow in any of the request parameters:
[code]
			int timestamp = ((Long)transactionData.get("timestamp")).intValue();
			short deadline = ((Long)transactionData.get("deadline")).shortValue();
			byte[] senderPublicKey = convert((String)transactionData.get("senderPublicKey"));
			long recipient = (new BigInteger((String)transactionData.get("recipient"))).longValue();
			int amount = ((Long)transactionData.get("amount")).intValue();

You're casting the value to a Long and then to an integer, but not checking if truncation occurred. [I would probably move the validation to the Transaction or a transaction factory function. You can also move the validation of positive and valid amounts and fees here].

(2) You're locking around every call to setBalance / setUnconfirmedBalance, which makes it easy to call the functions incorrectly if you forget to call them from within a lock. IMO, it would make sense to move some of that locking into the functions themselves so they can't be called incorrectly. [you can also change these functions to incrementXyz, since that's what you appear to be doing.

(3) Instead of a giant doGet function, imo, it would make sense to consider a chain of responsibility where you move each potential action into a separate object, e.g. :

Code:

interface GetHandler
{
    bool canHandle(string requestType);
    void handle(response, request);
}

};
[/code]

NXT

newbie

Activity: 23

Merit: 0

Quote from: FrictionlessCoin on January 03, 2014, 03:02:18 PM

Quote from: ImmortAlex on January 03, 2014, 09:29:53 AM

Found

Code:

static byte[] convert(String string)

and

Code:

static String convert(byte[] bytes)

ineffictive.
Need I suggest some solutions (like Apache Commons

)?

Edit: nevermind. I'm too strict this evening...

I don't think this developer has even heard of Apache commons.

I mean, it is obvious that he doesn't even know how to use a java package.

mister ixcoin alternative for bitcoin roflmao

iXcoin - The First Alternative to Bitcoin.

Been around since 2011, price has been going up since 2012.

Extremely secure, hash rate about 1/4th of Bitcoin.

Read the statistics page here: www.ixcoin.co

NXT

newbie

Activity: 23

Merit: 0

Quote from: FrictionlessCoin on January 03, 2014, 03:29:01 PM

Quote from: GröBkAz on January 03, 2014, 03:25:11 PM

Quote from: FrictionlessCoin on January 03, 2014, 02:59:25 PM

Quote from: ImmortAlex on January 03, 2014, 08:01:42 AM

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

The author did not even bother to run the code through static code analysis.

Also, where the hell are the JUnit tests?

How can anyone expect this kind of code to be used to handle their currency?

I do not think that this should be taken here as an opportunity to insult the developer. Please, show more humility for the work of others, even if you dont like the project.

This code is supposed to secure the currency that people own, so I expect top notch software development best practices, not work that looks like it came out of a high school coding project.

I am teling you like it is, if you can't handle the truth then so be it.

hey mister big mouth, if its not safe break it right now or stfu

theKnownUnknown

newbie

Activity: 16

Merit: 0

Now we know why this source code was not published before. This is a ticking time bomb. If you try to refactor this, it will explode. Do not take it personally

. Just my opininion and experience.

FrictionlessCoin

legendary

Activity: 868

Merit: 1000

Cryptotalk.org - Get paid for every post!

Quote from: ImmortAlex on January 03, 2014, 08:49:14 AM

I hope you already remove "double copy"

Code:

peers = ((HashMap)Nxt.peers.clone()).values();

from thread, which is start every second

Which is dumb as hell because concurrent collections handles this kind of stuff.

So why is this poorly designed code out in the wild securing a currency?

It is only a matter of time before some guy drains out all the money out of all accounts.

This is a ticking time bomb!

Don't tell me I did not warn you folks.

FrictionlessCoin

legendary

Activity: 868

Merit: 1000

Cryptotalk.org - Get paid for every post!

Quote from: Revelations86 on January 03, 2014, 03:18:25 PM

For us common folks, what does this mean in plan english? Grin

in plain english... the code has not been tested exhaustively.

So its like flying an airplane that hasn't been tested to fly.

FrictionlessCoin

legendary

Activity: 868

Merit: 1000

Cryptotalk.org - Get paid for every post!

Quote from: GröBkAz on January 03, 2014, 03:25:11 PM

Quote from: FrictionlessCoin on January 03, 2014, 02:59:25 PM

Quote from: ImmortAlex on January 03, 2014, 08:01:42 AM

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

The author did not even bother to run the code through static code analysis.

Also, where the hell are the JUnit tests?

How can anyone expect this kind of code to be used to handle their currency?

I do not think that this should be taken here as an opportunity to insult the developer. Please, show more humility for the work of others, even if you dont like the project.

This code is supposed to secure the currency that people own, so I expect top notch software development best practices, not work that looks like it came out of a high school coding project.

I am teling you like it is, if you can't handle the truth then so be it.

Topic: Nxt source code analysis (QA) - page 8. (Read 14118 times)