Merge upstream 1.9.13

[oneeman]

Description

Merge notes:

• 613af7c introduces state snapshots, which will in the future be used for a faster sync method called snap
  • See https://blog.ethereum.org/2020/07/17/ask-about-geth-snapshot-acceleration/ and https://github.com/ethereum/devp2p/blob/master/caps/snap.md for background
  • It is not yet enabled by default, but rather controlled by the --snapshot flag
  • Merge notes:
    • Some conflicts in core/blockchain.go, due to the chain repair PR cherry-pick, which chronologically came after this (in v1.9.20, August 2020). So in resolving them, I consulted the upstream chain repair PR, which made them trivial to resolve.
    • Errors in new tests added to core/blockchain_test.go. Fixed in a separate commit (replacing ethash.NewFaker() with mockEngine.NewFaker() and adding nils to account for the new transaction fields we added)
• 39f5023 made changes to eth_call and eth_estimateGas, around balance and gas price
  • Previously, to enable calls from accounts without a balance, geth was setting the account's balance to maxuint, to fake having enough balance to pay for gas. Note that, in our case, this would only work for transactions which use Celo (rather the cUSD) for the fee, which is why add special state transition to allow for gas estimation #825 was necessary.
  • Depending on the contract, the change of balance can affect the call's result (e.g. if the call depends on the sender's balance in any way). So this was removed, and instead a different way was introduced to allow accounts without a balance: to remove the defaultGasPrice used by Call() and EstimateGas(), and instead make the gas price default to zero if not specified as an argument, which is also how Parity was doing it
  • In celo-blockchain, if no gas price was specified (or gas price 0 was) we were using suggestPriceInCurrency(). Once the old 'set balance to maxuint' hack is removed, this suggested gas price becomes a problem if the account doesn't have enough to pay for gas. Since the gas price does not play any real role in Call() or EstimateGas(), I decided to remove this piece of code and leave the gas price as is (i.e. zero if not otherwise specified).
  • This led to errors during call or gas estimation, because there is a minimum gas price check in preCheck() in core/state_transition.go. This check can't be removed (without adding it elsewhere), because while it's redundant during mining, it is not redundant when processing blocks. So, instead, I took advantage of the work Victor did for add special state transition to allow for gas estimation #825 and modified it a little:
    • I renamed ApplyEstimatorMessage() to ApplyMessageWithoutGasPriceMinimum(), to make clear that it's not just for gas estimation and that it just sets the minimum gas price to zero. I made it no longer sets the gas price to zero, doing that instead inside DoEstimateGas(). This allowed removing the extra estimate argument we had added to DoCall() as well as the if that used it.
    • I made ApplyMessageWithoutGasPriceMinimum() use NewStateTransition() instead of a separate function like it, to reduce duplication. This does involve an unnecessary call to getGasPriceMinimum(), since the value it returns gets overwritten. If we'd rather avoid that unnecessary call, we can do so at the cost of a bit of duplication or by diverging a bit more from upstream.
    • To sum up, our old behavior was:
      • for eth_estimateGas, set gas price and price gas minimum to zero, and set account balance to maxuint
      • for eth_call, if gas price arg isn't passed in or is zero, set it to suggested gas price, and set account balance to maxuint
    • The new behavior is:
      • for eth_estimateGas, set gas price and gas price minimum to zero
      • for eth_call, set gas price minimum to zero
• 4690912 had already been cherry-picked in Update gosigar usage #1082
• d6c5f24 made changes to the shutdown process (to make them shut down in dependency order and wait for shutting down of components), and there were some conflicts:
  • eth/backend.go: In Stop(), we have s.stopAnnounce() that upstream doesn't. The announce thread is started last, after the protocol manager, so I positioned stopping it to be first, and confirmed that it uses a waitgroup, blocking until it's properly shut down.
  • eth/sync.go was refactored a little. A new chainSyncer type was added, and pm.synchronize() was split up, with some of it in cs.nextSyncOp() and some in cs.modeAndLocalHead(). The changes we had made in Chain repair fixes #1163 had to instead be made in cs.modeAndLocalHead(). However, that PR was itself a cherry-pick, and these changes were adapted to the fact that we had the pre-refactor version of eth/sync.go. So the right thing to do here was to grab the updated version of cs.modeAndLocalHead() from the upstream chain repair PR which Chain repair fixes #1163 was about, which I did in a separate commit to make it more explicit that this is what's going on.
