Remove mutable result from TransactionFrame into test-only classes #3391

jayz22 · 2022-04-04T18:07:19Z

Description

Resolves "TransactionFrame" part of #2163
Refactor for TransactionFrame class-family (including TransactionFrame, TransactionFrameBase, FeeBumpTransactionFrame, Fuzzer*TransactionFrame, TxSim*TransactionFrame).
Make the class immutable by removing the member TransactionResult, instead pass them externally.
Separate test code from production code by creating TestTransactionFrame and TestFeeBumpTransactionFrame that contains the old (mutable) behavior. This is to save from changing boilerplate code in tests everywhere that interacts with TransactionFrameBase (and its derived classes).

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

src/herder/Herder.h

src/herder/TransactionQueue.cpp

dmkozh · 2022-04-04T19:15:38Z

src/test/FuzzerImpl.cpp

@@ -984,7  985,8 @@ applyFuzzOperations(LedgerTxn& ltx, PublicKey const& sourceAccount,
 {
    auto txFramePtr = createFuzzTransactionFrame(ltx, sourceAccount, begin, end,
                                                 app.getNetworkID());
-    txFramePtr->attemptApplication(app, ltx);
+    TransactionResult txRes;


Since the result is not used anywhere, would it make sense to just allocate txRes in attemptApplication?

attemptApplication is also being used in applySetupOperations where txRes is used.

dmkozh · 2022-04-04T19:18:33Z

src/transactions/TransactionFrameBase.h


    virtual StellarMessage toStellarMessage() const = 0;
+
+#ifdef BUILD_TESTS


I haven't examined each usage of this, but it seems like you create test tx frames using the special method (makeTestTransactionFromWire). Would it be possible then to not have these methods in the public interface and only declare/implement them in the respective test-only classes?

That would be difficult, as many of the existing tests are calling those public methods via the base class pointer. To remove those test-only functions from the base class to the "Test" classes would require updating all (hundreds of) callers in the test code to pass in the additional parameters.

This is not entirely accurate.

See my other comment on TransactionTestFrame: I think that we should just use TransactionTestFrame in all test code instead of returning TransactionFrameBase or TransactionFrame. The change should be fairly transparent as we usually do things like
auto b1 = root.create("b1", paymentAmount); (that would return a TransactionTestFrame instead).

That way we can also move the other "mutators" from TransactionFrame, like addSignature that should only be used in test/simulation code.

src/transactions/TransactionFrame.cpp

dmkozh · 2022-04-04T19:24:42Z

src/transactions/TransactionFrame.cpp

@@ -735,25  747,27 @@ TransactionFrame::insertKeysForTxApply(UnorderedSet<LedgerKey>& keys) const
 }

 void
-TransactionFrame::markResultFailed()
+TransactionFrame::markResultFailed(TransactionResult& txResult)


Does this still need to be a member method of TransactionFrame? Seems like could be just a free internal function.

This is also being called in FuzzTransactionFrame::attemptApplication.

That's not a big issue, but I think in general it's better for classes to have less responsibilities when possible. Here it seems like the method no longer has anything to do with the TransactionFrame and could be either a public free function (given that it's reused), a static function or even removed altogether, given that it's one line and for some reason it's the only 'special' case for setting the result code (others seem to be just set in-place). In any case, that's up to your judgement.

I do see your point.

I am inclined to remove the method, since it is just one line of setting the result code, and we are already directly setting it for other kinds of code (other than txFAILED). The only thing I'm concerned is the comments in this function, which points out to some perf implication that caller might need to know. So I'm gonna keep this function for now and maybe think of a better place to put it later.

dmkozh · 2022-04-04T19:28:42Z

src/transactions/TransactionFrame.cpp

@@ -300,28  300,28 @@ TransactionFrame::makeOperation(Operation const& op, OperationResult& res,

 void
 TransactionFrame::resetResults(LedgerHeader const& header, int64_t baseFee,


Seems like this method still mutates TransactionFrame (it has mOperations.clear() call). Not sure if that's in the scope of this PR, just wanted to make sure this doesn't escape our attention.

Yes I also noticed that, I agree this is one place that needs attention.

this is because the change in this PR is incomplete: if you look at OperationFrame, it has a mResult that gets updated in a bunch of places.
Like when "apply" is called -> it ends up calling OperationFrame::doApply that mutates the result with things like innerResult().code(CLAIM_CLAIMABLE_BALANCE_DOES_NOT_EXIST);
I think it's just that doApply is missing a parameter so that it uses the result passed to TransactionFrame::apply (same thing in other places like doCheckValid

After you remove mResult from OperationFrame mOperations` should be set once and be immutable

the reason this is incomplete and unsafe is that OperationFrame::mResult ends up caching a reference to the Result that you use, this in turn can have catastrophic implications in production code if the the local variable goes out of scope before the TransactionFrame gets deleted.

src/transactions/test/TxEnvelopeTests.cpp

sisuresh

Nice work. This PR removes mResult from TransactionFrame without requiring many test changes, but it does mix in more #ifdef BUILD_TESTS code into non-test code, which isn't ideal.

I think Nico had an idea to use a wrapper class in tests (instead of using inheritance) where we replace TransactionFramePtr with a class that holds the TransactionFramePtr and txResult. The wrapper could imitate the TransactionFrame interface to keep test changes to a minimum, and the txResult member can be passed in wherever you need This would allow you to remove the new #ifdef BUILD_TESTS blocks. I'm not sure yet if there are issues with this approach, but it's something we should discuss.

src/ledger/LedgerManagerImpl.cpp

sisuresh · 2022-04-04T19:59:14Z

src/ledger/LedgerManagerImpl.cpp

        {
            LedgerTxn ltxTx(ltx);
-            tx->processFeeSeqNum(ltxTx, baseFee);
+            txs[i]->processFeeSeqNum(ltxTx, baseFee, txResults[i]);


This method makes the assumption that txResults is the same size as txs, but doesn't check that. I would use txResults.at(i) instead for bounds checking.

Thanks, this is a good catch. Also, I realize there already exists index that does what i does so I'm using that instead.

src/ledger/LedgerManagerImpl.cpp

sisuresh · 2022-04-04T20:03:54Z

src/ledger/LedgerManagerImpl.cpp

    {
        ZoneNamedN(txZone, "applyTransaction", true);
        auto txTime = mTransactionApply.TimeScope();
        TransactionMeta tm(2);
+        auto& tx = txs[i];
+        auto& txRes = txResults[i];


Same comment as above about txResults.at(i).

Fixed.
Although I realize this code is not in a try...catch block like processFeesSeqNums is and if out-of-bound exception did occur we are not handling it. I wonder if we should catch and throw here.

src/herder/TransactionQueue.cpp

sisuresh · 2022-04-05T20:16:50Z

src/transactions/TransactionFrameBase.cpp

@@ -24,4  28,24 @@ TransactionFrameBase::makeTransactionFromWire(Hash const& networkID,
        abort();
    }
 }
+#ifdef BUILD_TESTS
+TransactionFrameBasePtr
+TransactionFrameBase::makeTestTransactionFromWire(


Why is this in TransactionFrameBase? Can't you put this in a test file? Maybe TxTests?

src/herder/Herder.h

MonsieurNicolas

Good start, but the changes are only partial. I put a few recommendations in the comments that should allow to get this one over the line

MonsieurNicolas · 2022-04-21T23:17:41Z

src/test/TestTransactionFrame.h

+namespace stellar
+{
+
+class TestTransactionFrame : public TransactionFrame


This class can be reworked to avoid more copy/paste down the line: you can just add a member std::shared_ptr<TransactionFrameBase> mTx and derive from TransactionFrameBase -> you can implement a proxy pattern and update mResult as needed.

update: I am not even sure you should derive from TransactionFrameBase.

test code needs the version of the methods that don't have a "result"

the whole syncResult thing is misleading and does not catch all bugs if people are mixing using a local variable and using this class' member, so there is no real point in trying to keep those things in sync

for the few places where you need a TransactionFrameBase , you can just get a const reference to mTx.

MonsieurNicolas · 2022-04-21T23:42:45Z

src/transactions/TransactionFrame.cpp

@@ -300,28  300,28 @@ TransactionFrame::makeOperation(Operation const& op, OperationResult& res,

 void
 TransactionFrame::resetResults(LedgerHeader const& header, int64_t baseFee,


this is because the change in this PR is incomplete: if you look at OperationFrame, it has a mResult that gets updated in a bunch of places.
Like when "apply" is called -> it ends up calling OperationFrame::doApply that mutates the result with things like innerResult().code(CLAIM_CLAIMABLE_BALANCE_DOES_NOT_EXIST);
I think it's just that doApply is missing a parameter so that it uses the result passed to TransactionFrame::apply (same thing in other places like doCheckValid

MonsieurNicolas · 2022-04-21T23:46:29Z

src/transactions/TransactionFrame.cpp

@@ -300,28  300,28 @@ TransactionFrame::makeOperation(Operation const& op, OperationResult& res,

 void
 TransactionFrame::resetResults(LedgerHeader const& header, int64_t baseFee,


After you remove mResult from OperationFrame mOperations` should be set once and be immutable

MonsieurNicolas · 2022-04-21T23:49:51Z

src/transactions/TransactionFrameBase.h


    virtual StellarMessage toStellarMessage() const = 0;
+
+#ifdef BUILD_TESTS


This is not entirely accurate.

See my other comment on TransactionTestFrame: I think that we should just use TransactionTestFrame in all test code instead of returning TransactionFrameBase or TransactionFrame. The change should be fairly transparent as we usually do things like
auto b1 = root.create("b1", paymentAmount); (that would return a TransactionTestFrame instead).

That way we can also move the other "mutators" from TransactionFrame, like addSignature that should only be used in test/simulation code.

MonsieurNicolas

I added a few clarifying comments

MonsieurNicolas · 2022-04-22T15:57:38Z

src/test/TestTransactionFrame.cpp

+{
+
+void
+TestTransactionFrame::resetResults(LedgerHeader const& header, int64_t baseFee,


I'm adding a few more comments

MonsieurNicolas · 2022-04-22T16:02:02Z

src/test/TestTransactionFrame.h

+namespace stellar
+{
+
+class TestTransactionFrame : public TransactionFrame


update: I am not even sure you should derive from TransactionFrameBase.

test code needs the version of the methods that don't have a "result"

the whole syncResult thing is misleading and does not catch all bugs if people are mixing using a local variable and using this class' member, so there is no real point in trying to keep those things in sync

for the few places where you need a TransactionFrameBase , you can just get a const reference to mTx.

MonsieurNicolas · 2022-04-22T16:03:15Z

src/transactions/FeeBumpTransactionFrame.h

    StellarMessage toStellarMessage() const override;

    static TransactionEnvelope
    convertInnerTxToV1(TransactionEnvelope const& envelope);
+
+#ifdef BUILD_TESTS


note: you don't need any of this we directly use TestTransactionFrame in test code

MonsieurNicolas · 2022-04-22T16:05:25Z

src/transactions/TransactionFrame.cpp

@@ -300,28  300,28 @@ TransactionFrame::makeOperation(Operation const& op, OperationResult& res,

 void
 TransactionFrame::resetResults(LedgerHeader const& header, int64_t baseFee,


the reason this is incomplete and unsafe is that OperationFrame::mResult ends up caching a reference to the Result that you use, this in turn can have catastrophic implications in production code if the the local variable goes out of scope before the TransactionFrame gets deleted.

MonsieurNicolas · 2022-04-22T16:07:42Z

src/transactions/TransactionFrameBase.h

@@ -27,11  27,12 @@ class TransactionFrameBase
                            TransactionEnvelope const& env);

    virtual bool apply(Application& app, AbstractLedgerTxn& ltx,
-                       TransactionMeta& meta) = 0;
+                       TransactionMeta& meta, TransactionResult& txResult) = 0;


all methods exposed here should now be const (this would allow you to easily find remaining bugs related to mutating the object/caching)

If there are any non const methods left in derived classes, it probably means they belong to TestTransactionFrame (or there is something we missed outside of "result", in which case let me know!)

MonsieurNicolas · 2023-03-24T16:34:18Z

closing: stale and unlikely to land anytime soon

dmkozh requested changes Apr 4, 2022

View reviewed changes

jayz22 force-pushed the clean-up-tx-frame branch from e0e2539 to 7ee03bc Compare April 4, 2022 21:41

sisuresh reviewed Apr 5, 2022

View reviewed changes

jayz22 added 2 commits April 20, 2022 22:39

Remove TransactionResult from TransactionFrame classes

b9b0e44

Move mutable functionalities to test-only classes for TransactionFrame

2f606ad

jayz22 force-pushed the clean-up-tx-frame branch from f6b537b to 2f606ad Compare April 21, 2022 18:18

MonsieurNicolas requested changes Apr 21, 2022

View reviewed changes

MonsieurNicolas requested changes Apr 22, 2022

View reviewed changes

MonsieurNicolas closed this Mar 24, 2023

marta-lokhova mentioned this pull request Mar 5, 2024

Imlement overlay processing and ledger close in the background #4128

Open

10 tasks


		virtual StellarMessage toStellarMessage() const = 0;

		#ifdef BUILD_TESTS

		@@ -300,28 300,28 @@ TransactionFrame::makeOperation(Operation const& op, OperationResult& res,

		void
		TransactionFrame::resetResults(LedgerHeader const& header, int64_t baseFee,

Remove mutable result from TransactionFrame into test-only classes #3391

Remove mutable result from TransactionFrame into test-only classes #3391

Conversation

jayz22 commented Apr 4, 2022 • edited Loading

Description

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sisuresh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MonsieurNicolas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MonsieurNicolas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MonsieurNicolas commented Mar 24, 2023

jayz22 commented Apr 4, 2022 •

edited

Loading