CAP-0023 implementation #2591

sisuresh · 2020-06-22T17:21:51Z

Description

Implements https://github.com/stellar/stellar-protocol/blob/master/core/cap-0023.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

Completed a preliminary review of everything after the LedgerTxn changes, excluding the tests. Looks pretty good, but a few important comments in here.

src/transactions/ClaimClaimableBalanceOpFrame.cpp

src/transactions/CreateClaimableBalanceOpFrame.cpp

src/invariant/test/BucketListIsConsistentWithDatabaseTests.cpp

MonsieurNicolas

I did a full review of this PR - see comments.

Overall this is pretty close to mergeable (good job!):

I added a couple questions that happen to be CAP issues/suggestions that we missed
I think you have a few missing test cases that would have uncovered a couple bugs

MonsieurNicolas · 2020-06-27T19:19:24Z

src/xdr/Stellar-ledger-entries.x

+case CLAIM_PREDICATE_AFTER_ABSOLUTE_TIME:
+    int64 absAfter;
+case CLAIM_PREDICATE_BEFORE_RELATIVE_TIME:
+    int64 relBefore;    // Seconds since closeTime of the ledger in which the


you need to run clang-format manually on the .x files (make sure to comment the %#include before doing so)

MonsieurNicolas · 2020-06-27T21:37:50Z

src/xdr/Stellar-ledger-entries.x

+    CLAIM_PREDICATE_AND = 1,
+    CLAIM_PREDICATE_OR = 2,
+    CLAIM_PREDICATE_BEFORE_ABSOLUTE_TIME = 3,
+    CLAIM_PREDICATE_AFTER_ABSOLUTE_TIME = 4,


CAP question - should we remove the AFTER (or BEFORE) predicates and just add a NOT operator?

I thought about this (it was actually my original approach) but I had some concerns that it could be a future foot-gun. For example, suppose we added a predicate like CLAIM_PREDICATE_HASH_PREIMAGE where you have to reveal a SHA256-preimage in order to claim. NOT doesn't behave particularly well with this predicate because it would be satisfied by default. Maybe this concern is overblown, because you could always configure the predicates incorrectly anyway. What do you think? I'm pretty flexible on this.

MonsieurNicolas · 2020-06-27T21:44:10Z

src/test/TestAccount.cpp

+    REQUIRE(claimableBalance.amount == amount);
+    REQUIRE(claimableBalance.balanceID == balanceID);
+    REQUIRE(claimableBalance.createdBy == getPublicKey());
+    REQUIRE(claimableBalance.reserve ==


warning: signed vs unsigned comparison

Should be fixed.

MonsieurNicolas · 2020-06-27T21:45:16Z

src/transactions/CreateClaimableBalanceOpFrame.cpp

+int64_t
+relativeToAbsolute(TimePoint closeTime, int64_t relative)
+{
+    return closeTime > (INT64_MAX - relative) ? INT64_MAX


warning: signed/unsigned comparison

Should be fixed.

MonsieurNicolas · 2020-06-27T21:45:57Z

src/transactions/CreateClaimableBalanceOpFrame.cpp

+            return false;
+        }
+
+        auto ok = addBalance(header, sourceAccount, -amount);


warning ok shadows line 156

src/transactions/ClaimClaimableBalanceOpFrame.cpp

MonsieurNicolas · 2020-06-27T23:31:37Z

src/transactions/OperationFrame.cpp

@@ -47,7 +49,7 @@ getNeededThreshold(LedgerTxnEntry const& account, ThresholdLevel const level)

 shared_ptr<OperationFrame>
 OperationFrame::makeHelper(Operation const& op, OperationResult& res,
-                           TransactionFrame& tx)
+                           TransactionFrame& tx, size_t index)


per my other comment elsewhere, size_t is overkill for this as we later have to downcast it. Better to just downcast it once (here). Not sure why it was size_t before in TransactionFrame::makeOperation.

src/invariant/BucketListIsConsistentWithDatabase.cpp

src/main/Config.cpp

src/invariant/LedgerEntryIsValid.cpp

osman-20068

os

MonsieurNicolas

I've resolved all issues that I consider resolved; added a couple new ones.
Next step is to rebase on top of master and squash commits so that you can address the SQL related issues.

MonsieurNicolas · 2020-07-11T18:35:27Z

src/ledger/LedgerTxnClaimableBalanceSQL.cpp

+    mDatabase.getSession() << "CREATE TABLE claimablebalance ("
+                           << "balanceid    VARCHAR(56) " << coll
+                           << " PRIMARY KEY, "
+                           << "createdby    VARCHAR(56) " << coll


I'm dumb for not having noticed this earlier: why do we need to expose all those internal fields?
I think the minimal set is
balanceid(HEX) | claimablebalanceentry (XDR) | lastmodified (BIGINT)

so we end up with a lot less code

This makes sense to me, but I noticed that this wasn't done for the other ledger entries. I want to make sure there wasn't a specific reason for listing the fields individually before I change this. @jonjove?

The reason other ledger entries aren't like this is historical. It's possible we will move this direction in the long run. We already moved slightly in this direction with #2593.

Makes sense. Thanks

I made the change to use a claimablebalanceentry column. I also updated db-schema.md.

MonsieurNicolas · 2020-07-11T18:46:46Z

src/database/Database.cpp

@@ -440,6 +440,7 @@ Database::initialize()
        mApp.getLedgerTxnRoot().dropOffers();
        mApp.getLedgerTxnRoot().dropTrustLines();
        mApp.getLedgerTxnRoot().dropData();
+        mApp.getLedgerTxnRoot().dropClaimableBalances();


you still need to do this

MonsieurNicolas · 2020-07-11T18:54:00Z

src/invariant/LedgerEntryIsValid.cpp

@@ -102,6 +102,8 @@ LedgerEntryIsValid::checkIsValid(LedgerEntry const& le, uint32_t ledgerSeq,
        return checkIsValid(le.data.offer(), version);
    case DATA:
        return checkIsValid(le.data.data(), version);
+    case CLAIMABLE_BALANCE:
+        return checkIsValid(le.data.claimableBalance(), version);


maybe open an issue: this invariant should also check that we don't modify immutable fields. For claimable balance, all fields are immutable.

Added an issue - #2620

MonsieurNicolas · 2020-07-11T18:56:59Z

src/invariant/ConservationOfLumens.cpp

@@ -23,6 +23,24 @@ calculateDeltaBalance(std::shared_ptr<LedgerEntry const> const& current,
        return (current ? current->data.account().balance : 0) -
               (previous ? previous->data.account().balance : 0);
    }
+    if (let == CLAIMABLE_BALANCE)
+    {
+        int64_t reserveDelta =


this is written as if reserve and amount are mutable, maybe OK in this context (albeit a bit confusing as the code doesn't work at all if asset gets modified) but see my comment in the other invariant on validity

This will be simplified when we rebase CAP-33 on top, because the reserve is not stored in that case. But I the code should work correctly in the case that asset changed. This would obviously be a bug if it were to happen, but for clarity and safety the code should account for it (after all, invariants are all about catching bugs).

MonsieurNicolas · 2020-07-11T19:09:25Z

src/transactions/test/ClaimableBalanceTests.cpp

+
+                for (uint32_t i = 0; i < 4; i++)
+                {
+                    SECTION("predicate at level " + std::to_string(i))


from recent findings, let's be defensive and avoid using to_string -> use fmt::format instead

MonsieurNicolas · 2020-07-14T00:26:50Z

docs/db-schema.md

+
+Field | Type | Description
+------|------|---------------
+balanceid | VARCHAR(56) PRIMARY KEY | (BASE64)


balanceid is a base64 encoded xdr object -> type should be "XDR", also description should say that it's a ClaimableBalanceID ; we use BASE64 as a type when there is no good XDR type.

claimablebalanceentry is a based64 encoded xdr object -> type should be "XDR" (and description should say that it's a ClaimableBalanceEntry)

MonsieurNicolas · 2020-07-14T00:37:59Z

src/database/Database.cpp

@@ -440,6 +440,7 @@ Database::initialize()
        mApp.getLedgerTxnRoot().dropOffers();
        mApp.getLedgerTxnRoot().dropTrustLines();
        mApp.getLedgerTxnRoot().dropData();
+        mApp.getLedgerTxnRoot().dropClaimableBalances();


yes you need to call mApp.getLedgerTxnRoot().dropClaimableBalances(); from Database::applySchemaUpgrade when upgrading to schema version 13 otherwise the table won't be created

MonsieurNicolas · 2020-07-14T00:47:39Z

src/ledger/LedgerTxnClaimableBalanceSQL.cpp

+// under the Apache License, Version 2.0. See the COPYING file at the root
+// of this distribution or at http://www.apache.org/licenses/LICENSE-2.0
+
+#include "crypto/KeyUtils.h"


I don't think you need those 2 "crypto" includes

MonsieurNicolas · 2020-07-14T00:56:47Z

src/ledger/LedgerTxnClaimableBalanceSQL.cpp

+
+    mDatabase.getSession() << "DROP TABLE IF EXISTS claimablebalance;";
+    mDatabase.getSession() << "CREATE TABLE claimablebalance ("
+                           << "balanceid             VARCHAR(56) " << coll


56 is wrong (needs to be updated in the md file as well):
I think it's base64(4+32) = 4 * round_up(36/3) = 4*12 = 48

MonsieurNicolas · 2020-07-14T01:06:37Z

src/ledger/LedgerTxnClaimableBalanceSQL.cpp

+    mDatabase.getSession() << "CREATE TABLE claimablebalance ("
+                           << "balanceid             VARCHAR(56) " << coll
+                           << " PRIMARY KEY, "
+                           << "claimablebalanceentry TEXT " << coll


we should store not just ClaimableBalanceEntry but the full LedgerEntry in this column, this duplicates a small amount of data but avoids adding an extra column for ledgerext (this was added in the latest round of changes to the database)

I support this approach as well. We can always change it later if the performance is worse-than-desired, but this approach will considerably simplify future development.

This would also allow us to remove the lastmodified column right?

Nevermind. The lastmodified column is required.

I made the change to store the full LedgerEntry.

jonjove

Nice work! This looks largely correct to me at this point, and @MonsieurNicolas had already identified most of the remaining issues. Most of my comments are picking out minor details. The main issue here for me is that the tests are pretty confusing: it was hard for me to tell exactly what is being tested in various places, and also hard for me to tell if we achieved the desired coverage. For example, I wasn't able to pick out a test that created a claimable balance entry in a transaction where the operation and transaction have different source accounts (is there such a test?). Is there anything we can do to improve the overall clarity of the tests?

src/ledger/test/LedgerTestUtils.cpp

jonjove · 2020-07-14T14:48:44Z

src/ledger/LedgerTxnClaimableBalanceSQL.cpp

+    mDatabase.getSession() << "CREATE TABLE claimablebalance ("
+                           << "balanceid             VARCHAR(56) " << coll
+                           << " PRIMARY KEY, "
+                           << "claimablebalanceentry TEXT " << coll


I support this approach as well. We can always change it later if the performance is worse-than-desired, but this approach will considerably simplify future development.

src/ledger/LedgerTxnClaimableBalanceSQL.cpp

src/transactions/CreateClaimableBalanceOpFrame.cpp

jonjove · 2020-07-14T15:42:36Z

src/invariant/ConservationOfLumens.cpp

@@ -23,6 +23,24 @@ calculateDeltaBalance(std::shared_ptr<LedgerEntry const> const& current,
        return (current ? current->data.account().balance : 0) -
               (previous ? previous->data.account().balance : 0);
    }
+    if (let == CLAIMABLE_BALANCE)
+    {
+        int64_t reserveDelta =


This will be simplified when we rebase CAP-33 on top, because the reserve is not stored in that case. But I the code should work correctly in the case that asset changed. This would obviously be a bug if it were to happen, but for clarity and safety the code should account for it (after all, invariants are all about catching bugs).

src/transactions/test/ClaimableBalanceTests.cpp

jonjove · 2020-07-14T16:07:36Z

src/transactions/test/ClaimableBalanceTests.cpp

+            // authorize trustline
+            if (!isNative)
+            {
+                SECTION("no trust")


Does this all actually work as you intend? The sequential sections with intermittent code is a pretty confusing pattern. It leaves me wondering, what is supposed to happen here and what actually does happen here?

Are these early sections like "no trust" and "not authorized" even required? The code in those sections should have no effect other than paying fees and consuming sequence numbers, so the sections might not really be doing anything.

In summary, I'm confused by this.

I cleaned up the tests and squashed with 572598aa0be8290908319e5d1d98e83c09c5520f.

sisuresh · 2020-07-14T23:12:08Z

@jonjove You make a good point about the test cases. They're confusing to look at, and the pattern where I use sequential sections with intermittent code can be hard to follow. I did this to avoid duplicating code, but the end result isn't great. I'll fix this.

I wasn't able to pick out a test that created a claimable balance entry in a transaction where the operation and transaction have different source accounts

SECTION("validate Tx account is used in hash") tests this, but that's obviously not clear from the name. Being able to identify any tested functionality based on the SECTION name is a good requirement. I'll make a pass through the test cases and try to change -

Ambiguous section names.
Any Large SECTION that tests multiple code paths without making it clear.

jonjove

This looks almost ready to me now. The tests are vastly easier to parse, so thank you for redesigning that! The only test that I don't remember seeing is one in which we create a claimable balance and then claim it in the same transaction. I expect this to work, but I think we should test it because it is an edge case in the sense that if you are doing this then you aren't really taking advantage of the behavior of claimable balance.

jonjove · 2020-07-17T15:30:14Z

src/ledger/LedgerTxnClaimableBalanceSQL.cpp

+    mDatabase.getSession() << "CREATE TABLE claimablebalance ("
+                           << "balanceid             VARCHAR(48) " << coll
+                           << " PRIMARY KEY, "
+                           << "claimablebalanceentry TEXT NOT NULL, "


I don't think this name is a good description anymore, given that we are storing the LedgerEntry.

Changed to ledgerentry.

jonjove · 2020-07-17T18:34:54Z

src/transactions/test/ClaimableBalanceTests.cpp

+                    ex_CREATE_CLAIMABLE_BALANCE_NO_TRUST);
+            }
+
+            // create and modify trustlines regardless of asset type so the


I'm a little confused about this comment. acc1 can't have the same balance in both scenarios because you are executing some transactions only if !isNative. Do I misunderstand?

Ah the comment should be updated to say the number of subentries is the same in both scenarios.

jonjove · 2020-07-17T19:33:16Z

src/transactions/test/ClaimableBalanceTests.cpp

+                }
+            }
+
+            SECTION("multiple create and claims")


Not sure I understand the purpose of this test? Unless I'm mistaken, it effectively does "create -> claim -> fund -> create -> claim" with each step in its own transaction. Was the intention to do that as a single transaction?

The test here was to make sure that the claim doesn't have any unintended side effects that could get in the way of another create/claim later on.

sisuresh · 2020-07-17T23:26:09Z

@jonjove I made the last round of PR changes and squashed/rebased. I also added a test for creating and claiming in the same tx under the section create and claim in same tx.

jonjove · 2020-07-20T19:50:46Z

r+ 29c1cab

MonsieurNicolas · 2020-07-21T00:23:58Z

@latobarita: retry

sisuresh force-pushed the cap23-implementation branch from ee4394d to 2f195ed Compare June 22, 2020 23:49

sisuresh marked this pull request as ready for review June 22, 2020 23:49

jonjove suggested changes Jun 26, 2020

View reviewed changes

MonsieurNicolas requested changes Jun 27, 2020

View reviewed changes

sisuresh force-pushed the cap23-implementation branch 3 times, most recently from e1ce5de to 42f2e20 Compare June 30, 2020 20:35

sisuresh force-pushed the cap23-implementation branch from 42f2e20 to 8b447a5 Compare July 9, 2020 00:11

osman-20068 reviewed Jul 9, 2020

View reviewed changes

MonsieurNicolas requested changes Jul 11, 2020

View reviewed changes

sisuresh force-pushed the cap23-implementation branch from 8b447a5 to 0ac1858 Compare July 13, 2020 20:46

MonsieurNicolas requested changes Jul 14, 2020

View reviewed changes

jonjove reviewed Jul 14, 2020

View reviewed changes

sisuresh force-pushed the cap23-implementation branch from 0ac1858 to 42501fe Compare July 14, 2020 22:44

sisuresh force-pushed the cap23-implementation branch 2 times, most recently from 18ec217 to cb0d1cf Compare July 16, 2020 18:00

jonjove reviewed Jul 17, 2020

View reviewed changes

sisuresh and others added 11 commits July 17, 2020 16:20

bump protocol version

770cd83

Add new LedgerEntry and LedgerKey types for ClaimableBalance

bba5707

Refactor LedgerCmp and expand it for new LedgerEntryTypes

288ceb4

Update LedgerEntryKey for ClaimableBalance

a4a1ea0

Update LedgerTestUtils for ClaimableBalance

74b7f96

Update LedgerHashUtils for ClaimableBalance

901e6bb

Update LedgerTxnTests for ClaimableBalance

ea68303

Implement SQL for ClaimableBalance

6c71d56

Use claimable balance SQL in LedgerTxn

ba61550

add XDR for claimable balance ops

caf454e

add loadClaimableBalance to TransactionUtils

36852dd

sisuresh added 10 commits July 17, 2020 16:20

fix invariant tests

908cd23

Create ClaimClaimableBalanceOpFrame

2bb7008

Create CreateClaimableBalanceOpFrame

fd0588a

instantiate ClaimableBalance ops and pass index to create op

2c44ab3

fix invariants for ClaimableBalance

5161d4d

add tests for ClaimableBalance ops

7f07aa1

update BucketApplicator for ClaimableBalance

7d93e75

update bucket test for ClaimableBalance

0395ebe

add claimable balance to db-schema.md

34aaa7e

add claimable balance to VS project files

29c1cab

sisuresh force-pushed the cap23-implementation branch from cb0d1cf to 29c1cab Compare July 17, 2020 23:23

latobarita merged commit 34d7efd into stellar:master Jul 21, 2020

jonjove mentioned this pull request Aug 5, 2020

Provide an easier deposit/airdrop workflow when users don't have a trustline or account stellar/stellar-protocol#303

Closed

CAP-0023 implementation #2591

CAP-0023 implementation #2591

Conversation

sisuresh commented Jun 22, 2020 • edited Loading

Description

Checklist

jonjove left a 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

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

osman-20068 left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonjove 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

sisuresh commented Jul 14, 2020

jonjove 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

sisuresh commented Jul 17, 2020

jonjove commented Jul 20, 2020

MonsieurNicolas commented Jul 21, 2020

sisuresh commented Jun 22, 2020 •

edited

Loading