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

Optimism plugin #6146

Merged
merged 113 commits into from
Oct 27, 2023
Merged

Optimism plugin #6146

merged 113 commits into from
Oct 27, 2023

Conversation

deffrian
Copy link
Contributor

@deffrian deffrian commented Sep 29, 2023

Fixes Closes Resolves #
Nethermind optimism plugin

Changes

  • Core part of this plugin is custom transaction processing.
  • New fields in payload attributes.
  • Adds new config param: DisableGcOnNewPayload. During sync disabling gc causes memory leak
  • New field in genesis spec: stateUnavailable. Op-mainnet and op-goerli don't have state for block number zero.

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

Documentation

Requires documentation update

  • Yes
  • No

Requires explanation in Release Notes

  • Yes
  • No

Copy link
Member

@LukaszRozmej LukaszRozmej left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Further TODO things - I would like them to be captured to issues and linked with this PR:

  • Refactoring of Transactions (and TxDecoder and TransactionsForRpc) to be infectible by plugins
  • Refactoring of chain spec specific parts to be injectable by plugins

src/Nethermind/Nethermind.Optimism/OptimismPlugin.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.Consensus/AlwaysPoS.cs Outdated Show resolved Hide resolved

IEnumerable<Transaction> txs = _payloadAttrsTxSource.GetTransactions(parent, attrs.GasLimit, attrs);

Block block = new(blockHeader, txs, Array.Empty<BlockHeader>(), payloadAttributes?.Withdrawals);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

null check not needed

Copy link
Contributor Author

@deffrian deffrian Oct 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compiler can't understand that and fails with warning as error

src/Nethermind/Nethermind.Optimism/OptimismPlugin.cs Outdated Show resolved Hide resolved

public Task InitNetworkProtocol() => Task.CompletedTask;

public Task InitSynchronization()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this is needed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WHy do we have to init synchronization?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we would initialize syncronization anyways in the default InitializeNetwork, at least this one would work if p2p was enabled in Optimism.

Comment on lines +52 to +59
if (!WorldState.AccountExists(tx.SenderAddress!))
{
WorldState.CreateAccount(tx.SenderAddress!, 0, 1);
}
else
{
WorldState.IncrementNonce(tx.SenderAddress!);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit of hack that Validate mutates the state

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would love to push it to the next pr. Will require refactoring

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's also capture this in an issue

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@deffrian
Copy link
Contributor Author

deffrian commented Oct 26, 2023

I created three issues.
Two refactoring: #6221 #6222
And one for next step: #6220
@LukaszRozmej

@jmederosalvarado jmederosalvarado merged commit ffcbf90 into master Oct 27, 2023
61 checks passed
@jmederosalvarado jmederosalvarado deleted the optimism-master branch October 27, 2023 08:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants