Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add VRF nonce to the consensus. #590

Closed
wants to merge 40 commits into from
Closed

Add VRF nonce to the consensus. #590

wants to merge 40 commits into from

Conversation

Jim8y
Copy link
Contributor

@Jim8y Jim8y commented May 18, 2021

This pull request adds VRF support to the dBFT consensus.
Proof of the VRF only exists in the PrepareRequest message.


if (nonce == null)
{
Log($"Random number verification failed: {message.VRFProof}", LogLevel.Warning);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer throw an error in random method instead of break the consensus

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And maybe ask ChangeView.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And maybe ask ChangeView.

I just follow the way how you process other check failures. If you think ChangeView is better, I can add it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer throw an error in random method instead of break the consensus

Updated by adding exception

src/DBFTPlugin/ConsensusContext.cs Outdated Show resolved Hide resolved
private Tuple<byte[], uint> GetNonce(byte[] prikey)
{
byte[] aplha;
if (Block.Index > 1000)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the % with 1000?

try
{
if (message.BlockIndex > 1000)
// To prevent the primary uses the same block hash in Height (1000, 2000)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not understand this comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to use the 1000th ahead block hash as the current VRF input to generate the random number. However, if the block index is less than 1000, there will be out of index error. So I have to make the first 1000 block to use the prevHash as VRF input. The problem here is, if the first 1000 block uses prevHash, and Block of 1000 - 2000 use the hash of 1000th ahead block, it is possible that the same primary use the same block hash to generate random numbers, which means there might have a same random number, one among (0..1000), another among (1000, 2000).

byte[] nonce;
try
{
nonce = VRF.Verify(context.Validators[message.ValidatorIndex], message.VRFProof, message.PrevHash);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add the current merkletree in data

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we add the current Merkle tree in the data, the result would be mutable for the primary cause he can decide the Merkle tree.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

he can decide the Merkle tree.

But not the PrevHash, it makes the things more complicated

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the block hash is a normal and the most easy way to get a well known input. Currently, almost all projects that use VRF take block hash as input, either prehash or previous n blocks. But if u have a better input source, definitely it is a good thing.

src/DBFTPlugin/ConsensusContext.cs Outdated Show resolved Hide resolved
@nicolegys
Copy link
Contributor

Sometimes run into an exception.
31bytes
image
image

@nicolegys
Copy link
Contributor

@Liaojinghui I think here's why
image

Comment on lines 218 to 222
var kBytes = k.ToByteArray(true, true);

// Step 6: c = ECVRF_hash_points(H, Gamma, k*B, k*H)
var u_point = DerivePubkeyPoint(k.ToByteArray(true, true));
var v_point = h_point * kBytes;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed the error, the issue is neo added extra length check to the prikey @nicolegys

We should append leading zeros to kBytes after var kBytes = k.ToByteArray(true, true) in Prove method, because the operator * requires the second param to be 32 bits.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Liaojinghui I think you've forgotten this issue 0.0

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Liaojinghui I think you've forgotten this issue 0.0

My fault TT

@nicolegys
Copy link
Contributor

I sent a tx to extract neo from the CNs' multi-sig address, then the CNs threw exception and stop generating blocks, and cannot recover.
image

@Jim8y
Copy link
Contributor Author

Jim8y commented Jun 2, 2021

Fixed, I changed the index of nonce_tx in the txs, and missed checking its verification code. @nicolegys
Can we have a test project for the dbft~

@nicolegys
Copy link
Contributor

Fixed, I changed the index of nonce_tx in the txs, and missed checking its verification code. @nicolegys
Can we have a test project for the dbft~

I used the newest code, and the issue still existed. TwT

@nicolegys
Copy link
Contributor

Failed to generate block_1 in multiple CN mode.
Cause:
The primary (index=1) create a nonce_tx but it has no gas to pay for the network fee, then throw exception Insufficient GAS.

image

@nicolegys
Copy link
Contributor

If view changed, tx.Nonce is still the one created by old primary, but the backup CNs receive the new nonce, so CheckNonce will fail.
image

Copy link
Contributor

@nicolegys nicolegys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No nonce_tx now.

@@ -342,7 +354,8 @@ internal void EnsureMaxBlockLimitation(IEnumerable<Transaction> txs)
uint maxTransactionsPerBlock = neoSystem.Settings.MaxTransactionsPerBlock;

// Limit Speaker proposal to the limit `MaxTransactionsPerBlock` or all available transactions of the mempool
txs = txs.Take((int)maxTransactionsPerBlock);
txs = txs.Take((int)maxTransactionsPerBlock - 1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
txs = txs.Take((int)maxTransactionsPerBlock - 1);
txs = txs.Take((int)maxTransactionsPerBlock);

@nicolegys
Copy link
Contributor

Could you please also update HeaderFromJson in RpcClient?
https://github.com/Liaojinghui/neo-modules/blob/master/src/RpcClient/Utility.cs#L137

@@ -142,7 +142,7 @@ public static Header HeaderFromJson(JObject json, ProtocolSettings protocolSetti
PrevHash = UInt256.Parse(json["previousblockhash"].AsString()),
MerkleRoot = UInt256.Parse(json["merkleroot"].AsString()),
Timestamp = (ulong)json["time"].AsNumber(),
Nonce = (ulong)json["nonce"].AsNumber(),
Nonce = UInt64.Parse(json["nonce"].AsString()),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should useUInt64.Parse(json["nonce"].AsString(), NumberStyles.AllowHexSpecifier) to convert a hex string to uint64.

@Jim8y Jim8y mentioned this pull request Jun 10, 2021
@Jim8y Jim8y closed this Jun 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants