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

TransactionFrame Refactor #4307

Merged
merged 14 commits into from
Aug 1, 2024
Merged

Conversation

SirTyson
Copy link
Contributor

@SirTyson SirTyson commented May 7, 2024

Description

Resolves #2163

This PR makes all TransactionFrame variants immutable. All mutable state, such as TransactionResult and caches has been moved to the TransactionResultPayload class. Hash caches are still in TransactionFrame, but only because the contents they hash are immutable such that the cache can never be invalidated.

To maintain our current testing framework, TransactionTestFrame acts as a wrapper class, combining an immutable TransactionFrame with it's associated TransactionResultPayload object for easier testing.

Checklist

  • Reviewed the contributing document
  • Rebased on top of master (no merge commits)
  • Ran clang-format v8.0.0 (via make format or the Visual Studio extension)
  • Compiles
  • Ran all tests
  • If change impacts performance, include supporting evidence per the performance document

@SirTyson SirTyson force-pushed the tx-frame-refactor-2 branch 2 times, most recently from 8a587fe to ac003d6 Compare May 8, 2024 17:49
Copy link
Contributor

@dmkozh dmkozh left a comment

Choose a reason for hiding this comment

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

Thanks for dealing with this mess! I didn't have time to review everything, just mostly looked at the new interfaces.

src/herder/HerderImpl.cpp Outdated Show resolved Hide resolved
src/transactions/TransactionFrameBase.h Outdated Show resolved Hide resolved
src/transactions/TransactionResultPayload.h Outdated Show resolved Hide resolved
src/transactions/TransactionResultPayload.h Outdated Show resolved Hide resolved
src/transactions/TransactionResultPayload.h Outdated Show resolved Hide resolved
src/transactions/TransactionResultPayload.cpp Outdated Show resolved Hide resolved
src/transactions/TransactionResultPayload.cpp Outdated Show resolved Hide resolved
src/transactions/TransactionFrame.h Outdated Show resolved Hide resolved
@SirTyson SirTyson force-pushed the tx-frame-refactor-2 branch 2 times, most recently from 709e745 to 8fdb81e Compare May 16, 2024 23:18
src/ledger/LedgerManagerImpl.cpp Outdated Show resolved Hide resolved
src/main/CommandHandler.cpp Outdated Show resolved Hide resolved
src/transactions/TransactionResultPayload.cpp Outdated Show resolved Hide resolved
src/transactions/TransactionResultPayload.cpp Outdated Show resolved Hide resolved
src/transactions/TransactionResultPayload.h Outdated Show resolved Hide resolved
src/transactions/TransactionResultPayload.cpp Outdated Show resolved Hide resolved
src/transactions/TransactionResultPayload.cpp Outdated Show resolved Hide resolved
src/transactions/TransactionResultPayload.cpp Outdated Show resolved Hide resolved
src/transactions/TransactionResultPayload.cpp Outdated Show resolved Hide resolved
src/transactions/TransactionFrame.h Outdated Show resolved Hide resolved
src/transactions/TransactionFrame.cpp Outdated Show resolved Hide resolved
src/transactions/TransactionFrame.cpp Outdated Show resolved Hide resolved
src/transactions/TransactionFrame.cpp Outdated Show resolved Hide resolved
src/transactions/TransactionFrame.cpp Outdated Show resolved Hide resolved
src/transactions/TransactionFrame.cpp Outdated Show resolved Hide resolved
src/overlay/Peer.cpp Outdated Show resolved Hide resolved
src/herder/Herder.h Outdated Show resolved Hide resolved
src/herder/HerderImpl.cpp Outdated Show resolved Hide resolved
src/herder/TransactionQueue.cpp Outdated Show resolved Hide resolved
src/herder/TransactionQueue.cpp Show resolved Hide resolved
src/transactions/TransactionFrame.h Show resolved Hide resolved
src/transactions/TransactionResultPayload.cpp Outdated Show resolved Hide resolved
src/transactions/OperationFrame.h Show resolved Hide resolved
src/transactions/TransactionResultPayload.h Outdated Show resolved Hide resolved
src/transactions/TransactionFrame.h Outdated Show resolved Hide resolved
@dmkozh
Copy link
Contributor

dmkozh commented Jun 6, 2024

I actually think this won't be too much of a problem for parallelization: cached accounts only really matter for replay, so we won't have real-time network transactions racing with apply, because we're only dealing with past transactions here (of course later we'd need to add something defensive to the code to ensure that)

We also can keep the cache only used prior to protocol 8, right?

@marta-lokhova
Copy link
Contributor

We also can keep the cache only used prior to protocol 8, right?

I think that's what we do already? we're keeping it around to correctly apply the buggy behavior during replay, but we don't need this cache for normal in-sync operations.

@SirTyson
Copy link
Contributor Author

I've finished up another round of refactoring that hopefully should better divide the responsibilities of the TxFrames. Changes include:

  • Account cache is stored in TransactionFrame again
  • OperationsFrame is now immutable
  • TransactionFrame stores OperationFrames again
  • MutableTransactionResult (formerly TransactionResultPayload) has been simplified to only support basic get/set operations of result state
  • Soroban related state has been refactored into it's own class/interface
  • TransactionFrame and OperationFrame are responsible for all functions that modify the result state and write to the blockchain, but must take a MutableTransactionResult

There are three distinct responsibilities:

  1. Basic stateless wrapper around TransactionFrame XDR (TransactionFrame)
  2. Mutable result state of a transaction (MutableTransactionResult)
  3. Applying Transactions to the Blockchain (functions live in TransactionFrame, but requires MutableTransactionResult object).

I think TransactionFrame is still doing too much, and ideally I'd like to break the responsibility from 3. into a new class (something like TransactionFrameApplicator). For a first pass though, I think making OperationFrames immutable and be owned by TransactionFrame gives us a much clearer distinction of responsibility than the first refactor.

src/transactions/TransactionFrame.cpp Outdated Show resolved Hide resolved
src/transactions/TransactionFrame.cpp Show resolved Hide resolved
src/transactions/TransactionFrame.cpp Outdated Show resolved Hide resolved
src/transactions/OperationFrame.h Outdated Show resolved Hide resolved
src/transactions/TransactionFrame.h Outdated Show resolved Hide resolved
src/transactions/OperationFrame.cpp Outdated Show resolved Hide resolved
src/transactions/TransactionFrameBase.h Show resolved Hide resolved
src/herder/TransactionQueue.cpp Outdated Show resolved Hide resolved
src/herder/TransactionQueue.h Outdated Show resolved Hide resolved
Copy link
Contributor

@dmkozh dmkozh left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@SirTyson SirTyson force-pushed the tx-frame-refactor-2 branch 2 times, most recently from 8c25df2 to 1d0fc8d Compare July 1, 2024 17:57
Copy link
Contributor

@marta-lokhova marta-lokhova left a comment

Choose a reason for hiding this comment

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

just a few questions. I haven't finished reviewing everything yet, but it's moving in the right direction!

Also, any thoughts on the test/rollout plan for this change? Given the blast radius of a bug in any of the Transaction/OperationFrame codepaths, I think in addition to full replay, we could utilize the fuzzer (I'm not certain what shape it's in though)

src/transactions/TransactionFrame.h Show resolved Hide resolved
src/transactions/TransactionFrame.h Show resolved Hide resolved
src/ledger/LedgerManagerImpl.cpp Outdated Show resolved Hide resolved
src/ledger/LedgerManagerImpl.cpp Outdated Show resolved Hide resolved
src/transactions/OperationFrame.h Outdated Show resolved Hide resolved
src/transactions/OperationFrame.cpp Outdated Show resolved Hide resolved
src/transactions/OperationFrame.cpp Outdated Show resolved Hide resolved
@SirTyson SirTyson force-pushed the tx-frame-refactor-2 branch 2 times, most recently from ba0d9f5 to 9480709 Compare July 31, 2024 18:10
@marta-lokhova marta-lokhova added this pull request to the merge queue Aug 1, 2024
@SirTyson SirTyson closed this Aug 1, 2024
Merged via the queue into stellar:master with commit fba6821 Aug 1, 2024
14 checks passed
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.

Refactor TransactionFrame to be immutable
4 participants