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

silkworm: recreate tx batch and disable read-ahead in block execution #9293

Merged
merged 5 commits into from
Jan 25, 2024

Conversation

canepat
Copy link
Member

@canepat canepat commented Jan 23, 2024

No description provided.

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.

without defer rollback you will have tx leak if panic happens inside silkworm.ExecuteBlocks

@@ -468,6 +468,16 @@ Loop:
_, isMemoryMutation := txc.Tx.(*membatchwithdb.MemoryMutation)
if cfg.silkworm != nil && !isMemoryMutation {
blockNum, err = silkworm.ExecuteBlocks(cfg.silkworm, txc.Tx, cfg.chainConfig.ChainID, blockNum, to, uint64(cfg.batchSize), writeChangeSets, writeReceipts, writeCallTraces)
// Recreate tx because Silkworm has just done commit or abort on passed one
tx, tx_err := cfg.db.BeginRw(context.Background())
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. probably you need = instead of := to assign on existing tx variable (you doing it with batch).

  2. It's bad pattern: to open tx on one layer of abstraction and close/commit in another. In this case high-level code can't provide "committed all or nothing" guarantee.
    Better: if tx was created on top-level - then it must be closed/commit only at top-level, if on low-level - then close/commit on low-level.
    We already using both (see useExternalTx variable) - but not mixing them.

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. OK, I'm going to change it in this PR
  2. OK, I'll do it in a separate PR

@canepat
Copy link
Member Author

canepat commented Jan 24, 2024

without defer rollback you will have tx leak if panic happens inside silkworm.ExecuteBlocks

here you mean when txc.Tx is external, right? In the internal case, there's already defer txc.Tx.Rollback() after creation. I'll handle this in separate PR, see point 2. Anyway, silkworm.ExecuteBlocks wraps the received tx into a local object that aborts the tx in the destructor if not committed.

@canepat canepat marked this pull request as ready for review January 24, 2024 16:56
@AskAlexSharov AskAlexSharov merged commit fce6ece into devel Jan 25, 2024
13 checks passed
@AskAlexSharov AskAlexSharov deleted the silkworm_execution_recreate_tx_batch branch January 25, 2024 05:01
somnathb1 added a commit that referenced this pull request Mar 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants