-
Notifications
You must be signed in to change notification settings - Fork 107
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 v1.11.2 #205
Merge v1.11.2 #205
Conversation
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm.. people might be expecting this from our RPC even for L1->L2 txs unfortunately
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be if gcproc > flushInterval
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - need to test pruning as well as validation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's an extra set of parens here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see it's for the common.Hash
I think normally in the nitro codebase we express this as if genesisHash == (common.Hash{}) {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved the parens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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.
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