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

Filter deposit contract logs before pruning #9123

Merged
merged 12 commits into from
Feb 26, 2024
Merged

Filter deposit contract logs before pruning #9123

merged 12 commits into from
Feb 26, 2024

Conversation

somnathb1
Copy link
Contributor

This is a hack to not prune logs of certain special contracts, designated through chain config property, noPruneContracts

Background
CL needs deposit contract logs to create and maintain its own state (deposit tree, in particular). It requests the data through eth_getLogs. The naive approach is to not prune any logs of any contract since the block of the deposit contract. This results in a much bloated database for Logs. A validating node must be like that.

Proposed solution
Add the deposit contract to a special list. On execute stage, skip pruning Log table. In LogIndex stage, selectively prune logs, i.e. don't prune those of any of the special contracts' logs.

Results
Gnosis pruned node went from about 600GB to 320GB, and can still be a validating node (not battle tested yet)

@somnathb1 somnathb1 marked this pull request as draft January 3, 2024 10:31
Copy link
Collaborator

@AskAlexSharov AskAlexSharov left a comment

Choose a reason for hiding this comment

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

hmmm... interesting solution... way simpler than i expected. I don't have intuition how much it will reduce the space (on mainnet) - but maybe it will be very good. I like it.

@@ -175,6 +177,9 @@ func promoteLogIndex(logPrefix string, tx kv.RwTx, start uint64, endBlock uint64
}

for _, l := range ll {
if cfg.noPruneContracts != nil && !cfg.noPruneContracts[l.Address] && l.BlockNumber < pruneBlock {
Copy link
Collaborator

Choose a reason for hiding this comment

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

for performance maybe better place l.BlockNumber < pruneBlock before map check

@@ -82,12 +82,15 @@ type Config struct {
Clique *CliqueConfig `json:"clique,omitempty"`
Aura *AuRaConfig `json:"aura,omitempty"`
Bor *BorConfig `json:"bor,omitempty"`

//For not pruning these contracts (e.g. depsoit contract logs needed by CL)
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's add here more comments. if you know more information now. like: "why CL need it" and "in which mode (archive/non-archive) CL need it" and maybe something else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I am only aware that both modes need it initially for constructing deposit data. Deeper dive into CL logic may be needed for me. And here I am just trying to preserve the logs, by default, irrespective of the specification of prune= flag.

I must run a validator test for this, although I am getting the desired results through RPC, the CL may have more expectations that I am not aware of.

There are also talks about Deposit snapshots which, then, would make this whole jutsu pointless

@elee1766 elee1766 marked this pull request as ready for review January 3, 2024 12:09
@elee1766
Copy link
Contributor

elee1766 commented Jan 3, 2024

this looks correct.

the only thing I am wary of is that this means there will be a silent failure if there is an issue with the logic that deletes more than it should.

cl does a simple eth_getLogs, so if it's missing information it will just return as if there is nothing and continue without knowing better

I don't think it's a problem, but something to think about. maybe there is a simple solution for this

@somnathb1
Copy link
Contributor Author

this looks correct.

the only thing I am wary of is that this means there will be a silent failure if there is an issue with the logic that deletes more than it should.

cl does a simple eth_getLogs, so if it's missing information it will just return as if there is nothing and continue without knowing better

I don't think it's a problem, but something to think about. maybe there is a simple solution for this

So there is no integrity check on the constructed state on CL side for deposits?

@elee1766
Copy link
Contributor

elee1766 commented Jan 3, 2024

this looks correct.

the only thing I am wary of is that this means there will be a silent failure if there is an issue with the logic that deletes more than it should.

cl does a simple eth_getLogs, so if it's missing information it will just return as if there is nothing and continue without knowing better

I don't think it's a problem, but something to think about. maybe there is a simple solution for this

So there is no integrity check on the constructed state on CL side for deposits?

not that I can see. what will happen is someone using those receipts to validate would miss the deposits and not include them in their blocks I believe.

@somnathb1 somnathb1 marked this pull request as draft January 3, 2024 12:47
@AskAlexSharov
Copy link
Collaborator

@somnathb1 eth_getLogs with only address parameter? or also with topic?

@somnathb1
Copy link
Contributor Author

@somnathb1 eth_getLogs with only address parameter? or also with topic?

Both

@somnathb1 somnathb1 marked this pull request as ready for review February 26, 2024 12:54
@somnathb1 somnathb1 changed the title [DRAFT] Filter deposit contract logs before pruning Filter deposit contract logs before pruning Feb 26, 2024
@somnathb1 somnathb1 merged commit 59ae549 into devel Feb 26, 2024
7 checks passed
@somnathb1 somnathb1 deleted the som/pruned-hack branch February 26, 2024 16:44
mriccobene pushed a commit to mriccobene/erigon that referenced this pull request Mar 13, 2024
This is a hack to not prune logs of certain special contracts,
designated through chain config property, `noPruneContracts`

**Background**
CL needs deposit contract logs to create and maintain its own state
(deposit tree, in particular). It requests the data through
`eth_getLogs`. The naive approach is to not prune any logs of any
contract since the block of the deposit contract. This results in a much
bloated database for Logs. A validating node must be like that.

**Proposed solution**
Add the deposit contract to a special list. On execute stage, skip
pruning Log table. In LogIndex stage, selectively prune logs, i.e. don't
prune those of any of the special contracts' logs.

**Results**
Gnosis pruned node went from about 600GB to 320GB, and can still be a
validating node (not battle tested yet)
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