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

Merge upstream 1.9.11 #1209

Merged
merged 71 commits into from Nov 17, 2020
Merged

Merge upstream 1.9.11 #1209

merged 71 commits into from Nov 17, 2020

Conversation

oneeman
Copy link
Contributor

@oneeman oneeman commented Nov 3, 2020

Description

Merge was done in several steps, to avoid overwhelming merge conflicts. Some notes about individual merge commits are below.

Besides that, the main thing that bears mentioning is the introduction of DNS-based discovery (was already mostly ready but unused in the code, but finalized and enabled by default here). It is used for eth/istanbul only (not for LES), through proto.DialCandidates, which is then added as a source to p2pserver's discmix iterator. If we wanted to start using it ourselves, we should be able to, following the instructions at https://geth.ethereum.org/docs/developers/dns-discovery-setup. The only difference I'm aware of is that we'd need to include the right networkid (for mainnet, alfajores, or baklava) when running the crawl subcommand. I tried crawl out, and it currently doesn't work due to a problem with the networkid flag, so I opened a separate PR to fix that issue, #1212, since the affected code is already on master and not really part of this merge PR.

dcffb77

  • dummyChain in core/vm/runtime/runtime_test.go needed additional methods to satisfy the ChainContext interface, so I added them even though we also have a mockBlockchain type in istanbul test files
  • Upstream commit 058a4ac improved the performance of GetHashFn() in core/evm.go. In celo-blockchain, that file no longer exists, and that function has been moved to core/vm/runtime/runtime.go, so I updated that to the improved version.

049e171 (eth/65)

  • For the changes to eth/protocol.go, I instead made the corresponding changes to consensus/istanbul/protocol.go. In a later separate commit, I add a comment clarifying that Celo66 corresponds to eth/65
  • In eth/protocol_test.go, they did not update the protocol version used for TestForkIDSplit. This doesn't matter, since since the protocol change has nothing to do with that, but seemed like a good idea to have it use the newest version, so I updated it to 66
  • After merging this commit, TestSendTransactions63 and TestSendTransactions64 fail. This is also the case upstream, and is fixed in a later commit that was part of the same upstream PR (9938d95, Feb 13, 2020)

90caa2c (new dial scheduler)

  • Used the --ours merge strategy (after merging in from celo-blockchain master), since this is already on master from having been cherry picked

