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

Support Fee Bump Transactions (CAP 15) #2360

Closed
10 tasks
tamirms opened this issue Mar 4, 2020 · 8 comments · Fixed by #2406 or #2423
Closed
10 tasks

Support Fee Bump Transactions (CAP 15) #2360

tamirms opened this issue Mar 4, 2020 · 8 comments · Fixed by #2406 or #2423
Assignees
Milestone

Comments

@tamirms
Copy link
Contributor

tamirms commented Mar 4, 2020

The Stellar Core protocol 13 release will introduce a new type of transaction called Fee Bump transactions. Here is a summary of the changes (see https://github.com/stellar/stellar-protocol/blob/master/core/cap-0015.md for the full set of details):

  • The XDR representation of a transaction envelope has changed to be a union of legacy (pre protocol 13) transactions, normal transactions where the source account pays for their own transactions, and fee bump transactions where another account elects to pay for someone else's transaction

  • A fee bump transaction wraps a normal transaction with two extra pieces of information: the account which will be paying for the fee and the new fee amount the account is willing to pay. Consequently, the transaction hash of the fee bump transaction will differ from the hash of the fee bump's inner transaction.

  • A fee bump transaction envelope will contain the signature of the fee source account.

  • The fee bump transaction envelope will also contain the full contents of the inner transaction including the original source account, signatures, and fee of the inner transaction.


To support Fee Bump transactions we will need to implement the following changes in Horizon and the SDKs:

  • Update XDR definitions and auto generated code derived from XDR definitions in Horizon

  • Update XDR definitions and auto generated code derived from XDR definitions in all the SDKs

  • Update horizon schema to support fee bump transactions. I propose the following schema changes:

    • add inner_transaction_hash, inner_signatures, and fee_account columns to the history_transactions table
    • add index on inner_transaction_hash
    • if a transaction is not a fee bump transaction, inner_transaction_hash, inner_signatures, and fee_account will be NULL
    • if a transaction is a fee bump transaction, inner_transaction_hash will be the hash of the inner transaction wrapped by the fee bump, inner_signatures will be the set of signatures for the inner transaction, and fee_account will be the account paying the fee on behalf of the inner transaction
    • if a transaction is a fee bump transaction, transaction_hash will be the hash of the fee bump transaction, max_fee will be the fee set by the fee bump transaction, fee_charged will be the amount paid by the fee source account, and all remaining fields in the history_transactions row will be populated using fields from the inner transaction.
  • Update horizon ingestion to support fee bump transactions.

    • We can punt on adding any new types of effects for fee bump transactions because Horizon effects are already missing cases like "Account Debited" for transaction fees (see Evaluate missing effects, and consider exposing txmeta effects #2175)
    • We will still need to modify all ledger transaction processors because the way we extract operation results from the transaction results XDR object has changed
    • If a fee bump transaction succeeds, we will need to examine the results field from the TransactionResult XDR object.
    • If a fee bump transaction fails, we will need to examine the innerResult field from the TransactionResult XDR object.
    • When ingesting transactions we extract the transaction hash from the TransactionResultPair XDR object in the TransactionResultSet. In the case of a fee bump transaction, I assume the transaction hash in TransactionResultPair will correspond to the fee bump transaction, not the inner transaction. How should we obtain the transaction hash of the inner transaction? Should we calculate the inner transaction hash during ingestion? Or should TransactionResultPair be amended to include the inner transaction hash? @jonjove any thoughts on this topic?
  • Update the horizon protocol transaction response struct so that it includes the new fields added to the history_transactions table

  • Update SDKs so that they are compatible with the new horizon transaction response structure

  • Update horizon GET /transactions/{hash} handler so that hash can match either the transaction_hash or inner_transaction_hash column from the history_transactions table. In other words, we should be able to look up a transaction by either its hash or its inner hash (in the case the transaction is a fee bump)

  • Update txsub package in horizon so that a request to submit a normal transaction will respond with the transaction result from its corresponding fee bump transaction if the normal transaction was fee bumped.

  • Updates SDKs with functionality to create and submit fee bump transactions. Note that fee bump transactions have a different fee rate calculation compared to normal transactions, see https://github.com/stellar/stellar-protocol/blob/master/core/cap-0015.md#fee-rate .

  • Consider SEP 10 implications of fee bump transactions. We probably want to add the restriction that a fee bump transaction cannot be a valid SEP 10 challenge transaction.

@jonjove
Copy link

jonjove commented Mar 5, 2020

@tamirms Unfortunately TransactionResultPair cannot be changed directly. It doesn't have an extension point, and it is hashed into the LedgerHeader via TransactionResultSet. If the actual TransactionEnvelope is available to Horizon when it is processing results, then I think computing the inner hash is a fine approach. I note, however, that the TransactionResultPair was not designed like that. We could modify the TransactionResult to reveal the inner hash for fee-bump transactions, rather than the TransactionResultPair.

One thing that I don’t see addressed in this issue is that there won’t be any true TransactionResult for the inner transaction. But the “GET /transactions/{hash}” endpoint returns a field “result_xdr”. How do you plan to resolve this issue? I have discussed this with @ire-and-curses, so he may have some thoughts on this as well. It is possible to modify the TransactionResult to make this easier for you, we just need to agree on what is appropriate.

Below is a conceptual prototype of what the new TransactionResult XDR could be in order to resolve both of these issues. Be aware that this XDR is simplified, and definitely will not compile with xdrc.

enum TransactionResultCode
{
    txFEE_BUMP_INNER_SUCCESS = 1, // all operations succeeded for fee bump
    txSUCCESS = 0,                                    // all operations succeeded

    txFAILED = -1, // one of the operations failed (none were applied)

    txTOO_EARLY = -2,         // ledger closeTime before minTime
    txTOO_LATE = -3,          // ledger closeTime after maxTime
    txMISSING_OPERATION = -4, // no operation was specified
    txBAD_SEQ = -5,           // sequence number does not match source account

    txBAD_AUTH = -6,             // too few valid signatures / wrong network
    txINSUFFICIENT_BALANCE = -7, // fee would bring account below reserve
    txNO_ACCOUNT = -8,           // source account not found
    txINSUFFICIENT_FEE = -9,     // fee is too small
    txBAD_AUTH_EXTRA = -10,      // unused signatures attached to transaction
    txINTERNAL_ERROR = -11       // an unknown error occured

    txFEE_BUMP_INNER_FAILED = -12 // the inner transaction failed
};

struct TransactionResult
{
    int64 feeCharged; // actual fee charged for the transaction

    union switch (TransactionResultCode code)
    {
    case txFEE_BUMP_INNER_SUCCESS:
    case txFEE_BUMP_INNER_FAILED:
        TransactionResultPair innerResult;
    case txSUCCESS:
    case txFAILED:
        OperationResult results<>;
    default:
        void;
    }
    result;

    // reserved for future use
    union switch (int v)
    {
    case 0:
        void;
    }
    ext;
};

With XDR along these lines, fee-bump transactions will never return a TransactionResult with code txSUCCESS or txFAILED, but all other results are possible. If code is txFEE_BUMP_INNER_SUCCESS or txFEE_BUMP_INNER_FAILED then the inner code will never be txFEE_BUMP_INNER_SUCCESS or txFEE_BUMP_INNER_FAILED. As always, negative codes will indicate failure while non-negative codes will indicate success.

Would a change like this be helpful? I’m happy to optimize the XDR further if possible.

@tamirms
Copy link
Contributor Author

tamirms commented Mar 5, 2020

@jonjove and I discussed #2360 (comment) on slack. Following our conversation, here is my new proposal for Horizon responses:

We want to structure the Horizon responses so that the hash in the response always matches the hash in the GET /transactions/{hash} request. Additionally, we want it to be very easy to disambiguate normal transaction responses from inner and fee bump transaction responses. Finally, we want to be more explicit regarding which fields in the response are derived from the inner tx vs the outer fee bump tx.


  • If a GET /transactions/{hash} request queries for a normal (non fee bump) transaction the response from horizon will resemble the current format . The only difference is that there will be an extra string field called fee_account. In this case, fee_account will always equal source_account. Because the source account of the transaction is paying for the fee.

  • If a GET /transactions/{hash} request looks up a transaction using the fee bump tx hash (as opposed to the inner tx hash) the response from horizon will have the following form:


Attribute Type
id string This attribute has not changed from the current format.
paging_token string This attribute has not changed from the current format.
successful bool This attribute has not changed from the current format.
hash string The hash is equal to the fee bump tx hash, not the inner tx hash.
ledger number This attribute has not changed from the current format.
created_at ISO8601 string This attribute has not changed from the current format.
fee_account string This is a new field. It is set to account which is paying for the fee
source_account string This is the source account of the inner tx
source_account_sequence string This is the sequence number of the source account of the inner tx
max_fee number The the maximum fee the fee account was willing to pay for the fee bump tx.
fee_charged number The fee paid by the fee account when the transaction was applied to the ledger.
operation_count number The number of operations that are contained within the inner tx.
envelope_xdr string The XDR string for the fee bump tx envelope
result_xdr string The XDR string for the fee bump TransactionResult. Note that this XDR object also contains results from operations in the inner tx.
result_meta_xdr string The XDR string for TransactionMeta (note that the XDR definition hasn't changed for this field and that there is no concept of TransactionMeta applying to the inner transaction separately from the fee bump transaction).
fee_meta_xdr string The XDR string for LedgerEntryChanges xdr struct produced by taking fees for this transaction (note that the XDR definition hasn't changed for this field and that there is no reason to separate LedgerEntryChanges into inner tx changes and fee bump tx changes)
memo_type string The memo type for the inner tx.
memo string The memo for the inner tx.
signatures string[] The list of signatures used to sign the fee bump tx
valid_after RFC3339 date-time string Derived from the inner tx time bounds.
valid_before RFC3339 date-time string Derived from the inner tx time bounds.
inner_transaction Object with the following fields: max_fee, hash, signatures This object contains the max fee from the inner transaction, the hash of the inner transaction, and a list of signatures from the inner transaction

  • If a GET /transactions/{hash} request looks up a transaction using the inner tx hash (as opposed to the fee bump tx hash) the response from horizon will have the following form:

Attribute Type
id string This attribute has not changed from the current format.
paging_token string This attribute has not changed from the current format.
successful bool This attribute has not changed from the current format.
hash string The hash is equal to the fee bump tx hash, not the inner tx hash.
ledger number This attribute has not changed from the current format.
created_at ISO8601 string This attribute has not changed from the current format.
fee_account string This is a new field. It is set to account which is paying for the fee
source_account string This is the source account of the inner tx
source_account_sequence string This is the sequence number of the source account of the inner tx
max_fee number The the maximum fee the source account was willing to pay for the inner tx.
fee_charged number This will always be 0 because the source account does not pay for fees in a fee bump tx.
operation_count number The number of operations that are contained within the inner tx.
envelope_xdr string The XDR string for the fee bump tx envelope. Note that this XDR object also contains the inner tx envelope
result_xdr string The XDR string for the fee bump TransactionResult. Note that this XDR object also contains results from operations in the inner tx.
result_meta_xdr string The XDR string for TransactionMeta
fee_meta_xdr string The XDR string for LedgerEntryChanges xdr struct produced by taking fees for this transaction
memo string The memo for the inner tx.
signatures string[] The list of signatures used to sign the inner tx
valid_after RFC3339 date-time string Derived from the inner tx time bounds.
valid_before RFC3339 date-time string Derived from the inner tx time bounds.
fee_bump_transaction Object with the following fields: max_fee, fee_charged, hash, signatures All the fields in this object are derived from the fee bump transaction

Note that all the links in the horizon response will be based off the fee bump transaction hash.


I found a case where the Java SDK parses the transaction envelope XDR while deserializing the Horizon JSON response. This means that we will need to release the protocol 13 support in the SDKs before it is deployed in Horizon and Stellar Core. Otherwise, clients which use the protocol 12 version of the Java SDK will fail to parse Horizon responses corresponding to fee bump transactions.


Regarding how we obtain the hash of the inner transaction, I think it's best if Horizon computes the hash manually during ingestion using the transaction envelop. @jonjove mentions that it should be relatively easy for Stellar Core to provide the inner transaction hash somewhere in TransactionResult. But, I think it's best to lean on the side of caution by avoiding Stellar Core / XDR changes which are not absolutely necessary.

@abuiles
Copy link
Contributor

abuiles commented Mar 5, 2020

@tamirms when loading an inner transaction I wonder if we should generalize the fee_bump_transaction to parent_transaction or outer_transaction -- @jonjove do you think there could be scenarios where we end up with other transactions that have an innerTransaction field?

@jonjove
Copy link

jonjove commented Mar 6, 2020

@abuiles It's definitely possible that we one day might have other "nesting" transaction types. I had an idea for one a few months ago, although I don't currently think we will end up developing that idea.

@abuiles
Copy link
Contributor

abuiles commented Mar 6, 2020

@jonjove thanks!

@ire-and-curses
Copy link
Member

ire-and-curses commented Mar 10, 2020

We'll be pausing further design work on this until CAP 15 hits accepted state. Active discussion ongoing here.

@ire-and-curses
Copy link
Member

Ignore previous comment. I was talking about CAP 23. This one is good to go.

@bartekn
Copy link
Contributor

bartekn commented May 12, 2020

Seems like this has been fixed in the recent versions of Horizon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants