Nxt source code flaw reports

[gsan1]

For example, line 63 - initialBaseTarget = 153722867!?!?!

This is a magic number. 1 billion coins generate blocks at 1 block/min rate when BaseTarget == 153722867.

Is 1048576 also a magic number?

I think 1048576 is 1 megaybyte in bytes. I wonder why it is used to compare transactions.

Code:

if (fee * 1048576L / getBytes().length > o.fee * 1048576L / o.getBytes().length) {

You multiply the fee with 1MB (in bytes) and divide it through getBytes().length. getBytes().length should just be different, when the transaction has an attachment. Otherwise getBytes().length should always be 10, right? I wonder why you multiply the fee with the transaction length.

[liyi3c]

Code:

while (iterator.hasNext()) {
								
								Transaction transaction = iterator.next();
								if (transaction.timestamp + transaction.deadline * 60 < curTime || !transaction.validateAttachment()) {
									
									iterator.remove();
									
									Account account = accounts.get(Account.getId(transaction.senderPublicKey));
									synchronized (account) {
										
										account.setUnconfirmedBalance(account.unconfirmedBalance + (transaction.amount + transaction.fee) * 100L);
										
									}
									
									JSONObject removedUnconfirmedTransaction = new JSONObject();
									removedUnconfirmedTransaction.put("index", transaction.index);
									removedUnconfirmedTransactions.add(removedUnconfirmedTransaction);
									
								}
								
							}

transaction.deadline * 60 is minutes while curTime is second.

[Come-from-Beyond]

Code:

if (fee * 1048576L / getBytes().length > o.fee * 1048576L / o.getBytes().length) {

You multiply the fee with 1MB (in bytes) and divide it through getBytes().length. getBytes().length should just be different, when the transaction has an attachment. Otherwise getBytes().length should always be 10, right? I wonder why you multiply the fee with the transaction length.

This is used to sort transactions in descending order according to FEE/SIZE ratio.

[Come-from-Beyond]

transaction.deadline * 60 is minutes while curTime is second.

transaction.deadline * 60 converts minutes to seconds. No problems here.

[Meizirkki]

Line 170:

Code:

			height = Block.getLastBlock().height;

Lines 318-322

Code:

			if (Block.getLastBlock().height - height < 1440) {
				
				return 0;
				
			}

Aren't Block.getLastBlock().height and height the exact same number in the above function?

[gsan1]

Line 170:

Code:

			height = Block.getLastBlock().height;

Lines 318-322

Code:

			if (Block.getLastBlock().height - height < 1440) {
				
				return 0;
				
			}

Aren't Block.getLastBlock().height and height the exact same number in the above function?

Line 170 gets triggered when a new account is created -> So height in the account object means the height of the last block when creating the account.

Line 318 gets triggered when calculating the effective balance of the account. At this point Block.getLastBlock() "could" refer to another block  (because new blocks get added) as the one above.

So basically the code is checking if the user account is 1440 blocks old.

[Meizirkki]

Line 170:

Code:

			height = Block.getLastBlock().height;

Lines 318-322

Code:

			if (Block.getLastBlock().height - height < 1440) {
				
				return 0;
				
			}

Aren't Block.getLastBlock().height and height the exact same number in the above function?

Line 170 gets triggered when a new account is created -> So height in the account object means the height of the last block when creating the account.

Line 318 gets triggered when calculating the effective balance of the account. At this point Block.getLastBlock() "could" refer to another block  (because new blocks get added) as the one above.

So basically the code is checking if the user account is 1440 blocks old.

I see. Thanks

[Come-from-Beyond]

Line 4243

Block block = new Block(-1, 0, 0, transactions.size(), 1000000000, 0...

Please explain the -1 above...  thnx   

Version of the genesis block is "-1".

[Come-from-Beyond]

Why are there special conditions to synchronized (blocks) while numberOfBlocks< 60?

Interface shows not more than 60 last blocks.

[Come-from-Beyond]

...if you'd rather have me ask these questions in the other topic, please let me know...

It's ok. Let's stick to this thread, ur questions help others to understand the logic of the code.

[ImmortAlex]

what is the reason to add 15 milliseconds in pushBlock method to the current epochtime?

To counteract a time travel attack.

Code:

            int curTime = getEpochTime(System.currentTimeMillis());
            if (blockTimestamp > curTime + 15 || blockTimestamp <= Block.getLastBlock().timestamp) {

                return false;

            }

But getEpochTime() returns seconds, and block timestamp is seconds too. So 15 is seconds, right?

[Come-from-Beyond]

Code:

            int curTime = getEpochTime(System.currentTimeMillis());
            if (blockTimestamp > curTime + 15 || blockTimestamp <= Block.getLastBlock().timestamp) {

                return false;

            }

But getEpochTime() returns seconds, and block timestamp is seconds too. So 15 is seconds, right?

Yes.

[vamdor]

(1) How do you ensure "chronological order in the block chain".
(2) What are these "strict cryptographic rules"
(3) What is the mechanism of this "lottery".

I agree that these are fundamental questions, and are the only reason why I wanted to look into the code, or have some specifications. This coin advertises itself as the first 100% proof-of-stake system, without revealing anything about how they solved the problems that prevented others to create one.

Looking into the code I couldn't find anything addressing the usual concerns about 100% PoS. (or is this one of the "injected" bugs? the part of the code securing PoS is simply missing?)

Answer to (1) is pretty much the same as Bitcoin: it just builds a chain of blocks, stores everything, cross-checks new blocks with old ones. It adds some spin to how accounts and public keys are stored, but there is no real innovation here.

(2) and (3) are more interesting. The algorithms seems to be the straightforward PoS: From the previous block it creates a hash. Each second it creates a (hash('time elapsed since last block' ,  'own account key') * 'amount of coins on account') which it calls "target", and that determines whether the account has the right to create a new block or not. Additionally 'amount of coins' is zeroed out if the account has created a block within the last 1440 blocks.

I spent only around 30 mins reading the code, so at this point there are three options:
- There are somewhere else some additional rules to make the scheme actually secure that I haven't found.
- This is one of the purposefully injected bugs.
- The PoS is insecure and doesn't even attempt to address the issues that prompted the creators of other coins (such as peercoin) to also incorporate PoW to fix the shortcoming of PoS.

[xibeijan]

(1) How do you ensure "chronological order in the block chain".
(2) What are these "strict cryptographic rules"
(3) What is the mechanism of this "lottery".

I agree that these are fundamental questions, and are the only reason why I wanted to look into the code, or have some specifications. This coin advertises itself as the first 100% proof-of-stake system, without revealing anything about how they solved the problems that prevented others to create one.

Looking into the code I couldn't find anything addressing the usual concerns about 100% PoS. (or is this one of the "injected" bugs? the part of the code securing PoS is simply missing?)

Answer to (1) is pretty much the same as Bitcoin: it just builds a chain of blocks, stores everything, cross-checks new blocks with old ones. It adds some spin to how accounts and public keys are stored, but there is no real innovation here.

(2) and (3) are more interesting. The algorithms seems to be the straightforward PoS: From the previous block it creates a hash. Each second it creates a (hash('time elapsed since last block' ,  'own account key') * 'amount of coins on account') which it calls "target", and that determines whether the account has the right to create a new block or not. Additionally 'amount of coins' is zeroed out if the account has created a block within the last 1440 blocks.

I spent only around 30 mins reading the code, so at this point there are three options:
- There are somewhere else some additional rules to make the scheme actually secure that I haven't found.
- This is one of the purposefully injected bugs.
- The PoS is insecure and doesn't even attempt to address the issues that prompted the creators of other coins (such as peercoin) to also incorporate PoW to fix the shortcoming of PoS.

I agree strongly that a spec/whitepaper is desperately needed.  Can someone from the NXT community (who is capable) create this document, for peer review?

Meanwhile, you have not described (even vaguely) the particular issue(s) you are concerned about (e.g. is it a Sybil attack?).  Could you please post a reference?

Then, maybe someone can explain how it's either addressed or not a problem.

[bitcoinpaul]

I agree strongly that a spec/whitepaper is desperately needed.  Can someone from the NXT community (who is capable) create this document, for peer review?

https://bitcointalk.org/index.php?topic=397183.msg4294653#msg4294653

[Come-from-Beyond]

- There are somewhere else some additional rules to make the scheme actually secure that I haven't found.

What do we need an additional rule for?

- This is one of the purposefully injected bugs.

No

- The PoS is insecure and doesn't even attempt to address the issues that prompted the creators of other coins (such as peercoin) to also incorporate PoW to fix the shortcoming of PoS.

Scheme, proposed by BCNext, is a straightforward simulation of PoW. Each coin is just a small mining rig. Other PoS schemes work in other way and require workarounds based on PoW.

[ImmortAlex]

I am also trying to figure out why static final int BLOCK_HEADER_LENGTH needs to be 224 as well!?!  

Take a look at Block.getBytes(), line 745:

Code:

ByteBuffer buffer = ByteBuffer.allocate(4 + 4 + 8 + 4 + 4 + 4 + 4 + 32 + 32 + 64 + 64);

And then you'll see what is putted in this buffer.

[Come-from-Beyond]

Wouldn't it make more sense for...  

static final int MAX_PAYLOAD_LENGTH = 255 * 128;  

to be...

static final int MAX_PAYLOAD_LENGTH = 256 * 128; ?

We can place any number here. 255 is just the highest 8-bit unsigned number.

Looking at the code... I have a feeling that there might be a vulnerability in regard to generating Authorization Tokens and/or transaction sorting caused by incorrect length attributes/permissions... but I am not able to point to anything else besides the above for now.

There is no an inject flaw here. But possibility of a bug is still > 0%.

I am also trying to figure out why static final int BLOCK_HEADER_LENGTH needs to be 224 as well!?!  

Coz block header elements need 224 bytes of storage.

[xibeijan]

I agree strongly that a spec/whitepaper is desperately needed.  Can someone from the NXT community (who is capable) create this document, for peer review?

https://bitcointalk.org/index.php?topic=397183.msg4294653#msg4294653

Someone else needs to take over.

[xibeijan]

- There are somewhere else some additional rules to make the scheme actually secure that I haven't found.

Scheme, proposed by BCNext, is a straightforward simulation of PoW. Each coin is just a small mining rig. Other PoS schemes work in other way and require workarounds based on PoW.

I don't think anyone can answer his concerns unless he's specific about what he's concerned about.