Nxt source code analysis (QA)

[NXT]

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

[1714686862]

1714686862

[Jaguar0625]

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]

[pinarello]

I hope you already remove "double copy"

Code:

peers = ((HashMap<String, Peer>)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.

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

Pin

[FrictionlessCoin]

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

Pin

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.

[newsilike]

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

Pin

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 

[EvilDave]

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

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.

[NxtChoice]

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.

[ImmortAlex]

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

[User705]

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

[bitcool]

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.

[FrictionlessCoin]

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.   

[ImmortAlex]

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

[ImmortAlex]

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

[Come-from-Beyond]

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.

[gsan1]

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.

[ImmortAlex]

Nice check in Block.verifyBlockSignature()

Code:

Account account = accounts.get(Account.getId(generatorPublicKey));

....

if (!Arrays.equals(generatorPublicKey, account.publicKey)) {
    return false;
}

Seems like inside this check must be big red banner: "Now we need to use full 256 hash for account ID"

[Come-from-Beyond]

Nice check in Block.verifyBlockSignature()

Code:

Account account = accounts.get(Account.getId(generatorPublicKey));

....

if (!Arrays.equals(generatorPublicKey, account.publicKey)) {
    return false;
}

Seems like inside this check must be big red banner: "Now we need to use full 256 hash for account ID"

Why?

[ImmortAlex]

You get account ID from pubkey of block generator, get Account by this ID, then compare account pubkey with generator's one, and they didn't match. I think it's because of ID collision: two different pubkeys gives the same ID.
Account ID is SHA-256 hash of pubkey, but currently only 8 last bytes of hash is used. AFAIR there were talks about such collisions, and the conclusion was to use more bytes of hash only when we start to get collisions.
Am I miss something?

(It's not a talk about bug, I'm just try to understand code)

[Come-from-Beyond]

You get account ID from pubkey of block generator, get Account by this ID, then compare account pubkey with generator's one, and they didn't match. I think it's because of ID collision: two different pubkeys gives the same ID.
Account ID is SHA-256 hash of pubkey, but currently only 8 last bytes of hash is used. AFAIR there were talks about such collisions, and the conclusion was to use more bytes of hash only when we start to get collisions.
Am I miss something?

(It's not a talk about bug, I'm just try to understand code)

It's a feature.

[ImmortAlex]

Second day of code reading. Somewhere in the middle of the file So some new thoughts.

The problem is not a 6800 lines itself...
I don't like that code is not object-oriented well. It's more like plain C with structures. Direct access to variables, some small code dublications here and there, you know... Sometimes it's hard to track code and data flow. I hope at the end of reading I will find some global flaws but locally, inside small methods and code branches everything looks good. Doesn't check that curve math actually Hope it has no flaws.


Now I clearly see how they keep coins And I don't like it too.
For those who didn't know: transactions keep amount and fee in signed 32-bit integers. 1 billion max, no fractional part at all. Blocks keep total amount and total fee the same way. It need to be changed if we want to have 0.01 fee. And this is base part of internal data structures.
Account keep it's balance in 64-bit signed integer. And that value is 100 times greater than amount and fee in transaction. So there's room to have 0.01 precision (fixed-point math, you know).
Right now everytime when there's addition or substraction from account balance to transaction we need to multiply or divide by 100. Every time. Not in get/set methods, but in code directly. Shit, I'm afraid of this code.
This part must be:
1. Synced with each other. Type of variables for account, transaction and block must be the same.
2. Wrapped to some code which have a deal with that fixed math. If we want 0.0001 precision in future we must change only one place in code.

I hope Jean-Luc make refactorings already