Pages:
Author

Topic: Nxt source code analysis (QA) - page 7. (Read 14115 times)

legendary
Activity: 868
Merit: 1000
Cryptotalk.org - Get paid for every post!
January 05, 2014, 07:56:35 AM
#75
Next step is to calculate cost of attack.
Attacker can send 255 transactions in block. Let's use simple ordinary payment, 1 NXT amout, 1 NXT fee, 510 NXT for full block. 320+156*255 = 40100 bytes of memory.
According to guide, JRE in RPi can use up to 430Mb for heap. Okey, let's assume, that 401Mb is available for blocks and transactions. 401M / 40100 = 10000. Yes, just ten thousand blocks, and you RPi is dead. Or 5M NXT. F*ckin five million coins.


You can just use one NXT and bounce it around the network!

In fact, since everything is all HTTP based.  Someone can write a script.

Create 1000 accounts
Loop 10,000 times
    Loop through each account
        Send 1 IXC to each account



legendary
Activity: 868
Merit: 1000
Cryptotalk.org - Get paid for every post!
January 05, 2014, 07:50:13 AM
#74
Kind of Post Scriptum to previous analysis.

1. Java and it's unpredictable object's size, nice article in russian (sorry, guys Smiley ).
2. For those who don't look deeper: 10000 blocks is just 10 days in current rate (AFAIK, we're still not getting desired 1440 blocks per day). Ten days to kill big part of our decentralized network. And ten days more to kill the rest.
3. There will be short-time paradise for miners Smiley 255 coins every minute. Wow.
4. I believe that if such kind of attack will start, devs fix the problem in one or two day, so it is actually not a reason to sell you coins. Just some funsy math. Do you remember how fast C-f-B redo NRS from self-written http code to Jetty? Moving blockchain to DBMS is the same, more or less.

That's why bitcoind uses leveldb.  It would be joke to keep the entire block chain in memory.

So you are saying this NXT keeps the entire block chain in memory?  

Pathetic.

I mean, they likely haven't done any analysis of what kind of database to use as a backend.   There is no way that they'll be able to fix this problem in time for folks to realize that they can spam the network and blow up every JVM that sits on this network!

Well... clock is ticking....  waiting on the dbms implementation....

Also,  realize that they use Java serialization to store the transactions in file....  

So when these JVMs go 'out of memory error'... they can't save the transactions file.... and even if they do... that'll blow up!

HILARIOUS!

DO I GET THE PRIZE FOR SEEING THE FATAL FLAW!



legendary
Activity: 868
Merit: 1000
Cryptotalk.org - Get paid for every post!
January 05, 2014, 07:43:01 AM
#73
For people who are completely dismayed with the poor quality of the NXT code base and believe they can do better,  then please join this new project:  https://bitcointalksearch.org/topic/m.4305172

Your input will be welcome.
hero member
Activity: 784
Merit: 501
January 05, 2014, 04:04:20 AM
#72
Also maybe not the place for it, but a suggestion, what if say 1% of all the transaction fees went into a specific wallet to be used for paying people to develop the code?
Developers interested theirself to develop Nxt software and rise interest to Nxt because it rises cost ot their existing Nxt funds.
I don't think that any additional fee is needed. Donations are welcome as usual, but no more.
hero member
Activity: 784
Merit: 501
January 05, 2014, 03:58:21 AM
#71
ImmortAlex deserves a flood of donations for his work Smiley

Agree. 10K sent to the account in his signature.
Спасибо большое! Пригодится.
hero member
Activity: 527
Merit: 503
January 05, 2014, 03:29:55 AM
#70
Good thinking, thanks for your work ImmortAlex!

Also maybe not the place for it, but a suggestion, what if say 1% of all the transaction fees went into a specific wallet to be used for paying people to develop the code?

Seems to me like it'd be nice to support the developers, and when we have our own Nextcoin foundation similar to the Bitcoin foundation, we could have automatic funds to be always developing and improving the currency, so Nextcoin always has some funds to improve itself.  And there are so many directions this idea could be taken!  Nextcoin is going to be amazing some day.

