Nxt source code flaw reports

[ricot]

I'm still waiting on statements concerning these two problems:

- Waiting past the creation time of a block to add additinal future transactions gains malicious Bob more coins when forging and causes the creation of unneccessary orphan blocks.
- Malicious Anna can gain ~7.5% more forges by spamming the network with early-generated blocks.

Are both of those working as intended? If yes: The network will become a lot more inefficient in the near future  

- It's not a problem with TF, blocks r supposed to be forged in advance.

TF isn't the solution to everything... You still have to accept blocks that physically arrive after their creation date due to network latency. Meaning that the attack is possible even with TF as long as you can put future transactions into your block that shouldn't even have existed at that point. And you still will get a lot of orphans and wasted work...
I don't get what's so hard to understand in that attack and why it's even considered to be ok to have transactions that were created after the forge date in a block... Makes no sense at all. And I'm looking forward to the whitepaper where you explain the reasoning behind this.

- This can be fixed by using more than 1 thread for blockchain downloading.

No, it can't. The solution is to make processBlock behave just like the blockchain downloading. They are doing the exact same thing, just that one of them is active, the other one passive.
Just extract the code for accepting blocks within the blockchain downloading in a method and call that method in processBlock instead of calling just pushBlock.

[Come-from-Beyond]

TF isn't the solution to everything... You still have to accept blocks that physically arrive after their creation date due to network latency. Meaning that the attack is possible even with TF as long as you can put future transactions into your block that shouldn't even have existed at that point. And you still will get a lot of orphans and wasted work...
I don't get what's so hard to understand in that attack and why it's even considered to be ok to have transactions that were created after the forge date in a block... Makes no sense at all. And I'm looking forward to the whitepaper where you explain the reasoning behind this.

Actually TF solves most of the problems.
From https://bitcointalk.org/index.php?topic=364218.msg3888740#msg3888740:

2. Blocks can be generated in advance and sent to most of the miners before they become valid (timestamp validation), thus greatly reducing rate of orphaned blocks.

No, it can't. The solution is to make processBlock behave just like the blockchain downloading. They are doing the exact same thing, just that one of them is active, the other one passive.
Just extract the code for accepting blocks within the blockchain downloading in a method and call that method in processBlock instead of calling just pushBlock.

Why "it can't"? Look at futureBlocks and futureTransactions.

[ricot]

TF isn't the solution to everything... You still have to accept blocks that physically arrive after their creation date due to network latency. Meaning that the attack is possible even with TF as long as you can put future transactions into your block that shouldn't even have existed at that point. And you still will get a lot of orphans and wasted work...
I don't get what's so hard to understand in that attack and why it's even considered to be ok to have transactions that were created after the forge date in a block... Makes no sense at all. And I'm looking forward to the whitepaper where you explain the reasoning behind this.

Actually TF solves most of the problems.
From https://bitcointalk.org/index.php?topic=364218.msg3888740#msg3888740:

2. Blocks can be generated in advance and sent to most of the miners before they become valid (timestamp validation), thus greatly reducing rate of orphaned blocks.

Well, the key word in that sentence is CAN. But who would want to do that if he can get more fees by waiting as long as possible? With these things you always have to assume that all people are evil... and in exactly that sentence, you don't.

No, it can't. The solution is to make processBlock behave just like the blockchain downloading. They are doing the exact same thing, just that one of them is active, the other one passive.
Just extract the code for accepting blocks within the blockchain downloading in a method and call that method in processBlock instead of calling just pushBlock.

Why "it can't"? Look at futureBlocks and futureTransactions.

futureBlocks andfutureTransactions are part of the blockchain downloading. That part is fine and doesn't have such a problem.
The problem is in "processBlock". Look in there... no futureBlocks, no futureTransactions, no commonBlockId.

[Come-from-Beyond]

Well, the key word in that sentence is CAN. But who would want to do that if he can get more fees by waiting as long as possible? With these things you always have to assume that all people are evil... and in exactly that sentence, you don't.

I still don't see any problem with that even without TF.

futureBlocks andfutureTransactions are part of the blockchain downloading. That part is fine and doesn't have such a problem.
The problem is in "processBlock". Look in there... no futureBlocks, no futureTransactions, no commonBlockId.

This doesn't prevent from downloading blocks from 100 peers, does it?

[ricot]

Well, the key word in that sentence is CAN. But who would want to do that if he can get more fees by waiting as long as possible? With these things you always have to assume that all people are evil... and in exactly that sentence, you don't.

I still don't see any problem with that even without TF.

The main goal of a PoS currency is to make sure that it is fortunate for the forging entity to forge as fast as possible and therefore make the network fast and efficient.
If you can gain more fees by waiting, letting someone else create a block, then make that block an orphan by finally releasing your block, this goal is not satisfied.
So it is actually your goal to make the network less efficient to get the most value for yourself.
If you still don't understand the implications of this, please discuss with whoever wrote the TF idea, as this method makes a lot of the performance improvements of TF void.
If this is intended, I can guarantee you that someone will make a client that uses that behaviour and advertises it with "make a bit more money when forging than with the default client" and people will use it, because people are greedy.

futureBlocks andfutureTransactions are part of the blockchain downloading. That part is fine and doesn't have such a problem.
The problem is in "processBlock". Look in there... no futureBlocks, no futureTransactions, no commonBlockId.

This doesn't prevent from downloading blocks from 100 peers, does it?

Ok, the situation again.
Anna forges the block in the future and calls processBlock, and everyone accepts it. (so far, so good, no problems)
Enter Bob. Bob is behind a firewall. Bob can create a block which is better than Anna's, but because Bob waited for the right moment, he starts pushing his block later than Anna.
Bob now calls "processBlock" of all of his peers. All of them already have Anna's block and discard Bob's block because it isn't the next block after Anna's.
So still, after pushing the block out to all peers, Bob's superior block is only on Bob's computer.
How can it ever get out? It can't... The only other way for Bob's block to get to another peer is if someone calls Bob's "getBlockIds" etc. post methods. But since Bob is behind a firewall, noone can reach him in that way.
So as soon as someone forges a block based on Anna's block, Bob's block will become an orphan on his computer and no one will ever know that it existed. Poor Bob. And happy Anna, who succesfully stole the block from Bob.

If Bob doesn't sit behind a firewall, the situation is a bit better, but still inefficient. In this case, Bob also sends out his block using "processBlock" and all his peers reject it. But now, his peers will call "getBlockIds" etc. regularly and the block will progress thru the network using that (and only that) code path. Lots of wasted bandwidth, lots of delays, lots of not good.

[BloodyRookie]

Enter Bob. Bob is behind a firewall. Bob can create a block which is better than Anna's, but because Bob waited for the right moment, he starts pushing his block later than Anna.

Maybe I don't get what you mean, but that's a reason for him not to wait for releasing a block.

[Come-from-Beyond]

The main goal of a PoS currency is to make sure that it is fortunate for the forging entity to forge as fast as possible and therefore make the network fast and efficient.

I disagree with this statement. The main goal of a PoS/PoW currency is to guarantee eventual consistency of the public ledger. Nothing else.

Bob now calls "processBlock" of all of his peers. All of them already have Anna's block and discard Bob's block because it isn't the next block after Anna's.

Depends on cumulative difficulty. If Bob's block is better, then Alice's one will be orphaned. If Alice's blocks is better then Bob's one will be ignored.

[ricot]

Enter Bob. Bob is behind a firewall. Bob can create a block which is better than Anna's, but because Bob waited for the right moment, he starts pushing his block later than Anna.

Maybe I don't get what you mean, but that's a reason for him not to wait for releasing a block.

Bob is using the default client, which waits until exactly the time the block should be forged, to send it.

And please be aware that we're talking about two completely separate issues, one where sending a block later benefits you and one where sending a block earlier benefits you. So actually, the worst thing you can do is send it at the time that you should send it. :p

[ricot]

Bob now calls "processBlock" of all of his peers. All of them already have Anna's block and discard Bob's block because it isn't the next block after Anna's.

Depends on cumulative difficulty. If Bob's block is better, then Alice's one will be orphaned. If Alice's blocks is better then Bob's one will be ignored.

Enough.
Here's the code for "processBlock". Look at it and tell me where it checks for cumulated difficulty and whether the new block is better than lastBlock.

Code:

case "processBlock":
	{
		
		int version = ((Long)request.get("version")).intValue();
		int blockTimestamp = ((Long)request.get("timestamp")).intValue();
		long previousBlock = (new BigInteger((String)request.get("previousBlock"))).longValue();
		int numberOfTransactions = ((Long)request.get("numberOfTransactions")).intValue();
		int totalAmount = ((Long)request.get("totalAmount")).intValue();
		int totalFee = ((Long)request.get("totalFee")).intValue();
		int payloadLength = ((Long)request.get("payloadLength")).intValue();
		byte[] payloadHash = convert((String)request.get("payloadHash"));
		byte[] generatorPublicKey = convert((String)request.get("generatorPublicKey"));
		byte[] generationSignature = convert((String)request.get("generationSignature"));
		byte[] blockSignature = convert((String)request.get("blockSignature"));

		Block block = new Block(version, blockTimestamp, previousBlock, numberOfTransactions, totalAmount, totalFee, payloadLength, payloadHash, generatorPublicKey, generationSignature, blockSignature);
		
		ByteBuffer buffer = ByteBuffer.allocate(BLOCK_HEADER_LENGTH + payloadLength);
		buffer.order(ByteOrder.LITTLE_ENDIAN);
		
		buffer.put(block.getBytes());
		
		JSONArray transactionsData = (JSONArray)request.get("transactions");
		for (int i = 0; i < transactionsData.size(); i++) {
			
			buffer.put(Transaction.getTransaction((JSONObject)transactionsData.get(i)).getBytes());
			
		}
		
		boolean accepted = Block.pushBlock(buffer, true);
		response.put("accepted", accepted);
		
	}
	break;

[Come-from-Beyond]

Bob now calls "processBlock" of all of his peers. All of them already have Anna's block and discard Bob's block because it isn't the next block after Anna's.

Depends on cumulative difficulty. If Bob's block is better, then Alice's one will be orphaned. If Alice's blocks is better then Bob's one will be ignored.

Enough.
Here's the code for "processBlock". Look at it and tell me where it checks for cumulated difficulty and whether the new block is better than lastBlock.

Code:

case "processBlock":
	{
		
		int version = ((Long)request.get("version")).intValue();
		int blockTimestamp = ((Long)request.get("timestamp")).intValue();
		long previousBlock = (new BigInteger((String)request.get("previousBlock"))).longValue();
		int numberOfTransactions = ((Long)request.get("numberOfTransactions")).intValue();
		int totalAmount = ((Long)request.get("totalAmount")).intValue();
		int totalFee = ((Long)request.get("totalFee")).intValue();
		int payloadLength = ((Long)request.get("payloadLength")).intValue();
		byte[] payloadHash = convert((String)request.get("payloadHash"));
		byte[] generatorPublicKey = convert((String)request.get("generatorPublicKey"));
		byte[] generationSignature = convert((String)request.get("generationSignature"));
		byte[] blockSignature = convert((String)request.get("blockSignature"));

		Block block = new Block(version, blockTimestamp, previousBlock, numberOfTransactions, totalAmount, totalFee, payloadLength, payloadHash, generatorPublicKey, generationSignature, blockSignature);
		
		ByteBuffer buffer = ByteBuffer.allocate(BLOCK_HEADER_LENGTH + payloadLength);
		buffer.order(ByteOrder.LITTLE_ENDIAN);
		
		buffer.put(block.getBytes());
		
		JSONArray transactionsData = (JSONArray)request.get("transactions");
		for (int i = 0; i < transactionsData.size(); i++) {
			
			buffer.put(Transaction.getTransaction((JSONObject)transactionsData.get(i)).getBytes());
			
		}
		
		boolean accepted = Block.pushBlock(buffer, true);
		response.put("accepted", accepted);
		
	}
	break;

