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 1.13 #279

Merged
merged 115 commits into from
Jan 24, 2024
Merged

Merge 1.13 #279

merged 115 commits into from
Jan 24, 2024

Conversation

tsahee
Copy link
Contributor

@tsahee tsahee commented Jan 18, 2024

Merges geth 1.13.0 to stylus.
Uses Tristian's https://github.com/Tristan-Wilson/MyScripts/blob/main/bin/merge-helper
Note that merging 27841 had a bug which was fixed by a separate (currently last) commit

holiman and others added 30 commits August 10, 2023 06:49
… (#27891)

This change removes a chainconfig parameter passed into rawdb.ReadLogs, which is not used nor needed.
It also modifies the filter loop slightly, avoiding a labeled break and instead using a method.

This change does not modify any behaviour.
build(deps): bump github.com/supranational/blst

Bumps [github.com/supranational/blst](https://github.com/supranational/blst) from 0.3.11-0.20230406105308-e9dfc5ee724b to 0.3.11.
- [Release notes](https://github.com/supranational/blst/releases)
- [Commits](https://github.com/supranational/blst/commits/v0.3.11)

---
updated-dependencies:
- dependency-name: github.com/supranational/blst
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Block takes a number and a hash. The spec is unclear on what should happen in this case, leaving it an implemenation detail. With this change, we return an error in case both number and hash are passed in.
* all: activate pbss

* core/rawdb: fix compilation error

* cma, core, eth, les, trie: address comments

* cmd, core, eth, trie: polish code

* core, cmd, eth: address comments

* cmd, core, eth, les, light, tests: address comment

* cmd/utils: shorten log message

* trie/triedb/pathdb: limit node buffer size to 1gb

* cmd/utils: fix opening non-existing db

* cmd/utils: rename flag name

* cmd, core: group chain history flags and fix tests

* core, eth, trie: fix memory leak in snapshot generation

* cmd, eth, internal: deprecate flags

* all: enable state tests for pathdb, fixes

* cmd, core: polish code

* trie/triedb/pathdb: limit the node buffer size to 256mb

---------

Co-authored-by: Martin Holst Swende <[email protected]>
Co-authored-by: Péter Szilágyi <[email protected]>
This should fix #27726. With enough load, it might happen that the SetPongHandler 
callback gets invoked before the call to SetReadDeadline is made in pingLoop. When 
this occurs, the socket will end up with a 30s read deadline even though it got the pong,
which will lead to a timeout.

The fix here is processing the pong on pingLoop, synchronizing with the code that 
sends the ping.
Remove duplication in signer
---------

Co-authored-by: GDdark <[email protected]>
Co-authored-by: Felix Lange <[email protected]>
The Go authors updated golang/x/ext to change the function signature of the slices sort method. 
It's an entire shitshow now because x/ext is not tagged, so everyone's codebase just 
picked a new version that some other dep depends on, causing our code to fail building.

This PR updates the dep on our code too and does all the refactorings to follow upstream...
This upgrades to the latest release of ckzg, and also attempts to fix some blst-related
build errors that occur on launchpad.net.
This PR removes the newly added txpool.Transaction wrapper type, and instead adds a way
of keeping the blob sidecar within types.Transaction. It's better this way because most
code in go-ethereum does not care about blob transactions, and probably never will. This
will start mattering especially on the client side of RPC, where all APIs are based on
types.Transaction. Users need to be able to use the same signing flows they already
have.

However, since blobs are only allowed in some places but not others, we will now need to
add checks to avoid creating invalid blocks. I'm still trying to figure out the best place
to do some of these. The way I have it currently is as follows:

- In block validation (import), txs are verified not to have a blob sidecar.
- In miner, we strip off the sidecar when committing the transaction into the block.
- In TxPool validation, txs must have a sidecar to be added into the blobpool.
  - Note there is a special case here: when transactions are re-added because of a chain
    reorg, we cannot use the transactions gathered from the old chain blocks as-is,
    because they will be missing their blobs. This was previously handled by storing the
    blobs into the 'blobpool limbo'. The code has now changed to store the full
    transaction in the limbo instead, but it might be confusing for code readers why we're
    not simply adding the types.Transaction we already have.

Code changes summary:

- txpool.Transaction removed and all uses replaced by types.Transaction again
- blobpool now stores types.Transaction instead of defining its own blobTx format for storage
- the blobpool limbo now stores types.Transaction instead of storing only the blobs
- checks to validate the presence/absence of the blob sidecar added in certain critical places
This fixes a regression where -txlookuplimit was not applied anymore.
This change fixes a bug in js tracer, where `ctx.GasPrice.toString(16)` returns a number string in base `10`.
This raises the JSON-RPC batch request limits significantly for the engine API endpoint.
The limits are now also hard-coded, so users won't get them wrong. I have chosen these limits:

    maximum batch items: 2000
    maximum batch response size: 250MB

While it would also be possible to disable batch limits completely for the engine API, 
I think having some limits is a good safety net against misbehaving CLs. Since this
 isn't configurable, we really want to ensure this limit will never become an issue in the
 CL/EL communication, so I set them quite high.

---------

Signed-off-by: jsvisa <[email protected]>
Co-authored-by: Felix Lange <[email protected]>
FromBig returns true *when overflow occurs*
…d in genesis (#27895)

This changes the forkID calculation to ignore time-based forks that occurred before the
genesis block. It's supposed to be done this way because the spec says:

> If a chain is configured to start with a non-Frontier ruleset already in its genesis, that is NOT considered a fork.
This change fixes the a potential race by using mutexes when the m.cache is read or modified.
ReadSkeletonHeader can return nil if the header is missing, so we should
not access fields on it. Note that calling .Hash() on a nil header is fine, so there 
is no need to actually check for nil.

Co-authored-by: Martin Holst Swende <[email protected]>
Optimizations:

- Previously, if a transaction was reverting, EstimateGas would exhibit worst-case behavior and binary search up to the max gas limit (~40 state-clone + tx executions). This change allows EstimateGas to return after only a single unconstrained execution in this scenario.
- Uses the gas used from the unconstrained execution to bias the remaining binary search towards the likely solution in a simple way that doesn't impact the worst case. For a typical contract-invoking transaction, this reduces the median number of state-clone+executions from 25 to 18 (28% reduction).

Cleanup:

- added & improved function + code comments
- correct the EstimateGas documentation to clarify the gas limit determination is at latest block, not pending, if the blockNr is unspecified.
27891 remove chainconfig param from ReadLogs.
Removing this param from our code required using ReadRawReceipts,
and filling the missing log fields that ReadReceipts does.
Conflicts:
  params/version.go
trivial whitespace conflict. Took the correct version.
PR 27841 made modifications to TxData - removed blob-specific functions,
and added encode and decode functions.

 Additions:
  core/types/arb_types.go:
Fixed out internal types to follow the new TxData interface.

 Conflicts:
  core/types/transaction.go
Conflicts in decodeTyped because decoding changed. Adapted out types to
the new way.

  core/types/tx_blob.go:
Our changes to TxData conflicted with the PRs. Applied both.

  miner/worker.go
both changed call to ApplyTransaction, applied both.
upstream PR 27702 implements eth_getBlockReceipts.
To do so it extracts marshalreceipts.

 Conflicts:
  internal/ethapi/api.go

We add a bunch of fields to receipts returned, which required
adding ctx and Backend to marshalReceipts.
 Conflicts:
  internal/ethapi/api.go:
We modified call to DoCall, upstream moved it to an internal function,
kept our modifications in the new place.
 Conflicts:
  internal/ethapi/api.go:
Add/Add conflicts for API fields. Accepted both adds.
 Conflicts:
  eth/catalyst/simulated_beacon.go

The PR changes simulated beacon. Took upstream version as-is.
upstream PR #27956 ignores the-0 block when importing.
Also ignore pre-arbitrum-genesis blocks.
 Conflicts:
  cmd/evm/internal/t8ntool/transition.go
Callsite for IsShanghai moved to a different function, moved our
modification there.

 Additions:
  go-ethereum/cmd/evm/testdata/29/exp.json
Added Arbitrum field to expected receipt.
 Conflicts:
  core/blockchain.go
We changed writeBlockWithState, and upstream added a return value to
TrieDB().Size() - that extra value is ignored both upstream and
after merge.

 Additions:
  arbitrum/recordingdb.go
The same change for TrieDB().Size()
miner: add to build block with EIP-4844 blobs

 Conflicts:
  miner/worker.go
simple conflict because we added a return param. accepted both changes.
core/types: transaction and receipt encoding/decoding optimizations

 Conflicts:
  core/types/transaction.go
conflict due to us adding a parameter to decodeTyped function. Added
both changes.
 beacon/engine, eth/catalyst, miner: EIP-4788 CL/EL protocol updates
 (#27872)

 Conflicts:
  eth/catalyst/api.go
callSite to IsShanghai moved. New callsirte also calls IsCancun.
I solved the conflict by assuming IsCancun will also accept ArbOS,
without modifying IsCancun itself - this will be done in the next commit
to keep things separate and easier to review.

  miner/worker.go
callsite to ApplyTransaction moved, we have a change that adds a new
(here ignored) value
This commit doesn't yet use the parameter, but does align callsites.
core/state: simplify storage trie update and commit (#28030)

 Conflicts:
  core/state/statedb.go
Trivial conflicts - we added a field to revision struct, upstream
removed following lines - included both changes.

 Additions:
  arbitrum_types/txoptions.go:
The way to get storage root for an account changed. Update our code.
core, trie: cleanup trie database (#28062)

 Conflicts:
  core/state/statedb.go
Both us and upstream added fields to the end of StateDb. kept both.
Copy link

cla-bot bot commented Jan 18, 2024

We require contributors to sign our Contributor License Agreement. In order for us to review and merge your code, please sign the linked documents below to get yourself added. https://na3.docusign.net/Member/PowerFormSigning.aspx?PowerFormId=b15c81cc-b5ea-42a6-9107-3992526f2898&env=na3&acct=6e152afc-6284-44af-a4c1-d8ef291db402&v=2

Tristan-Wilson
Tristan-Wilson previously approved these changes Jan 22, 2024
Copy link
Member

@Tristan-Wilson Tristan-Wilson left a comment

Choose a reason for hiding this comment

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

Looks good with one super minor thing to fix.

Thanks for putting the conflicting upstream PR number in the title, that made it very easy to get needed context.

@@ -45,7 +50,7 @@ func TestHeaderVerification(t *testing.T) {
headers[i] = block.Header()
}
// Run the header checker for blocks one-by-one, checking for both valid and invalid nonces
chain, _ := NewBlockChain(rawdb.NewMemoryDatabase(), nil, nil, gspec, nil, ethash.NewFaker(), vm.Config{}, nil, nil)
Copy link
Member

Choose a reason for hiding this comment

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

We don't have to do anything this merge, but I think we never actually set the extra parameter we added in geth's code and only in one place in Nitro. It might be worth making a special NewBlockChainNitro function that has the extra parameter to avoid all the merge conflicts every time, and possibly have the original forward to it.

Copy link
Member

Choose a reason for hiding this comment

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

  • git grep -oe 'NewBlockChain([^,]*, [^,]*, [^n][^,]*,.*' to see for yourself

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ooh, that's a good idea!
another option I thought about: moving the extra parameter into cacheConfig

Comment on lines 775 to 776
logs[i][j].BlockNumber = number
logs[i][j].BlockNumber = number
Copy link
Member

Choose a reason for hiding this comment

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

Duplicate line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice catch! thanks!

Copy link

cla-bot bot commented Jan 24, 2024

We require contributors to sign our Contributor License Agreement. In order for us to review and merge your code, please sign the linked documents below to get yourself added. https://na3.docusign.net/Member/PowerFormSigning.aspx?PowerFormId=b15c81cc-b5ea-42a6-9107-3992526f2898&env=na3&acct=6e152afc-6284-44af-a4c1-d8ef291db402&v=2

Copy link
Collaborator

@PlasmaPower PlasmaPower left a comment

Choose a reason for hiding this comment

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

Changes since Tristan's approval LGTM, and I'd like to get this merged for 4844 support

@PlasmaPower PlasmaPower merged commit 6eb9331 into master Jan 24, 2024
2 of 3 checks passed
@PlasmaPower PlasmaPower deleted the merge-1.13 branch January 24, 2024 01:45
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.