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

Common: add YoloV3 network #1129

Merged
merged 10 commits into from
Mar 5, 2021
Merged

Common: add YoloV3 network #1129

merged 10 commits into from
Mar 5, 2021

Conversation

jochem-brouwer
Copy link
Member

This PR is aimed towards the eip2718-eip2930 branch and creates the YoloV3 block in Common. We successfully import the block, but sadly it does not seem we are connecting to the network. My hunch is, that this is because we don't have the right fork hashes.

Genesis block source
Genesis block as reported by web3

The goal of this PR is to successfully connect and sync with YoloV3 (which is also why it is targeted towards the eip2718-eip2930 branch, because we need support for these EIPs in order to sync).

@codecov
Copy link

codecov bot commented Feb 26, 2021

Codecov Report

Merging #1129 (f43b9fb) into master (5620105) will increase coverage by 0.00%.
The diff coverage is 81.25%.

Impacted file tree graph

Flag Coverage Δ
block 81.61% <ø> (-0.22%) ⬇️
blockchain 84.13% <ø> (-0.07%) ⬇️
client 87.04% <ø> (ø)
common 87.29% <100.00%> (+0.05%) ⬆️
devp2p 84.33% <50.00%> (-0.01%) ⬇️
ethash 82.08% <ø> (ø)
tx 86.00% <100.00%> (+0.49%) ⬆️
vm 82.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@holgerd77
Copy link
Member

