Nxt source code flaw reports

[Come-from-Beyond]

We need guys like him in our team. Hey, big stakeholders, do u hear me?

Definitely. ricot found most interesting flaws in last days, not a small local mistakes, but broken logic in execution flow.

Btw, did you miss my observation in shadows of big ricot investigation? Any comment, please...

I need more time to check it, do u know how to fix it if it's a bug?

[ricot]

We need guys like him in our team. Hey, big stakeholders, do u hear me?

Definitely. ricot found most interesting flaws in last days, not a small local mistakes, but broken logic in execution flow.

Btw, did you miss my observation in shadows of big ricot investigation? Any comment, please...

I need more time to check it, do u know how to fix it if it's a bug?

The crude fix would be to just add a synchronize(blocks) around the whole processBlock thingy
The proper fix: Writing a proper software that doesn't mix the servlet with the block processing


[edit]
Btw: There seems to be another bug or attack whose result is that a node responds to getCumulativeDifficulty with {"error":"java.lang.NullPointerException"}
My client hasn't experienced that behaviour yet, but if anyone's has, it's another nice chance to debug.

[ImmortAlex]

We need guys like him in our team. Hey, big stakeholders, do u hear me?

Definitely. ricot found most interesting flaws in last days, not a small local mistakes, but broken logic in execution flow.

Btw, did you miss my observation in shadows of big ricot investigation? Any comment, please...

I need more time to check it, do u know how to fix it if it's a bug?

There's no simple way.
First of all, syncronization code is already rewritten, so any of my suggestions can be invalid.
Simplest way is totally sync "processBlock" request and loading thread on blocks, so either pushBlock() rejects new block as already loaded, or loading thread choose another commonBlockId.
But it is looks like dirty hack. I personally don't like total syncronizations.
I think, Jean-Luc as author of refactorings must check my assumptions on newer code.

[ImmortAlex]

Btw: There seems to be another bug or attack whose result is that a node responds to getCumulativeDifficulty with {"error":"java.lang.NullPointerException"}
My client hasn't experienced that behaviour yet, but if anyone's has, it's another nice chance to debug.

No need to debug. Look at analyse() method:

Code:

                height = Block.getLastBlock().height + 1;
                lastBlock = getId();
                blocks.put(lastBlock, this);
                baseTarget = Block.getBaseTarget();
                cumulativeDifficulty = blocks.get(previousBlock).cumulativeDifficulty.add(two64.divide(BigInteger.valueOf(baseTarget)));

This code set lastBlock with block that have null cumulativeDifficulty, then calc it.
"getCumulativeDifficulty" executes asynchronously and can catch last block in state with cumulativeDifficulty = null, so

Code:

response.put("cumulativeDifficulty", Block.getLastBlock().cumulativeDifficulty.toString());

throws NPE.

Good catch!

[theKnownUnknown]

Is it one of the flaws that blocks do not contain the hashcode of their previous block?

[ImmortAlex]

Is it one of the flaws that blocks do not contain the hashcode of their previous block?

Where have you seen such flaw?

[ricot]

Nice one, but that's not it.
The nodes I mean reply with the Nullpointer every time.
You can try wget -q -S -O - --post-data='{"protocol":1,"requestType":"getCumulativeDifficulty"}' 'http://node81.nxtbase.com:7874/nxt'
this one has been in that state for a few hours now.

[theKnownUnknown]

Block class does not contain hashcode of previous block, only block id and pushBlock() does not check anything into this direction. Isn't it possible to modify old blocks afterwards (By the original author of the block)?

[ImmortAlex]

Nice one, but that's not it.
The nodes I mean reply with the Nullpointer every time.
You can try wget -q -S -O - --post-data='{"protocol":1,"requestType":"getCumulativeDifficulty"}' 'http://node81.nxtbase.com:7874/nxt'
this one has been in that state for a few hours now.

Code:

http://node81.nxtbase.com:7874/nxt?requestType=getState

{"lastBlock":"2680262203532249785","numberOfAliases":0,"numberOfBlocks":0,"numberOfPeers":182,"lastBlockchainFeeder":null,"totalMemory":709558272,"freeMemory":637372280,"maxMemory":912261120,"numberOfTransactions":0,"numberOfUsers":0,"version":"0.5.0","numberOfOrders":0,"time":3726746,"availableProcessors":1,"numberOfAssets":0,"numberOfAccounts":0}

It's dead, man. Dead and smells bad.

[ferment]

Nice one, but that's not it.
The nodes I mean reply with the Nullpointer every time.
You can try wget -q -S -O - --post-data='{"protocol":1,"requestType":"getCumulativeDifficulty"}' 'http://node81.nxtbase.com:7874/nxt'
this one has been in that state for a few hours now.

Code:

http://node81.nxtbase.com:7874/nxt?requestType=getState

{"lastBlock":"2680262203532249785","numberOfAliases":0,"numberOfBlocks":0,"numberOfPeers":182,"lastBlockchainFeeder":null,"totalMemory":709558272,"freeMemory":637372280,"maxMemory":912261120,"numberOfTransactions":0,"numberOfUsers":0,"version":"0.5.0","numberOfOrders":0,"time":3726746,"availableProcessors":1,"numberOfAssets":0,"numberOfAccounts":0}