Something you'd have to have people vote on of course..  or maybe a better solution that I all around like better, put an optional radio button in every forger's wallet that says donate O-0% O-1%(default?) O-2% O-3% O-4% of all my NXT forged to the 'Nextcoin Foundation'.

Short term, I think it'd bring some peace of mind to people that these problems will be fixed and you'll be able to pay people to fix them and have a stronger development team behind the project.  Because I'll be honest the code scares me a little seeing how much work is needed to really compete against other cryptos.

Though I suspect the current developers are largely initial investors, who do still have a bunch of NXT of their own at this point to provide them with some incentive Smiley
legendary
Activity: 2142
Merit: 1010
Newbie
January 05, 2014, 03:06:05 AM
#69
ImmortAlex deserves a flood of donations for his work Smiley

Agree. 10K sent to the account in his signature.
hero member
Activity: 616
Merit: 500
January 05, 2014, 03:00:24 AM
#68
ImmortAlex deserves a flood of donations for his work Smiley
hero member
Activity: 527
Merit: 503
January 05, 2014, 02:50:54 AM
#67
About what I figured.. though I am surprised that it's all in one file and you are saying that only one person can work on it at a time.

It may take more time right now but I say fix any big bugs so people don't lose their money, then first thing you should do is refactor the code, split it up, and allow multiple people to work on it.

I read as much as I could about the idea behind Nextcoin and if you guys pull this off, most of the people reading this will make a lot of money off of it, a few years from now.. this fixes all the major problems with Bitcoin and I think it's genius.  Seriously this is by far the best idea for a crypto I've seen yet.

But it needs a lot of work to get there.. and in order to do so, I feel like you are going to need a team and to subversion it.
hero member
Activity: 784
Merit: 501
January 05, 2014, 01:17:29 AM
#66
Kind of Post Scriptum to previous analysis.

1. Java and it's unpredictable object's size, nice article in russian (sorry, guys Smiley ).
2. For those who don't look deeper: 10000 blocks is just 10 days in current rate (AFAIK, we're still not getting desired 1440 blocks per day). Ten days to kill big part of our decentralized network. And ten days more to kill the rest.
3. There will be short-time paradise for miners Smiley 255 coins every minute. Wow.
4. I believe that if such kind of attack will start, devs fix the problem in one or two day, so it is actually not a reason to sell you coins. Just some funsy math. Do you remember how fast C-f-B redo NRS from self-written http code to Jetty? Moving blockchain to DBMS is the same, more or less.
legendary
Activity: 1470
Merit: 1004
January 05, 2014, 12:58:48 AM
#65
Thanks ImmortAlex, please keep up the excellent analysis.
hero member
Activity: 784
Merit: 501
January 04, 2014, 11:47:52 PM
#64
Okey, let's talk about one more TODO in you list Smiley
The blocks, transactions and accounts cache, and how much it will cost to kill our RPi nodes.

As we know since the beginning, blocks.nxt and transactions.nxt is just a result of standard ObjectInputStream work.
At startup they are fully loaded to memory and are there till the end.
So, NRS keeps full blockchain in memory all the time.

Let's do rough calculations. Class Block:
Code:
        int version;
        int timestamp;
        long previousBlock;
        int numberOfTransactions;
        int totalAmount, totalFee;
        int payloadLength;
        byte[] payloadHash;
        byte[] generatorPublicKey;
        byte[] generationSignature;
        byte[] blockSignature;
        byte[] previousBlockHash;
        int index;
        long[] transactions;
        long baseTarget;
        int height;
        long nextBlock;
        BigInteger cumulativeDifficulty;
This is from 0.5.0 actually (sorry for decompilation), but there's almost no difference with 0.4.7.
With 32-bit pointers and no alignment this gives us about 320 bytes for block without transactions, plus 8 byte for each transaction. 64-bit JRE with memory alignment can easily expand this to ~400 bytes. I can't tell you exact numbers, because it heavily depends on JRE implementation, operating system and so on.
Right now we have about 33000 block, which occupies about 11Mb of memory. I think, I should speak "at least 11Mb". Without accounting of long[] transactions...

