Neo: Use cryptographically safe random numbers on nonce for "Neo 3.0 MinerTx"

[igormcoelho]

Current nonce is quite weak on MinerTx, and as it should be abolished together will all others (except InvocationTx), it's a good time to drastically increase this entropy with a cryptographically safe random generator (with much larger scale than 4 or 8 bytes, let's take 32 or more).
All languages have means to take this from the host system (as bindings for /dev/random for example), and it will improve a lot the usability of such numbers.

[igormcoelho]

Thanks @coranos for pointint this out: https://github.com/neo-project/neo/issues/195

[vncoelho]

I was talking to him offline to create an issue about it, you are fast, brother.
Your proposed idea looks already good.

[coranos]

Cool. If I get a bug bonus let me know how to collect. It's been a while (nearly 12 months)

[igormcoelho]

In fact, this is just an improvement @coranos (not bug), as the situation you mentioned is not possible.. MinerTx hash is always verified before submission on block (it must be unique). So even if nonce has been "reused", it's never accepted (no repeated tx hash is allowed on Neo blockchain).
Anyway, it would be nice to have better randomization here.

[coranos]

Are you sure? Miner Transactions aren't added to the mempool:
https://github.com/neo-project/neo/blob/a69b0ce1ea217843e8f16dd37526a75b1fceac01/neo/Ledger/Blockchain.cs#L394
https://github.com/neo-project/neo/blob/a69b0ce1ea217843e8f16dd37526a75b1fceac01/neo/Ledger/Blockchain.cs#L395

and I don't see anywhere that the miner transaction's hash is specifically sent to ContainsTransaction. (no instantiation calls the below method that I can find)
https://github.com/neo-project/neo/blob/a69b0ce1ea217843e8f16dd37526a75b1fceac01/neo/Ledger/Blockchain.cs#L190

If it's just me not reading the code right, I apologize for the wild goose chase.

[igormcoelho]

I guess this is the infinite loop that guarantees that:

https://github.com/neo-project/neo/blob/master/neo/Consensus/ConsensusContext.cs#L367-L384

[coranos]

Cool, at least it's fixed. 🔀

[shargon]

Agree with this, we should use a big nonce

[vncoelho]

@shargon and @igormcoelho, maybe it is time to PR this, right? The minertx was already abolished.

Let's create a draft.

[shargon]

We don't have minner tx in neo3, so we can close this, feel free to open again if it's necessary