refactor(execution_deposits_exporter): export deposits up to head and… #939
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
guybrush
commented
Oct 10, 2024
•
guybrush
commented
guybrush
force-pushed
the
BEDS-585/export-deposits-to-head
branch
7 times, most recently
from
ab6ecb3 to
0575c81
Compare
invis-bitfly
requested changes
Oct 10, 2024
Comment on lines
166
to
175
| var nearestELBlock sql.NullInt64 | ||
| err = db.ReaderDb.Get(&nearestELBlock, "select exec_block_number from blocks where slot <= $1 and exec_block_number > 0 order by slot desc limit 1", res.Data.Finalized.Epoch*utils.Config.Chain.ClConfig.SlotsPerEpoch) | ||
| err = db.ReaderDb.Get(&nearestELBlock, "select exec_block_number from blocks where exec_block_number > 0 order by slot desc limit 1") |
There was a problem hiding this comment.
this is really meh. it made a bit more sense when we were following the finalized checkpoints (we had to resolve the slot to a exec block somehow) but even that wasnt that clean (you can resolve slot => exec block num using /eth/v2/beacon/blocks/{block_id} data.message.body.execution_payload.block_number )
i would rather
- listen to block events from erigon by adding a new
OnExecutionHeadlistener to the modular exporter system - resolve the slot to a exec block num using the beacon api as described above
- do this query in the actual exporting loop below (makes no difference doing it here or there, besides having to store it variable when doing the former), though I'm not a big fan of having exporters depend on other exporters because this makes locating issues hard and can cause compounding delays.
guybrush
force-pushed
the
BEDS-585/export-deposits-to-head
branch
2 times, most recently
from
e33d9ac to
cab4d83
Compare
Deploying beaconchain with
|
| Latest commit: |
cab4d83
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://3de1d4d4.beaconchain.pages.dev |
| Branch Preview URL: | https://beds-585-export-deposits-to.beaconchain.pages.dev |
guybrush
force-pushed
the
BEDS-585/export-deposits-to-head
branch
4 times, most recently
from
7af102c to
99acbd5
Compare
* export deposits up to head * handle reorgs by reexporting blocks since lastExportedFinalizedBlock * persist lastExportedFinalizedBlock in redis * refactor export-logic from multiple event-based goroutines with canceling to single loop * aggregated deposits and cached view are only updated when export is on head * avoid fetching old blocks if no deposits are on the chain * avoid duplicate work by caching exported tx-hashes BEDS-585
guybrush
force-pushed
the
BEDS-585/export-deposits-to-head
branch
from
99acbd5 to
b5099bf
Compare
invis-bitfly
requested changes
Oct 11, 2024
Comment on lines
318
to
321
| if d.exportedTxsCache.Contains(fmt.Sprintf("%x", depositLog.Raw.TxHash.Bytes())) { | ||
| log.Debugf("skipping already exported deposit-tx: block: %d, tx: %x", depositLog.Raw.BlockNumber, depositLog.Raw.TxHash.Bytes()) | ||
| continue | ||
| } |
There was a problem hiding this comment.
this is wrong and very dangerous. i meant only caching the validity of the signature, not skipping seen txns
the current solution can result in the following issue:
- there are 3 deposit txns on chain, each with one deposit:
txnA:0,txnB:1,txnC:2(:nis the merkle tree index)txnA:0,txnB:1,txnC:2gets pushed to the db
- the chain gets reorged and a new txn (
txnD) with 1 deposit get inserted aftertxnA- there are now 4 deposits on chain:
txnA:0,txnD:1,txnB:2,txnC:3 - the lru cache skips the re-processing of txnB & txnC
txnD:1gets pushed to the db
- there are now 4 deposits on chain:
- a new txn with a single deposit gets added (
txnE)txnE:4gets pushed to the db
the result db state would be
txnA:0,txnD:1,txnC:2,txnE:4
instead of
txnA:0,txnD:1,txnB:2,txnC:3,txnE:4
Comment on lines
+188
to
+203
| g.Go(func() error { | ||
| headSlot, err := d.CL.GetSlot("head") | ||
| if err != nil { | ||
| return fmt.Errorf("error getting head-slot: %w", err) | ||
| } | ||
| headBlock = headSlot.Data.Message.Body.ExecutionPayload.BlockNumber | ||
| return nil | ||
| }) | ||
| g.Go(func() error { | ||
| finSlot, err := d.CL.GetSlot("finalized") | ||
| if err != nil { | ||
| return fmt.Errorf("error getting finalized-slot: %w", err) | ||
| } | ||
| finBlock = finSlot.Data.Message.Body.ExecutionPayload.BlockNumber | ||
| return nil | ||
| }) |
There was a problem hiding this comment.
this could be done in onHead & onFinalizedCheckpoint to follow the event driven approach
but since export() is in a fixed loop i would not bother with it rn since it would yield no benefit
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.