It's dead, man. Dead and smells bad.

Yes, there is some combination of circumstances (aka bug) that prevent a portion of my nodes from never getting the blockchain. I'm not the best with tcpdump but I could probably dig around.

[ImmortAlex]

Block class does not contain hashcode of previous block, only block id and pushBlock() does not check anything into this direction. Isn't it possible to modify old blocks afterwards (By the original author of the block)?

It contains prev block hash in 0.5.0. At least I see it in decompiled code.
Don't know, is it fixed flaw, or injected.

[theKnownUnknown]

Mh demotivating to find flaws in old code that are fixed already in the new one.

[Come-from-Beyond]

Is it one of the flaws that blocks do not contain the hashcode of their previous block?

Only 64 bits of 256 bit r used to reference previous block.

[gsan1]

I took a look at the peer handling. There is a thread running, which gets any peer from my list and asks him for his peers, to which I perhaps could connect. So it makes a post request to any peer "getPeers". Let's take a look at the code:

Code:

case "getPeers":
{
    JSONArray peers = new JSONArray();
    for (Peer otherPeer : Nxt.peers.values()) {

        if (otherPeer.blacklistingTime == 0 && otherPeer.announcedAddress.length() > 0) {

            peers.add(otherPeer.announcedAddress);

        }

    }
    response.put("peers", peers);

}
break;

 

The other peer is answering with his peers list, where blacklistingTime = 0 and announcedAddress > 0. Seems okay. But wait - does that mean that he is also answering with peers he is not or not more connected with (maybe because they are down)? Would'nt it be better to answer only the peers he is currently connected with?

[Come-from-Beyond]

Mh demotivating to find flaws in old code that are fixed already in the new one.

New version has TF enabled.

[Come-from-Beyond]

Would'nt it be better to answer only the peers he is currently connected with?

Yes. New version already has this fixed.

[ImmortAlex]

Nice one, but that's not it.
The nodes I mean reply with the Nullpointer every time.
You can try wget -q -S -O - --post-data='{"protocol":1,"requestType":"getCumulativeDifficulty"}' 'http://node81.nxtbase.com:7874/nxt'
this one has been in that state for a few hours now.

Code:

http://node81.nxtbase.com:7874/nxt?requestType=getState

{"lastBlock":"2680262203532249785","numberOfAliases":0,"numberOfBlocks":0,"numberOfPeers":182,"lastBlockchainFeeder":null,"totalMemory":709558272,"freeMemory":637372280,"maxMemory":912261120,"numberOfTransactions":0,"numberOfUsers":0,"version":"0.5.0","numberOfOrders":0,"time":3726746,"availableProcessors":1,"numberOfAssets":0,"numberOfAccounts":0}

It's dead, man. Dead and smells bad.

Yes, there is some combination of circumstances (aka bug) that prevent a portion of my nodes from never getting the blockchain. I'm not the best with tcpdump but I could probably dig around.

This is very unusial situation.
Take a look at "getState" query:

Code:

response.put("numberOfBlocks", blocks.size());

There's no call like remove() on blocks collection. Even popBlock() doesn't flush block from it. So it can be empty only because of bad startup. Very bad startup, because even if blocks.nxt doesn't exists or damaged, genesis block is synthesized and putted to collection.

Or there may be some new code which we do not have.

[Come-from-Beyond]

This is very unusial situation.
Take a look at "getState" query:

Code:

response.put("numberOfBlocks", blocks.size());

There's no call like remove() on blocks collection. Even popBlock() doesn't flush block from it. So it can be empty only because of bad startup. Very bad startup, because even if blocks.nxt doesn't exists or damaged, genesis block is synthesized and putted to collection.

That's correct. Something really bad happened during initialization.

[ferment]

This is very unusial situation.
Take a look at "getState" query:

Code:

response.put("numberOfBlocks", blocks.size());

There's no call like remove() on blocks collection. Even popBlock() doesn't flush block from it. So it can be empty only because of bad startup. Very bad startup, because even if blocks.nxt doesn't exists or damaged, genesis block is synthesized and putted to collection.

That's correct. Something really bad happened during initialization.

Yes, these nodes were started from deleted *.nxt.

[ferment]

This is very unusial situation.
Take a look at "getState" query:

Code:

response.put("numberOfBlocks", blocks.size());

There's no call like remove() on blocks collection. Even popBlock() doesn't flush block from it. So it can be empty only because of bad startup. Very bad startup, because even if blocks.nxt doesn't exists or damaged, genesis block is synthesized and putted to collection.

That's correct. Something really bad happened during initialization.

So, I got it to catchup by deleting *.nxt again, changing 'communicationLoggingMask' to 7. It's worth mentioning that communicationLoggingMask didn't log until I deleted *.nxt and restarted.

Everything looked good so I stopped java with supervisorctl, edited web.xml to turn off log, and started NRS again. Then it got stuck again back at the genesis block.

A SIGTERM shouldn't corrupt anything?