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/foundation release/1.12.2 #560

Merged
merged 283 commits into from
Sep 6, 2023
Merged

Conversation

ziogaschr
Copy link
Member

@ziogaschr ziogaschr commented Aug 29, 2023

Merges ethereum/go-ethereum branch release/1.12.2 to core-geth master.

Notes


  • Do you agree on replacing Clique with Catalyst for --dev mode? We still allow use --dev.pow for ethash. 9d4ea2e
  • This change follows the go-ethereum interfaces, where a Block header can have withdrawalsRoot while the Block itself can have withdrawals. 23da7ca
    ...

TODO

stephenfire and others added 30 commits June 7, 2023 12:40
rm error when marshal block to rpc type allen
This PR adds server-side limits for JSON-RPC batch requests. Before this change, batches
were limited only by processing time. The server would pick calls from the batch and
answer them until the response timeout occurred, then stop processing the remaining batch
items.

Here, we are adding two additional limits which can be configured:

- the 'item limit': batches can have at most N items
- the 'response size limit': batches can contain at most X response bytes

These limits are optional in package rpc. In Geth, we set a default limit of 1000 items
and 25MB response size.

When a batch goes over the limit, an error response is returned to the client. However,
doing this correctly isn't always possible. In JSON-RPC, only method calls with a valid
`id` can be responded to. Since batches may also contain non-call messages or
notifications, the best effort thing we can do to report an error with the batch itself is
reporting the limit violation as an error for the first method call in the batch. If a batch is
too large, but contains only notifications and responses, the error will be reported with
a null `id`.

The RPC client was also changed so it can deal with errors resulting from too large
batches. An older client connected to the server code in this PR could get stuck
until the request timeout occurred when the batch is too large. **Upgrading to a version
of the RPC client containing this change is strongly recommended to avoid timeout issues.**

For some weird reason, when writing the original client implementation, @fjl worked off of
the assumption that responses could be distributed across batches arbitrarily. So for a
batch request containing requests `[A B C]`, the server could respond with `[A B C]` but
also with `[A B] [C]` or even `[A] [B] [C]` and it wouldn't make a difference to the
client.

So in the implementation of BatchCallContext, the client waited for all requests in the
batch individually. If the server didn't respond to some of the requests in the batch, the
client would eventually just time out (if a context was used).

With the addition of batch limits into the server, we anticipate that people will hit this
kind of error way more often. To handle this properly, the client now waits for a single
response batch and expects it to contain all responses to the requests.

---------

Co-authored-by: Felix Lange <[email protected]>
Co-authored-by: Martin Holst Swende <[email protected]>
This adds two ways to check for subscription support. First, one can now check
whether the transport method (HTTP/WS/etc.) is capable of subscriptions using
the new Client.SupportsSubscriptions method.

Second, the error returned by Subscribe can now reliably be tested using this
pattern:
    
    sub, err := client.Subscribe(...)
    if errors.Is(err, rpc.ErrNotificationsUnsupported) {
        // no subscription support
    }

---------

Co-authored-by: Felix Lange <[email protected]>
node: Delete the unused error from return parameters of Node.Attach() func
We had to do this workaround because it wasn't possible to export typed arrays from
JS to []byte. This was added in dop251/goja@2352993, so we can use the better way now.
The logs in this function are pulled straight from disk in rawdb.ReadRawReceipts and 
also modified in receipts.DeriveFields, so removing the copy should be fine.
fix some typos

Signed-off-by: cui fliter <[email protected]>
fix typos

Co-authored-by: john <[email protected]>
* all: move main transaction pool into a subpool

* go.mod: remove superfluous updates

* core/txpool: review fixes, handle txs rejected by all subpools