9938d95 (second part of eth/65)

  • This removed metrics from eth/, namely the files eth/metrics.go and eth/fetcher/metrics.go. Removing eth/metrics.go had conflicts with ours, but they just had to do with how we renamed eth to istanbul and the versions, so not relevant to the fact that the file was deleted. The rationale for this removal can be seen on the upstream PR: core, eth: announce based transaction propagation ethereum/go-ethereum#20234 (comment) ("Removed the metrics from the eth package as all network packets are already covered by p2p generically, so it's better not to have to maintain this code."). None of the metrics removed or renamed are among the ones we list in our docs (https://docs.celo.org/validator-guide/monitoring#metrics)
    • Removed files/metrics:
      • Some of these seem to be covered in https://github.com/celo-org/celo-blockchain/blob/ba75a9ad8a16b050d84956b8bf045dca0ca9989c/eth/downloader/metrics.go:
        var (
        propTxnInPacketsMeter = metrics.NewRegisteredMeter("eth/prop/txns/in/packets", nil)
        propTxnInTrafficMeter = metrics.NewRegisteredMeter("eth/prop/txns/in/traffic", nil)
        propTxnOutPacketsMeter = metrics.NewRegisteredMeter("eth/prop/txns/out/packets", nil)
        propTxnOutTrafficMeter = metrics.NewRegisteredMeter("eth/prop/txns/out/traffic", nil)
        propHashInPacketsMeter = metrics.NewRegisteredMeter("eth/prop/hashes/in/packets", nil)
        propHashInTrafficMeter = metrics.NewRegisteredMeter("eth/prop/hashes/in/traffic", nil)
        propHashOutPacketsMeter = metrics.NewRegisteredMeter("eth/prop/hashes/out/packets", nil)
        propHashOutTrafficMeter = metrics.NewRegisteredMeter("eth/prop/hashes/out/traffic", nil)
        propBlockInPacketsMeter = metrics.NewRegisteredMeter("eth/prop/blocks/in/packets", nil)
        propBlockInTrafficMeter = metrics.NewRegisteredMeter("eth/prop/blocks/in/traffic", nil)
        propBlockOutPacketsMeter = metrics.NewRegisteredMeter("eth/prop/blocks/out/packets", nil)
        propBlockOutTrafficMeter = metrics.NewRegisteredMeter("eth/prop/blocks/out/traffic", nil)
        reqHeaderInPacketsMeter = metrics.NewRegisteredMeter("eth/req/headers/in/packets", nil)
        reqHeaderInTrafficMeter = metrics.NewRegisteredMeter("eth/req/headers/in/traffic", nil)
        reqHeaderOutPacketsMeter = metrics.NewRegisteredMeter("eth/req/headers/out/packets", nil)
        reqHeaderOutTrafficMeter = metrics.NewRegisteredMeter("eth/req/headers/out/traffic", nil)
        reqBodyInPacketsMeter = metrics.NewRegisteredMeter("eth/req/bodies/in/packets", nil)
        reqBodyInTrafficMeter = metrics.NewRegisteredMeter("eth/req/bodies/in/traffic", nil)
        reqBodyOutPacketsMeter = metrics.NewRegisteredMeter("eth/req/bodies/out/packets", nil)
        reqBodyOutTrafficMeter = metrics.NewRegisteredMeter("eth/req/bodies/out/traffic", nil)
        reqStateInPacketsMeter = metrics.NewRegisteredMeter("eth/req/states/in/packets", nil)
        reqStateInTrafficMeter = metrics.NewRegisteredMeter("eth/req/states/in/traffic", nil)
        reqStateOutPacketsMeter = metrics.NewRegisteredMeter("eth/req/states/out/packets", nil)
        reqStateOutTrafficMeter = metrics.NewRegisteredMeter("eth/req/states/out/traffic", nil)
        reqReceiptInPacketsMeter = metrics.NewRegisteredMeter("eth/req/receipts/in/packets", nil)
        reqReceiptInTrafficMeter = metrics.NewRegisteredMeter("eth/req/receipts/in/traffic", nil)
        reqReceiptOutPacketsMeter = metrics.NewRegisteredMeter("eth/req/receipts/out/packets", nil)
        reqReceiptOutTrafficMeter = metrics.NewRegisteredMeter("eth/req/receipts/out/traffic", nil)
        miscInPacketsMeter = metrics.NewRegisteredMeter("eth/misc/in/packets", nil)
        miscInTrafficMeter = metrics.NewRegisteredMeter("eth/misc/in/traffic", nil)
        miscOutPacketsMeter = metrics.NewRegisteredMeter("eth/misc/out/packets", nil)
        miscOutTrafficMeter = metrics.NewRegisteredMeter("eth/misc/out/traffic", nil)
        )
      • var (
        propAnnounceInMeter = metrics.NewRegisteredMeter("eth/fetcher/prop/announces/in", nil)
        propAnnounceOutTimer = metrics.NewRegisteredTimer("eth/fetcher/prop/announces/out", nil)
        propAnnounceDropMeter = metrics.NewRegisteredMeter("eth/fetcher/prop/announces/drop", nil)
        propAnnounceDOSMeter = metrics.NewRegisteredMeter("eth/fetcher/prop/announces/dos", nil)
        propBroadcastInMeter = metrics.NewRegisteredMeter("eth/fetcher/prop/broadcasts/in", nil)
        propBroadcastOutTimer = metrics.NewRegisteredTimer("eth/fetcher/prop/broadcasts/out", nil)
        propBroadcastDropMeter = metrics.NewRegisteredMeter("eth/fetcher/prop/broadcasts/drop", nil)
        propBroadcastDOSMeter = metrics.NewRegisteredMeter("eth/fetcher/prop/broadcasts/dos", nil)
        headerFetchMeter = metrics.NewRegisteredMeter("eth/fetcher/fetch/headers", nil)
        bodyFetchMeter = metrics.NewRegisteredMeter("eth/fetcher/fetch/bodies", nil)
        headerFilterInMeter = metrics.NewRegisteredMeter("eth/fetcher/filter/headers/in", nil)
        headerFilterOutMeter = metrics.NewRegisteredMeter("eth/fetcher/filter/headers/out", nil)
        bodyFilterInMeter = metrics.NewRegisteredMeter("eth/fetcher/filter/bodies/in", nil)
        bodyFilterOutMeter = metrics.NewRegisteredMeter("eth/fetcher/filter/bodies/out", nil)
        )
    • Added metrics:
      • var (
        blockAnnounceInMeter = metrics.NewRegisteredMeter("eth/fetcher/block/announces/in", nil)
        blockAnnounceOutTimer = metrics.NewRegisteredTimer("eth/fetcher/block/announces/out", nil)
        blockAnnounceDropMeter = metrics.NewRegisteredMeter("eth/fetcher/block/announces/drop", nil)
        blockAnnounceDOSMeter = metrics.NewRegisteredMeter("eth/fetcher/block/announces/dos", nil)
        blockBroadcastInMeter = metrics.NewRegisteredMeter("eth/fetcher/block/broadcasts/in", nil)
        blockBroadcastOutTimer = metrics.NewRegisteredTimer("eth/fetcher/block/broadcasts/out", nil)
        blockBroadcastDropMeter = metrics.NewRegisteredMeter("eth/fetcher/block/broadcasts/drop", nil)
        blockBroadcastDOSMeter = metrics.NewRegisteredMeter("eth/fetcher/block/broadcasts/dos", nil)
        headerFetchMeter = metrics.NewRegisteredMeter("eth/fetcher/block/headers", nil)
        bodyFetchMeter = metrics.NewRegisteredMeter("eth/fetcher/block/bodies", nil)
        headerFilterInMeter = metrics.NewRegisteredMeter("eth/fetcher/block/filter/headers/in", nil)
        headerFilterOutMeter = metrics.NewRegisteredMeter("eth/fetcher/block/filter/headers/out", nil)
        bodyFilterInMeter = metrics.NewRegisteredMeter("eth/fetcher/block/filter/bodies/in", nil)
        bodyFilterOutMeter = metrics.NewRegisteredMeter("eth/fetcher/block/filter/bodies/out", nil)
        )
      • var (
        txAnnounceInMeter = metrics.NewRegisteredMeter("eth/fetcher/transaction/announces/in", nil)
        txAnnounceKnownMeter = metrics.NewRegisteredMeter("eth/fetcher/transaction/announces/known", nil)
        txAnnounceUnderpricedMeter = metrics.NewRegisteredMeter("eth/fetcher/transaction/announces/underpriced", nil)
        txAnnounceDOSMeter = metrics.NewRegisteredMeter("eth/fetcher/transaction/announces/dos", nil)
        txBroadcastInMeter = metrics.NewRegisteredMeter("eth/fetcher/transaction/broadcasts/in", nil)
        txBroadcastKnownMeter = metrics.NewRegisteredMeter("eth/fetcher/transaction/broadcasts/known", nil)
        txBroadcastUnderpricedMeter = metrics.NewRegisteredMeter("eth/fetcher/transaction/broadcasts/underpriced", nil)
        txBroadcastOtherRejectMeter = metrics.NewRegisteredMeter("eth/fetcher/transaction/broadcasts/otherreject", nil)
        txRequestOutMeter = metrics.NewRegisteredMeter("eth/fetcher/transaction/request/out", nil)
        txRequestFailMeter = metrics.NewRegisteredMeter("eth/fetcher/transaction/request/fail", nil)
        txRequestDoneMeter = metrics.NewRegisteredMeter("eth/fetcher/transaction/request/done", nil)
        txRequestTimeoutMeter = metrics.NewRegisteredMeter("eth/fetcher/transaction/request/timeout", nil)
        txReplyInMeter = metrics.NewRegisteredMeter("eth/fetcher/transaction/replies/in", nil)
        txReplyKnownMeter = metrics.NewRegisteredMeter("eth/fetcher/transaction/replies/known", nil)
        txReplyUnderpricedMeter = metrics.NewRegisteredMeter("eth/fetcher/transaction/replies/underpriced", nil)
        txReplyOtherRejectMeter = metrics.NewRegisteredMeter("eth/fetcher/transaction/replies/otherreject", nil)
        txFetcherWaitingPeers = metrics.NewRegisteredGauge("eth/fetcher/transaction/waiting/peers", nil)
        txFetcherWaitingHashes = metrics.NewRegisteredGauge("eth/fetcher/transaction/waiting/hashes", nil)
        txFetcherQueueingPeers = metrics.NewRegisteredGauge("eth/fetcher/transaction/queueing/peers", nil)
        txFetcherQueueingHashes = metrics.NewRegisteredGauge("eth/fetcher/transaction/queueing/hashes", nil)
        txFetcherFetchingPeers = metrics.NewRegisteredGauge("eth/fetcher/transaction/fetching/peers", nil)
        txFetcherFetchingHashes = metrics.NewRegisteredGauge("eth/fetcher/transaction/fetching/hashes", nil)
        )
  • There are some function names ending in 64, for the old protocol version (eth/64). In normal code: txsyncLoop64 and SendTransactions64. Did not rename them to match the istanbul version numbers instead, but in comments which mention eth/6x I mentioned which celo/6x they correspond to. In the test code there are many to test with several protocol versions. For those, I did update it, since there the function name is paired with a protocol number that's given as an argument to a generic test function, and so it'd be odd to have a mismatch.
  • A couple of the tests relied on deterministic random numbers for the order in which transactions are requested. Since Celo transactions have new fields to do with the fees, our hashes were different and so the indexes used in the tests had to be modified for the tests to pass. I did this in a separate commit

