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

Miscellaneous Runtime Fixes & Improvements - Raul #1674

Merged
merged 75 commits into from
Feb 25, 2019
Merged

Miscellaneous Runtime Fixes & Improvements - Raul #1674

merged 75 commits into from
Feb 25, 2019

Conversation

rauljordan
Copy link
Contributor

@rauljordan rauljordan commented Feb 21, 2019

This is part of #1565


Description

Write why you are making the changes in this pull request

This PR has been a major effort to get runtime working for our beacon node custom configuration with 8 validators. We have identified a ton of bugs that are critical and are mostly small fixes to our code.

Write a summary of the changes you are making

Below are all the changes done in this PR to achieve a functional runtime. The chain still does not advance indefinitely, but this is a starting point for future PRs to make the runtime bulletproof.

  • Improve logging significantly for important items throughout the beacon node lifecycle and state transition to make for easier debugging and understanding occurrences at epoch boundaries
  • Log slotsSinceGenesis as slot-GENESIS_SLOT for more human readable numbers
  • Only verify RANDAO if verify signatures is true in the state transition
  • Do not overwrite the beacon state's pending attestations but instead just append to the slice
  • Verify the attestation's shard block root hash against params.BeaconConfig().ZeroHash
  • Fix epoch processing to occur at (slot+1)%SLOTS_PER_EPOCH==0 (critical)
  • Initialize ValidatorRegistryUpdateEpoch to FarFutureEpoch
  • Do not return an error if validator is not due for exit, just return nil
  • Pass in the operations service into the blockchain service initializer
  • Subscribe to incoming attestations in the operations service
  • Query for attestations which have fulfilled the MIN_ATTESTATION_INCLUSION_DELAY period
  • Do not delete attestations upon fetching them, instead, let the operations service cleanup do so
  • Fix epoch and justified epoch boundary root calculations to align with spec
  • If the beacon state's slot is the genesis slot, set the justified and epoch boundary roots to the genesis block's root instead of the zero hash
  • If beaconState.ValidatorRegistryUpdateEpoch == helpers.SlotToEpoch(req.EpochStart)-1, set the registryChange flag to true when requesting assignments
  • Modify custom config to increase ejection balance so validators do not exit at epoch 1
  • Modify validator client's RoleAt function to do nothing if the slot is the genesis slot
  • Include the demo config in the validator client to allow the validator to match the beacon node's configuration

How to Run

Make sure you checkout this branch, then deploy a deposit contract to Goerli using:

bazel run //contracts/deposit-contract/deployContract -- --httpPath=https://goerli.prylabs.net --privKey=YOURKEY --chainStart=8 --minDeposit=100 --maxDeposit=3200 --customChainstartDelay 240

Make sure you have a new, fresh datadir for your beacon node and copy the deposit contract address to use below:

bazel run //beacon-chain -- --web3provider wss://goerli.prylabs.net/websocket --enable-powchain --datadir ~/Desktop/beacon --demo-config --deposit-contract ADDRESS

Then, create 8 unique validator clients with the following command:

bazel run //validator -- accounts create --password 123456 --keystore-path /unique/path

Keep note of each of their deposit data hex strings, you'll need them later. Then, start the 8 validator clients with the command:

bazel run //validator -- --password 123456 --demo-config --keystore-path /unique/path

Then, use the script below and paste it in the console of a web3 enabled site such as etherscan.io while your beacon node + 8 validators are running:

/**
 * Useful for testing deposits with a deployed contract via metamask.
 *
 * Note: Assumes deposit contract is deployed with max deposit of 3200gwei.
 */

var address = "DEPOSIT_CONTRACT_ADDRESS"; // change me!
var d = web3.eth.contract(ABI_HERE).at(address);

var cb = function(idk, receipt) {
    console.log("TX = " + receipt);
};

// Add a list of deposit data hex strings here
depositData = [
 // ADD EVERY VALIDATOR DEPOSIT DATA HEX STRING BELOW...
];

depositData.forEach(data => {
    web3.eth.sendTransaction({to: d.address, value: web3.toWei(3200000, 'gwei'), data: d.deposit.getData(data), gas: 1000000}, cb);
});

For Future Runs: you can keep the 8 validator keystore directories and don't need to copy paste their deposit data again into the js file from above. Make sure you delete the beacon node's datadir, however, on a fresh run.

@codecov
Copy link

codecov bot commented Feb 21, 2019

Codecov Report

Merging #1674 into master will decrease coverage by 0.71%.
The diff coverage is 77.48%.

@@            Coverage Diff             @@
##           master    #1674      +/-   ##
==========================================
- Coverage   72.92%   72.21%   -0.72%     
==========================================
  Files         100       99       -1     
  Lines        7148     7066      -82     
==========================================
- Hits         5213     5103     -110     
- Misses       1478     1494      +16     
- Partials      457      469      +12

@rauljordan rauljordan requested review from nisdas, terencechain and prestonvanloon and removed request for nisdas February 22, 2019 20:49
@rauljordan rauljordan self-assigned this Feb 22, 2019
@rauljordan rauljordan added Ready For Review A pull request ready for code review Priority: Critical Highest, immediate priority item labels Feb 22, 2019
@rauljordan rauljordan added this to the Sapphire milestone Feb 22, 2019
log.WithFields(logrus.Fields{
"epoch": helpers.CurrentEpoch(beaconState),
}).Info("Verifying randao")
log.Infof("Pubkey: %#x", proposer.Pubkey)
Copy link
Member

Choose a reason for hiding this comment

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

Can these be fields on the above line? Verifying randao

@@ -113,7 +113,7 @@ func TestProposeBlock_UsePendingDeposits(t *testing.T) {

m.proposerClient.EXPECT().PendingAttestations(
gomock.Any(), // ctx
gomock.Eq(&ptypes.Empty{}),
gomock.Any(),
Copy link
Member

Choose a reason for hiding this comment

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

Why changing all of these? If the type has changed, can you use the proper type now?

Copy link
Member

@prestonvanloon prestonvanloon left a comment

Choose a reason for hiding this comment

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

I have a few minor comments. I also think this logging is going to be very verbose, but we can revisit logging levels at another time.

I suspect that you didn't choose debug for a lot of these logs because p2p is overwhelming.
I would recommend that debug logging for p2p be some other flag entirely because it's not very useful to us.

Copy link
Member

@prestonvanloon prestonvanloon left a comment

Choose a reason for hiding this comment

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

Please fix trivial comments, otherwise LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: Critical Highest, immediate priority item Ready For Review A pull request ready for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants