Merge v1.11.2 #205
Merged
Merge v1.11.2 #205
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This changes how we read performance metrics from the Go runtime. Instead of using runtime.ReadMemStats, we now rely on the API provided by package runtime/metrics. runtime/metrics provides more accurate information. For example, the new interface has better reporting of memory use. In my testing, the reported value of held memory more accurately reflects the usage reported by the OS. The semantics of metrics system/memory/allocs and system/memory/frees have changed to report amounts in bytes. ReadMemStats only reported the count of allocations in number-of-objects. This is imprecise: 'tiny objects' are not counted because the runtime allocates them in batches; and certain improvements in allocation behavior, such as struct size optimizations, will be less visible when the number of allocs doesn't change. Changing allocation reports to be in bytes makes it appear in graphs that lots more is being allocated. I don't think that's a problem because this metric is primarily interesting for geth developers. The metric system/memory/pauses has been changed to report statistical values from the histogram provided by the runtime. Its name in influxdb has changed from geth.system/memory/pauses.meter to geth.system/memory/pauses.histogram. We also have a new histogram metric, system/cpu/schedlatency, reporting the Go scheduler latency.
* eth/catalyst: fix time-dependent (flaky) test * eth: increase timeout on TestTransactionPropagation
This fixes a problem in the SizeConstrainedLRU. The SCLRU uses an underlying simple lru which is not thread safe. During the Get operation, the recentness of the accessed item is updated, so it is not a pure read-operation. Therefore, the mutex we need is a full mutex, not RLock. This PR changes the mutex to be a regular Mutex, instead of RWMutex, so a reviewer can at a glance see that all affected locations are fixed.
This PR changes the pending tx subscription to return RPCTransaction types instead of normal Transaction objects. This will fix the inconsistencies with other tx returning API methods (i.e. getTransactionByHash), and also fill in the sender value for the tx. co-authored by @s1na
Co-authored-by: Felix Lange <fjl@twurst.com>
It seems there is no fully typed library implementation of an LRU cache. So I wrote one. Method names are the same as github.com/hashicorp/golang-lru, and the new type can be used as a drop-in replacement. Two reasons to do this: - It's much easier to understand what a cache is for when the types are right there. - Performance: the new implementation is slightly faster and performs zero memory allocations in Add when the cache is at capacity. Overall, memory usage of the cache is much reduced because keys are values are no longer wrapped in interface.
rpc: fix connection tracking in Server When upgrading to mapset/v2 with generics, the set element type used in rpc.Server had to be changed to *ServerCodec because ServerCodec is not 'comparable'. While the distinction is technically correct, we know all possible ServerCodec types, and all of them are comparable. So just use a map instead.
closes #26188
Implements TSTORE and TLOAD as specified by the following EIP: https://eips.ethereum.org/EIPS/eip-1153 https://ethereum-magicians.org/t/eip-1153-transient-storage-opcodes/553 Co-authored-by: Sara Reynolds <snreynolds2506@gmail.com> Co-authored-by: Martin Holst Swende <martin@swende.se> Co-authored-by: Gary Rong <garyrong0905@gmail.com>
This adds a way to specify HTTP headers per request. Co-authored-by: Martin Holst Swende <martin@swende.se> Co-authored-by: Felix Lange <fjl@twurst.com>
Use noopTracer as a base for other native tracers to avoid extra boilerplate for unimplemented hooks.
This PR improves and extends the tests a bit
This removes an RPC test which takes > 90s to execute, and updates the internal/guide tests to use lighter scrypt parameters. Co-authored-by: Felix Lange <fjl@twurst.com>
This prevents DoS when connected to a malicious ethstats server.
This makes a couple of sometimes-failing tests less brittle.
This change logs the path checked when encountering low disk space.
Implements EIP-3651, "Warm Coinbase", for Shanghai hardfork. Specification: https://eips.ethereum.org/EIPS/eip-3651.
While investigating #22374, I noticed that the Sync operation of the freezer does not take the table lock. It also doesn't call sync for all files if there is an error with one of them. I doubt this will fix anything, but didn't want to drop the fix on the floor either.
* core/rawdb: fix freezer validation * core/rawdb: address comment
This PR should makes it easier to sign EIP-712 typed data via the accounts.Wallet API, by using the mimetype for typed data. Co-authored-by: nasdf <keenan.nemetz@gmail.com>
This is to cater for more node providers.
web3.js's eth_call which we were defaulting to doesn't have the stateOverrides parameter, so this param wasn't working in the console.
refactoring without logic change
A few tests weren't updated when we changed TriesInMemory to DefaultTriesInMemory.
tsahee
requested changes
Mar 26, 2023
There was a problem hiding this comment.
Generally this seems good.
My review was based off commits after the merge commit and pointers from the detailed PR (thanks! it was really helpful!)
Added some minor comments.
As for testing - we should manually test at least:
- validating (jit validation is good enough) at least 1M blocks from arbitrum one. Need to test both existing on-chain WASM and "latest" that's generated by this code.
- fetching a receipt from pre-nitro arb1 and making sure new receipt is identical to old one.
| @@ -20,6 +20,7 @@ | |||
| "logsBloom": "0x00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000", | |||
| "receipts": [ | |||
| { | |||
| "gasUsedForL1": "0x0", | |||
There was a problem hiding this comment.
instead of all of those - maybe we could just add omitempty to receipt's Un/MarshalJson?
GasUsedForL1 hexutil.Uint64 `json:"gasUsedForL1"`
There was a problem hiding this comment.
Hmm.. people might be expecting this from our RPC even for L1->L2 txs unfortunately
There was a problem hiding this comment.
Ok, will leave it as-is
core/blockchain.go
Outdated
| prevNum = uint64(-number) | ||
| } | ||
| flushInterval := time.Duration(atomic.LoadInt64(&bc.flushInterval)) | ||
| // If we exceeded out time allowance, flush an entire trie to disk | ||
| if bc.gcproc > bc.cacheConfig.TrieTimeLimit && prevEntry != nil { |
There was a problem hiding this comment.
this should be if gcproc > flushInterval
There was a problem hiding this comment.
Alright, changed it. I think they should be the same value anyway because flushInterval is set with TrieTimeLimit in NewBlockChain.
core/blockchain.go
Outdated
| triegcEntry := tmp.(trieGcEntry) | ||
| if uint64(-number) > uint64(blockLimit) || triegcEntry.Timestamp > uint64(timeLimit) { | ||
| bc.triegc.Push(triegcEntry, number) | ||
| root, number := bc.triegc.Pop() |
There was a problem hiding this comment.
our variable (unlike mainline) is not a root, but a triegc entry that has both root and timestamp.. rename "root" to triegcEntry, or something similar..
trie/trie.go
Outdated
| @@ -284,6 +285,12 @@ func (t *Trie) tryUpdate(key, value []byte) error { | |||
| return nil | |||
| } | |||
|
|
|||
| // Arbitrum TODO: This was removed upstream, we should fix our code and remove it. | |||
There was a problem hiding this comment.
agree with this TODO :)
Notice that DiskDB was removed from TrieDb but added to state.Database
The simplest solution seems to be adding a KeyValueStore member to recordingKV, and initializing it with something like:
newRecordingKV(r.db.TrieDb(), r.db.DiskDB())
There was a problem hiding this comment.
Done, thanks for the advice.
Conflicts: core/blockchain.go core/state/pruner/pruner.go
|
Most of the conflicts came from #186, @PlasmaPower can you please check my resolution of them? |
tsahee
previously approved these changes
Apr 18, 2023
PlasmaPower
reviewed
May 5, 2023
core/blockchain.go
Outdated
|
|
||
| if chainConfig.IsArbitrum() { | ||
| genesisHash = rawdb.ReadCanonicalHash(db, chainConfig.ArbitrumChainParams.GenesisBlockNum) | ||
| if (genesisHash == common.Hash{}) { |
There was a problem hiding this comment.
There's an extra set of parens here
There was a problem hiding this comment.
Ah, I see it's for the common.Hash I think normally in the nitro codebase we express this as if genesisHash == (common.Hash{}) {
Tristan-Wilson
added a commit
that referenced
this pull request
Aug 3, 2023
rpc/handler.go: conflict with upstream's PR (ethereum/go-ethereum#26681) to add batch request and response size limits, and our own implementation of it. Removed our implementation (#198, which was moved inside batchCallBuffer.pushResponse in the v1.11.2 merge #205) in favor of upstream.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR merges in upstream's v1.11.2 into our fork's master branch. There were a number of conflicts that have been resolved, see the notes below and on individual commit messages.
Nitro is now successfully building (OffchainLabs/nitro#1517) against this PR.
There are still some tests with Nitro still need to be performed, see @tsahee's comment #205 (review)
Notes from merge
Status of tests in go-ethereum package
Some tests on master were already failing, along with some tests on the upstream v1.11.2 branch. All tests that were passing on master are still passing on this branch and I fixed a few more, so although the tests aren't all passing, I don't think we have broken anything new.
Failing only on v1.1.2 merge branch
--- FAIL: TestCallTracerNativeWithLog (0.00s) - new in v1.11.2, probably fails for same reason as TestCallTracerNative
Failing in both origin/master and v1.11.2 merge branch
--- FAIL: TestT8n (0.03s) - exact text match not expecting our L1GasUsed param, fixed on this branch
--- FAIL: TestCallTracerNative (0.00s)
--- FAIL: TestZeroValueToNotExitCall (0.00s)
Fails in both v1.11.2 branch and in upstream/v1.11.2
--- FAIL: TestConsoleWelcome (0.07s) - fails due to version string having git commit, fixed on this branch
--- FAIL: TestAttachWelcome (5.37s) - fails due to version string having git commit, fixed on this branch
--- FAIL: TestSequentialRead (0.00s)
--- FAIL: TestSequentialReadByteLimit (0.00s)
--- FAIL: TestFreezerReadonly
--- FAIL: TestRandom (0.00s)
Conflict resolution notes (change X conflicting change -> resolution)
These are my notes from resolving the conflicts in the merge commit itself. There are also other fixes, mostly fixes relating to the initial merge, applied as separate commits after the merge commit.
X changes to applyTransaction args core, cmd: fill blockNumber in logs ethereum/go-ethereum#26345 -> used latest args with resultFilter appended
CaptureArbitrumTransfer#105 X callTracer was moved around in this file upstrem