Update to go-ethereum 1.8.1 #702

pedropombeiro · 2018-02-23T18:16:30Z

This PR updates our vendored geth to 1.8.1, while making sure that any updates to whisper v6 in upstream are reverted to v5.

This involved:

Deleting 0011-geth-17-whisperv6-70fbc87.patch, since v6 no longer needs patching
Remove DNS rebinding protection for localhost, so that e2e tests pass
Address type and method signature changes
Add patch to downgrade usage of Whisper v6 to v5 in some geth 1.8.1 vendor files
Add patches required for cross-compiled builds starting with geth 1.8.0.

Changes of interest in 1.8.1:

Geth now writes less state to disk, slowing down datadir growth (#15857)
The RPC server now protects against DNS rebinding attacks (#15962)
The experimental peer discovery v5 protocol has changed, breaking backwards compatibility with previous releases. Geth 1.8 will not discover LES servers running geth 1.7.
Several bugs around CHT handling are resolved (#15692, #16039)
Light client peer count is now properly limited (#16010)
Whisper protocol v6 as described in EIP-627 is implemented (#15666, #15701, #15802, #15578, #15631, #16046)
Discovery v4 and the experimental discovery v5 protocol now run on the same UDP port (#15200)
Several issues in the experimental discovery v5 protocol are resolved (#15995, #16036)
The GasLimit and GasUsed fields of blocks and transactions have type uint64 instead of *big.Int. This is a breaking API change. (#15466)

Closes #665

mandrigin · 2018-02-23T21:59:43Z

@pombeirp what about other geth-17* patches? They need to be removed as well...

pedropombeiro · 2018-02-23T22:15:48Z

@mandrigin you're right, I was so focused on making the transition to 1.8.1 work well that in the end I overlooked removing the 1.7 patches. But I'm not sure that the one 1.7-only patch can be removed. If you see the differences, the behavior upstream is not the same as in our patch:

So maybe I need to rename this patch to 0010-geth-18-fix-npe-in-filter-system.patch?

mandrigin · 2018-02-23T22:17:52Z

@pombeirp hmm that's weird, I'm a pretty much sure I saw that fixed on master... I'll double check.

adambabik · 2018-02-24T18:42:44Z

geth/common/utils.go

+	if args.Input == nil && args.Data == nil {
+		// If neither Input nor Data were initialized, let's pass an empty slice in Input
+		emptyByteArray := hexutil.Bytes(make([]byte, 0))
+		args.Input = &emptyByteArray


If Input is a pointer why can't it be nil?

@adambabik the changes to the SendTxArgs struct were based on the fact that is looked like a clone of the one in vendor/github.com/ethereum/go-ethereum/internal/ethapi/api.go, so I assumed we wanted to keep them in sync (even the comments in our struct are identical). If we have the freedom over our own struct, we should have a comment on it that clarifies that. I'll add one for your review.

adambabik · 2018-02-24T18:45:46Z

geth/common/rpccall.go

 	if err != nil {
 		return nil
 	}

-	return (*hexutil.Big)(parsedValue)
+	_v := hexutil.Uint64(parsedValue)


_v is a bit strange in Go. v := hexutil.Uint64(parsedValue) looks ordinary.

adambabik · 2018-02-24T18:48:13Z

geth/common/types.go

 	Nonce    *hexutil.Uint64 `json:"nonce"`
+	// We accept "data" and "input" for backwards-compatibility reasons. "input" is the
+	// newer name and should be preferred by clients.
+	Data  *hexutil.Bytes `json:"data"`


Do we need to change the data type to *hexutil.Bytes?

I don't think so, I was trying to keep it similar to geth's struct. Will change it to Input hexutil.Bytes.

adambabik · 2018-02-24T18:53:01Z

geth/common/types.go

 	Nonce    *hexutil.Uint64 `json:"nonce"`
+	// We accept "data" and "input" for backwards-compatibility reasons. "input" is the


It's our internal struct, so maybe we can remove all Data field and fix all occurrences in this PR?

adambabik · 2018-02-24T18:56:23Z

geth/common/utils.go

@@ -143,6 +144,11 @@ func Fatalf(reason interface{}, args ...interface{}) {

 // CreateTransaction returns a transaction object.
 func CreateTransaction(ctx context.Context, args SendTxArgs) *QueuedTx {
+	if args.Input == nil && args.Data == nil {


I don't get why we need this condition.

This was more of a sanity check during the migration that slipped through.

adambabik · 2018-02-24T18:58:21Z

geth/transactions/txqueue_manager.go

+		input = args.Data
+	}
+	if input == nil {
+		return hash, errors.New("Missing Input/Data byte array")


That should be totally fine. Do you get an error if data passed to tx := types.NewTransaction(nonce, toAddr, value, gas, gasPrice, data) is nil?

adambabik · 2018-02-24T19:02:29Z

@pombeirp Overall it looks good. I am not really sure about these changes to SendTxArgs. It's our struct and we have full control over it so we can clean it immediately without having two fields to store data/input. Also, it looks like it may be beneficial to change its Gas field type to non-pointer.

pedropombeiro · 2018-02-26T09:36:56Z

@adambabik thanks for the review so far. Regarding Gas, what you're suggesting is to use 0 in cases where nil was being used (e.g. RPCCall.ParseGas)?

…t of #638

…endor files. Part of #638

… adding exception for localhost. Part of #638

….0. Only applied during build. Part of #638

…s comment in PR #702. Part of #638

…` and `TestCallRawResultGetTransactionReceipt()`. Part of #665

…s comment in PR #702. Part of #638

pedropombeiro · 2018-02-26T13:42:25Z

Adding missing tests on bloom filter logic.

adambabik

Much cleaner 👏 LGTM

mandrigin

👍 huge job! just a few questions...

mandrigin · 2018-02-26T19:22:55Z

geth/common/rpccall.go

@@ -94,23 +94,23 @@ func (r RPCCall) ParseValue() *hexutil.Big {

 // ParseGas returns the hex big associated with the call.
 // nolint: dupl
-func (r RPCCall) ParseGas() *hexutil.Big {
+func (r RPCCall) ParseGas() hexutil.Uint64 {


Does it make any sense to actually return also an error here, what do you think?

mandrigin · 2018-02-26T19:27:50Z

geth/transactions/ethtxclient.go

@@ -84,8 +84,8 @@ func toCallArg(msg ethereum.CallMsg) interface{} {
 	if msg.Value != nil {
 		arg["value"] = (*hexutil.Big)(msg.Value)
 	}
-	if msg.Gas != nil {
-		arg["gas"] = (*hexutil.Big)(msg.Gas)
+	if msg.Gas != 0 {


Is 0 for sure an invalid value for Gas?
Before the logic assumed that there are 3 types of values: non-set (nil), set, but 0, set, != 0.
Here we lost one state.

if so I'd extracted it to a constant to simplify changes in the future

Hence my question above (#702 (comment)). I'm not against keeping the old version that uses nil, tbh.

Reverted the changes to Gas type. I don't think that we gain much by doing that.

mandrigin · 2018-02-26T19:30:40Z

geth/transactions/txqueue_manager.go

 		}
+	} else {
+		gas = uint64(args.Gas)


what if gas set here is < defaultGas?

mandrigin · 2018-02-26T19:33:19Z

_assets/patches/geth/README.md

 - [`0014-whisperv6-notifications.patch`](./0014-whisperv6-notifications.patch) — adds Whisper v6 notifications (need to be reviewed and documented)
 - [`0015-whisperv6-envelopes-tracing.patch`](./0015-whisperv6-envelopes-tracing.patch) — adds Whisper v6 envelope tracing (need to be reviewed and documented)
+- [`0017-geth-18-downgrade-to-whisperv5.patch`](./0017-geth-18-downgrade-to-whisperv5.patch) — some files in geth 1.8 import Whisper v6, instead of v5. This patch ensures v5 is used everywhere


Can we also have a word here about the xgo patches?

Did you see the README.md file in the geth-xgo folder? Or are you just saying we should add a mention here to that folder?

ah, missed that file. it's fine then.

mandrigin

⭐️

pedropombeiro self-assigned this Feb 23, 2018

pedropombeiro requested review from adambabik and divan February 23, 2018 18:16

pedropombeiro force-pushed the feature/geth-1.8.1 branch from 5ea4092 to 70faac8 Compare February 23, 2018 18:18

adambabik suggested changes Feb 24, 2018

View reviewed changes

Pedro Pombeiro and others added 6 commits February 26, 2018 11:02

Update github.com/ethereum/go-ethereum package to 1.8.1 branch. Par…

260b3d4

…t of #638

Fix code due to some signature changes. Part of #638

60c5ee4

use upstream for whisper backend

1d7a8dd

Add patch to downgrade usage of Whisper v6 to v5 in some geth 1.8.1 v…

8b7b329

…endor files. Part of #638

Take into account the DNS rebinding protection introduced in 1.8.0 by…

8302e24

… adding exception for localhost. Part of #638

Add patches required for cross-compiled builds starting with geth 1.8…

9b3113d

….0. Only applied during build. Part of #638

pedropombeiro force-pushed the feature/geth-1.8.1 branch from 70faac8 to 15f1027 Compare February 26, 2018 10:07

pedropombeiro pushed a commit that referenced this pull request Feb 26, 2018

Address comments in PR #702. Part of #638

1c9a4ce

pedropombeiro pushed a commit that referenced this pull request Feb 26, 2018

Address comment in Make SendTxArgs.Gas a hexutil.Uint64 to addres…

15f1027

…s comment in PR #702. Part of #638

pedropombeiro force-pushed the feature/geth-1.8.1 branch from 15f1027 to 6a9fe8f Compare February 26, 2018 10:30

pedropombeiro pushed a commit that referenced this pull request Feb 26, 2018

Address comments in PR #702. Part of #638

2c7481c

pedropombeiro pushed a commit that referenced this pull request Feb 26, 2018

Address comment in Make SendTxArgs.Gas a hexutil.Uint64 to addres…

6a9fe8f

…s comment in PR #702. Part of #638

pedropombeiro force-pushed the feature/geth-1.8.1 branch from 6a9fe8f to 355bd76 Compare February 26, 2018 10:36

pedropombeiro pushed a commit that referenced this pull request Feb 26, 2018

Address comments in PR #702. Part of #638

4494c4a

pedropombeiro pushed a commit that referenced this pull request Feb 26, 2018

Address comment in Make SendTxArgs.Gas a hexutil.Uint64 to addres…

355bd76

…s comment in PR #702. Part of #638

pedropombeiro mentioned this pull request Feb 26, 2018

Update codebase to leverage Whisper v6 #703

Merged

4 tasks

Pedro Pombeiro added 3 commits February 26, 2018 14:09

Update expected JSON result in `TestRegressionGetTransactionReceipt()…

565f32e

…` and `TestCallRawResultGetTransactionReceipt()`. Part of #665

Fix some failing e2e tests. Part of #638

98030ee

Address comments in PR #702. Part of #638

53fac7f

pedropombeiro force-pushed the feature/geth-1.8.1 branch from 355bd76 to c3c2eb0 Compare February 26, 2018 13:10

pedropombeiro pushed a commit that referenced this pull request Feb 26, 2018

Address comment in Make SendTxArgs.Gas a hexutil.Uint64 to addres…

c3c2eb0

…s comment in PR #702. Part of #638

adambabik approved these changes Feb 26, 2018

View reviewed changes

mandrigin suggested changes Feb 26, 2018

View reviewed changes

pedropombeiro force-pushed the feature/geth-1.8.1 branch from c3c2eb0 to 53fac7f Compare February 27, 2018 08:04

mandrigin approved these changes Feb 27, 2018

View reviewed changes

pedropombeiro merged commit e4cbce1 into develop Feb 27, 2018

adambabik deleted the feature/geth-1.8.1 branch December 27, 2019 10:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update to go-ethereum 1.8.1 #702

Update to go-ethereum 1.8.1 #702

pedropombeiro commented Feb 23, 2018 •

edited

Loading

mandrigin commented Feb 23, 2018

pedropombeiro commented Feb 23, 2018 •

edited

Loading

mandrigin commented Feb 23, 2018

adambabik Feb 24, 2018

pedropombeiro Feb 26, 2018

adambabik Feb 24, 2018

pedropombeiro Feb 26, 2018

adambabik Feb 24, 2018

pedropombeiro Feb 26, 2018

adambabik Feb 24, 2018

adambabik Feb 24, 2018

pedropombeiro Feb 26, 2018

adambabik Feb 24, 2018

adambabik commented Feb 24, 2018

pedropombeiro commented Feb 26, 2018 •

edited

Loading

pedropombeiro commented Feb 26, 2018

adambabik left a comment

mandrigin left a comment

mandrigin Feb 26, 2018

mandrigin Feb 26, 2018

mandrigin Feb 26, 2018

pedropombeiro Feb 26, 2018

pedropombeiro Feb 27, 2018

mandrigin Feb 26, 2018

mandrigin Feb 26, 2018

pedropombeiro Feb 26, 2018

mandrigin Feb 26, 2018

mandrigin left a comment

		Nonce *hexutil.Uint64 `json:"nonce"`
		// We accept "data" and "input" for backwards-compatibility reasons. "input" is the

Update to go-ethereum 1.8.1 #702

Update to go-ethereum 1.8.1 #702

Conversation

pedropombeiro commented Feb 23, 2018 • edited Loading

mandrigin commented Feb 23, 2018

pedropombeiro commented Feb 23, 2018 • edited Loading

mandrigin commented Feb 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adambabik commented Feb 24, 2018

pedropombeiro commented Feb 26, 2018 • edited Loading

pedropombeiro commented Feb 26, 2018

adambabik left a comment

Choose a reason for hiding this comment

mandrigin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mandrigin left a comment

Choose a reason for hiding this comment

pedropombeiro commented Feb 23, 2018 •

edited

Loading

pedropombeiro commented Feb 23, 2018 •

edited

Loading

pedropombeiro commented Feb 26, 2018 •

edited

Loading