26284ec

  • They updated their CI configuration to use Go 1.13.8 instead of 1.13.6. Looking at our CircleCI config, we use the docker image circleci/golang:1.13 (1.13.15) for the build, for the e2e tests we use celohq/node10-gcloud:v3 with GO_VERSION=1.13.7, and in our Dockerfile we use golang:1.13-alpine(1.13.15). Open question: should we update GO_VERSION for the e2e tests to 1.13.15? Any reason not to?
  • Upstream enables DNS discovery by default. Since we don't have the DNS entries for discovery, I defined the mainnet, baklava and alfajores URLs to be "", which means it will be disabled when using them, but enabled if we later on add those urls in bootnodes.go
  • In cmd/utils/flags.go, at the end of SetEthConfig, there is a check for networkId == 1 (ethereum mainnet). I changed this to == params.MainnetNetworkId.

ac72787:

  • Significant conflicts in p2p/metrics.go. I made sure to preserve the metrics we've added in that file, listed below. Of these, the two handshake metrics were being updated in handshakeDone(), which no longer exists. Instead, in a separate commit, I made it update them p2p/server.go, where handshakeDone() was being called from. Our added metrics:
    • p2p_serves_handshakes (ingressConnectWithHandshakeMeter)
    • p2p_dials_handshakes (egressConnectWithHandshakeMeter)
    • p2p/peers/discovered (discoveredPeersCounter)
    • p2p/peers/validators (activeValidatorsPeerGauge)
    • p2p/peers/proxies (activeProxiesPeerGauge)
  • The changes in this commit remove the handshakeDone() and the meteredPeerCount counter, which suffered from a race condition that led to p2p.Server connections not being tracked properly #1172. As a result, when this PR is merged we can close that issue.

Tested

Automated tests pass, but some are flaky. Will look into them to see whether or not it may be anything specific to this PR and whether they also happen on upstream 1.9.11.

Related issues

Backwards compatibility

No breaking changes either at the protocol level or at the local use level.

Reference links

1.9.11 release notes: https://github.com/ethereum/go-ethereum/releases/tag/v1.9.11
1.9.11 PRs: https://github.com/ethereum/go-ethereum/milestone/97?closed=1
1.9.11 commits: https://github.com/ethereum/go-ethereum/commits/master?before=6a62fe399b68ab9e3625ef5e7900394f389adc3a+35&branch=master

karalabe and others added 30 commits January 20, 2020 12:32
* log: delete RotatingFileHandler

We added this for the dashboard, which is gone now. The
handler never really worked well and had data race and file
handling issues.

* internal/debug: remove unused RotatingFileHandler setup code
* abidump: implement abi dump command

* cmd/abidump: add license
This replaces the JavaScript interpreter used by the console with goja,
which is actively maintained and a lot faster than otto. Clef still uses otto
and eth/tracers still uses duktape, so we are currently dependent on three
different JS interpreters. We're looking to replace the remaining uses of otto
soon though.
This change makes the client attempt to reconnect when a write fails.
We already had reconnect support, but the reconnect would previously
happen on the next call after an error. Being more eager leads to a
smoother experience overall.
Fixes #20467

Co-authored-by: meowsbits <45600330+meowsbits@users.noreply.github.com>
* signer: replace otto with goja

* go.mod: remove Otto
* trie:  make db insert use size instead of full data

* core/state: minor optimization in state onleaf allocation

* trie: implement dedicated committer and hasher

* trie: use dedicated committer/hasher

* trie: linter nitpicks

