XLS-56d: Atomic/Batch Transactions #197

mvadari · 2024-05-06T17:07:05Z

Discussion thread can be found here: #162

This PR will be left open for easier commenting of specific lines/sections. The discussion will also still be available for ease of commenting for non-technical people.

mDuo13

I am somewhat concerned about the greater potential for front-running with batch transactions. It seems like that is the "use case" that receives the biggest direct benefit from this feature even though we would rather discourage front-running.

Related, I think the specification should make it explicit how the batch affects the canonical order of transactions. It seems like the intent is for the canonical order to contain the outer transaction followed by the successful inner transactions in direct sequence, and that would start from wherever the outer transaction gets placed in the canonical order?

It might also be worth adding a subsection to Security about transaction fees. Specifically, make a note about how the outer transaction pays fees for all the inner transactions even if using modes like ONLYONE where some of those transactions can fail, and how this is a precaution against exploits that could make the whole network do the work of processing candidate transactions that won't pay transaction fees.

XLS-0056d-batch/README.md

mDuo13 · 2024-08-07T00:55:09Z

XLS-0056d-batch/README.md

+
+Some objects, such as [offers](https://xrpl.org/docs/references/protocol/ledger-data/ledger-entry-types/offer/#offer-id-format) and [escrows](https://xrpl.org/docs/references/protocol/ledger-data/ledger-entry-types/escrow/#escrow-id-format), use the sequence number of the creation transaction as a part of their ledger entry ID generation, to ensure uniqueness of the IDs.
+
+To get around this, a "phantom sequence number" will be used instead. The "phantom sequence number" will be equal to `BatchTxn.TicketSequence ?? (BatchTxn.Sequence + BatchTxn.BatchIndex)`. Note that this means that any 


I don't understand what the ?? operator is supposed to mean here. Also, the second sentence is incomplete.

It's using JS syntax - ?? is a nullish coalescing operator.

mDuo13 · 2024-08-07T01:06:35Z

XLS-0056d-batch/README.md

+
+This spec supports four modes: 
+* `ALLORNOTHING` or `tfAllOrNothing` (with a value of `0x00000001`)
+* `ONLYONE` or `tfOnlyOne` (with a value of `0x00000002`)


I suggest renaming this mode to FIRST (tfFirst) to reflect that the order of inner transactions matters.

That might imply to some people that it's something to do with the first transaction in the batch, rather than the first success.

mDuo13 · 2024-08-07T01:07:37Z

XLS-0056d-batch/README.md

+The `Flags` field represents the **batch mode** of the transaction. Exactly one must be specified in a `Batch` transaction.
+
+This spec supports four modes: 
+* `ALLORNOTHING` or `tfAllOrNothing` (with a value of `0x00000001`)


You could optionally rename this to ALL (tfAll) which I think conveys the same idea as well or better. (The "nothing" case wouldn't really be a success in my book!)

I thought tfAll might be confused with tfIndependent.

tequdev · 2024-08-09T02:37:36Z

XLS-0056d-batch/README.md

+|:---------|:-----------|:---------------|:------------|
+|`Account`|✔️|`string`|`STAccount`|
+|`SigningPubKey`| |`string`|`STBlob`|
+|`Signature`| |`string`|`STBlob`|


Wouldn't it be easier to understand if the field name is the same as TxnSignature used as CommonField?

It would technically have to be TxnsSignature, so I'd prefer leaving it as Signature. But it's not a strong opinion.

If we use Signer the the sign checks that currently exist would need to be updated/duplicated. So i'm using TxnSignature.

@dangell7 just to confirm: I should update the field here to be TxnSignature?

@dangell7 are we still using Signer here? The code shows a separate BatchSigner.

We are using BatchSigner. Thats whats in the spec I think (JSON). I guess we could do BatchSigners/Signer and add Signers as an optional on the Signer object... if thats what you mean.

Above I said Signer but meant Signature we are using TxnSignature so that the signing/verification was the same between the two (signle/multi) but I think I could use Signature for just the BatchSigner and leave the signers the same.

Up to you.

XLS-0056d-batch/README.md

mDuo13 · 2024-08-07T20:08:26Z

XLS-0056d-batch/README.md

+
+Each inner transaction will contain the metadata for its own processing. Only the inner transactions that were actually committed to the ledger will be included. This makes it easier for legacy systems to still be able to process `Batch` transactions as if they were normal.
+
+There will also be a pointer back to the parent outer transaction (`parent_batch`), for ease of development (similar to the `nftoken_id` field).


Is this "pointer" meant to be part of the canonical (hashed) metadata, or added afterward? It seems like something that would be useful to have in the canonical metadata, but if so it should be in PascalCase.

I was thinking added afterward, but I'm not sure if that's possible. cc @dangell7

I'm not sure this is possible. How would the inner txn have any context of the outer batch unless this is computed ahead of time.

Should it be added to the metadata then?

The metadata on the inner txn? We could maybe point the PreviousTxnID back to the batch? or add something similar to that?

Just to clarify you want to link the inner txn back to the outer txn?

Just to clarify you want to link the inner txn back to the outer txn?

Yes, I think that would be very useful.

XLS-0056d-batch/README.md

dangell7 · 2024-09-05T22:50:53Z

XLS-0056d-batch/README.md

+|FieldName | Required? | JSON Type | Internal Type |
+|:---------|:-----------|:---------------|:------------|
+|`TransactionHash`|✔️|`string`|`STUInt256`|
+|`TransactionResult`|✔️|`string`|`STBlob`|


I don't think we can duplicate the field name here with another type? Maybe we use sfBatchResult? Also if this is an STBlob that means that the response value will be a hex. I have updated the PR.

What about something like sfInnerResult? sfBatchResult could be confused to mean the result of the whole Batch.

sfInnerResult works

dangell7 · 2024-09-09T12:03:00Z

XLS-0056d-batch/README.md

+
+In other words, a single-account `Batch` transaction will have the same sequence number in the outer transaction _and_ all the inner transactions.
+
+#### 3.1.3. `TicketSequence`


Need to clarify something here. We add 1 on inner txn where outer.account == inner.account because we assume the outer txn consumed a sequence. If we use a ticket on the outer txn then we cannot add one but we wont know that based on the data from sfBatchTxn.

Why dont we just pre calculate the sequence? If the failure is anything but tes or tec then the txn is reverted regardless of the atomic type. If its a tes or tec it will consume the sequence. So there's not much value in calculating it in the getSeqProxy idk.

What do you mean by pre-calculate the sequence?

Ignore that comment

dangell7 · 2024-09-10T09:45:36Z

An Inner account delete should be blocked if the same account is submitting the batch.

dangell7 · 2024-09-10T12:08:42Z

I'm currently signing the entire txn for BatchSigners as I do not understand the TxnIDs completely and have not implemented that. It will also require more custom logic in both the rippled and repos to sign an array of hashes and a flag field.

dangell7 · 2024-09-10T12:19:28Z

RE BatchIndex

Before it was thought that the batch index was the index of the transaction for a specific account. Now or with the addition of tickets, it is more correctly the index of the transaction for an account/sequence/ticket. Previously we were adding +1 to the sequence calculation if the batch txn was the outer account, however if using a combination of sequence and tickets there is no way to know. So the index needs to be calculated prior to submission.

Some examples.

OuterTxn: Alice using ticket
InnerTxn: Alice using sequence (Index = 0)
InnerTxn: Alice using sequence (Index = 1)

OuterTxn: Alice using ticket
InnerTxn: Alice using ticket (Index = 1)
InnerTxn: Alice using sequence (Index = 0)

OuterTxn: Alice using sequence
InnerTxn: Alice using ticket (Index = 0)
InnerTxn: Alice using sequence (Index = 1)

XLS-0056d-batch/README.md created from https://stackedit.io/

cf8d749

mvadari marked this pull request as draft May 6, 2024 17:07

mvadari added 3 commits July 31, 2024 17:27

XLS-0056d-batch/README.md updated from https://stackedit.io/

b7096cc

XLS-0056d-batch/README.md updated from https://stackedit.io/

720c6be

XLS-0056d-batch/README.md updated from https://stackedit.io/

9ae47bb

mDuo13 reviewed Aug 7, 2024

View reviewed changes

tequdev reviewed Aug 9, 2024

View reviewed changes

mDuo13 reviewed Aug 9, 2024

View reviewed changes

mvadari marked this pull request as ready for review August 19, 2024 13:44

mvadari added 3 commits August 19, 2024 09:47

XLS-0056d-batch/README.md updated from https://stackedit.io/

d56822b

XLS-0056d-batch/README.md updated from https://stackedit.io/

0f7827c

XLS-0056d-batch/README.md updated from https://stackedit.io/

3d6d1c8

dangell7 reviewed Sep 5, 2024

View reviewed changes

dangell7 reviewed Sep 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

XLS-56d: Atomic/Batch Transactions #197

XLS-56d: Atomic/Batch Transactions #197

mvadari commented May 6, 2024 •

edited

Loading

mDuo13 left a comment

mDuo13 Aug 7, 2024

mvadari Aug 19, 2024

mDuo13 Aug 7, 2024

mvadari Aug 19, 2024

mDuo13 Aug 7, 2024

mvadari Aug 19, 2024

tequdev Aug 9, 2024

mvadari Sep 4, 2024

dangell7 Sep 10, 2024

mvadari Sep 10, 2024

dangell7 Sep 10, 2024

mvadari Oct 2, 2024

dangell7 Oct 2, 2024

mDuo13 Aug 7, 2024

mvadari Sep 4, 2024

dangell7 Sep 9, 2024

mvadari Sep 10, 2024

dangell7 Sep 10, 2024

mvadari Sep 11, 2024

dangell7 Sep 5, 2024

mvadari Sep 10, 2024

dangell7 Sep 10, 2024

dangell7 Sep 9, 2024

mvadari Sep 10, 2024

dangell7 Sep 10, 2024

dangell7 commented Sep 10, 2024

dangell7 commented Sep 10, 2024

dangell7 commented Sep 10, 2024


		Some objects, such as [offers](https://xrpl.org/docs/references/protocol/ledger-data/ledger-entry-types/offer/#offer-id-format) and [escrows](https://xrpl.org/docs/references/protocol/ledger-data/ledger-entry-types/escrow/#escrow-id-format), use the sequence number of the creation transaction as a part of their ledger entry ID generation, to ensure uniqueness of the IDs.

		To get around this, a "phantom sequence number" will be used instead. The "phantom sequence number" will be equal to `BatchTxn.TicketSequence ?? (BatchTxn.Sequence + BatchTxn.BatchIndex)`. Note that this means that any


		Each inner transaction will contain the metadata for its own processing. Only the inner transactions that were actually committed to the ledger will be included. This makes it easier for legacy systems to still be able to process `Batch` transactions as if they were normal.

		There will also be a pointer back to the parent outer transaction (`parent_batch`), for ease of development (similar to the `nftoken_id` field).


		In other words, a single-account `Batch` transaction will have the same sequence number in the outer transaction _and_ all the inner transactions.

		#### 3.1.3. `TicketSequence`

XLS-56d: Atomic/Batch Transactions #197

Are you sure you want to change the base?

XLS-56d: Atomic/Batch Transactions #197

Conversation

mvadari commented May 6, 2024 • edited Loading

mDuo13 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

Choose a reason for hiding this comment

dangell7 commented Sep 10, 2024

dangell7 commented Sep 10, 2024

dangell7 commented Sep 10, 2024

mvadari commented May 6, 2024 •

edited

Loading