• 8f05cfa introduces a CLI flag to speed up submission of mined blocks at the cost of more memory. Since the new flag and its associated changes only affect ethash (which we removed), I just verified there are no changes in it relevant to us and used the 'ours' merge strategy.
• 228a297 has a fix for TestCustomGenesis(). The test uses geth init with custom genesis values, then runs geth to get the genesis block's nonce and compare. However, the nonce used in the custom genesis values was the same as the nonce on ethereum's mainnet, so the test would pass even if the init step failed, because after init's failure geth uses the normal mainnet genesis. This commit fixed that by setting a different nonce. In our case, we were testing for the timestamp, but we also had a similar issue. We check for the timestamp being 0, but the check uses a regex check, so it passes even if the init step failed, because mainnet's genesis timestamp is 1587571200, and that contains "0". So I merged using strategy 'ours' to discard their changes, and then in a separate commit fixed it by using timestamps that won't lead to false positives in a regex check (because they're not substrings of 1587571200).
• 3cf7d2e refactors eth_call's DoCall() function by extracting a function that converts args to an EVM message. Resolving the conflicts was trivial. The differences are just the extra arguments we have relating to fee currency and gateway fee, and a different argument ordering in GetEVM().
• 8dc8941 changes the call signature of opcodes in core/vm/instructions.go to use a callCtx struct instead of so many separate arguments. The conflicts were trivial, but the upstream PR mentions it's in preparation for EIP-2315, which seems interesting.
• b7394d7 introduces a new implementation of discovery v5, which could in the future replace v4 entirely, but is not used yet (except in devp2p discv5).
  • The conflicts in cmd/devp2p/discv4cmd.go, were around our use of networkId in discovery and parseBootnodes() (which is a little different for us than for upstream) being moved to be at the end of the file. I verified that devp2p discv4 still works.
  • In a separate commit, I updated the tests for the new code to include testNetworkId when calling enode.NewLocalNode()
  • I didn't manage to get devp2p discv5 working, I'm not sure if it's because the protocol is different than what discover/discv5 uses or for some other reason. I added networkid as an expected flag for devp2p discv5 in a separate commit, because it uses the same configuration code as devp2p discv4 and so expects that flag. When we want to start using the new implementation of discovery v5, we would (probably?) want to modify it to use the network id similar to how we did with v4. Regardless, checking into whether and how we can use the new implementation would be a substantial task by itself.
• c8e9a91 upgrades golint-ci to 1.24.0. I found that this version gives many more errors, and also sometimes gives spurious typechecking errors (which would make linting in CI flaky). I upgraded to 1.25.0, which was pointed to in the github issue about it. This also resulted in many less lint warnings, though I don't understand why. Upgrading to the latest version (1.33.0) results in even more warnings than with 1.24.0, so I decided not to do it as part of this merge. We can either do it in subsequent merges as upstream moves to newer versions or as part of a standalone linting project.
• Merge up to 023b87b includes a PR which makes it possible to run the HTTP and WS rpc services on the same port. I don't know if we'd benefit from using this functionality once it's in a release, but I wanted to call it out as potentially a nice change
• 15540ae deprecates the --testnet flag (which points to ropsten), and makes automated tests ("whenever possible") use the ropsten config. We already removed the --testnet flag, so that change is moot. So I used the --ours merge strategy and then in a separate commit applied changes from this PR which were.
• 0851646 starts implementing lespay which is planned as part of les/4. For context around lespay, see https://github.com/zsfelfoldi/incentives and https://gist.github.com/zsfelfoldi .
  • One of the conflicts (in les/client.go) was with an if we added that isn't relevant anymore (it was from when istanbul wasn't the only consensus type supported), so I removed it in a separate commit before the merge.
  • The conflicts were not large or difficult, but the PR mentions that a rework of the les server pool will come later (and did come in 1.9.15).
  • Besides the conflicts, there was a compile error that had to be fixed in send() in les/txrelay.go, since we send transactions individually, so to resolve it peer.sendTxs(reqID, len(ll), enc) had to become peer.sendTxs(reqID, 1, enc)
• 5a20cc0 and eb2fd82 were already done when we updated to use go 1.14.2.
• 00064dd updates account/abi to be compatible with Solidity 0.6.0. There was a single conflict, but it was just code that we added and code that they added and they were unrelated, so the solution was to keep both. I missed that one of the newly added tests had to be tweaked in order to pass with our modified signature for NewSimulatedBackend(). I did that in a separate commit at the end.
• the merge up to cbc4ac2 is just the version and trust checkpoint updates (and we don't have checkpoints), so I used strategy 'ours'.
• After finishing the merge, I scanned through the list of commits from 1.9.14 to see if there is anything that we should cherry-pick as part of this PR (notable bugs introduced in 1.9.13 that were noticed and fixed in 1.9.14). I cherry picked two:
  • b0bbd47 (cherry-picked as c2789c8), which fixes a regression that made shutting down take longer while syncing.
  • c2147ee (cherry-picked as 8924d6c), which fixes that snapshot mode was inadvertently being enabled if --gcmode archive was used.

Tested

• Unit and integration tests pass
• geth starts syncing to mainnet, baklava, and alfajores (using the respective flags as appropriate) in full mode
• geth syncs successfully to mainnet, baklava, and alfajores (using the respective flags as appropriate) in lightest mode

Backwards compatibility

There are breaking changes, described above, to eth_call and eth_estimateGas. They would appear in two possible cases:

1. Contract calls which depend on the sender's balance, which were previously giving incorrect results (based on the modified balance of maxuint) and will now give correct results.
2. eth_call calls with a non-zero gas price specified, which will now fail if the sender's account doesn't have enough funds to cover the fee. In contractkit, etc., we use gas price of zero, so it won't be affected. If someone does run into this, they should be able to understand the reason and remove the specified gas price (which isn't needed for eth_call).

[oneeman]

I merged in master (after having merged the 1.9.12 PR into master). There were a few conflicts arising out of #1194 (see the commit message on the merge commit above), but they were not difficult.

[trianglesphere]

I've looked through everything, and the only thing that I feel like I need to think about more is the change in the gas estimation.

I haven't looked at how we externally interface with this, but from my initial reading I'm not sure that we're not going to accidentally break gas currency estimations from web3.

[trianglesphere]

@asaj Has this been handled in the provider / does the new gas price logic make sense?

[oneeman]

To clarify, this doesn't affect gas estimation, because for gas estimation we were previously setting the gas price to zero in core.ApplyEstimatorMessage, and after this change we are setting it to zero below in DoEstimateGas.

The behavior change is as follows (copied from the PR notes):
[copy/pasted]
To sum up, our old behavior was:

• for eth_estimateGas, set gas price and price gas minimum to zero, and set account balance to maxuint
• for eth_call, if gas price arg isn't passed in or is zero, set it to suggested gas price, and set account balance to maxuint

The new behavior is:

• for eth_estimateGas, set gas price and gas price minimum to zero
• for eth_call, set gas price minimum to zero

[end copy/pasted]

My understanding is that in the provider we set the gas price to zero, so that these changes won't cause any problems. It would also not cause problems if the gas price is null (which is interpreted as being zero).

The only cases where it would make a difference are:

1. If you specify a non-zero gas price in CELO and your account doesn't have enough funds to pay for the gas. In that case, the pre-PR behavior would have allowed for a successful call (because of the hack from upstream of setting the balance to maxuint). Post-PR, the call would say the account doesn't have enough funds to pay the fee.
2. If the call itself somehow depends on the account's balance, in which case the pre-PR result would have been incorrect (due to the faking of the balance as maxuint) and the post-PR result would be correct.

[trianglesphere]

Ok, I've wrapped my head around the gas price changes, and it looks good to me.