cap-0018 - Implementation after removing low-level auth checks starting from v10 #2387

sisuresh · 2020-01-14T17:46:46Z

Description

Implements https://github.com/stellar/stellar-protocol/blob/master/core/cap-0018.md

Checklist

Reviewed the contributing document
Rebased on top of master (no merge commits)
Ran clang-format v5.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

jonjove

This is a much cleaner implementation, much easier to review and reason about. I left a few small comments, although I haven't reviewed the new AllowTrust tests yet.

src/transactions/AllowTrustOpFrame.cpp

src/invariant/InvariantUtils.cpp

src/invariant/LedgerEntryIsValid.cpp

jonjove · 2020-01-14T22:23:28Z

src/transactions/OfferExchange.cpp

@@ -50,7 +50,7 @@ canSellAtMost(LedgerTxnHeader const& header, LedgerTxnEntry const& account,
        return std::max({getAvailableBalance(header, account), int64_t(0)});
    }

-    if (trustLine && trustLine.isAuthorized())
+    if (trustLine && trustLine.isAuthorizedToMaintainLiabilities())


Do we actually need to check authorization here at all starting from version 10? Same thing for the other overload.

This is a good point. We don't need to check as the authorization is checked prior to this for v10 and up. I did this because it looked cleaner than header.current().ledgerVersion >= 10 || trustLine.isAuthorized(), but I think the intention is clearer if we only check the authorization pre v10. I'll make the change.

MonsieurNicolas

just a few minor questions. not sure if you guys discussed defensive coding practices

src/invariant/LiabilitiesMatchOffers.cpp

src/transactions/OfferExchange.cpp

src/transactions/TransactionUtils.cpp

src/util/ValidityUtils.cpp

src/transactions/TransactionUtils.cpp

jonjove

This looks good. I left a few very minor comments on one of the tests. But I also had some thoughts while reviewing the PR that aren't necessarily specific to any single line:

