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

[DO NOT MERGE] Experimental rebasing of a patch set #54

Draft
wants to merge 578 commits into
base: arr4n/patch-0-experiment
Choose a base branch
from

Conversation

ARR4N
Copy link
Collaborator

@ARR4N ARR4N commented Oct 10, 2024

Warning

DO NOT MERGE. This is purely demonstrative as an exploration of a rebase strategy.

How this works

Follows the strategy of rebasing patch-set branches.

#7 was the first libevm functionality so I used that to act as the first "patch". It was merged as 5429fd87c8.

  1. Create the equivalent of a patch0 branch:
$ git checkout 5429fd87c8
$ git switch -c arr4n/patch-0-experiment
$ git push
  1. Perform the rebase:
$ git checkout -b arr4n/rebased-patch-0-experiment
$ git rebase -i geth-v1.14.11

The -i was to inspect the commits being rebased and ensure that only the #7 "patch" one was included.

  1. After fixing the conflicts and adding all changes to the staging area:
$ go test ./...
$ git rebase --continue
$ git push
  1. Open this PR.

Note

The conflicting files are the exact same ones that I needed to edit to make this branch work. core/vm/{contracts,evm}.go and params/config.go had conflicts during the rebase and the others had to be fixed to conform with upstream changes.

This is because the strategy results in an ambiguous history.

The strategy recommends:

$ git checkout arr4n/patch-0-experiment
$ git merge --no-ff arr4n/rebased-patch-0-experiment

however this doesn't account for the inevitable conflicts. Note that the --no-ff is redundant because there is no fast-forward path to merging and would have to be replaced with -X theirs to function as demonstrated in the diagram.

Although I don't recommend the -X theirs alternative, I pursued it for the purpose of a complete demonstration. We see that both versions of the same commit would then exist in the history, with the 500+ geth commits between them:

$ git log --graph --oneline | grep -n "chore: squash"
3:| * 154820ccd chore: squash `arr4n/libevm` into `libevm` (#7)
591:* | 5429fd87c chore: squash `arr4n/libevm` into `libevm` (#7)

Soundness check: GitHub says we are 584 commits behind, so these numbers are reasonable.

Although they both exist, a diff between them with git diff 5429fd87c 154820ccd includes all of the intermediate geth changes so we can't easily review patch updates. Inspecting only the latest one with git show 154820ccd displays the current version of the patch set, but not how it evolved. We can see this by inspecting the modifications to RunPrecompiledContract and noting that they include the *tracing.Hooks parameter, which was one of the rebasing conflicts that had to be resolved.

Adding --unified=0 to remove noisy context:

index 104d2ba81..c56c37274 100644
--- a/core/vm/contracts.go
+++ b/core/vm/contracts.go
@@ -223 +223 @@ func ActivePrecompiles(rules params.Rules) []common.Address {
-func RunPrecompiledContract(p PrecompiledContract, input []byte, suppliedGas uint64, logger *tracing.Hooks) (ret []byte, remainingGas
 uint64, err error) {
+func (args *evmCallArgs) RunPrecompiledContract(p PrecompiledContract, input []byte, suppliedGas uint64, logger *tracing.Hooks) (ret 
[]byte, remainingGas uint64, err error) {

The merge commit will always be empty with git show because of the use of -X theirs.

Note

Although I may not have exhausted all options, I can't find a straightforward way to show how the patch set was updated.

How this was tested

n/a

SangIlMo and others added 30 commits June 4, 2024 10:59
…ateGas (ethereum#29738)

* internal/ethapi: recap higher args.Gas with block GasLimit in DoEstimateGas

* internal/ethapi: fix gas estimator capping code

* internal/ethapi: fix test

* fix goimports lint (remove space)

---------

Co-authored-by: Péter Szilágyi <[email protected]>
enode.Node was recently changed to store a cache of endpoint information. The IP address in the cache is a netip.Addr. I chose that type over net.IP because it is just better. netip.Addr is meant to be used as a value type. Copying it does not allocate, it can be compared with ==, and can be used as a map key.

This PR changes most uses of Node.IP() into Node.IPAddr(), which returns the cached value directly without allocating.
While there are still some public APIs left where net.IP is used, I have converted all code used internally by p2p/discover to the new types. So this does change some public Go API, but hopefully not APIs any external code actually uses.

There weren't supposed to be any semantic differences resulting from this refactoring, however it does introduce one: In package p2p/netutil we treated the 0.0.0.0/8 network (addresses 0.x.y.z) as LAN, but netip.Addr.IsPrivate() doesn't. The treatment of this particular IP address range is controversial, with some software supporting it and others not. IANA lists it as special-purpose and invalid as a destination for a long time, so I don't know why I put it into the LAN list. It has now been marked as special in p2p/netutil as well.
Fixes an issue where discovery responses were not recognized.
* cmd/utils, consensus/beacon, core/state: when configured via stub  flag: prefetch all reads from account/storage tries, terminate prefetcher synchronously.

* cmd, core/state: fix nil panic, fix error handling, prefetch nosnap too

* core/state: expand prefetcher metrics for reads and writes separately

* cmd/utils, eth: fix noop collect witness flag

---------

Co-authored-by: Péter Szilágyi <[email protected]>
…m#29974)

* .golangci.yml: enable check for consistent receiver name

* beacon/light/sync: fix receiver name

* core/txpool/blobpool: fix receiver name

* core/types: fix receiver name

* internal/ethapi: use consistent receiver name 'api' for handler object

* signer/core/apitypes: fix receiver name

* signer/core: use consistent receiver name 'api' for handler object

* log: fix receiver name
* fix: Optimize regular initialization

* modify var name

* variable change to private types
…rom trie Commit (ethereum#29869)

* core/state, eth/protocols, trie, triedb/pathdb:  remove unused error return from trie Commit

* move set back to account-trie-update block scoping for easier readability

* address review

* undo tests submodule change

* trie:  panic if BatchSerialize returns an error in Verkle trie Commit

* trie: verkle comment nitpicks

---------

Co-authored-by: Péter Szilágyi <[email protected]>
karalabe and others added 29 commits September 20, 2024 16:43
…thereum#30069)

This PR integrates witness-enabled block production, witness-creating
payload execution and stateless cross-validation into the `engine` API.
The purpose of the PR is to enable the following use-cases (for API
details, please see next section):

- Cross validating locally created blocks:
- Call `forkchoiceUpdatedWithWitness` instead of `forkchoiceUpdated` to
trigger witness creation too.
- Call `getPayload` as before to retrieve the new block and also the
above created witness.
- Call `executeStatelessPayload` against another client to
cross-validate the block.

- Cross validating locally processed blocks:
- Call `newPayloadWithWitness` instead of `newPayload` to trigger
witness creation too.
- Call `executeStatelessPayload` against another client to
cross-validate the block.

- Block production for stateless clients (local or MEV builders):
- Call `forkchoiceUpdatedWithWitness` instead of `forkchoiceUpdated` to
trigger witness creation too.
- Call `getPayload` as before to retrieve the new block and also the
above created witness.
- Propagate witnesses across the consensus libp2p network for stateless
Ethereum.

- Stateless validator validation:
- Call `executeStatelessPayload` with the propagated witness to
statelessly validate the block.

*Note, the various `WithWitness` methods could also *just be* an
additional boolean flag on the base methods, but this PR wanted to keep
the methods separate until a final consensus is reached on how to
integrate in production.*

---

The following `engine` API types are introduced:

```go
// StatelessPayloadStatusV1 is the result of a stateless payload execution.
type StatelessPayloadStatusV1 struct {
	Status          string      `json:"status"`
	StateRoot       common.Hash `json:"stateRoot"`
	ReceiptsRoot    common.Hash `json:"receiptsRoot"`
	ValidationError *string     `json:"validationError"`
}
```

- Add `forkchoiceUpdatedWithWitnessV1,2,3` with same params and returns
as `forkchoiceUpdatedV1,2,3`, but triggering a stateless witness
building if block production is requested.
- Extend `getPayloadV2,3` to return `executionPayloadEnvelope` with an
additional `witness` field of type `bytes` iff created via
`forkchoiceUpdatedWithWitnessV2,3`.
- Add `newPayloadWithWitnessV1,2,3,4` with same params and returns as
`newPayloadV1,2,3,4`, but triggering a stateless witness creation during
payload execution to allow cross validating it.
- Extend `payloadStatusV1` with a `witness` field of type `bytes` if
returned by `newPayloadWithWitnessV1,2,3,4`.
- Add `executeStatelessPayloadV1,2,3,4` with same base params as
`newPayloadV1,2,3,4` and one more additional param (`witness`) of type
`bytes`. The method returns `statelessPayloadStatusV1`, which mirrors
`payloadStatusV1` but replaces `latestValidHash` with `stateRoot` and
`receiptRoot`.
This is a work-around for a strange issue with travis, specifically,
`os=osx, go: 1.23.1`. When this is used, the actual go that ends up
being used is `go1.19.4 darwin/amd64 `.

Using `which go`, it told me that the `go` in the path was a softlink at
`/Users/travis/gopath/bin/go1.23.1 `. However, this was not true: using
`command -v go`, it told me that the actual `go` that was used is a
softlink at `/usr/local/bin/go`.

This change rewrites the `/usr/local/bin/go` softlink to point to the
binary at `/Users/travis/gopath/bin/go1.23.1`, so we get the right
go-version.
Make `setEtherbase` fall thorugh and handle `miner.pending.feeRecipient` after showing deprecation-warning for `miner.etherbase`-flag.
…0493)

This pull request skips the state snapshot update if the base layer is
not existent, eliminating the numerous warning logs after an unclean
shutdown.

Specifically, Geth will rewind its chain head to a historical block
after unclean shutdown and state snapshot will be remained as unchanged
waiting for recovery. During this period of time, the snapshot is unusable
and all state updates should be ignored/skipped for state snapshot update.
… global gascap is 0 (ethereum#30474)

In ethereum#27720, we introduced RPC global gas cap. A value of `0` means an unlimited gas cap. However, this was not the case for simulated calls. This PR fixes the behaviour.
This change exits with error if user provided a `--state.scheme` which is neither `hash` nor `path`
)

Extends the opcontext interface to include accessor for code being executed in current context. While it is possible to get the code via `statedb.GetCode`, that approach doesn't work for initcode.
…#30459)

This change adds more comprehensive benchmarks with a wider-variety of input sizes for g1 and g2 multi exponentiation.
…thereum#30506)

This PR fixes two tests, which had a tendency to sometimes write to the `*testing.T` `log` facility after the test function had completed, which is not allowed. This PR fixes it by using waitgroups to ensure that the handler/logwriter terminates before the test exits.

closes ethereum#30505
This update should only affect the fuzzers, as far as I know. But it
seems like it might also fix some arm/macos compilation issue in
ethereum#30494

Closes ethereum#30494 (I think)
…rollback (ethereum#30495)

Here we move the method that drops all transactions by temporarily increasing the fee
into the TxPool itself. It's better to have it there because we can set it back to the
configured value afterwards. This resolves a TODO in the simulated backend.
core/txpool/blobpool: return all reinject-addresses
…ue upon rollback" (ethereum#30521)

Reverts ethereum#30495

You are free to create a proper Clear method if that's the best way. But
one that does a proper cleanup, not some hacky call to set gas which
screws up logs, metrics and everything along the way. Also doesn't work
for legacy pool local transactions.

The current code had a hack in the simulated code, now we have a hack in
live txpooling code. No, that's not acceptable. I want the live code to
be proper, meaningful API, meaningful comments, meaningful
implementation.
…reum#30473)

Use types.Sender(signer, tx) to utilize the transaction's sender cache
and avoid repeated address recover.
This PR removes the dependencies on `lightchaindata` db as the light
protocol has been deprecated and removed from the codebase.
* feat: pseudo-generic extra payloads in `params.ChainConfig` and `params.Rules`

* feat: `params.ExtraPayloadGetter` for end-user type safety

* refactor: payloads only available through `params.ExtraPayloadGetter`

* chore: make `libevm/examples/extraparams` a `params` testable example

* doc: `libevm/pseudo` package comments and improved readability

* doc: `params.*Extra*` comments and improved readability

* doc: `params.ExtraPayloadGetter` comments and improved readability

* doc: `params/config.libevm_test.go` comments and improved readability

* refactor: simplify `params.ChainConfig.UnmarshalJSON()`

* refactor: abstract new/nil-pointer creation into `pseudo.Constructor`s

* feat: precompile override via `params.Extras` hooks

* doc: flesh out `PrecompileOverride()` in example

* doc: complete commentary and improve readability

* refactor: `ChainConfig.Hooks()` + `Rules` equivalent

* chore: rename precompiles test file in keeping with geth equivalent

* feat: stateful precompiles + allowlist hooks

The allowlist hooks are included in this commit because they allow for the same functionality as stateful precompiles in `ava-labs/coreth` and `ava-labs/subnet-evm`.

* fix: `StateTransition.canExecuteTransaction()` used `msg.From` instead of `To`

* test: `params.RulesHooks.CanCreateContract` integration

* test: `params.RulesHooks.CanExecuteTransaction` integration

* test: `vm.NewStatefulPrecompile()` integration

* refactor: simplify test of `CanCreateContract`

* refactor: abstract generation of random `Address`/`Hash` values

* doc: full documentation + readability refactoring/renaming

* fix: remove circular dependency in tests
@ARR4N ARR4N added the DO NOT MERGE This PR is not meant to be merged in its current state label Oct 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DO NOT MERGE This PR is not meant to be merged in its current state
Projects
None yet
Development

Successfully merging this pull request may close these issues.