* core/state, trie: avoid unnecessary storage trie load+commit

* trie: review feedback, mainly docs + minor changes

* trie: start deprecating old hasher

* trie: fix misspell+lint

* trie: deprecate hasher.go, make proof framework use new hasher

* trie: rename pure_committer/hasher to committer/hasher

* trie, core/state: fix review concerns

* trie: more review concerns

* trie: make commit collapse into hashnode, don't touch dirtyness

* trie: goimports fixes

* trie: remove panics
* add regression tests for #20611

* eth/tracers: fix panics occurring for invalid params in js-tracers

Co-authored-by: Martin Holst Swende <martin@swende.se>
* core/vm/runtime: add test for blockhash

* core/evm: less iteration in blockhash

* core/vm/runtime: nitpickfix

Co-authored-by: Péter Szilágyi <peterke@gmail.com>
Adds the 'geth dumpgenesis' command, which writes the configured
genesis in JSON format to stdout. This provides a way to generate the
data (structure and content) that can then be used with the 'geth init'
command.
* trie: make hasher parallel when number of changes are large

* trie: remove unused field dirtyCount

* trie: rename unhashedCount/unhashed
For longer records and subtree entries, the deployer created two
separate TXT records. This doesn't work as intended because the client
will receive the two records in arbitrary order. The fix is to encode
longer values as "string1""string2" instead of "string1", "string2".
This encoding creates a single record on AWS Route53.
The feature update allows the GraphQL API endpoint to retrieve
transaction signature R,S,V parameters.

Co-authored-by: amitshah <amitshah0t7@gmail.com>
Co-authored-by: Felix Lange <fjl@twurst.com>
These methods can be helpful when migrating existing timer code.
Or Neeman added 15 commits October 30, 2020 16:32
With this commit, tests TestSendTransactions63 and TestSendTransactions64 are failing, but that is also the case upstream and fixed in a later commit in the same patch version (9938d95)

Conflicts:
  eth/downloader/peer.go
  eth/fetcher/block_fetcher.go
  eth/handler.go
  eth/handler_test.go
  eth/helper_test.go
  eth/peer.go
  eth/protocol.go
  eth/protocol_test.go
  eth/sync_test.go
Used strategy 'ours' since this has already been done on master.
Conflicts:
  eth/fetcher/tx_fetcher_test.go
  eth/handler.go
  eth/metrics.go
  eth/peer.go
  eth/sync.go
The indexes used in these tests are fixed according to the deterministic random shuffles done (only for the tests) by TxFetcher.  Since Celo transactions have additional fields, their hashes are different, leading to different orderings before and after the shuffle, so the indexes had to be updated to match the tests' intents.
Conflicts:
	cmd/utils/flags.go
	eth/config.go
	eth/gen_config.go
	node/service.go
	params/bootnodes.go
The removed code included updating the ingressConnectWithHandshakeMeter and egressConnectWithHandshakeMeter metrics.  This will be fixed in a separate commit to make it explicit that it was a fix necessitated by the merge.

Conflicts:
	p2p/metrics.go
@oneeman oneeman requested review from tkporter, mcortesi and kevjue and removed request for tkporter November 5, 2020 21:36
@oneeman oneeman marked this pull request as ready for review November 5, 2020 21:36
Copy link
Contributor

@mcortesi mcortesi left a comment

Choose a reason for hiding this comment

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

LGTM.

One minor issue... i suspect that geth dumpgenesis won't work for us, at least istanbul config things might be missing. We should check if this is the case. If it is i'm fine with just creating an issue to fix it, and merge anyway

Conflicts:
  .travis.yml
  appveyor.yml
  build/checksums.txt
@oneeman
Copy link
Contributor Author

oneeman commented Nov 16, 2020

LGTM.

One minor issue... i suspect that geth dumpgenesis won't work for us, at least istanbul config things might be missing. We should check if this is the case. If it is i'm fine with just creating an issue to fix it, and merge anyway

I confirmed that dumpgenesis includes the config.istanbul property. I tested for the mainnet genesis block, and the output included epoch, policy, and lookbackwindow, which are the ones we specify in the mainnet config (omitted ones like RequestTimeout get the default hard-coded values)

@mcortesi mcortesi merged commit 424eb52 into master Nov 17, 2020
@mcortesi mcortesi deleted the oneeman/merge-upstream-1.9.11 branch November 17, 2020 15:31
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.

p2p.Server connections not being tracked properly