Merge v1.11.3 #218

Tristan-Wilson · 2023-05-11T20:51:38Z

Notes for initial merge commit and resolution

setHeadBeyondRoot call site had minor change of header.NumberU64() to head.Number.Uint64(), was missing arbitrum SnapshotRestoreMaxGas and diskRootFound return. Added it back in.
Many minor changes of header.NumberU64() to head.Number.Uint64().
ethapi.Backend changed CurrentBlock() return from *types.Block to *types.Header, updated NodeInterfaceBackendAPI
types.TxData interface adds effectiveGasPrice, placeholders currently for arb txs.
flatCallTracer is missing various arbitrum tracers.Tracer methods, added
stubs.
See comments on subsequent commits for other changes.

Notes on geth unit tests

 Fail on vanilla v1.11.3
 --- FAIL: TestSequentialRead (0.00s)
 --- FAIL: TestSequentialReadByteLimit (0.00s)
 --- FAIL: TestFreezerReadonly (0.00s)
 --- FAIL: TestRandom (0.00s)

 Fails expectedly due to our modifications to Tree.Cap function
 FAIL    github.com/ethereum/go-ethereum/core/rawdb      0.154s
 --- FAIL: TestPostCapBasicDataAccess (0.00s)

 Already was failing on our branch.
 FAIL
 FAIL    github.com/ethereum/go-ethereum/core/state/snapshot     3.550s
 --- FAIL: TestCallTracerNative (0.00s)
     --- FAIL: TestCallTracerNative/simpleOnlytop (0.00s)
 --- FAIL: TestCallTracerNativeWithLog (0.00s)
     --- FAIL: TestCallTracerNativeWithLog/simple (0.00s)
     --- FAIL: TestCallTracerNativeWithLog/calldata (0.00s)
     --- FAIL: TestCallTracerNativeWithLog/withOnlyTopCall (0.00s)
     --- FAIL: TestCallTracerNativeWithLog/txPartialFailed (0.00s)
     --- FAIL: TestCallTracerNativeWithLog/notopic (0.00s)
     --- FAIL: TestCallTracerNativeWithLog/delegatecall (0.00s)
     --- FAIL: TestCallTracerNativeWithLog/multilogs (0.00s)
     --- FAIL: TestCallTracerNativeWithLog/multiContracts (0.01s)
 --- FAIL: TestZeroValueToNotExitCall (0.00s)
 FAIL    github.com/ethereum/go-ethereum/eth/tracers/internal/tracetest  0.086s

Add usage examples

… (#26685)

ci: disable coverage reporting in appveyor and travis

This improves the speed of DHT crawling by using concurrent requests. It also removes logging of individual DNS updates.

…377) Adds support for a native call tracer with the Parity format, which outputs call frames in a flat array. This tracer accepts the following options: - `convertParityErrors: true` will convert error messages to match those of Parity - `includePrecompiles: true` will report all calls to precompiles. The default matches Parity's behavior where CALL and STATICCALLs to precompiles are excluded Incompatibilities with Parity include: - Parity removes the result object in case of failure. This behavior is maintained with the exception of reverts. Revert output usually contains useful information, i.e. Solidity revert reason. - The `gasUsed` field accounts for intrinsic gas (e.g. 21000 for simple transfers) and refunds unlike Parity - Block rewards are not reported Co-authored-by: Sina Mahmoodi <itz.s1na@gmail.com>

This fixes an issue where the withdrawal index was not calculated correctly for multiple withdrawals in a single block. Co-authored-by: Gary Rong <garyrong0905@gmail.com> Co-authored-by: Felix Lange <fjl@twurst.com>

* ethdb/pebble: fix range compaction * ethdb/pebble: add comment

* include withdrawals in ethclient responses * omit empty withdrawals array in json serialization

* core: params: schedule Shanghai on goerli * core/forkid: fix comment

This change adds a struct field EffectiveGasPrice in types.Receipt. The field is present in RPC responses, but not in the Go struct, and thus can't easily be accessed via ethclient. Co-authored-by: PulsarAI <dev@pulsar-systems.fi>

Fixes a race in TestNewPayloadOnInvalidTerminalBlock where setting the TTD raced with the miner. Solution: set the TTD on the blockchain config not the genesis config. Also fixes a race in CopyHeader which resulted in race reports all over the place.

This PR changes metrics collection to actually measure the time interval between collections, rather than assume 3 seconds. I did some ad hoc profiling, and on slower hardware (eg, my Raspberry Pi 4) I routinely saw intervals between 3.3 - 3.5 seconds, with some being as high as 4.5 seconds. This will generally cause the CPU gauge readings to be too high, and in some cases can cause impossibly large values for the CPU load metrics (eg. greater than 400 for a 4 core CPU). --------- Co-authored-by: Felix Lange <fjl@twurst.com>

This fixes a regression introduced by #26723. Fixes #26816.

…(#26799) This change fixes a flaw where, in certain scenarios, the block sealer did not accurately reset the remaining gas after failing to include an invalid transaction. Fixes #26791

…al (#26667) Checks that Transaction.MarshalJSON and newRPCTransaction JSON output can be parsed by Transaction.UnmarshalJSON --------- Co-authored-by: Martin Holst Swende <martin@swende.se>

Fixes a minor error in the testdata

…6779) Fixes #26073

This PR mitigates an issue with Ledger's on-device RLP deserialization, see LedgerHQ/app-ethereum#409 Ledger's RLP deserialization code does not validate the length of the RLP list received, and it may prematurely enter the signing flow when a APDU chunk boundary falls immediately before the EIP-155 chain_id when deserializing a transaction. Since the chain_id is uninitialized, it is 0 during this signing flow. This may cause the user to accidentally sign the transaction with chain_id = 0. That signature would be returned from the device 1 packet earlier than expected by the communication loop. The device blocks the second-to-last packet waiting for the signer flow, and then errors on the successive packet (which contains the chain_id, zeroed r, and zeroed s) Since the signature's early arrival causes successive errors during the communication process, geth does not parse the improper signature produced by the device, and therefore no improperly-signed transaction can be created. User funds are not at risk. We mitigate by selecting the highest chunk size that leaves at least 4 bytes in the final chunk.

… (#26698) This ensures the "withdrawals" field will always be present in responses to getPayloadBodiesByRangeV1 and getPayloadBodiesByHashV1. --------- Co-authored-by: Felix Lange <fjl@twurst.com>

Conflicts/build failures resolved: setHeadBeyondRoot call site had minor change of header.NumberU64() to head.Number.Uint64(), was missing arbitrum SnapshotRestoreMaxGas and diskRootFound return. Added it back in. Many minor changes of header.NumberU64() to head.Number.Uint64(). ethapi.Backend changed CurrentBlock() return from *types.Block to *types.Header, updated NodeInterfaceBackendAPI types.TxData interface adds effectiveGasPrice, placeholders currently for arb txs. flatCallTracer is missing various arbitrum tracers.Tracer methods, added stubs.

Tristan-Wilson · 2023-05-11T20:55:21Z

Upstream introduced the flock dependency ethereum/go-ethereum#26633 and currently this is causing the Nitro build to fail because it seems like various platform specific syscall definitions are not implemented for GOOS=js.

$ GOOS=js GOARCH=wasm go build -o target/tmp/replay.wasm ./cmd/replay/...
# github.com/gofrs/flock
../../go/pkg/mod/github.com/gofrs/flock@v0.8.1/flock_unix.go:28:30: undefined: syscall.LOCK_EX
../../go/pkg/mod/github.com/gofrs/flock@v0.8.1/flock_unix.go:39:30: undefined: syscall.LOCK_SH
../../go/pkg/mod/github.com/gofrs/flock@v0.8.1/flock_unix.go:57:20: undefined: syscall.Flock
../../go/pkg/mod/github.com/gofrs/flock@v0.8.1/flock_unix.go:67:20: undefined: syscall.Flock
../../go/pkg/mod/github.com/gofrs/flock@v0.8.1/flock_unix.go:97:20: undefined: syscall.Flock
../../go/pkg/mod/github.com/gofrs/flock@v0.8.1/flock_unix.go:97:50: undefined: syscall.LOCK_UN
../../go/pkg/mod/github.com/gofrs/flock@v0.8.1/flock_unix.go:119:29: undefined: syscall.LOCK_EX
../../go/pkg/mod/github.com/gofrs/flock@v0.8.1/flock_unix.go:131:29: undefined: syscall.LOCK_SH
../../go/pkg/mod/github.com/gofrs/flock@v0.8.1/flock_unix.go:151:17: undefined: syscall.Flock
../../go/pkg/mod/github.com/gofrs/flock@v0.8.1/flock_unix.go:151:52: undefined: syscall.LOCK_NB
../../go/pkg/mod/github.com/gofrs/flock@v0.8.1/flock_unix.go:151:52: too many errors

I haven't decided what to do yet. One of the options would be to roll back the introduction of flock in our fork.

Most of the changes were related to to upstream's change of CurrentBlock to return a *types.Header instead of *types.Block. The build is currently failing due to the issue mentioned here: OffchainLabs/go-ethereum#218 (comment)

Tristan-Wilson · 2023-05-11T21:18:26Z

Looking through the flock source https://github.com/gofrs/flock, there's no implementation which makes sense for GOOS=js, so I'll see about removing the dependency from our fork.

Tristan-Wilson · 2023-05-11T21:27:51Z

This fails with a similar error if I revert 09a9ccd, which uses tsdb

GOOS=js GOARCH=wasm go build -o target/tmp/replay.wasm ./cmd/replay/...
package github.com/offchainlabs/nitro/cmd/replay
        imports github.com/ethereum/go-ethereum/consensus
        imports github.com/ethereum/go-ethereum/core/rawdb
        imports github.com/prometheus/tsdb/fileutil
        imports golang.org/x/sys/unix: build constraints exclude all Go files in /home/tristan/go/pkg/mod/golang.org/x/sys@v0.6.0/unix

Tristan-Wilson · 2023-05-11T21:41:02Z

Looks like we already fixed this in #19 and I accidentally removed it during the merge, will bring it back in.

This is copying the form of #19 but using flock instead of tsdb/fileutil as it is preferred by upstream.

call.go has the exact command to use to regenerate this file //go:generate go run github.com/fjl/gencodec -type callFrame -field-override callFrameMarshaling -out gen_callframe_json.go

tsahee

mostly LGTM - I highlighted two areas for review by @PlasmaPower, and one comment in receipt.go

tsahee · 2023-06-01T00:23:26Z

core/types/arb_types.go

@@ -100,6 +100,13 @@ func (tx *ArbitrumUnsignedTx) setSignatureValues(chainID, v, r, s *big.Int) {

 }

+func (tx *ArbitrumUnsignedTx) effectiveGasPrice(dst *big.Int, baseFee *big.Int) *big.Int {


For all these specifically I'd like @PlasmaPower to review

These LGTM as they match behavior with the gasFeeCap() and gasTipCap() impls which say that these txs only specify a fee cap and have a tip cap of zero. That means they'll always just pay the basefee.

tsahee · 2023-06-01T00:27:03Z

core/rawdb/fileutil.go

@@ -19,12 +19,13 @@

 package rawdb

-import "github.com/prometheus/tsdb/fileutil"
+import "github.com/gofrs/flock"


this LGTM, but @PlasmaPower should also review

LGTM -- longer term we might want to move the FileLock interface to a common file between this and filelock_mock.go but no need to make that change now

tsahee · 2023-06-01T00:36:26Z

core/types/receipt.go

 		// block location fields
 		rs[i].BlockHash = hash
 		rs[i].BlockNumber = new(big.Int).SetUint64(number)
 		rs[i].TransactionIndex = uint(i)

 		if rs[i].Type != ArbitrumLegacyTxType {
 			// The contract address can be derived from the transaction itself
-			if txs[i].To() == nil && rs[i].ContractAddress == (common.Address{}) {
+			if txs[i].To() == nil {


We don't want to (and possibly cannot) recalculate ContractAddress if it is already filled (by decodeStoredReceiptRLP),

ah, I see this doesn't happen for legacy Tx so I think we always can recalculate it.. question is if we always should

I added the check back in, thanks for pointing this out.

I get this error from a nitro test when adding the check back in:

$ go test -run "TestContractTxDeploy" ./system_tests ... contract_tx_test.go:102: [expected address 0x0000000000000000000000000000123412341234 nonce 0 to deploy to 0x733A0385EB916548E1a2087F038f0dE75043bEF7 but got 0x0000000000000000000000000000000000000000]

This check is already inside this if statement, so adding it back into the upper if statement would change behavior by causing it to set ContractAddress to empty if it was previously set. So I think this code is good as-is.

ah yeah, I see that inner if statement was added recently. Makes sense :)

PlasmaPower

LGTM

holiman and others added 30 commits February 17, 2023 15:34

ci: disable coverage reporting in appveyor and travis

a0d63bc

params: begin v.1.11.3 release cycle

f6a7cc6

log: improve documentation (#26753)

a36c68f

Add usage examples

core/rawdb, node: use standalone flock dependency (#26633)

09a9ccd

eth: use the last announced finalized block as the sync ancient limit…

b3ae073

… (#26685)

Merge pull request #26721 from holiman/nocover

ee530c0

ci: disable coverage reporting in appveyor and travis

cmd/devp2p: faster crawling + less verbose dns updates (#26697)

c155c8e

This improves the speed of DHT crawling by using concurrent requests. It also removes logging of individual DNS updates.

core: improve withdrawal index assignment in GenerateChain (#26756)

2ea48f8

This fixes an issue where the withdrawal index was not calculated correctly for multiple withdrawals in a single block. Co-authored-by: Gary Rong <garyrong0905@gmail.com> Co-authored-by: Felix Lange <fjl@twurst.com>

ethdb/pebble: fix range compaction (#26771)

98b0ea6

* ethdb/pebble: fix range compaction * ethdb/pebble: add comment

ethdb/pebble: fix max memorytable size (#26776)

2bb622c

ethclient: include withdrawals in ethclient block responses (#26778)

e1b98f4

* include withdrawals in ethclient responses * omit empty withdrawals array in json serialization

all: change chain head markers from block to header (#26777)

cd31f2d

core/rawdb, ethdb/pebble: disable pebble on openbsd (#26801)

19f74fa

core: fix a merge fault (#26802)

0101895

README, go.mod, event, internal/version: bump min Go to 1.19 (#26803)

403cac7

travi: remove strange leftover Go version

27e5982

core, params: schedule Shanghai on goerli (#26795)

d865a5d

* core: params: schedule Shanghai on goerli * core/forkid: fix comment

eth: remove admin.peers[i].eth.head and difficulty (#26804)

4c23fe9

core/types: add EffectiveGasPrice in Receipt (#26713)

8718614

This change adds a struct field EffectiveGasPrice in types.Receipt. The field is present in RPC responses, but not in the Go struct, and thus can't easily be accessed via ethclient. Co-authored-by: PulsarAI <dev@pulsar-systems.fi>

ethclient: fix panic when requesting missing blocks (#26817)

4688d3c

This fixes a regression introduced by #26723. Fixes #26816.

core, miner: revert block gas counter in case of invalid transaction …

77e33e5

…(#26799) This change fixes a flaw where, in certain scenarios, the block sealer did not accurately reset the remaining gas after failing to include an invalid transaction. Fixes #26791

internal/ethapi: add tests for transaction types JSON marshal/unmarsh…

39be753

…al (#26667) Checks that Transaction.MarshalJSON and newRPCTransaction JSON output can be parsed by Transaction.UnmarshalJSON --------- Co-authored-by: Martin Holst Swende <martin@swende.se>

cmd/evm: correct alloc for t8n testdata (#26822)

cb1f6bd

Fixes a minor error in the testdata

eth/tracers/native: set created address to nil in case of failure (#2…

41af42e

…6779) Fixes #26073

beacon/engine: don't omit empty withdrawals in ExecutionPayloadBodies…

78429f7

… (#26698) This ensures the "withdrawals" field will always be present in responses to getPayloadBodiesByRangeV1 and getPayloadBodiesByHashV1. --------- Co-authored-by: Felix Lange <fjl@twurst.com>

build: update to go 1.20.2 (#26824)

a54d91a

Tristan-Wilson added 6 commits May 9, 2023 10:45

Post merge build fix

a9df09c

effectiveGasPrice impl for arbitrum txs

6371ba9

Merge remote-tracking branch 'origin/master' into merge-v1.11.3

67329cb

Set placeholder value for arbos version in tracer

ea42ed9

Update arbitrum pkg for CurrentBlock retrn Header

451f8fb

Tristan-Wilson added 7 commits May 11, 2023 14:56

Wrap flock dep usage with conditional for js

a7f574e

This is copying the form of #19 but using flock instead of tsdb/fileutil as it is preferred by upstream.

Fix reversed sign in nil block check

b0d399c

Merge remote-tracking branch 'origin/master' into merge-v1.11.3

05908e4

Pass ArbOSVersion to flatCallTracer

41a1d2a

Regenerate gen_callframe_json using correct cmd

f9cc921

call.go has the exact command to use to regenerate this file //go:generate go run github.com/fjl/gencodec -type callFrame -field-override callFrameMarshaling -out gen_callframe_json.go

Add EVMTransfers to flatCallTracer

c915a2b

Fix callTracer EVMTransfer GetResult

ab37f4f

Tristan-Wilson marked this pull request as ready for review May 16, 2023 19:35

Tristan-Wilson requested a review from tsahee May 16, 2023 19:38

Tristan-Wilson mentioned this pull request May 16, 2023

Update to build against geth v1.11.3 merge branch OffchainLabs/nitro#1631

Merged

Tristan-Wilson requested a review from PlasmaPower May 16, 2023 20:53

Tristan-Wilson mentioned this pull request May 19, 2023

Merge v1.11.4 #221

Merged

tsahee reviewed Jun 1, 2023

View reviewed changes

Tristan-Wilson added 3 commits June 2, 2023 13:06

Restore ContractAddress check before recalc

37a27c1

Merge remote-tracking branch 'origin/master' into merge-v1.11.3

3470205

Fix Receipt ContractAddress DeriveFields merge err

31e9c0c

PlasmaPower approved these changes Jun 6, 2023

View reviewed changes

PlasmaPower merged commit 9aced57 into master Jun 6, 2023

PlasmaPower deleted the merge-v1.11.3 branch June 6, 2023 05:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge v1.11.3 #218

Merge v1.11.3 #218

Tristan-Wilson commented May 11, 2023 •

edited

Loading

Tristan-Wilson commented May 11, 2023

Tristan-Wilson commented May 11, 2023

Tristan-Wilson commented May 11, 2023

Tristan-Wilson commented May 11, 2023

tsahee left a comment •

edited

Loading

tsahee Jun 1, 2023

PlasmaPower Jun 6, 2023

tsahee Jun 1, 2023

PlasmaPower Jun 6, 2023

tsahee Jun 1, 2023

tsahee Jun 1, 2023

Tristan-Wilson Jun 2, 2023

Tristan-Wilson Jun 5, 2023

PlasmaPower Jun 6, 2023

PlasmaPower Jun 6, 2023

PlasmaPower left a comment

		@@ -100,6 +100,13 @@ func (tx ArbitrumUnsignedTx) setSignatureValues(chainID, v, r, s big.Int) {

		}

		func (tx ArbitrumUnsignedTx) effectiveGasPrice(dst big.Int, baseFee big.Int) big.Int {

Merge v1.11.3 #218

Merge v1.11.3 #218

Conversation

Tristan-Wilson commented May 11, 2023 • edited Loading

Notes for initial merge commit and resolution

Notes on geth unit tests

Tristan-Wilson commented May 11, 2023

Tristan-Wilson commented May 11, 2023

Tristan-Wilson commented May 11, 2023

Tristan-Wilson commented May 11, 2023

tsahee left a comment • edited Loading

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

Choose a reason for hiding this comment

PlasmaPower left a comment

Choose a reason for hiding this comment

Tristan-Wilson commented May 11, 2023 •

edited

Loading

tsahee left a comment •

edited

Loading