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

Fix sync #5550

Merged
merged 1 commit into from
Apr 9, 2019
Merged

Fix sync #5550

merged 1 commit into from
Apr 9, 2019

Conversation

gumb0
Copy link
Member

@gumb0 gumb0 commented Apr 8, 2019

No description provided.

gumb0
gumb0 commented Apr 8, 2019
View reviewed changes
libethereum/BlockChainSync.cpp
@@ -217,7 +217,7 @@ bool BlockChainSync::requestDaoForkBlockHeader(NodeID const& _peerID)
return false;

m_daoChallengedPeers.insert(_peerID);
m_host.peer(_peerID).requestBlockHeaders(daoHardfork, 1, 0, false);
m_host.peer(_peerID).requestBlockHeaders(static_cast<unsigned>(daoHardfork), 1, 0, false);
Copy link
Member Author

@gumb0 gumb0 Apr 8, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

After daoHardfork type was changed to u256 in #5539, overload resolution preferred requestBlockHeaders(h256 const&, ...) instead of requestBlockHeaders(unsigned, ...); sync with the main net was not working because of this.

Copy link
Member

Choose a reason for hiding this comment

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

Why do we need the other overload?

Copy link
Member Author

@gumb0 gumb0 Apr 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

eth wire protocol defines requests for headers both by number and by hash. We request the latest known block header by hash, when we begin to sync from the peer. Because we receive only this hash in the Status message.

Copy link
Member Author

Choose a reason for hiding this comment

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

Strangely Status message spec includes block number,too https://github.com/ethereum/devp2p/blob/master/caps/eth.md#status-0x00
But quick experiment shows that no one actually sends block number, all Status messages contain only hash. Aleth also doesn't send it.

@gumb0 gumb0 requested review from chfast and halfalicious April 8, 2019 15:20
@halfalicious
Copy link
Contributor

halfalicious commented Apr 8, 2019 •
edited
Loading

Thanks for fixing this! What sorts of errors were you seeing? Would it maybe make sense for me to look into resurrecting the blockchain sync tests PR you created a while back so we can catch these issues during PRs?

chfast
chfast approved these changes Apr 8, 2019
View reviewed changes
libethereum/BlockChainSync.cpp
@@ -217,7 +217,7 @@ bool BlockChainSync::requestDaoForkBlockHeader(NodeID const& _peerID)
return false;

m_daoChallengedPeers.insert(_peerID);
m_host.peer(_peerID).requestBlockHeaders(daoHardfork, 1, 0, false);
m_host.peer(_peerID).requestBlockHeaders(static_cast<unsigned>(daoHardfork), 1, 0, false);
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need the other overload?

@halfalicious
Copy link
Contributor

halfalicious commented Apr 9, 2019 •
edited
Loading

Thanks for fixing this! What sorts of errors were you seeing? Would it maybe make sense for me to look into resurrecting the blockchain sync tests PR you created a while back so we can catch these issues during PRs?

Never mind about the errors you were seeing, tried it myself and am seeing "invalid genesis hash" (which makes sense after looking at the changes + overload since daoHardForkBlock was being treated as a block hash rather than a block number):

DEBUG 04-08 20:16:41 p2p  ethcap 865a6325…|Parity-Ethereum/v2.2.9-stable-5d5b372-20190203/x86_64-linux-gnu/rustc1.31.1 Status: 63 / 3 / #41941023…, TD: 19014003065923188 = #eab82579…
DEBUG 04-08 20:16:41 p2p  sync   865a6325…|Parity-Ethereum/v2.2.9-stable-5d5b372-20190203/x86_64-linux-gnu/rustc1.31.1 Peer not suitable for sync: Invalid genesis hash.
TRACE 04-08 20:16:41 p2p  net    865a6325…|Parity-Ethereum/v2.2.9-stable-5d5b372-20190203/x86_64-linux-gnu/rustc1.31.1 Disconnecting (our reason: Subprotocol reason.)
TRACE 04-08 20:16:41 p2p  net    865a6325…|Parity-Ethereum/v2.2.9-stable-5d5b372-20190203/x86_64-linux-gnu/rustc1.31.1 <- [ 0x10 ]
TRACE 04-08 20:16:41 p2p  net    865a6325…|Parity-Ethereum/v2.2.9-stable-5d5b372-20190203/x86_64-linux-gnu/rustc1.31.1 Closing 52.176.100.77:30303 (Subprotocol reason.)
DEBUG 04-08 20:16:41 p2p  net    865a6325…|Parity-Ethereum/v2.2.9-stable-5d5b372-20190203/x86_64-linux-gnu/rustc1.31.1 Closing peer session :-(

and

DEBUG 04-08 20:16:41 p2p  ethcap 03bc9d46…|GMC/v2.8.1-final-c19417a9/linux-amd64/go1.11.1 Status: 63 / 7762959 / #4eba28a4…, TD: 26113618853360678998 = #a4aad700…
DEBUG 04-08 20:16:41 p2p  sync   03bc9d46…|GMC/v2.8.1-final-c19417a9/linux-amd64/go1.11.1 Peer not suitable for sync: Invalid genesis hash.
TRACE 04-08 20:16:41 p2p  net    03bc9d46…|GMC/v2.8.1-final-c19417a9/linux-amd64/go1.11.1 Disconnecting (our reason: Subprotocol reason.)
TRACE 04-08 20:16:41 p2p  net    03bc9d46…|GMC/v2.8.1-final-c19417a9/linux-amd64/go1.11.1 <- [ 0x10 ]
TRACE 04-08 20:16:41 p2p  net    03bc9d46…|GMC/v2.8.1-final-c19417a9/linux-amd64/go1.11.1 Closing 213.202.254.206:30003 (Subprotocol reason.)

@halfalicious
Copy link
Contributor

halfalicious commented Apr 9, 2019 •
edited
Loading

Why do we need the other overload?

Agreed, I did a quick source search and I'm not seeing any code calling it...as such I think it can be removed.

halfalicious
halfalicious suggested changes Apr 9, 2019
View reviewed changes
Copy link
Contributor

@halfalicious halfalicious left a comment

Choose a reason for hiding this comment

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

I think we should remove the overload

@gumb0
Copy link
Member Author

gumb0 commented Apr 9, 2019

@halfalicious It's needed here

aleth/libethereum/BlockChainSync.cpp

Line 256 in ba3f45a

peer.requestBlockHeaders(peer.latestHash(), 1, 0, false);

@chfast
Copy link
Member

chfast commented Apr 9, 2019

I see. The problem is than u256 implicitly converts to h256...

@chfast chfast merged commit b5c1e10 into master Apr 9, 2019
@chfast chfast deleted the fix-sync branch April 9, 2019 14:28
@halfalicious halfalicious mentioned this pull request Apr 10, 2019
Closed
@halfalicious
Copy link
Contributor

Actually, the issue this was fixing wasn't the cause of the mainnet sync "invalid genesis hash" issue that I was seeing...failing the dao challenge would manifest as "Peer from another fork":

aleth/libethereum/BlockChainSync.cpp

Lines 438 to 443 in dbbfd5b

if (m_daoChallengedPeers.find(_peerID) != m_daoChallengedPeers.end())
{
if (verifyDaoChallengeResponse(_r))
syncPeer(_peerID, false);
else
m_host.disablePeer(_peerID, "Peer from another fork.");

@gumb0
Copy link
Member Author

gumb0 commented Apr 10, 2019

This issue looked like the sync was never proceeding after (invalid) DAO fork challenge, and was just stuck.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@halfalicious halfalicious halfalicious requested changes

@chfast chfast chfast approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gumb0 @halfalicious @chfast