boolean accepted = Block.pushBlock(buffer, true);

Edit: I got ur point. I confused pushBlock with pull block thread.

[Come-from-Beyond]

Anna forges the block in the future and calls processBlock, and everyone accepts it. (so far, so good, no problems)
Enter Bob. Bob is behind a firewall. Bob can create a block which is better than Anna's, but because Bob waited for the right moment, he starts pushing his block later than Anna.
Bob now calls "processBlock" of all of his peers. All of them already have Anna's block and discard Bob's block because it isn't the next block after Anna's.
So still, after pushing the block out to all peers, Bob's superior block is only on Bob's computer.
How can it ever get out? It can't... The only other way for Bob's block to get to another peer is if someone calls Bob's "getBlockIds" etc. post methods. But since Bob is behind a firewall, noone can reach him in that way.
So as soon as someone forges a block based on Anna's block, Bob's block will become an orphan on his computer and no one will ever know that it existed. Poor Bob. And happy Anna, who succesfully stole the block from Bob.

If Bob doesn't sit behind a firewall, the situation is a bit better, but still inefficient. In this case, Bob also sends out his block using "processBlock" and all his peers reject it. But now, his peers will call "getBlockIds" etc. regularly and the block will progress thru the network using that (and only that) code path. Lots of wasted bandwidth, lots of delays, lots of not good.

Anna can't forge a block far in the future. Some nodes accessible on the Internet will have Bob's block and eventually network will orphane Anna's block. Does it look legit?

[Come-from-Beyond]

Code:

if (version != 1) {
				
				return false;
				
			}

I know my question is not applicable to the Genesis Block, which has -1 as its version, but where is it ensured that version will be 1 in all other cases, so that blocks get pushed?

For example:

Code:

Block block = new Block(1, getEpochTime(System.currentTimeMillis()), lastBlock, newTransactions.size(), 0, 0, 0, null, Crypto.getPublicKey(secretPhrase), null, new byte[64]);

[ricot]

boolean accepted = Block.pushBlock(buffer, true);

Edit: I got ur point. I confused pushBlock with pull block thread.

Finally  
You see, a better code quality would have helped here...
Btw: I'm doing all my analyses on a very modified code which is much easier to look at. You should definitely consider refactoring as a main action before other features.

Actually, I'd very much recommend rewriting it completely.
Putting all the essential calculation stuff into a library and making a servlet that just calls the library.
And then make unit tests for every class, every function.
You're dealing with a market cap of 63M$, your users should feel save and should be able to verify that they can feel that way.


As for my other point:
Aren't two of your currency's goals to be environmentally friendly and fast? If it's better for a node in the system to behave more inefficiently to gain an advantage, those goals are hard to fulfill.

[edit]

Anna can't forge a block far in the future. Some nodes accessible on the Internet will have Bob's block and eventually network will orphane Anna's block. Does it look legit?

Anna can forge and successfully transmit a block 15 seconds into the future. All my calculations are based on that 15 second advantage. So if Bob's block is 10s before Anna's block, all peers will already have had Anna's block for 5 seconds when Bob's block arrives.

[edit2]
Actually, when looking at the code that starts the block generation for Bob:
If Anna managed to broadcast her block to Bob before he starts forging (that 15 second window), Bob won't even start to forge because the Thread that starts forging only looks at Block.getLastBlock() as a forging target.
Poor Bob is only getting poorer and will never be happy, now even if he doesn't sit behind a firewall. -> This is now an exploit applicable for ALL forging accounts. You can increase your forging "luck" by sending out blocks 15 seconds earlier and making sure that you spam everyone with them.

[FrictionlessCoin]

Actually, I'd very much recommend rewriting it completely.
Putting all the essential calculation stuff into a library and making a servlet that just calls the library.
And then make unit tests for every class, every function.
You're dealing with a market cap of 63M$, your users should feel save and should be able to verify that they can feel that way.

Yes, same sentiment here.

Make the critical stuff testable too.

Gut the entire code base.

It's becoming completely unmaintainable at its current form.

Let's start from scratch and do this right!

http://bitcoin-ng.boards.net/

[bidji29]

The reason this 15 second period exist is to prevent slighly unsynchronized node to be excluded from the network am i right?

So if Anna send block 15sec too soon. Some nodes will accept it but some other will refuse it because it's >15sec for them?

[FrictionlessCoin]

The reason this 15 second period exist is to prevent slighly unsynchronized node to be excluded from the network am i right?

So if Anna send block 15sec too soon. Some nodes will accept it but some other will refuse it because it's >15sec for them?

When you got magic numbers like this that try to fudge timing details,  then it is likely the code in entirely wrong and needs some serious analysis or simulation.

I wouldn't trust code like this that tries to add some arbitrary delay to fix a fundamental flaw in the distributed algorithm.

That is why for asynchronous parallel processes, you better have a good spec. of you'll be forever hunting down bugs.

[ricot]

The reason this 15 second period exist is to prevent slighly unsynchronized node to be excluded from the network am i right?

So if Anna send block 15sec too soon. Some nodes will accept it but some other will refuse it because it's >15sec for them?

Yes, some nodes will decline the block because it's e.g. 16s in the future. That's why Anna has to spam the block until the peer accepts it.
In my original description of the attack I also mentioned that sending a block which is more than 15s in the future puts you on the blacklist, however, the method you use to send the block to your peer does not check whether you are on the blacklist. So you can safely just spam the network with your futuristic blocks.

[bidji29]

The reason this 15 second period exist is to prevent slighly unsynchronized node to be excluded from the network am i right?

So if Anna send block 15sec too soon. Some nodes will accept it but some other will refuse it because it's >15sec for them?

Yes, some nodes will decline the block because it's e.g. 16s in the future. That's why Anna has to spam the block until the peer accepts it.
In my original description of the attack I also mentioned that sending a block which is more than 15s in the future puts you on the blacklist, however, the method you use to send the block to your peer does not check whether you are on the blacklist. So you can safely just spam the network with your futuristic blocks.

So the code should checking for blacklist it seems.
If you push your block 30sec earlier, would the node desynchronised 15sec in the future  accept it? It make things even worth


But anyway, this 15 sec windows seems an overkill. Maybe i'm wrong but it's easy to synchronise yourself to internet time. What about a 3sec windows?
The abuse would still be present, but weaker.

[xibeijan]

Btw: This thing would never go thru any kind of (government mandated) review in the banking or automotive industry, just saying...

Agree. This code is just an implementation of the idea, not a finished product.

Don't know how your investors should feel about this.

As I've said from the beginning, the amazing ideas and innovations behind NXT are the important part.  I don't care if it's was conceived and implemented by a 7 year old (prodigy?).  The ideas (and the extremely clever chaps behind the NXT project) are why NXT is a game changer.  Implementations can be improved (or rewritten).

I'd rather have a stake in a technology with an amazing capabilities (and people) behind it, which solves a important previously unsolved problems, than a clean implementation that does nothing to improve the world.

N.B. I believe the last two issues posted by ricot

I'm still waiting on statements concerning these two problems:

- Waiting past the creation time of a block to add additinal future transactions gains malicious Bob more coins when forging and causes the creation of unneccessary orphan blocks.
- Malicious Anna can gain ~7.5% more forges by spamming the network with early-generated blocks.

Are both of those working as intended? If yes: The network will become a lot more inefficient in the near future  

are relatively minor and can be fixed.

[ferment]

Does it really stuck in this case? Have u checked?

Yes, I started with 2 well known Peer, both had hallmarks. I logged the method calls in the peer class:

Good find. I've been plagued with that on my nodes. Tip sent...