Skip to content
This repository has been archived by the owner on Oct 28, 2021. It is now read-only.

Skip dao challenge if top block < hard fork block #5580

Merged
merged 2 commits into from
May 2, 2019

Conversation

halfalicious
Copy link
Contributor

@halfalicious halfalicious commented Apr 28, 2019

Fix #5579

As a syncing optimization, skip performing the dao hard fork challenge if our top block < dao hard fork block since we can still successfully sync these blocks from ETC nodes. Note that sync from these peers will fail starting with the dao hard fork block because we validate that the dao balance is refunded in Block::performIrregularModifications so we don't run the risk of syncing on an ETC chain after the dao fork block:

aleth/libethereum/Block.cpp

Lines 690 to 701 in 60fbc2f

void Block::performIrregularModifications()
{
u256 const& daoHardfork = m_sealEngine->chainParams().daoHardforkBlock;
if (daoHardfork != 0 && info().number() == daoHardfork)
{
Address recipient("0xbf4ed7b27f1d666546e30d74d50d173d20bca754");
Addresses allDAOs = childDaos();
for (Address const& dao: allDAOs)
m_state.transferBalance(dao, recipient, m_state.balance(dao));
m_state.commit(State::CommitBehaviour::KeepEmptyAccounts);
}
}

@halfalicious halfalicious requested review from gumb0 and chfast and removed request for gumb0 April 28, 2019 05:35
@halfalicious
Copy link
Contributor Author

These tests are failing on linux:

229 - GeneralStateTests/stSStoreTest (Timeout)
566 - JsonRpcSuite/adminEthVmTrace (Failed)

I suspect the timeout might just be something we hit from time-to-time based on how long the tests take to execute.

Here's the VM trace failure:

563/575 Test #566: JsonRpcSuite/adminEthVmTrace ..........................................................................................***Failed    0.33 sec
Running tests using path: "/home/builder/project/test/jsontests"
Running 1 test case...
Test Case "adminEthVmTrace": 
unknown location(0): fatal error: in "JsonRpcSuite/adminEthVmTrace": jsonrpc::JsonRpcException: Exception -32001 : The response is invalid: null

/home/builder/project/test/unittests/libweb3jsonrpc/jsonrpc.cpp(707): last checkpoint

*** 1 failure is detected (5 failures are expected) in the test module "Master Test Suite"
ERROR: Expected a single test to be passed: adminEthVmTrace

I haven't seen this kind of failure before and I'm not familiar with this test, I'll investigate.

As a syncing optimization, skip performing the dao hard fork challenge
if our top block < dao hard fork block since we can still successfully
sync these blocks from ETC nodes. Note that sync will fail starting with
the dao hard fork block because we validate that the dao balance is
refunded (in Block::performIrregularModifications).
@codecov-io
Copy link

Codecov Report

Merging #5580 into master will decrease coverage by 0.02%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5580      +/-   ##
==========================================
- Coverage   62.11%   62.09%   -0.03%     
==========================================
  Files         347      347              
  Lines       29107    29107              
  Branches     3302     3302              
==========================================
- Hits        18080    18074       -6     
- Misses       9847     9853       +6     
  Partials     1180     1180

@halfalicious
Copy link
Contributor Author

Made the change slightly cleaner by performing the comparison in BlockChainSync::requestDaoForkBlockheader and comparing against the daoHardforkBlock retrieved from the chain config, rather than performing the comparison in BlockChainSync::onPeerStatus and defining a new constexpr for the dao hard fork block.

@halfalicious
Copy link
Contributor Author

These tests are failing on linux:

229 - GeneralStateTests/stSStoreTest (Timeout)
566 - JsonRpcSuite/adminEthVmTrace (Failed)

I suspect the timeout might just be something we hit from time-to-time based on how long the tests take to execute.

Here's the VM trace failure:

563/575 Test #566: JsonRpcSuite/adminEthVmTrace ..........................................................................................***Failed    0.33 sec
Running tests using path: "/home/builder/project/test/jsontests"
Running 1 test case...
Test Case "adminEthVmTrace": 
unknown location(0): fatal error: in "JsonRpcSuite/adminEthVmTrace": jsonrpc::JsonRpcException: Exception -32001 : The response is invalid: null

/home/builder/project/test/unittests/libweb3jsonrpc/jsonrpc.cpp(707): last checkpoint

*** 1 failure is detected (5 failures are expected) in the test module "Master Test Suite"
ERROR: Expected a single test to be passed: adminEthVmTrace

I haven't seen this kind of failure before and I'm not familiar with this test, I'll investigate.

Looks like these were one-off issues since they didn't repro on a subsequent run.

The new failing tests are known to be unreliable due to issues @gumb0 called out in #5544

517 - network/net/pingNotSentAfterPongForKnownNode (Failed)
521 - network/net/packetsWithChangedEndpointSuite/neighbours (Failed)

@gumb0
Copy link
Member

gumb0 commented Apr 29, 2019

Looks fine, but it would be nice to check how we pass DAO fork block after this (so sync ~2M mainnet blocks)

One potential complication is related to the parallel pipeline of the sync - at the moment when we check the highest block number (taken from the database) here, further blocks might be already downloaded, and around DAO fork block they might have been downloaded from the ETC nodes.
In theory eventually we should reject all ETC blocks and proceed with syncing ETH chain, but we better check that it's not broken.

@halfalicious
Copy link
Contributor Author

Looks fine, but it would be nice to check how we pass DAO fork block after this (so sync ~2M mainnet blocks)

One potential complication is related to the parallel pipeline of the sync - at the moment when we check the highest block number (taken from the database) here, further blocks might be already downloaded, and around DAO fork block they might have been downloaded from the ETC nodes.
In theory eventually we should reject all ETC blocks and proceed with syncing ETH chain, but we better check that it's not broken.

Restarted my sync on Linux, hope to have hit 2M+ blocks by tomorrow morning (assuming I don't hit the "sync gets stuck" issue again).

@halfalicious
Copy link
Contributor Author

halfalicious commented May 2, 2019

Looks fine, but it would be nice to check how we pass DAO fork block after this (so sync ~2M mainnet blocks)
One potential complication is related to the parallel pipeline of the sync - at the moment when we check the highest block number (taken from the database) here, further blocks might be already downloaded, and around DAO fork block they might have been downloaded from the ETC nodes.
In theory eventually we should reject all ETC blocks and proceed with syncing ETH chain, but we better check that it's not broken.

Restarted my sync on Linux, hope to have hit 2M+ blocks by tomorrow morning (assuming I don't hit the "sync gets stuck" issue again).

@gumb0 Sync'd my Linux machine to over 2M blocks but then hit a core dump a bit after 2M. Was able to resume syncing on relaunch but I'm going to try restarting syncing from genesis to see if I hit this again and if so debug.

@gumb0
Copy link
Member

gumb0 commented May 2, 2019

It might be not a new issue, we have some reports about sync crashes like #5046 and #4584

@halfalicious
Copy link
Contributor Author

@gumb0 I resync'd past the DAO fork block and Aleth didn't crash. I did hit the sync stuck issue again despite Aleth consistently having between 5 and 10 peers and I'm seeing "stack offloading: depth 80" messages since block 1.7M, but I'll merge my changes and investigate those separately since they're probably unrelated.

Here's the log:
dao_sync_logs_stack_offload.txt

@halfalicious halfalicious merged commit 332c023 into master May 2, 2019
@halfalicious halfalicious deleted the dao-sync-optimization branch May 2, 2019 16:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Only perform DAO hard fork challenge if Aleth's top block is >= DAO hard fork block
3 participants