Feature/geth like system tx #7252

LukaszRozmej · 2024-07-09T11:06:45Z

Changes

Decouples system transaction from normal transaction processing
Makes system transaction support Aura and non-Aura versions
Moves BeaconBlockRootHandler to use system transactions

Types of changes

What types of changes does your code introduce?

Bugfix (a non-breaking change that fixes an issue)
New feature (a non-breaking change that adds functionality)
Breaking change (a change that causes existing functionality not to work as expected)
Optimization
Refactoring
Documentation update
Build-related changes
Other: Description

Testing

Requires testing

Yes
No

If yes, did you write tests?

Yes
No

Notes on testing

Sync some Aura chains in archive.

Documentation

Requires documentation update

Yes
No

Requires explanation in Release Notes

Yes
No

src/Nethermind/Nethermind.Blockchain/BeaconBlockRoot/BeaconBlockRootHandler.cs

src/Nethermind/Nethermind.Evm/TransactionProcessing/TransactionProcessor.cs

MarekM25 · 2024-08-05T11:46:59Z

src/Nethermind/Nethermind.Evm/TransactionProcessing/SystemTransactionProcessor.cs

+ /// Hacky flag to execution options, to pass information how original validate should behave.
+ /// Needed to decide if we need to subtract transaction value.
+ /// </summary>
+ private const int OriginalValidate = 2 << 30;


Is it needed? I don't understand the idea

in what conditions value should be subtract in system calls?

estimateGas

src/Nethermind/Nethermind.Evm/TransactionProcessing/TransactionProcessor.cs

MarekM25 · 2024-08-05T12:16:34Z

src/Nethermind/Nethermind.Evm/TransactionProcessing/TransactionProcessor.cs

- // If sender is SystemUser subtracting value will cause InsufficientBalanceException
- if (validate || !tx.IsSystem())
- WorldState.SubtractFromBalance(tx.SenderAddress, tx.Value, spec);
+ PayValue(tx, spec, opts);


Can we check NoValidation options in PayValue in SystemTransactionProcessor instead of OriginalValidate?

No, as we force NoValidation

src/Nethermind/Nethermind.Evm/TransactionProcessing/SystemTransactionProcessor.cs

LukaszRozmej · 2024-08-15T10:14:52Z

src/Nethermind/Nethermind.Evm/TransactionProcessing/TransactionProcessor.cs

- // Fixes eth_estimateGas.
- // If sender is SystemUser subtracting value will cause InsufficientBalanceException
- if (validate || !tx.IsSystem())
- WorldState.SubtractFromBalance(tx.SenderAddress, tx.Value, spec);


OriginalValidate was introduced due to this condition. For every other condition validation off is equivalent with transaction being system transaction, but for this one condition we need to decouple this logic, and even if it is system transaction validation can be on/off independently, this is used in eth_estimateGas

Ok, thanks. I understand the logic. On the other hand, this logic assumes that there are sysCalls that pay values. Based on my quick check, that’s not the case. In sysCalls, we set Value = 0. This means we could simplify the original check and remove that hacky edge case flag.

So original logic could be:
if (validate && !tx.IsSystem())
then we don't need the OriginalValidate at all

src/Nethermind/Nethermind.Evm/TransactionProcessing/TransactionProcessor.cs

LukaszRozmej · 2024-08-15T10:18:41Z

src/Nethermind/Nethermind.Evm/TransactionProcessing/SystemTransactionProcessor.cs

+ /// Hacky flag to execution options, to pass information how original validate should behave.
+ /// Needed to decide if we need to subtract transaction value.
+ /// </summary>
+ private const int OriginalValidate = 2 << 30;


estimateGas

src/Nethermind/Nethermind.Evm/TransactionProcessing/TransactionProcessor.cs

LukaszRozmej · 2024-08-15T10:20:42Z

src/Nethermind/Nethermind.Evm/TransactionProcessing/TransactionProcessor.cs

- // If sender is SystemUser subtracting value will cause InsufficientBalanceException
- if (validate || !tx.IsSystem())
- WorldState.SubtractFromBalance(tx.SenderAddress, tx.Value, spec);
+ PayValue(tx, spec, opts);


No, as we force NoValidation

src/Nethermind/Nethermind.Evm/TransactionProcessing/SystemTransactionProcessor.cs

MarekM25 · 2024-08-15T11:23:19Z