Will give this a short look now. We also still have this problem in the client (at least I am 80% sure on this), that the first 1-2 bootnode connections are (often? always?) not successful due to all the services not yet set up properly, this might also be the cause for the problems. Extremely hard to debug though (at least I wasn't successful on this yet).

We'll see. 😄

@holgerd77
Copy link
Member

Phew, had to fix 3-4 (!) partly serious bugs for this in a row (but luckily I had a streak here in this case 😁).

For the forkHash there is an internal helper function in Common - calcForkHash() - which can be used to calculate the correct forkhashes. I spent some longer time to implement the algorithm correctly there, so I think this can normally be trusted. This method actually also jumps in when there is no forkhash provided in the chain files, then the forkhash is just always re-calculated on the fly, adding this to the files is mainly for performance reasons (we might as well just expose _calcForkHash() publicly, likely useful for others in other contexts as well).

The second "bug" is a bit eyebrow raising, I noticed that on a received ETH status reply (for completely unknown reason I once managed to get one before the bug fix 😜 ), the next forkblock by geth - if unknown - is apparently announced as the empty buffer and not the zero-Buffer, e.g. this devp2p:eth Received STATUS message from 3.9.20.133:30303: : [V:64, NID:34180983699157880, TD:157509, BestH:bdf8438, GenH:f1f2876, ForkHash: 0xa308affe, ForkNext: NaN] +1ms is a respective received status message. This is actually deviating from the spec (or can this also be some RLP encoding thing with zero vs empty?) - since the spec is saying "FORK_NEXT: Block number (uint64) of the next upcoming fork, or 0 if no next fork is known." here. Anyhow, I changed this to match and this worked. Might open a PR towards the EIP with some suggestion for clarification.

Last thing: also stumbled upon the high chainID - this time in the client - could be fixed relatively easy.

Anyhow: this is now working (so: the connection part 😋 ) 🎉 , so: happy debugging! 👍

Cool cool work @jochem-brouwer, so great that we are finally here now! 😄

@jochem-brouwer
Copy link
Member Author

Is there a problem in client with the high chain id? The number is high, but not so high that we run into rounding errors.

If we are connecting now does that also mean we are syncing?

@jochem-brouwer
Copy link
Member Author

jochem-brouwer commented Feb 27, 2021

Looks like we are actually syncing some blocks?

Then I get an error

ERROR [02-27|13:23:27] Error: Number can only safely store up to 53 bits
    at assert (/Volumes/Ethereum/ethereumjs-vm/node_modules/bn.js/lib/bn.js:6:21)
    at BN.toNumber (/Volumes/Ethereum/ethereumjs-vm/node_modules/bn.js/lib/bn.js:540:7)
    at LegacyTransaction._validateTxV (/Volumes/Ethereum/ethereumjs-vm/packages/tx/src/legacyTransaction.ts:267:30)
    at new LegacyTransaction (/Volumes/Ethereum/ethereumjs-vm/packages/tx/src/legacyTransaction.ts:109:12)
    at Function.fromValuesArray (/Volumes/Ethereum/ethereumjs-vm/packages/tx/src/legacyTransaction.ts:66:14)
    at Function.fromBlockBodyData (/Volumes/Ethereum/ethereumjs-vm/packages/tx/src/transactionFactory.ts:77:32)
    at Function.fromValuesArray (/Volumes/Ethereum/ethereumjs-vm/packages/block/src/block.ts:94:28)
    at /Volumes/Ethereum/ethereumjs-vm/packages/client/lib/sync/fetcher/blockfetcher.ts:59:20
    at Array.map (<anonymous>)
    at BlockFetcher.request (/Volumes/Ethereum/ethereumjs-vm/packages/client/lib/sync/fetcher/blockfetcher.ts:53:36)

Should be rather easy to fix though, we call toNumber on very large v values (this is caused by the high chain id).

@jochem-brouwer
Copy link
Member Author

Tests are going to fail, because this PR now needs these changes to ethereumjs-util; ethereumjs/ethereumjs-util#290

I can report that we are successfully syncing with the YoloV3 network. I am currently at block 33424, head is at 79783.

What a great milestone we have achieved here! 🎉 😄

@jochem-brouwer
Copy link
Member Author

image

We synced to (almost) head of chain! We can use this opportunity to figure out what should happen at the head of the chain. It looks like the block fetcher stops once we are synced.

Note: to test this locally, pull the ethereumjs-util and checkout the fix-ecrecover branch. Now build it. Copy the dist/signature* files to node_modules of our monorepo (so the root) and overwrite the old signature files.

@jochem-brouwer
Copy link
Member Author

jochem-brouwer commented Feb 27, 2021

Did a re-sync, I noticed that at some point I get this error (error also happened on the first sync)

Peer error: Error: Request timed out after 6000ms id=9e1096aa address=3.9.20.133:30303 transport=rlpx protocols=eth inbound=false

(This is the bootnode). I wonder if we get banned from this peer, might be useful to take a look here. After this happens, we retrigger bootstrap, then it throws the same error. Then after a new re-bootstrap the peer suddenly likes us again.

@jochem-brouwer jochem-brouwer mentioned this pull request Mar 1, 2021
4 tasks
Base automatically changed from eip2718-eip2930 to master March 4, 2021 20:58
@holgerd77
Copy link
Member

Hi @jochem-brouwer, hope that was ok to merge in #1048, there was just so much other work heaping up! We'll continue to have a close eye on this of course and won't release anything prematurely, so don't worry! 😄

What is the plan here? Do we want to cherry-pick the respective commits? What is the first relevant commit here?

@jochem-brouwer
Copy link
Member Author

I will check this tomorrow but I think we can directly rebase this one and once we integrate the util package I think we can directly merge this 👍

@holgerd77
Copy link
Member

Absolutely no chance for a rebase here - that's what I meant with warning here a bit 😋 - once you have done a rebase on a parent branch all the commits are different and git perceives (close to) all changes done as concurrent changes and you have to manually fuddle yourself through everything, which is close to not doable (just tested locally and it already completely breaks at the first commit of #1048 😄 ).

But nevermind, I think this really should be a good candidate for cherry-picking, will give this a try.

@holgerd77
Copy link
Member

Ok, have cherry-picked the new commits from this branch and force-pushed. There is still an error occurring on the yolov3 client run, generally this looks good though I would say.

Error is:

INFO [03-05|11:02:42] Started libp2p server.
INFO [03-05|11:02:45] Listener up transport=rlpx url=enode://c904debea44d4a797208a3a07994a80aa0e80293700a960a2c638827c1cf85f3e134d04d0f188ebbc5483430e8e06936a4163685ec5e922c42565f2567fae153@[::]:30303
INFO [03-05|11:02:45] Listener up transport=libp2p url=/ip4/127.0.0.1/tcp/50580/ws/p2p/QmPeAfmNKut6emwQqqqpa6xddg8oBPG8WDLDxZmfWj5go9
INFO [03-05|11:02:45] Started eth service.
INFO [03-05|11:02:54] Imported blocks count=50 number=1 hash=1053e007... hardfork=berlin peers=1
INFO [03-05|11:02:54] Executed blocks count=50 first=0 hash=f1f2876e... hardfork=berlin last=50 hash=f3d236ac... with txs=0
INFO [03-05|11:02:54] Executed blocks count=5 first=50 hash=f3d236ac... hardfork=berlin last=55 hash=4fe2c891... with txs=0
ERROR [03-05|11:02:54] Error: invalid transactions: errors at tx 0: Invalid Signature (block number=73 hash=c56fbbd792fac0da5c8d20112c317346b6e07586afe3b68ba866a184d6aa3ad0)     at BlockHeader._error (/ethereumjs-monorepo/packages/block/src/header.ts:762:15)
    at Block.validateData (/ethereumjs-monorepo/packages/block/src/block.ts:264:25)
    at Block.validate (/ethereumjs-monorepo/packages/block/src/block.ts:250:16)
    at /ethereumjs-monorepo/packages/blockchain/src/index.ts:880:9
    at Blockchain.runWithLock (/ethereumjs-monorepo/packages/blockchain/src/index.ts:407:21)
    at Blockchain._putBlockOrHeader (/ethereumjs-monorepo/packages/blockchain/src/index.ts:851:5)
    at Blockchain.putBlock (/ethereumjs-monorepo/packages/blockchain/src/index.ts:806:5)
    at Blockchain.putBlocks (/ethereumjs-monorepo/packages/blockchain/src/index.ts:792:7)
    at Chain.putBlocks (/ethereumjs-monorepo/packages/client/lib/blockchain/chain.ts:275:5)
    at BlockFetcher.store (/ethereumjs-monorepo/packages/client/lib/sync/fetcher/blockfetcher.ts:84:5)

Not sure if I have forgotten to cherry-pick a commit or this is something else. Have temporarily pushed the old branch to #1137 to ease debugging eventually.

@holgerd77 holgerd77 marked this pull request as ready for review March 5, 2021 11:02
@holgerd77
Copy link
Member

Ok, this is now working again, will merge this in. 😄

Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

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

LGTM

@holgerd77 holgerd77 merged commit 11a29cf into master Mar 5, 2021
@holgerd77 holgerd77 deleted the yolov3-client branch March 5, 2021 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants