Nxt source code flaw reports - page 39.

ferment

full member

Activity: 168

Merit: 100

IDEX - LIVE Real-time DEX

Quote from: Come-from-Beyond on January 06, 2014, 11:33:37 AM

Quote from: ImmortAlex on January 06, 2014, 11:30:45 AM

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?

ferment

full member

Activity: 168

Merit: 100

IDEX - LIVE Real-time DEX

Quote from: Come-from-Beyond on January 06, 2014, 11:33:37 AM

Quote from: ImmortAlex on January 06, 2014, 11:30:45 AM

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.

Come-from-Beyond

legendary

Activity: 2142

Merit: 1009

Newbie

Quote from: ImmortAlex on January 06, 2014, 11:30:45 AM

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.

ImmortAlex

hero member

Activity: 784

Merit: 501

Quote from: ferment on January 06, 2014, 11:19:03 AM

Quote from: ImmortAlex on January 06, 2014, 11:14:16 AM

Quote from: ricot on January 06, 2014, 11:09:16 AM

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

legendary

Activity: 2142

Merit: 1009

Newbie

Quote from: gsan1 on January 06, 2014, 11:22:50 AM

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

Yes. New version already has this fixed.

Come-from-Beyond

legendary

Activity: 2142

Merit: 1009

Newbie

Quote from: theKnownUnknown on January 06, 2014, 11:22:15 AM

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

New version has TF enabled.

gsan1

newbie

Activity: 50

Merit: 0

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

legendary

Activity: 2142

Merit: 1009

Newbie

Quote from: theKnownUnknown on January 06, 2014, 11:05:34 AM

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.

theKnownUnknown

newbie

Activity: 16

Merit: 0

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

ImmortAlex

hero member

Activity: 784

Merit: 501

Quote from: theKnownUnknown on January 06, 2014, 11:12:18 AM

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.

ferment

full member

Activity: 168

Merit: 100

IDEX - LIVE Real-time DEX

Quote from: ImmortAlex on January 06, 2014, 11:14:16 AM

Quote from: ricot on January 06, 2014, 11:09:16 AM

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

hero member

Activity: 784

Merit: 501

Quote from: ricot on January 06, 2014, 11:09:16 AM

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.

theKnownUnknown

newbie

Activity: 16

Merit: 0

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)?

ricot

newbie

Activity: 56

Merit: 0

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.

ImmortAlex

hero member

Activity: 784

Merit: 501

Quote from: theKnownUnknown on January 06, 2014, 11:05:34 AM

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

Where have you seen such flaw?

theKnownUnknown

newbie

Activity: 16

Merit: 0

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

ImmortAlex

hero member

Activity: 784

Merit: 501

Quote from: ricot on January 06, 2014, 10:46:19 AM

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!

ImmortAlex

hero member

Activity: 784

Merit: 501

Quote from: Come-from-Beyond on January 06, 2014, 10:36:18 AM

Quote from: ImmortAlex on January 06, 2014, 09:51:54 AM

Quote from: Come-from-Beyond on January 06, 2014, 09:44:12 AM

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.

ricot

newbie

Activity: 56

Merit: 0

Quote from: Come-from-Beyond on January 06, 2014, 10:36:18 AM

Quote from: ImmortAlex on January 06, 2014, 09:51:54 AM

Quote from: Come-from-Beyond on January 06, 2014, 09:44:12 AM

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 Wink

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.

Come-from-Beyond

legendary

Activity: 2142

Merit: 1009

Newbie

Quote from: ImmortAlex on January 06, 2014, 09:51:54 AM

Quote from: Come-from-Beyond on January 06, 2014, 09:44:12 AM

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?

Topic: Nxt source code flaw reports - page 39. (Read 113359 times)