Okey, take a look at class Transaction:
Code:
        byte type, subtype;
        int timestamp;
        short deadline;
        byte[] senderPublicKey;
        long recipient;
        int amount, fee;
        long referencedTransaction;
        byte[] signature;
        Attachment attachment;

        int index;
        long block;
        int height;
156 bytes without attachement (f.e. alias). Up to 200 in real world. 65000 transactions for now. More than 10Mb of memory. Plus 65000*8 = 0.5Mb of links from blocks to transactions (or we can assume that size of transaction is 164 bytes).

Actual things is even worse because blocks and transactions is not linear lists, but hash maps.
Actual things can be better, because of generatorPublicKey and senderPublicKey can be shared between blocks, transactions and accounts.

I'm bit tired and don't want to calculate size of accounts - it's too small comparing to blocks and transactions.

Next step is to calculate cost of attack.
Attacker can send 255 transactions in block. Let's use simple ordinary payment, 1 NXT amout, 1 NXT fee, 510 NXT for full block. 320+156*255 = 40100 bytes of memory.
According to guide, JRE in RPi can use up to 430Mb for heap. Okey, let's assume, that 401Mb is available for blocks and transactions. 401M / 40100 = 10000. Yes, just ten thousand blocks, and you RPi is dead. Or 5M NXT. F*ckin five million coins.

This is very rough calculations. Very. My intuition tell me, that things are worse actually. Cheeper. Faster.
And then, after RPi, this attack will kill cheap 2Gb VPS's. And then - 32-bit JRE's, which actually hardly can get 2Gb of heap.
And before that we will hear the song of death from our HDD/SSD, tired to save that .nxt files everytime you get new fat block.

Solution?
I think devs know it and has in TODO list Smiley We need to use some embedded DB and cache in memory only what we need right now: couple of last blocks.
sr. member
Activity: 392
Merit: 250
January 04, 2014, 01:21:51 PM
#63
One more thought about memory usage.
I see a lot of getBytes().length calls on Transaction and Block. Especially in Transaction.compareTo() this can create a lot of temporaty byte arrays in short time. But length can be easily and much faster calculated without creating array. I think, even for any Attachment.
Not fixed yet, but already I have a //TODO: cache? on top of those getBytes() methods. And indeed need to do it for caching the length.
sr. member
Activity: 392
Merit: 250
January 04, 2014, 01:19:48 PM
#62
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].
I have added a few more checks for the values of Transaction.amount and Transaction.fee in 0.5.0. I agree the validation checks are all over the place and need to be cleaned up.
Quote
(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.
This was already on my list of things to fix, and I have fixed in in 0.5.0, but thanks for noticing.

Quote
(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
I know, just no time to do that yet.
hero member
Activity: 784
Merit: 501
January 04, 2014, 11:08:43 AM
#61
One more thought about memory usage.
I see a lot of getBytes().length calls on Transaction and Block. Especially in Transaction.compareTo() this can create a lot of temporaty byte arrays in short time. But length can be easily and much faster calculated without creating array. I think, even for any Attachment.
hero member
Activity: 784
Merit: 501
January 04, 2014, 10:25:31 AM
#60
Second day of code reading. Somewhere in the middle of the file Smiley 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 Smiley but locally, inside small methods and code branches everything looks good. Doesn't check that curve math actually Smiley Hope it has no flaws.


Now I clearly see how they keep coins Smiley 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 Smiley
legendary
Activity: 2142
Merit: 1010
Newbie
January 04, 2014, 08:19:54 AM
#59
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.
hero member
Activity: 784
Merit: 501
January 04, 2014, 08:11:34 AM
#58
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)
legendary
Activity: 2142
Merit: 1010
Newbie
January 04, 2014, 07:52:13 AM
#57
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" Smiley

Why?
hero member
Activity: 784
Merit: 501
January 04, 2014, 07:34:16 AM
#56
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" Smiley
Pages:
Jump to: