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

1648 initialization and finalization sections fix #1657

Draft
wants to merge 16 commits into
base: arith-dev
Choose a base branch
from

Conversation

lorenzogentile404
Copy link
Collaborator

@lorenzogentile404 lorenzogentile404 commented Dec 13, 2024

@lorenzogentile404 lorenzogentile404 linked an issue Dec 13, 2024 that may be closed by this pull request
this.addFragments(
ImcFragment.forTxInit(hub), ContextFragment.initializeExecutionContext(hub), txFragment);

boolean isRevertedTransaction = false; // TODO: how to get this?
Copy link
Collaborator

Choose a reason for hiding this comment

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

You have to defer to post transaction to finish the initialization section.

@Getter private final AccountSnapshot senderAfterPayingForTransaction;
@Getter private final AccountSnapshot senderAfterPayingForGas;
@Getter private final AccountSnapshot senderAfterPayingForValue;
@Getter private final AccountSnapshot senderAfterPayingForGasAndValue;
Copy link
Collaborator

@OlivierBBB OlivierBBB Dec 14, 2024

Choose a reason for hiding this comment

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

I would suggest, for the sheer purpose of readability, to have the following snapshots be part of the class definition

private final AccountSnapshot senderGasPayment
private final AccountSnapshot senderGasPaymentNew
private final AccountSnapshot senderValueTransfer
private final AccountSnapshot senderValueTransferNew
private final AccountSnapshot recipientValueReception
private final AccountSnapshot recipientValueReceptionNew
private AccountSnapshot senderUndoingValueTransfer
private AccountSnapshot senderUndoingValueTransferNew
private AccountSnapshot recipientUndoingValueReception
private AccountSnapshot recipientUndoingValueReceptionNew

I've started explicitly introducing all required AccountSnapshot's as it became unmanageable otherwise in other sections (CallSection or CreateSection for instance.) As we handle a couple of hundred transactions total at a time max this isn't too much of an imposition on memory etc. And it makes intent more clear.

Copy link
Collaborator Author

@lorenzogentile404 lorenzogentile404 Dec 15, 2024

Choose a reason for hiding this comment

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

Do we actually need all of them? senderGasPaymentNew and senderValueTransfer should be the same. Also, I find clearer the notation "before" and "after". Anyway, let's discuss it tomorrow.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also, senderValueTransferNew and senderUndoingValueTransfer, recipientValueReceptionNew and recipientUndoingValueReception should be the same.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note: whenever variables represent the same thing, create a deep copy of the underlying objects.

@@ -37,7 +37,9 @@

public class TxInitializationSection extends TraceSection {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This has to implements PostTransactionDefer or whatever it's called.

Copy link
Collaborator

Choose a reason for hiding this comment

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

For this it would be ideal if we could get access to the refunded gas from Besu directly. I'm not sure we have it available here, or how, but we would need the values that are available here https://github.com/hyperledger/besu/blob/98780efd15ee3223de2c0cf2d144c8aaee30d5db/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/MainnetTransactionProcessor.java#L502

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.

Initialization and finalization sections fix
2 participants