* core/txpool: typos
… (#27481)

This change ensures Reheap will be called even before the London fork activates.
Since Reheap would otherwise only be called through `SetBaseFee` after London,
the list would just keep growing if the fork was not enabled or not reached yet.
… (#27477)

floatingRatio is a constant and always non-zero. So there is no need to
check for == 0.
Fixes #27301, a crash that could occur during txpool reorg handling.
Package rpc uses cgo to find the maximum UNIX domain socket path 
length. If exceeded, a warning is printed. This is the only use of cgo in this
package. It seems excessive to depend on cgo just for this warning, so
we now hard-code the usual limit for Linux instead.

---------

Co-authored-by: Felix Lange <[email protected]>
Also adds Address.Less for sorting use in other packages.

---------

Co-authored-by: Felix Lange <[email protected]>
meowsbits and others added 9 commits August 29, 2023 07:50
Conflicts:
      core/genesis.go
      core/txpool/txpool.go
      core/vm/jump_table.go
      params/types/genesisT/genesis.go
      tests/init.go
By Go conventions the Error method should
return a descriptive string.

When I was debugging the simulated beacon
test, the reported error was hardly usedful,
describing only that an error existed, but lacking
any contextual detail.

Now the engine api will return a more
detailed error and that will hopefully
help debuggers in the future.

Date: 2023-08-29 09:01:11-06:00
Signed-off-by: meows <[email protected]>
This logic was just wrong because of
misplaced negation.

Date: 2023-08-29 09:10:51-06:00
Signed-off-by: meows <[email protected]>
This reverts commit ae0efa1.

Reason: it caused a test failure TestGetBlockBodiesByRangeInvalidParams
where that test compares the have/want errors as strings
and I can't be bothered to adjust the test.

It would be better, probably, to use errors.Is with wrapped
errors.
CI tests were failing because the Cancun
suite was not skipped.

Core-Geth still only supports EVMCv7, so
all forks thereafter need to be skipped.

AFAIK an upgrade to EVMCv10 should catch
up to compatibility for these later forks.

Date: 2023-08-29 09:55:26-06:00
Signed-off-by: meows <[email protected]>
@ziogaschr
Copy link
Member Author

Note: I still want to test the functionality while running in --dev mode

ziogaschr and others added 2 commits August 31, 2023 14:59
… number

In order to check eip4895 features for ETC-style
block number-based activations, the attributes get
a block number field which is allowed to be nil.

As usual for core-geth, activation numbers are OR'd
with activation times.

Date: 2023-09-01 10:18:47-06:00
Signed-off-by: meows <[email protected]>
Copy link
Contributor

@diega diega left a comment

Choose a reason for hiding this comment

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

This looks really well. Just made two microscopical comments

cmd/clef/README.md Outdated Show resolved Hide resolved
cmd/devp2p/README.md Outdated Show resolved Hide resolved
ziogaschr and others added 3 commits September 4, 2023 13:03
Co-authored-by: Diego López León <[email protected]>
Co-authored-by: Diego López León <[email protected]>
... but reference forks by name to
keep the important context.

Date: 2023-09-04 05:22:48-06:00
Signed-off-by: meows <[email protected]>
@meowsbits
Copy link
Member

Two new fields are added to the Genesis type: ExcessBlobGas and BlobGasUsed. While these, along with BaseFee, are relevant to ETHereum chains, they are not relevant for all chains that core-geth supports, resulting in empty (null) fields.

What do you think about adding omitempty tags for these fields?

diff --git a/params/types/genesisT/gen_genesis.go b/params/types/genesisT/gen_genesis.go
index 0136309ee6..12f3d10b05 100644
--- a/params/types/genesisT/gen_genesis.go
+++ b/params/types/genesisT/gen_genesis.go
@@ -33,9 +33,9 @@ func (g Genesis) MarshalJSON() ([]byte, error) {
 		Number        math.HexOrDecimal64                         `json:"number"`
 		GasUsed       math.HexOrDecimal64                         `json:"gasUsed"`
 		ParentHash    common.Hash                                 `json:"parentHash"`
-		BaseFee       *math.HexOrDecimal256                       `json:"baseFeePerGas"`
-		ExcessBlobGas *math.HexOrDecimal64                        `json:"excessBlobGas"`
-		BlobGasUsed   *math.HexOrDecimal64                        `json:"blobGasUsed"`
+		BaseFee       *math.HexOrDecimal256                       `json:"baseFeePerGas,omitempty"`
+		ExcessBlobGas *math.HexOrDecimal64                        `json:"excessBlobGas,omitempty"`
+		BlobGasUsed   *math.HexOrDecimal64                        `json:"blobGasUsed,omitempty"`
 	}
 	var enc Genesis
 	enc.Config = g.Config
diff --git a/params/types/genesisT/genesis.go b/params/types/genesisT/genesis.go
index f2be54ec74..97676daee0 100644
--- a/params/types/genesisT/genesis.go
+++ b/params/types/genesisT/genesis.go
@@ -55,9 +55,9 @@ type Genesis struct {
 	Number        uint64      `json:"number"`
 	GasUsed       uint64      `json:"gasUsed"`
 	ParentHash    common.Hash `json:"parentHash"`
-	BaseFee       *big.Int    `json:"baseFeePerGas"` // EIP-1559
-	ExcessBlobGas *uint64     `json:"excessBlobGas"` // EIP-4844
-	BlobGasUsed   *uint64     `json:"blobGasUsed"`   // EIP-4844
+	BaseFee       *big.Int    `json:"baseFeePerGas,omitempty"` // EIP-1559
+	ExcessBlobGas *uint64     `json:"excessBlobGas,omitempty"` // EIP-4844
+	BlobGasUsed   *uint64     `json:"blobGasUsed,omitempty"`   // EIP-4844
 }
 
 func (g *Genesis) GetElasticityMultiplier() uint64 {

@ziogaschr
Copy link
Member Author

Two new fields are added to the Genesis type: ExcessBlobGas and BlobGasUsed. While these, along with BaseFee, are relevant to ETHereum chains, they are not relevant for all chains that core-geth supports, resulting in empty (null) fields.

What do you think about adding omitempty tags for these fields?

diff --git a/params/types/genesisT/gen_genesis.go b/params/types/genesisT/gen_genesis.go
index 0136309ee6..12f3d10b05 100644
--- a/params/types/genesisT/gen_genesis.go
+++ b/params/types/genesisT/gen_genesis.go
@@ -33,9 +33,9 @@ func (g Genesis) MarshalJSON() ([]byte, error) {
 		Number        math.HexOrDecimal64                         `json:"number"`
 		GasUsed       math.HexOrDecimal64                         `json:"gasUsed"`
 		ParentHash    common.Hash                                 `json:"parentHash"`
-		BaseFee       *math.HexOrDecimal256                       `json:"baseFeePerGas"`
-		ExcessBlobGas *math.HexOrDecimal64                        `json:"excessBlobGas"`
-		BlobGasUsed   *math.HexOrDecimal64                        `json:"blobGasUsed"`
+		BaseFee       *math.HexOrDecimal256                       `json:"baseFeePerGas,omitempty"`
+		ExcessBlobGas *math.HexOrDecimal64                        `json:"excessBlobGas,omitempty"`
+		BlobGasUsed   *math.HexOrDecimal64                        `json:"blobGasUsed,omitempty"`
 	}
 	var enc Genesis
 	enc.Config = g.Config
diff --git a/params/types/genesisT/genesis.go b/params/types/genesisT/genesis.go
index f2be54ec74..97676daee0 100644
--- a/params/types/genesisT/genesis.go
+++ b/params/types/genesisT/genesis.go
@@ -55,9 +55,9 @@ type Genesis struct {
 	Number        uint64      `json:"number"`
 	GasUsed       uint64      `json:"gasUsed"`
 	ParentHash    common.Hash `json:"parentHash"`
-	BaseFee       *big.Int    `json:"baseFeePerGas"` // EIP-1559
-	ExcessBlobGas *uint64     `json:"excessBlobGas"` // EIP-4844
-	BlobGasUsed   *uint64     `json:"blobGasUsed"`   // EIP-4844
+	BaseFee       *big.Int    `json:"baseFeePerGas,omitempty"` // EIP-1559
+	ExcessBlobGas *uint64     `json:"excessBlobGas,omitempty"` // EIP-4844
+	BlobGasUsed   *uint64     `json:"blobGasUsed,omitempty"`   // EIP-4844
 }
 
 func (g *Genesis) GetElasticityMultiplier() uint64 {

Makes sense to me too. Might be that a test can break, in case they check for this fields, but maybe it makes sense to modify those tests as well.

@meowsbits
Copy link
Member

Yea, my rationale for removing them is mostly based on the pattern of extensive use of omitempty in the chain config types, which is obviously a closely related data type. So I would infer and assume that similar tests would be used for both data types, and that therefore checks for the existence or absence of fields should have similar expectations (permitting, or even expecting, that zero-value fields are omitted).

@meowsbits meowsbits force-pushed the merge/foundation-release/1.12.2 branch from 3e5299b to 00b4a67 Compare September 4, 2023 13:58
meowsbits and others added 5 commits September 4, 2023 08:03
Date: 2023-09-04 08:03:21-06:00
Signed-off-by: meows <[email protected]>
These fields are not supported by all
chains which core-geth supports.
Since chain config data types omit empty values,
it follows that genesis ought follow suit.

Date: 2023-09-04 08:38:51-06:00
Signed-off-by: meows <[email protected]>
…oregeth,params/types/goethereum: problem: TestEquivalent_Features fails"

This reverts commit a5e0c62.
The ECIP does not specify valid activation
blocks yet, so we should not code them.
https://github.com/ethereumclassic/ECIPs/blob/64423c73b7416c2f8e40d4c66bf40b715c458b8c/_specs/ecip-1109.md

Date: 2023-09-04 10:34:38-06:00
Signed-off-by: meows <[email protected]>
@ziogaschr ziogaschr merged commit bcbe345 into master Sep 6, 2023
5 checks passed
@ziogaschr ziogaschr deleted the merge/foundation-release/1.12.2 branch September 6, 2023 10:48
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.