I'm not sure that the defensive programming approach that we took in this PR is the right one. I am of the opinion that, starting from protocol 10 (or maybe 13) we should throw if we reach addBalance, addBuyingLiabilities, etc with !isAuthorizedToMaintainLiabilities. I think this is the correct approach because we are already depending on the client code to check that it is correct between isAuthorizedToMaintainLiabilities and isAuthorized. If the client code has permitted those functions to be reached in the case mentioned above, then there is definitely a logic error. We shouldn't carry on processing the transaction if there is definitely a logic error, instead we should return txINTERNAL_ERROR.
We added defensive programming checks to getMaxAmountReceive but they are missing from the analogous function getAvailableBalance (which didn't require any changes in this PR).
The changes in the first commit were based off of the assumption that we always check authorization before reaching those functions, but that isn't true on the path through releaseLiabilities (see, for example, ManageOfferOpFrameBase.cpp:224 and note that the offer loaded there does not necessarily have the same assets as specified in the operation). Now assuming that everything works correctly, this shouldn't cause any issues because an offer in the ledger should have authorized trust lines. But we took similar precautions at OfferExchange.cpp:1112.

src/transactions/test/PathPaymentTests.cpp

src/transactions/test/OfferTests.cpp

sisuresh · 2020-04-02T02:04:32Z

@jonjove

This looks good. I left a few very minor comments on one of the tests. But I also had some thoughts while reviewing the PR that aren't necessarily specific to any single line:

I'm not sure that the defensive programming approach that we took in this PR is the right one. I am of the opinion that, starting from protocol 10 (or maybe 13) we should throw if we reach addBalance, addBuyingLiabilities, etc with !isAuthorizedToMaintainLiabilities. I think this is the correct approach because we are already depending on the client code to check that it is correct between isAuthorizedToMaintainLiabilities and isAuthorized. If the client code has permitted those functions to be reached in the case mentioned above, then there is definitely a logic error. We shouldn't carry on processing the transaction if there is definitely a logic error, instead we should return txINTERNAL_ERROR.

I added a commit that throws if we reach those methods starting in protocol 10. I like this approach as the assumptions made are clear and enforced.

We added defensive programming checks to getMaxAmountReceive but they are missing from the analogous function getAvailableBalance (which didn't require any changes in this PR).

I added an authorization check in getAvailableBalance. This isn't necessary, but I think it makes sense because there's no reason to retrieve the balance of an unauthorized asset (since nothing can be done with that asset).

The changes in the first commit were based off of the assumption that we always check authorization before reaching those functions, but that isn't true on the path through releaseLiabilities (see, for example, ManageOfferOpFrameBase.cpp:224 and note that the offer loaded there does not necessarily have the same assets as specified in the operation). Now assuming that everything works correctly, this shouldn't cause any issues because an offer in the ledger should have authorized trust lines. But we took similar precautions at OfferExchange.cpp:1112.

This is an interesting scenario. The authorization shouldn't matter since releaseLiabilities is removing offers. However, if there are unauthorized offers in the ledger, we will throw in addBuyingLiabilities and addSellingLiabilities (with my latest change). Because of the new invariants, I think the check at OfferExchange.cpp:1112 is unnecessary because releaseLiabilities is called on the existing offer at OfferExchange.cpp:1100, which will do the auth check anyways. If you agree, I will remove the check on OfferExchange.cpp:1112.

jonjove

This is an interesting scenario. The authorization shouldn't matter since releaseLiabilities is removing offers. However, if there are unauthorized offers in the ledger, we will throw in addBuyingLiabilities and addSellingLiabilities (with my latest change). Because of the new invariants, I think the check at OfferExchange.cpp:1112 is unnecessary because releaseLiabilities is called on the existing offer at OfferExchange.cpp:1100, which will do the auth check anyways. If you agree, I will remove the check on OfferExchange.cpp:1112.

Yes, I agree that the check on OfferExchange.cpp:1112 is unnecessary now. I think it would be worth adding a comment where releaseLiabilitites is used on OfferExchange.cpp:1099 indicating that this is being checked there. Have you checked that everything makes sense at other places where releaseLiabilities is used, such as AllowTrustOpFrame.cpp:135 (there are a few other places too)?

jonjove · 2020-04-02T08:47:03Z

src/transactions/TransactionUtils.cpp

@@ -429,14 +448,18 @@ getAvailableBalance(LedgerTxnHeader const& header, LedgerEntry const& le)
    }
    else if (le.data.type() == TRUSTLINE)
    {
+        if (ledgerVersionMinV10 && !isAuthorizedToMaintainLiabilities(le))


Any reason not to use checkAuthorization here? I know you don't need the return value (so a comment about that would be warranted) but it looks like it would cover the throw part correctly.

I just thought it was easier to read this way. But using checkAuthorization will work as well, and will allow us to keep the throw logic in one spot. I'll make the change.

sisuresh · 2020-04-02T16:10:40Z

This is an interesting scenario. The authorization shouldn't matter since releaseLiabilities is removing offers. However, if there are unauthorized offers in the ledger, we will throw in addBuyingLiabilities and addSellingLiabilities (with my latest change). Because of the new invariants, I think the check at OfferExchange.cpp:1112 is unnecessary because releaseLiabilities is called on the existing offer at OfferExchange.cpp:1100, which will do the auth check anyways. If you agree, I will remove the check on OfferExchange.cpp:1112.

Yes, I agree that the check on OfferExchange.cpp:1112 is unnecessary now. I think it would be worth adding a comment where releaseLiabilitites is used on OfferExchange.cpp:1099 indicating that this is being checked there. Have you checked that everything makes sense at other places where releaseLiabilities is used, such as AllowTrustOpFrame.cpp:135 (there are a few other places too)?

Yeah the other releaseLiabilities calls look good. Related to this - Do you think it makes sense to throw if unauthorized liabilities are being released? Obviously the scenario shouldn't happen, but if it was encountered for some reason, you could make the argument that the offers should still be pulled (and log an error).

MonsieurNicolas · 2020-04-04T15:59:22Z

r+ 1ca2deb

jonjove reviewed Jan 14, 2020

View reviewed changes

MonsieurNicolas requested changes Mar 10, 2020

View reviewed changes

MonsieurNicolas mentioned this pull request Mar 17, 2020

Cap18 #2356

Closed

6 tasks

MonsieurNicolas added the protocol label Mar 17, 2020

sisuresh force-pushed the removeLowLevelAuthChecks branch from 4c10221 to ead4492 Compare March 18, 2020 01:17

jonjove reviewed Mar 19, 2020

View reviewed changes

src/util/ValidityUtils.cpp Outdated Show resolved Hide resolved

src/transactions/TransactionUtils.cpp Outdated Show resolved Hide resolved

sisuresh force-pushed the removeLowLevelAuthChecks branch from ead4492 to 16d6d5a Compare March 19, 2020 20:06

sisuresh force-pushed the removeLowLevelAuthChecks branch from 16d6d5a to 902b5bc Compare March 26, 2020 22:35

MonsieurNicolas reviewed Apr 1, 2020

View reviewed changes

src/transactions/TransactionUtils.cpp Show resolved Hide resolved

sisuresh force-pushed the removeLowLevelAuthChecks branch from 902b5bc to 6ffe2ed Compare April 1, 2020 03:34

jonjove reviewed Apr 1, 2020

View reviewed changes

src/transactions/test/PathPaymentTests.cpp Outdated Show resolved Hide resolved

src/transactions/test/OfferTests.cpp Outdated Show resolved Hide resolved

src/transactions/test/OfferTests.cpp Outdated Show resolved Hide resolved

src/transactions/test/OfferTests.cpp Show resolved Hide resolved

sisuresh force-pushed the removeLowLevelAuthChecks branch 2 times, most recently from cade8ee to 89fe97f Compare April 2, 2020 01:29

jonjove reviewed Apr 2, 2020

View reviewed changes

sisuresh force-pushed the removeLowLevelAuthChecks branch from 89fe97f to 8d6f542 Compare April 2, 2020 16:02

sisuresh added 3 commits April 3, 2020 11:59

only do auth checks from v9 or less

e6f5e4c

change AllowTrustOp authorize flag to uint32

2966991

added new flag to maintain liabilities

1ca2deb

sisuresh force-pushed the removeLowLevelAuthChecks branch from 8d6f542 to 1ca2deb Compare April 3, 2020 19:08

latobarita merged commit 046dfd8 into stellar:master Apr 4, 2020

abuiles mentioned this pull request May 28, 2020

SEP0029: SDK Support [META] stellar/stellar-protocol#575

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cap-0018 - Implementation after removing low-level auth checks starting from v10 #2387

cap-0018 - Implementation after removing low-level auth checks starting from v10 #2387

sisuresh commented Jan 14, 2020 •

edited

Loading

jonjove left a comment

jonjove Jan 14, 2020

sisuresh Jan 16, 2020

MonsieurNicolas left a comment

jonjove left a comment

sisuresh commented Apr 2, 2020 •

edited

Loading

jonjove left a comment

jonjove Apr 2, 2020

sisuresh Apr 2, 2020

sisuresh commented Apr 2, 2020

MonsieurNicolas commented Apr 4, 2020

cap-0018 - Implementation after removing low-level auth checks starting from v10 #2387

cap-0018 - Implementation after removing low-level auth checks starting from v10 #2387

Conversation

sisuresh commented Jan 14, 2020 • edited Loading

Description

Checklist

jonjove left a comment

Choose a reason for hiding this comment

jonjove Jan 14, 2020

Choose a reason for hiding this comment

sisuresh Jan 16, 2020

Choose a reason for hiding this comment

MonsieurNicolas left a comment

Choose a reason for hiding this comment

jonjove left a comment

Choose a reason for hiding this comment

sisuresh commented Apr 2, 2020 • edited Loading

jonjove left a comment

Choose a reason for hiding this comment

jonjove Apr 2, 2020

Choose a reason for hiding this comment

sisuresh Apr 2, 2020

Choose a reason for hiding this comment

sisuresh commented Apr 2, 2020

MonsieurNicolas commented Apr 4, 2020

sisuresh commented Jan 14, 2020 •

edited

Loading

sisuresh commented Apr 2, 2020 •

edited

Loading