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

Make ConsensusContext public (master-2.x) #1892

Merged
merged 2 commits into from
Sep 3, 2020
Merged

Make ConsensusContext public (master-2.x) #1892

merged 2 commits into from
Sep 3, 2020

Conversation

devhawk
Copy link
Contributor

@devhawk devhawk commented Aug 31, 2020

neo express must duplicate ConsensusContext implementation for gas preload. If ConsensusContext was public, neo-express could simply use it

neo express must duplicate ConsensusContext implementation for gas preload. If ConsensusContext was public, neo-express could simply use it
@erikzhang
Copy link
Member

What is "gas preload"?

@devhawk
Copy link
Contributor Author

devhawk commented Sep 1, 2020

In Neo Express for Neo 2, when you create privatenet you can specify how much gas you want to create. Neo-express then creates enough empty blocks to generate that much gas + a transfer and claim transaction. This automatically creates the requested gas in the genesis account, ready to go for dev and test scenarios.

❯ neo-express create --preload-gas 5000
Created 1 node privatenet at ~/default.neo-express.json
    Note: The private keys for the accounts in this file are are *not* encrypted.
          Do not use these accounts on MainNet or in any other system where security is a concern.
Creating 625 empty blocks to preload 5000 GAS
  Creating Block 100
  Creating Block 200
  Creating Block 300
  Creating Block 400
  Creating Block 500
  Creating Block 600
Preload complete. 5000 GAS loaded into genesis account.

I had to replicate the logic in neo-express because the ConsensusContext type is internal. But I don't want to maintain duplicate code nor re-implement it in Neo Express for Neo 3. It would be much more straightforward for Neo Express if ConensusContext was a public type.

@shargon
Copy link
Member

shargon commented Sep 1, 2020

I will be faster if you create the blocks without consensus, only signed by consensus nodes, isn't it?

@devhawk
Copy link
Contributor Author

devhawk commented Sep 1, 2020

I will be faster if you create the blocks without consensus, only signed by consensus nodes, isn't it?

Speed isn't really an issue here. A developer can create 1000s of GAS in a few seconds. My goal is correctness - the blocks generated by preload need to match the ones generated by consensus, so using the ConsensusContext seems like the best approach.

@erikzhang
Copy link
Member

In Neo3, it will generate 30,000,000 GAS during initialization, I think this should be enough for testing.

internal override void Initialize(ApplicationEngine engine)
{
UInt160 account = Blockchain.GetConsensusAddress(Blockchain.StandbyValidators);
Mint(engine, account, 30_000_000 * Factor);
}

@erikzhang
Copy link
Member

And there is a constructor in ConsensusService that doesn't require a ConsensusContext.

public ConsensusService(IActorRef localNode, IActorRef taskManager, Store store, Wallet wallet)
: this(localNode, taskManager, new ConsensusContext(wallet, store))
{
}

@devhawk
Copy link
Contributor Author

devhawk commented Sep 2, 2020

In Neo3, it will generate 30,000,000 GAS during initialization, I think this should be enough for testing.

Agreed, preload gas is not an option for neo express for neo 3. We do have other scenarios for creating offline transactions related to configuring a blockchain for test/debug scenarios that need the same behavior.

And there is a constructor in ConsensusService that doesn't require a ConsensusContext.

I'm not sure how that's relevant. I'm using ConsensusContext directly. I'm not creating an instance of ConsensusService.

@erikzhang
Copy link
Member

Which method do you use? ConsensusContext.CreateBlock()?

@devhawk
Copy link
Contributor Author

devhawk commented Sep 2, 2020

Which method do you use? ConsensusContext.CreateBlock()?

Yes. The full code I would use is:

// relay any transations via neoSystem.Blockchain.Ask

var ctx = new ConsensusContext(nodeWallet, Blockchain.Singleton.Store);
ctx.Reset(0);
ctx.MakePrepareRequest();
ctx.MakeCommit();
var block = ctx.CreateBlock();

var blockRelay = neoSystem.Blockchain.Ask<RelayResult>(block).Result;

@erikzhang
Copy link
Member

@shargon What do you think? Is it worth making ConsensusContext public?

@shargon
Copy link
Member

shargon commented Sep 3, 2020

@shargon What do you think? Is it worth making ConsensusContext public?

I think that It's harmless

Copy link
Member

@shargon shargon left a comment

Choose a reason for hiding this comment

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

For neo3 we have better options

@devhawk
Copy link
Contributor Author

devhawk commented Sep 3, 2020

For neo3 we have better options

@shargon can you provide more details? I have a similar master branch PR open #1893. But if there is a better way for the neo 3 version of neo express to accomplish the same task, I'm happy to use it

@shargon
Copy link
Member

shargon commented Sep 3, 2020

@shargon can you provide more details? I have a similar master branch PR open #1893. But if there is a better way for the neo 3 version of neo express to accomplish the same task, I'm happy to use it

Yes, as I said here

I will be faster if you create the blocks without consensus, only signed by consensus nodes, isn't it?

I think that it's easy to create a chain without consensus, you only need to do a for, increase the height, change the validator, change the prevHash, change NextConsensus, and sign it with the consensus wallet. The solution could be done in neo2 too but we have a mint transction, it could be "harder".

But now that we have states, maybe it's easier like you said, I am not sure, so it's ok for me this change.

@devhawk
Copy link
Contributor Author

devhawk commented Sep 3, 2020

I think that it's easy to create a chain without consensus, you only need to do a for, increase the height, change the validator, change the prevHash, change NextConsensus, and sign it with the consensus wallet. The solution could be done in neo2 too but we have a mint transction, it could be "harder".

This seems harder than the code from my earlier comment

@erikzhang erikzhang merged commit 6e309c8 into neo-project:master-2.x Sep 3, 2020
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.

3 participants