@kamilchodola this PR will need a special attention from QA team (pls don't start now, wait for green-light). We will have to test AuRa sys calls.

Recommended things:

resync our gnosis archive
sync Energyweb/volta archive/non-archive
sync Chiado
perhaps, update validators (but could be done with normal cycle)

If we have any working Posdao tests than we could use it, but I guess it's not possible

benaadams · 2024-08-21T11:23:44Z

Aura failure looks legit (after a number of reruns)

Failed priority_should_return_correctly [77 ms]
Error Message:
Expected root to be a collection with 3 item(s), but {[email protected], [email protected]}
contains 1 item(s) less than
{[email protected], [email protected], [email protected]}.

tanishqjasoria

I still do not understand the use of OriginalValidate, is it supposed to be for a case in future where we transfer value in SystemTransaction? because AFAIK we dont do that right now?

src/Nethermind/Nethermind.Consensus/Processing/TransactionProcessorAdapterExtensions.cs

src/Nethermind/Nethermind.Blockchain/BeaconBlockRoot/BeaconBlockRootHandler.cs

src/Nethermind/Nethermind.Specs/SystemTransactionReleaseSpec.cs

src/Nethermind/Nethermind.Blockchain/BeaconBlockRoot/BeaconBlockRootHandler.cs

…plementations

…ec.IsEip158Enabled

Co-authored-by: MarekM25 <[email protected]> Co-authored-by: Ahmad Bitar <[email protected]>

LukaszRozmej requested review from kamilchodola, deffrian, MarekM25, rjnrohit, Demuirgos and benaadams July 9, 2024 11:06

LukaszRozmej mentioned this pull request Jul 9, 2024

Add System Transactions to mainnet #6206

Closed

16 tasks

MarekM25 reviewed Aug 5, 2024

View reviewed changes

LukaszRozmej commented Aug 15, 2024

View reviewed changes

LukaszRozmej mentioned this pull request Aug 21, 2024

fix OverridableReleaseSpec EIP158 and adjust TxPriorityContractTests #7351

Closed

12 tasks

LukaszRozmej changed the base branch from master to fix/overridable-release-spe-eip158 August 21, 2024 13:02

LukaszRozmej marked this pull request as ready for review August 21, 2024 13:02

LukaszRozmej changed the base branch from fix/overridable-release-spe-eip158 to master August 22, 2024 00:31

MarekM25 self-requested a review August 22, 2024 09:07

MarekM25 approved these changes Aug 22, 2024

View reviewed changes

MarekM25 requested review from flcl42 and tanishqjasoria August 22, 2024 10:08

tanishqjasoria reviewed Aug 22, 2024

View reviewed changes

src/Nethermind/Nethermind.Consensus/Processing/TransactionProcessorAdapterExtensions.cs Show resolved Hide resolved

benaadams approved these changes Aug 22, 2024

View reviewed changes

src/Nethermind/Nethermind.Blockchain/BeaconBlockRoot/BeaconBlockRootHandler.cs Show resolved Hide resolved

flcl42 reviewed Aug 23, 2024

View reviewed changes

src/Nethermind/Nethermind.Specs/SystemTransactionReleaseSpec.cs Outdated Show resolved Hide resolved

flcl42 reviewed Aug 23, 2024

View reviewed changes

src/Nethermind/Nethermind.Blockchain/BeaconBlockRoot/BeaconBlockRootHandler.cs Outdated Show resolved Hide resolved

LukaszRozmej requested review from flcl42 and tanishqjasoria August 29, 2024 14:13

flcl42 approved these changes Aug 29, 2024

View reviewed changes

LukaszRozmej added 4 commits September 6, 2024 05:59

Refactor

1a5384b

handle checking if Aura

b57902d

RecoverSenderIfNeeded special case

ec93cac

override Refund

1d99700

LukaszRozmej and others added 26 commits September 6, 2024 06:00

simplify passing novalidation flag

4fffbdb

constructor

5b91129

fix benchmarks

c2669f4

fix charset

a2509ab

fix substracting tx value for system transactions

f800c8d

simplify

703a08e

test

db1aced

fix proofs for system address

50f1900

whitespace

27da03f

fix

be51981

fixes

7757419

Remove obsolete subs

a8c6f4c

fix

7a67ea2

rather throw error

8985258

test fixes

e656a29

small changes in 4788 address

6ac4a2d

fix some tests

80ded31

Remove LogManager from BeaconBlockRootHandler

f092ff0

fix Benchmarks.sln

b476760

Chiado fix

57cc5f7

fix OverridableReleaseSpec EIP158 and adjust TxPriorityContractTests

33ab945

fix OverridableReleaseSpec EIP158 and adjust TxPriorityContractTests

2c6f92d

Apply suggested changes in review to be more in line with EIP-4788 im…

3ffaa70

…plementations

make SystemTransactionReleaseSpec.IsEip158Enabled also check inner sp…

df86409

…ec.IsEip158Enabled

cleanup

0b18f51

fix rebase

cddd937

smartprogrammer93 force-pushed the feature/geth-like-system-tx branch from e102b0b to cddd937 Compare September 6, 2024 03:22

LukaszRozmej merged commit 268625b into master Sep 9, 2024
66 checks passed

LukaszRozmej deleted the feature/geth-like-system-tx branch September 9, 2024 09:53

rjnrohit pushed a commit that referenced this pull request Sep 12, 2024

Feature/geth like system tx (#7252)

bec7a5d

Co-authored-by: MarekM25 <[email protected]> Co-authored-by: Ahmad Bitar <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/geth like system tx #7252

Feature/geth like system tx #7252

LukaszRozmej commented Jul 9, 2024

MarekM25 Aug 5, 2024

MarekM25 Aug 5, 2024

LukaszRozmej Aug 15, 2024

MarekM25 Aug 5, 2024

LukaszRozmej Aug 15, 2024

LukaszRozmej Aug 15, 2024

MarekM25 Aug 15, 2024 •

edited

Loading

LukaszRozmej Aug 15, 2024

LukaszRozmej Aug 15, 2024

MarekM25 commented Aug 15, 2024

benaadams commented Aug 21, 2024

tanishqjasoria left a comment

Feature/geth like system tx #7252

Feature/geth like system tx #7252

Conversation

LukaszRozmej commented Jul 9, 2024

Changes

Types of changes

What types of changes does your code introduce?

Testing

Requires testing

If yes, did you write tests?

Notes on testing

Documentation

Requires documentation update

Requires explanation in Release Notes

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

MarekM25 Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarekM25 commented Aug 15, 2024

benaadams commented Aug 21, 2024

tanishqjasoria left a comment

Choose a reason for hiding this comment

MarekM25 Aug 15, 2024 •

edited

Loading