This PR makes `filtermaps.ChainView` thread safe because it is used
concurrently both by the indexer and multiple matcher threads. Even
though it represents an immutable view of the chain, adding a mutex lock
to the `blockHash` function is necessary because it does so by extending
its list of non-canonical hashes if the underlying blockchain is
changed.
The unsafe concurrency did cause a panic once after running the unit
tests for several hours and it could also happen during live operation.
This PR makes the conditions for using a map rendering snapshot stricter
so that whenever a reorg happens, only a snapshot of a common ancestor
block can be used. The issue fixed in
https://github.com/ethereum/go-ethereum/pull/31642 originated from using
a snapshot that wasn't a common ancestor. For example in the following
reorg scenario: `A->B`, then `A->B2`, then `A->B2->C2`, then `A->B->C`
the last reorg triggered a render from snapshot `B` saved earlier. Now
this is possible under certain conditions but extra care is needed, for
example if block `B` crosses a map boundary then it should not be
allowed. With the latest fix the checks are sufficient but I realized I
would just feel safer if we disallowed this rare and risky scenario
altogether and just render from snapshot `A` after the last reorg in the
example above. The performance difference if a few milliseconds and it
occurs rarely (about once a day on Holesky, probably much more rare on
Mainnet).
Note that this PR only makes the snapshot conditions stricter and
`TestIndexerRandomRange` does check that snapshots are still used
whenever it's obviously possible (adding blocks after the current head
without a reorg) so this change can be considered safe. Also I am
running the unit tests and the fuzzer and everything seems to be fine.
This pull request improves error handling for local transaction submissions.
Specifically, if a transaction fails with a temporary error but might be
accepted later, the error will not be returned to the user; instead, the
transaction will be tracked locally for resubmission.
However, if the transaction fails with a permanent error (e.g., invalid
transaction or insufficient balance), the error will be propagated to the user.
These errors returned in the legacyPool are regarded as temporary failure:
- `ErrOutOfOrderTxFromDelegated`
- `txpool.ErrInflightTxLimitReached`
- `ErrAuthorityReserved`
- `txpool.ErrUnderpriced`
- `ErrTxPoolOverflow`
- `ErrFutureReplacePending`
Notably, InsufficientBalance is also treated as a permanent error, as
it’s highly unlikely that users will transfer funds into the sender account
after submitting the transaction. Otherwise, users may be confused—seeing
their transaction submitted but unaware that the sender lacks sufficient funds—and
continue waiting for it to be included.
---------
Co-authored-by: lightclient <lightclient@protonmail.com>
closes#31401
---------
Co-authored-by: Marius van der Wijden <m.vanderwijden@live.de>
Co-authored-by: Guillaume Ballet <3272758+gballet@users.noreply.github.com>
Co-authored-by: Felix Lange <fjl@twurst.com>
closes#31310
This has been requested a few times in the past and I think it is a nice
quality-of-life improvement for users. At a predetermined interval,
there will now be a "Fork ready" log when a future fork is scheduled,
but not yet active.
It can only possibly print after block import, which kinda avoids the
scenario where the client isn't progressing or is syncing and the user
thinks it's "ready" because it sees a ready log.
New output:
```console
INFO [03-08|21:32:57.472] Imported new potential chain segment number=7 hash=aa24ee..f09e62 blocks=1 txs=0 mgas=0.000 elapsed="874.916µs" mgasps=0.000 snapdiffs=973.00B triediffs=7.05KiB triedirty=0.00B
INFO [03-08|21:32:57.473] Ready for fork activation fork=Prague date="18 Mar 25 19:29 CET" remaining=237h57m0s timestamp=1,742,322,597
INFO [03-08|21:32:57.475] Chain head was updated number=7 hash=aa24ee..f09e62 root=19b0de..8d32f2 elapsed="129.125µs"
```
Easiest way to verify this behavior is to apply this patch and run `geth
--dev --dev.period=12`
```patch
diff --git a/params/config.go b/params/config.go
index 9c7719d901..030c4f80e7 100644
--- a/params/config.go
+++ b/params/config.go
@@ -174,7 +174,7 @@ var (
ShanghaiTime: newUint64(0),
CancunTime: newUint64(0),
TerminalTotalDifficulty: big.NewInt(0),
- PragueTime: newUint64(0),
+ PragueTime: newUint64(uint64(time.Now().Add(time.Hour * 300).Unix())),
BlobScheduleConfig: &BlobScheduleConfig{
Cancun: DefaultCancunBlobConfig,
Prague: DefaultPragueBlobConfig,
```
BroadcastTransactions needs the Sender address to route message flows
from the same Sender address consistently to the same random subset of
peers. It however spent considerable time calculating the Sender
addresses, even if the Sender address was already calculated and cached
in other parts of the code.
Since we only need the mapping, we can use any signer, and the one that
had already been used is a better choice because of cache reuse.
This is an attempt at fixing #31601. I think what happens is the startup
logic will try to get the full block body (it's `bc.loadLastState`) and
fail because genesis block has been pruned from the freezer. This will
cause it to keep repeating the reset logic, causing a deadlock.
This can happen when due to an unsuccessful sync we don't have the state
for the head (or any other state) fully, and try to redo the snap sync.
---------
Co-authored-by: Gary Rong <garyrong0905@gmail.com>
This fixes an issue where running geth with `--history.chain postmerge`
would not work on an empty database.
```
ERROR[04-16|23:11:12.913] Chain history database is pruned to unknown block tail=0
Fatal: Failed to register the Ethereum service: unexpected database tail
```
This PR fixes a bug in the map renderer that sometimes used an obsolete
block log value pointer to initialize the iterator for rendering from a
snapshot. This bug was triggered by chain reorgs and sometimes caused
indexing errors and invalid search results. A few other conditions are
also made safer that were not reported to cause issues yet but could
potentially be unsafe in some corner cases. A new unit test is also
added that reproduced the bug but passes with the new fixes.
Fixes https://github.com/ethereum/go-ethereum/issues/31593
Might also fix https://github.com/ethereum/go-ethereum/issues/31589
though this issue has not been reproduced yet, but it appears to be
related to a log index database corruption around a specific block,
similarly to the other issue.
Note that running this branch resets and regenerates the log index
database. For this purpose a `Version` field has been added to
`rawdb.FilterMapsRange` which will also make this easier in the future
if a breaking database change is needed or the existing one is
considered potentially broken due to a bug, like in this case.
Our metrics related to dial errors were off. The original error was not
wrapped, so the caller function had no chance of picking it up.
Therefore the most common error, which is "TooManyPeers", was not
correctly counted.
The metrics were originally introduced in
https://github.com/ethereum/go-ethereum/pull/27621
I was thinking of various possible solutions.
- the one proposed here wraps both the new error and the origial error.
It is not a pattern we use in other parts of the code, but works. This
is maybe the smallest possible change.
- as an alternate, I could write a proper `errProtoHandshakeError` with
it's own wrapped error
- finally, I'm not even sure we need `errProtoHandshakeError`, maybe we
could just pass up the original error.
---------
Signed-off-by: Csaba Kiraly <csaba.kiraly@gmail.com>
Co-authored-by: Felix Lange <fjl@twurst.com>
Thank you, @holiman, for being an integral part of the Go-Ethereum
and for your invaluable contributions over the years.
This will always be your home and you're welcome back anytime!
I added the history mode configuration in eth/ethconfig initially, since
it seemed like the logical place. But it turns out we need access to the
intended pruning setting at a deeper level, and it actually needs to be
integrated with the blockchain startup procedure.
With this change applied, if a node previously had its history pruned,
and is subsequently restarted **without** the `--history.chain
postmerge` flag, the `BlockChain` initialization code will now verify
the freezer tail against the known pruning point of the predefined
network and will restore pruning status. Note that this logic is quite
restrictive, we allow non-zero tail only for known networks, and only
for the specific pruning point that is defined.
As of now, Geth disconnects peers only on protocol error or timeout,
meaning once connection slots are filled, the peerset is largely fixed.
As mentioned in https://github.com/ethereum/go-ethereum/issues/31321,
Geth should occasionally disconnect peers to ensure some churn.
What/when to disconnect could depend on:
- the state of geth (e.g. sync or not)
- current number of peers
- peer level metrics
This PR adds a very slow churn using a random drop.
---------
Signed-off-by: Csaba Kiraly <csaba.kiraly@gmail.com>
Co-authored-by: Felix Lange <fjl@twurst.com>
Our previous success metrics gave success even if a peer disconnected
right after connection. These metrics only count peers that stayed
connected for at least 1 min. The 1 min limit is an arbitrary choice. We do
not use this for decision logic, only statistics.
When we instantiate a sub-logger via
`go-ethereum/internal/testlog/logger.With`, we copy the reference to the
`bufHandler` from the parent logger. However, internally,
`go-ethereum/internal/testlog/logger.With` calls `log/slog/Logger.With`
which creates a new handler instance (via
`internal/bufHandler.WithAttrs`).
This PR modifies sub-logger instantiation to use the newly-instantiated
handler, instead of copying the reference from the parent instance. The
type cast from `slog.Handler` to `*bufHandler` in
`internal/testlog/Logger.With` is safe here because a
`internal/testlog/Logger` can only be instantiated with a `*bufHandler`
as the underlying handler type.
Note, that I've also removed a pre-existing method that broke the above
assumption. However, this method is not used in our codebase.
I'm not sure if the assumption holds for forks of geth (e.g. optimism
has modified the testlogger somewhat allowing test loggers to accept
arbitrary handler types), but it seems okay to break API compatibility
given that this is in the `internal` package.
closes https://github.com/ethereum/go-ethereum/issues/31533
before this changes, this will result in numerous test failures:
```
> go test -run=Eth2AssembleBlock -c
> stress ./catalyst.test
```
The reason is that after creating/inserting the test chain, there is a
race between the txpool head reset and the promotion of txs added from
tests.
Ensuring that the txpool state is up to date with the head of the chain
before proceeding fixes these flaky tests.
This fix allows Trezor to support full 32bit chainId in geth, with the
next version of firmware.
For `chainId > 2147483630` case, Trezor returns signature bit only.
- Trezor returns only signature parity for `chainId > 2147483630` case.
- for `chainId == 2147483630` case, Trezor returns `MAX_UINT32` or `0`,
but it doesn't matter.
(`2147483630 * 2 + 35` = `4294967295`(`MAX_UINT32`))
chainId | returned signature_v | compatible issue
---------|------------------------|--------------------
0 < chainId <= 255 | chainId * 2 + 35 + v | no issue (firmware `1.6.2`
for Trezor one)
255 < chainId <= 2147483630 | chainId * 2 + 35 + v | ***fixed.***
*firmware `1.6.3`*
chainId > 2147483630 | v | *firmware `1.6.3`*
Please see also: full 32bit chainId support for Trezor
- Trezor one: https://github.com/trezor/trezor-mcu/pull/399 ***merged***
- Trezor model T: https://github.com/trezor/trezor-core/pull/311
***merged***
---------
Signed-off-by: Guillaume Ballet <3272758+gballet@users.noreply.github.com>
Co-authored-by: Guillaume Ballet <3272758+gballet@users.noreply.github.com>
Fixes#31169
The TestTransactionForgotten test was flaky due to real time
dependencies. This PR:
- Replaces real time with mock clock for deterministic timing control
- Adds precise state checks at timeout boundaries
- Verifies underpriced cache states and cleanup
- Improves test reliability by controlling transaction timestamps
- Adds checks for transaction re-enqueueing behavior
The changes ensure consistent test behavior without timing-related
flakiness.
---------
Co-authored-by: Zsolt Felfoldi <zsfelfoldi@gmail.com>
This PR proposes a change to the authorizations' validation introduced
in commit cdb66c8. These changes make the expected behavior independent
of the order of admission of authorizations, improving the
predictability of the resulting state and the usability of the system
with it.
The current implementation behavior is dependent on the transaction
submission order: This issue is related to authorities and the sender of
a transaction, and can be reproduced respecting the normal nonce rules.
The issue can be reproduced by the two following cases:
**First case**
- Given an empty pool.
- Submit transaction `{ from: B, auths [ A ] }`: is accepted.
- Submit transaction `{ from: A }`: Is accepted: it becomes the one
in-flight transaction allowed.
**Second case**
- Given an empty pool.
- Submit transaction `{ from: A }`: is accepted
- Submit transaction `{ from: B, auths [ A ] }`: is rejected since there
is already a queued/pending transaction from A.
The expected behavior is that both sequences of events would lead to the
same sets of accepted and rejected transactions.
**Proposed changes**
The queued/pending transactions issued from any authority of the
transaction being validated have to be counted, allowing one transaction
from accounts submitting an authorization.
- Notice that the expected behavior was explicitly forbidden in the case
"reject-delegation-from-pending-account", I believe that this behavior
conflicts to the definition of the limitation, and it is removed in this
PR. The expected behavior is tested in
"accept-authorization-from-sender-of-one-inflight-tx".
- Replacement tests have been separated to improve readability of the
acceptance test.
- The test "allow-more-than-one-tx-from-replaced-authority" has been
extended with one extra transaction, since the system would always have
accepted one transaction (but not two).
- The test "accept-one-inflight-tx-of-delegated-account" is extended to
clean-up state, avoiding leaking the delegation used into the other
tests. Additionally, replacement check is removed to be tested in its
own test case.
**Expected behavior**
The expected behavior of the authorizations' validation shall be as
follows:
Notice that replacement shall be allowed, and behavior shall remain
coherent with the table, according to the replaced transaction.
---------
Co-authored-by: lightclient <lightclient@protonmail.com>
Make UPnP more robust
- Once a random port was mapped, we try to stick to it even if a UPnP
refresh fails. Previously we were immediately moving back to try the
default port, leading to frequent ENR changes.
- We were deleting port mappings before refresh as a possible
workaround. This created issues in some UPnP servers. The UPnP (and PMP)
specification is explicit about the refresh requirements, and delete is
clearly not needed (see
https://github.com/ethereum/go-ethereum/pull/30265#issuecomment-2766987859).
From now on we only delete when closing.
- We were trying to add port mappings only once, and then moved on to
random ports. Now we insist a bit more, so that a simple failed request
won't lead to ENR changes.
Fixes https://github.com/ethereum/go-ethereum/issues/31418
---------
Signed-off-by: Csaba Kiraly <csaba.kiraly@gmail.com>
Co-authored-by: Felix Lange <fjl@twurst.com>
During my benchmarks on Holesky, around 10% of all CPU time was spent in
PUSH2
```
ROUTINE ======================== github.com/ethereum/go-ethereum/core/vm.newFrontierInstructionSet.makePush.func1 in github.com/ethereum/go-ethereum/core/vm/instructions.go
16.38s 20.35s (flat, cum) 10.31% of Total
740ms 740ms 976: return func(pc *uint64, interpreter *EVMInterpreter, scope *ScopeContext) ([]byte, error) {
. . 977: var (
40ms 40ms 978: codeLen = len(scope.Contract.Code)
970ms 970ms 979: start = min(codeLen, int(*pc+1))
200ms 200ms 980: end = min(codeLen, start+pushByteSize)
. . 981: )
670ms 2.39s 982: a := new(uint256.Int).SetBytes(scope.Contract.Code[start:end])
. . 983:
. . 984: // Missing bytes: pushByteSize - len(pushData)
410ms 410ms 985: if missing := pushByteSize - (end - start); missing > 0 {
. . 986: a.Lsh(a, uint(8*missing))
. . 987: }
12.69s 14.94s 988: scope.Stack.push2(*a)
10ms 10ms 989: *pc += size
650ms 650ms 990: return nil, nil
. . 991: }
. . 992:}
```
Which is quite crazy. We have a handwritten encoder for PUSH1 already,
this PR adds one for PUSH2.
PUSH2 is the second most used opcode as shown here:
https://gist.github.com/shemnon/fb9b292a103abb02d98d64df6fbd35c8 since
it is used by solidity quite significantly. Its used ~20 times as much
as PUSH20 and PUSH32.
# Benchmarks
```
BenchmarkPush/makePush-14 94196547 12.27 ns/op 0 B/op 0 allocs/op
BenchmarkPush/push-14 429976924 2.829 ns/op 0 B/op 0 allocs/op
```
---------
Co-authored-by: jwasinger <j-wasinger@hotmail.com>
This pull request introduces two constraints in the blobPool:
(a) If the sender has a pending authorization or delegation, only one
in-flight
executable transaction can be cached.
(b) If the authority address in a SetCode transaction is already
reserved by
the blobPool, the transaction will be rejected.
These constraints mitigate an attack where an attacker spams the pool
with
numerous blob transactions, evicts other transactions, and then cancels
all
pending blob transactions by draining the sender’s funds if they have a
delegation.
Note, because there is no exclusive lock held between different subpools
when processing transactions, it's totally possible the SetCode
transaction
and blob transactions with conflict sender and authorities are accepted
simultaneously. I think it's acceptable as it's very hard to be
exploited.
---------
Co-authored-by: lightclient <lightclient@protonmail.com>
This PR improves error handling in the remotedb package by fixing two
issues:
1. In the `Has` method, we now properly propagate errors instead of
silently returning false. This makes the behavior more predictable and
helps clients better understand when there are connection issues.
2. In the `New` constructor, we add a nil check for the client parameter
to prevent potential panics. This follows Go best practices for
constructor functions.
These changes make the code more robust and follow Go's error handling
idioms without requiring any changes to other parts of the codebase.
Changes:
- Modified `Has` method to return errors instead of silently returning
false
- Added nil check in `New` constructor
- Fixed field name in constructor to match struct definition
Co-authored-by: lightclient <lightclient@protonmail.com>
Add GetHeaderByNumber and GetReceiptsByNumber to allow more efficient API request filling from Era files.
Here we are modifying the port mapping logic so that existing port
mappings will only be removed when they were previously created by geth.
The AddAnyPortMapping functionality has been adapted to work consistently
between the IGDv1 and IGDv2 backends.
This PR adds a new `--beacon.checkpoint.file` config flag to geth and
blsync which specifies a checkpoint import/export file. If a file with
an existing checkpoint is specified, it is used for initialization
instead of the hardcoded one (except when `--beacon.checkpoint` is also
specified simultaneously). Whenever the client encounters a new valid
finality update with a suitable finalized beacon block root at an epoch
boundary, it saves the block root in hex format to the checkpoint file.
This adds the test description text to the output, instead of keeping it
as a Go comment. Logs are visible in the hive UI where these tests run,
while Go comments are not.
This pull request introduces new sync logic for pruning mode. The downloader will now skip
insertion of block bodies and receipts before the configured history cutoff point.
Originally, in snap sync, the header chain and other components (bodies and receipts) were
inserted separately. However, in Proof-of-Stake, this separation is unnecessary since the
sync target is already verified by the CL.
To simplify the process, this pull request modifies `InsertReceiptChain` to insert headers
along with block bodies and receipts together. Besides, `InsertReceiptChain` doesn't have
the notion of reorg, as the common ancestor is always be found before the sync and extra
side chain is truncated at the beginning if they fall in the ancient store. The stale
canonical chain flags will always be rewritten by the new chain. Explicit reorg logic is
no longer required in `InsertReceiptChain`.
This is for the implementation of Portal Network in the Shisui client.
Their handler needs access to the node object in order to send further
calls to the requesting node. This is a breaking API change but it
should be fine, since there are basically no known users of TALKREQ
outside of Portal network.
---------
Signed-off-by: thinkAfCod <q315xia@163.com>
Co-authored-by: Felix Lange <fjl@twurst.com>
When resending the WHOAREYOU packet, a new nonce and random IV should not
be generated. The sent packet needs to match the previously-sent one exactly
in order to make the handshake retry work.
---------
Co-authored-by: Felix Lange <fjl@twurst.com>
This is an alternative to #31309
With eth/68, transaction announcement must have transaction type and
size. So in announceTransactions, we need to query the transaction from
transaction pool with its hash. This creates overhead in case of blob
transaction which needs to load data from billy and RLP decode. This
commit creates a lightweight lookup from transaction hash to transaction
size and a function GetMetadata to query transaction type and
transaction size given the transaction hash.
---------
Co-authored-by: Gary Rong <garyrong0905@gmail.com>
This PR refactors the `nodeSet` structure in the path database to use
separate maps for account and storage trie nodes, resulting in
performance improvements. The change maintains the same API while
optimizing the internal data structure.
