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

Turn NoPruneContracts into DepositContract #10062

Merged
merged 7 commits into from
May 1, 2024
Merged

Turn NoPruneContracts into DepositContract #10062

merged 7 commits into from
May 1, 2024

Conversation

yperbasis
Copy link
Member

Since EIP-6110 requires deposit contract address in the config, and the NoPruneContracts logic was used only for deposit contracts, it makes sense to turn NoPruneContracts into DepositContract.

@somnathb1
Copy link
Contributor

somnathb1 commented Apr 25, 2024

  • Deposit contract is only relevant to Ethereum and Ethereum-like chains, others may use more than one such contract and/or different naming.
  • Even though it isn't a directly requested feature, there may be other contracts important to the node runner and/or L2's.
  • The plan is to be able to add other contracts to this list in future. This PR is restricting that extensibility.

Further, the noPruneContracts list is for just that - not pruning logs of those contracts. Putting deposit contract against it is saying you can't have a deposit contract in the config without it not being pruned. For, let's assume that the CL no longer needs historical deposit contract logs in CHAIN1, that would mean removing this functionality altogether, or adding another flag for CHAIN2, CHAIN3... that says "dontPruneDepositLogs" or something - circling all the way back.

@yperbasis
Copy link
Member Author

  • Deposit contract is only relevant to Ethereum and Ethereum-like chains, others may use more than one such contract and/or different naming.
  • Even though it isn't a directly requested feature, there may be other contracts important to the node runner and/or L2's.
  • The plan is to be able to add other contracts to this list in future. This PR is restricting that extensibility.

Further, the noPruneContracts list is for just that - not pruning logs of those contracts. Putting deposit contract against it is saying you can't have a deposit contract in the config without it not being pruned. For, let's assume that the CL no longer needs historical deposit contract logs in CHAIN1, that would mean removing this functionality altogether, or adding another flag for CHAIN2, CHAIN3... that says "dontPruneDepositLogs" or something - circling all the way back.

Yes, I understand, but my preference is for simplicity over generality, especially when we don't have a strong ask for such extended functionality.

@yperbasis
Copy link
Member Author

But OK, let me redo it slightly and leave the logic generic.

@yperbasis yperbasis marked this pull request as draft April 25, 2024 11:59
@yperbasis yperbasis changed the title Turn NoPruneContracts into DepositContract Turn NoPruneContracts into DepositContract in chain config Apr 25, 2024
@yperbasis yperbasis marked this pull request as ready for review April 25, 2024 12:21
@somnathb1
Copy link
Contributor

somnathb1 commented Apr 25, 2024

Granted that for now, deposit contract should be added to the noPruneContracts (in Ethereum-like chains) - this shouldn't be hard-coded in the flow, when we didn't need to.
Now, it's overall neither simple nor is the config of Deposit contract separate from pruning logic.

@yperbasis
Copy link
Member Author

yperbasis commented Apr 26, 2024

Granted that for now, deposit contract should be added to the noPruneContracts (in Ethereum-like chains) - this shouldn't be hard-coded in the flow, when we didn't need to. Now, it's overall neither simple nor is the config of Deposit contract separate from pruning logic.

OK, I'll revert to the simpler original version then. What I want to avoid is having duplicate NoPruneContracts & DepositContract in chain configs. And we de need DepositContract for EIP-6110. We can revisit this if we decide to extend the no prune logic to non-deposit contracts, but currently it doesn't look like a priority.

@yperbasis yperbasis changed the title Turn NoPruneContracts into DepositContract in chain config Turn NoPruneContracts into DepositContract Apr 26, 2024
@somnathb1
Copy link
Contributor

One more point: we are shifting our approach anyways with Erigon3, which will not have the logs pruning logic. 6110 will be a part of Erigon3, i presume.

@yperbasis yperbasis added the do-not-merge PR that is in a merge-able state but is waiting for something else to take place before merging label Apr 26, 2024
@yperbasis
Copy link
Member Author

As discussed, will merge after the switch to e3

@yperbasis yperbasis changed the base branch from release/2.60 to main May 1, 2024 07:25
@yperbasis yperbasis removed the do-not-merge PR that is in a merge-able state but is waiting for something else to take place before merging label May 1, 2024
@yperbasis
Copy link
Member Author

yperbasis commented May 1, 2024

I've rebased this to e3 and created a follow-up Issue #10153. @somnathb1 @racytech please approve

@yperbasis yperbasis merged commit ce24256 into main May 1, 2024
10 checks passed
@yperbasis yperbasis deleted the deposit_contract branch May 1, 2024 10:59
yperbasis added a commit that referenced this pull request Oct 1, 2024
AskAlexSharov pushed a commit that referenced this pull request Oct 21, 2024
@VBulikov VBulikov mentioned this pull request Feb 5, 2025
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.

3 participants