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

created atomic transactions chain hip draft #551

Merged

Conversation

se7enarianelabs
Copy link
Contributor

Signed-off-by: Piotr Swierzy [email protected]

Description:

@netlify
Copy link

netlify bot commented Aug 24, 2022

Deploy Preview for hedera-hips ready!

Name Link
🔨 Latest commit df787fe
🔍 Latest deploy log https://app.netlify.com/sites/hedera-hips/deploys/652706e38915990008d843df
😎 Deploy Preview https://deploy-preview-551--hedera-hips.netlify.app/hip/hip-551
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

se7enarianelabs and others added 3 commits August 24, 2022 21:01
Signed-off-by: Piotr Swierzy <[email protected]>
changed name to batch transactions
Signed-off-by: Piotr Swierzy <[email protected]>
mgarbs
mgarbs previously approved these changes Sep 19, 2022
Signed-off-by: Michael Garber <[email protected]>
mgarbs and others added 3 commits October 4, 2022 14:31
@NilsonBertola
Copy link

Hello everyone.

In order to add 2 more batch scenarios, that can be very helpfull to some use cases.

Scenario 1: Mint -> Transfer

Me as the owner of the admin of a token, want to gift Alice some of my tokens. So I want in a Mint and Transfer the tokens in a single transaction.

Scenario 2: Grant KYC -> Wipe tokens from Alice account -> Revoke KYC

Me as the owner of the token regreted my decision to give tokens to Alice and Revoked KYC from Alice account to my token. Further in order to not have tokens flying away, I want to remove tokens from Alice account where doesn't KYC'ed anymore. Since it is not possible to wipe an non-KYC'ed account tried this before and transaction shows an error.
So I want to batch in a single transaction Grant KYC -> Wipe tokens from Alice account -> Revoke KYC in order to not have an gap to Alice transfer those tokens to somewhere else.

Tks @mgarbs for the talk we had on discord channel

Copy link
Contributor

@Nana-EC Nana-EC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting idea.
Left some comments in the form of questions

HIP/hip-551.md Outdated

## User stories

As a Hedera token service user, I want to be able to unfreeze an account, send an NFT, and freeze it again in one ACID transaction, that way I can achieve an account-bound NFT(nontransferable NFT) collection, without using the hedera smart contract service.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: should number the list

Suggested change
As a Hedera token service user, I want to be able to unfreeze an account, send an NFT, and freeze it again in one ACID transaction, that way I can achieve an account-bound NFT(nontransferable NFT) collection, without using the hedera smart contract service.
1. As a Hedera token service user, I want to be able to unfreeze an account, send an NFT, and freeze it again in one ACID transaction, that way I can achieve an account-bound NFT(nontransferable NFT) collection, without using the hedera smart contract service.

## Specification
We could create a new protobuf message based on `SignedTransaction` message:
```
message BatchTransaction {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: Is it fair to say the assumption or requirement is that the submitter is authorized to carry out all the batch transactions?
For example to carry out some of the example one be both the treasurer, in another case the freezeKey owner.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The submitter does not need to be authorized to carry out all the batch transaction, every transaction in the batch needs to be signed by all parties involved in the individual transaction.

It would work like every transaction in Hedera, where the submitter can be any account as long as the transaction is properly signed

/**
* TransactionBodies serialized into bytes, which must be signed
*/
repeated bytes bodyBytes = 1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: do each of these transactions result in externalized individual transaction records in the record stream?
If individual records are exposed is there a concept of parent and child transactions? If so you should supply an example of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand you question completely, but in my opinion, the batch transaction consists of individual transactions, but because there are batched, everyone needs to be successful, or they are reverted.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't really work with the current HAPI protobuf model. bodyBytes includes a transaction ID. You need to sign a single logical "parent" transaction ID in this case. The list of "transactions" should not have individual transaction ids associated with them because they should be assigned by the ledger upon execution (with appropriate nonces attached to them).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead, it might be possible to modify the TransactionBody protobuf message with a new field inside the data field that would be a repeated field of TransactionBodyXXXX (now this is where it gets murky, should it be a repeated field of encoded transaction body XXX, or should it be an explicitly defined new message that is a union of all the possible TransactionBodyXXXX types) Either way, with an ordered list of TransactionBodyXXXX you can achieve the desired outcome and only have to generate the enclosing Transaction ID for the outer TransactionBody envelope.

```
## Backwards Compatibility

## Security Implications
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: Are there cases where multiple operators are involved or will the batch transactions always see a single entity carrying out some steps?
If there's a case where the batch contains entity A doing one thing and entity B doing something else there would be potential security considerations to address

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the transaction can involve multiple parties, but every individual transaction in the batch needs to fulfill the standard transaction signing requirements, so I don't see any added security implications.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Nana-EC are you referring to the client operator here?

## Reference Implementation

## Rejected Ideas
We rejected the idea to add the support of conditional branching to the batch transactions,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you expand on this. Did you mean you rejected the case where essentially some transactions are carried out but not all?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, basically we won't support conditional branching, understood as if transaction B is not successful then execute transaction C.

HIP/hip-551.md Outdated
if the community asks for it.<br>
Initial set:
```
Unfreeze -> Transfer -> Freeze
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any non HTS cases ? Is this for HTS only or all HAPI transactions?
If there's a set on non applicable transactions those should be called out

Copy link
Collaborator

@mgarbs mgarbs May 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be more practical if these were for all HAPI transactions. Any application that needs a hapi transaction to succeed in order to do something else would use this. Gated topics or consensus messages have huge potential for content creation use cases. Here are a couple of scenarios:
Transfer -> Topic Create -> Submit Topic Message
Transfer -> Submit Topic Message
Submit Topic Message -> Transfer

You wouldn't want these transactions to get incredibly long. Just a few transactions that, with the signatures included is less than the 6kb limit

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ability to combine HTS with HCS would be super powerful if they are batched. So, would also agree for all HAPI transactions to reach the maximum value out of this HIP proposal.

/**
* TransactionBodies serialized into bytes, which must be signed
*/
repeated bytes bodyBytes = 1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: Also how should the network manage when one of these transaction has a child transaction?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the parent transaction is in the batch and the batch isn't successful, then the child transaction should also be reverted.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's how i see it. All or nothing.

/**
* TransactionBodies serialized into bytes, which must be signed
*/
repeated bytes bodyBytes = 1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: Are there any applicable byte size limits for consideration by the SDK and the network itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, the standard transaction size limit should be respected

HIP/hip-551.md Outdated

The existing implementation of transactions in the Hedera network does not allow multiple different HAPI transactions to be called in one single network transaction that would have all the ACID properties.
This makes it impossible to create more complicated flows without using smart contracts (which do not support all the HAPI transactions at this point) and listening to the mirror node to check the status of the previous transaction.
This way we can also achieve an abstraction away from smart contracts
Copy link

@pwoosauce pwoosauce May 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few more very powerful transaction chains that I would love to see incorporated into the motivation and user stories for this HIP.

This proposal enhances contract-less development on Hedera as well as smart contract orchestration and security on Hedera.

All of the below transaction chains will improve user experience, account security, contract security, contract compatibility, and contract transparency.

  • Associate -> Contract Call
    • The contract call is going to send a token to the user. The user must associate the token first, or the contract will revert.
  • Allowance Approval -> Contract Call
    • The contract call is going to take tokens from a user. The user must grant a token allowance first, or the contract will revert.
  • Allowance Approval -> Allowance Approval -> Associate -> Contract Call
    • The contract call is going to take 2 different tokens from a user and send the user an unassociated token that has already been created on the network. The user must grant token allowances first and associate the receivable token, or the contract will revert.
  • Allowance Approval -> Allowance Approval -> Account Update -> Contract Call
    • The contract call is going to take 2 different tokens from a user and send the user an unassociated token that will be created in the contract call. The user must grant token allowances first and have an available auto association slot, or the contract will revert.
  • Contract Call -> Contract Call
    • First contract call is an external contract. Second contract call is a contract validating the results of the first contract. The second contract will revert if the first contract does not produce the intended results. This could be a security enhancement and improve contract call extensibility.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also means that if any of the contract calls at the end are reverted, then the approvals, associates, and account updates would be undone. This would improve the security of the network by reducing dangling allowances and auto association slots.

se7enarianelabs and others added 2 commits May 29, 2023 13:57
…ns to the initial set, and added a security implication.

Signed-off-by: Piotr Swierzy <[email protected]>
@nickpoorman
Copy link
Contributor

nickpoorman commented Jun 13, 2023

Looking to finalize this HIP

I have a few questions as I think through the implementation. As I see it, there are two implementation paths that diverge as it relates to signing transactions.

@se7enarianelabs @NilsonBertola @bugbytesinc @Ashe-Oro @Nana-EC @pwoosauce Looking for your feedback here on the implementation paths described below so that we may finalize this HIP.

Implementation Path 1: We create a new Protobuf message based on SignedTransaction that looks like the following:

message BatchSignedTransactions {
	repeated SignedTransaction signed_transactions = 1;
}

signedTransactionBytes could then hold either a SignedTransaction or a BatchSignedTransactions.

The benefit of Implementation Path 1 is that each transaction will have all the same signature requirements as if they were separate transactions submitted to the network individually, with the additional behavior of rolling back all the transactions if any of them fail. This allows for fine-grained signing of each transaction in the batch but could result in extra cost due to the verification of more signatures --although, no more so than if they had been sent individually.

Also, in the case of Implementation Path 1, all transactions in the batch would need to have their transactionID field set such that the transactions would be valid as if they had been submitted individually, ie. each transactionID may need to have a transactionValidStart incremented by a nanosecond of each other. nodeAccountID would all need to be set to the same value. This is an implementation detail that the SDK could abstract away.

Implementation Path 2: We add SchedulableTransactionBody (with a wrapper) as a repeated field in the TransactionBody data field.

message BatchedTransactions {
  repeated SchedulableTransactionBody batched_transactions = 1;
}

...
message TransactionBody {
  ...
  oneof data {
     ...
     BatchedTransactions batched_transactions = 53;
  }
}

The benefit of Implementation Path 2 (and the difference from Implementation Path 1) would be a single SigMap; thus, the node would use all the signatures for verifying the transactions contained within. The benefit here, due to our current pricing model, is that the batch transaction would ultimately be cheaper since there are fewer signatures to verify. It may also be more natural as now there would only be a single transactionID, nodeAccountID, etc... From a code standpoint, it would behave similarly to a ScheduleCreate. This allows for signing the batch as a whole but results in the loss of signing individual transactions within the batch.

Also, in the case of Implementation Path 2, you would have a single memo field as they would all be sharing a single TransactionBody.


One last possibility (although I don't particularly like the complexity of it) could be to do both Implementation 1 and Implementation 2 and apply the same atomic rollback mechanism in case any part fails. This would give people the ability to choose if they want the fine-grained signing of each transaction in the batch or the signing of the batch as a whole.


Finally, a point of clarity that I think is important to highlight: We would assume that any failure, including but not limited to, any individual transaction hitting a throttle, would cause the entire batch to fail, and the fees would have to be levied for any work that had been done up to that point.

@bugbytesinc
Copy link
Contributor

bugbytesinc commented Jun 14, 2023

Path 1 creates a Replay Attack vulnerability. If one TX fails, an actor could crack open the batched TX and replay individual signed transactions without the consent of the original signer.

In short, you can't sign individual transactions within a batch for safety reasons.

@bugbytesinc
Copy link
Contributor

What will be the security posture regarding the movement of tokens? Will it be only the Payer of the wrapped set of XXXTransactionBody will be allowed to move tokens (if they have the required allowance) or will transfers of tokens be allowed if the true owning account signs the batch of transactions?

@pwoosam
Copy link

pwoosam commented Jun 15, 2023

Great point @bugbytesinc implementation path 1 would likely create a replay attack, but it does offer superior composability of transactions.

Because the BatchSignedTransaction would be submitted to the network and recorded as a failure, the signatures and metadata for the child transactions would also be on the network. That is probably enough for an bad actor to recreate and submit the individual transaction for a replay attack. However, if the parent transaction fails the child transactions should be marked as failures as well. This should cause duplicate transaction errors upon replay, preventing the replay attack.

And also, if the bad actor is the a consensus node receiving the BatchSignedTransaction, the consensus node could pick only the childTransaction that the attacker wants to execute and ignore the others. This would become a problem with community nodes.


What I like about implementation path 1 is that it enables composability of transactions. Not every transaction involved in the BatchSignedTransaction needs to be aware that they will be executed in a batch of transactions. I would strongly prefer that each transaction is signed individually over all participants needing to sign a BatchSignedTransaction.

Requiring all parties to sign a BatchSignedTransaction would limit the composability of transactions from multiple sources.


I would like to see implementation path 1, but the replay attack is concerning.

To prevent this replay attack, each child transaction's signature must be different from an equivalent individual transaction not in a BatchSignedTransaction. Perhaps each child transaction's signature should be signed again by the BatchSignedTransaction's TransactionId's account and the doubly-signed signature should replace the original transaction's signature. Basically, the if a transaction is a child transaction of a BatchSignedTransaction, then the original child transaction's signature must not be stored on the network. The signature must be modified to include the BatchSignedTransaction's TransactionId in some way.

@bugbytesinc
Copy link
Contributor

A couple of issues, first one that comes to mind is that is starting to add to many separate signatures to the payload, it will become impractical to add more than a hand full of transactions due to message size limitations. Also, signing then bundling multiple transactions is difficult to express in an SDK or more importantly with a wallet. Why 3 signatures when one would suffice?

Additionally, you can't safely mitigate the replay attack of path 1. For example, pre-check failures do not result in the propagation of the transaction to the network. A precheck errored batched TX could then be broken apart and re-packaged - same problem as before.

@bugbytesinc
Copy link
Contributor

Not every transaction involved in the BatchSignedTransaction needs to be aware that they will be executed in a batch of transactions.

If that is the case, what we have today works.

It is extremely important to be made aware of this condition for the types of constructs this is trying to support. I only am willing to let A happen if I know that B happened than C happens, otherwise I'm not signing this.

@bugbytesinc
Copy link
Contributor

For example, I'm not going to give an "allowance" to saucer swap unless I know that immediately afterwards that liquidity is sent to the contract. If that fails, I don't want the allowance to ever have happened, and I don't want that authorization to be replayable if the contract fails.

@pwoosam
Copy link

pwoosam commented Jun 15, 2023

Not every transaction involved in the BatchSignedTransaction needs to be aware that they will be executed in a batch of transactions.

Sorry, I meant not every signer needs to know that the transaction they signed is in a BatchSignedTransaction. This is important for adding validation contract calls to the end of 3rd party transactions. If I want to perform validation that a SmartContract is actually doing what the creator said it's going to do, I must be able to create a BatchSignedTransaction without requiring the 3rd party to sign the entire batch. I only want to need them to sign their individual transaction.

@pwoosam
Copy link

pwoosam commented Jun 15, 2023

For example, I'm not going to give an "allowance" to saucer swap unless I know that immediately afterwards that liquidity is sent to the contract. If that fails, I don't want the allowance to ever have happened, and I don't want that authorization to be replayable if the contract fails.

I agree, the replay must be prevented. I also do not want to give an allowance unless I know the allowance will be exercised or reverted in the same transaction.

@pwoosam
Copy link

pwoosam commented Jun 15, 2023

A couple of issues, first one that comes to mind is that is starting to add to many separate signatures to the payload, it will become impractical to add more than a hand full of transactions due to message size limitations. Also, signing then bundling multiple transactions is difficult to express in an SDK or more importantly with a wallet. Why 3 signatures when one would suffice?

Additionally, you can't safely mitigate the replay attack of path 1. For example, pre-check failures do not result in the propagation of the transaction to the network. A precheck errored batched TX could then be broken apart and re-packaged - same problem as before.

A batched transaction's child transaction signatures must be invalid for individual transactions. This would prevent a replay attack. The child transaction signatures should only be valid when they are children of the parent BatchSignedTransaction. This means the signatures of all child transactions must change when signing the BatchSignedTransaction. The original individual signatures will not be submitted to the network.

@nickpoorman
Copy link
Contributor

Path 1 creates a Replay Attack vulnerability. If one TX fails, an actor could crack open the batched TX and replay individual signed transactions without the consent of the original signer.

@bugbytesinc I believe we would add all the transactions inside the batch to the "duplicate list" whether they were successfully processed or not. That way, any transactions that come in while those would still be valid (max is probably around 3 minutes) would be considered duplicates and would not be processed.

@pwoosam
Copy link

pwoosam commented Jun 16, 2023

@bugbytesinc I believe we would add all the transactions inside the batch to the "duplicate list" whether they were successfully processed or not. That way, any transactions that come in while those would still be valid (max is probably around 3 minutes) would be considered duplicates and would not be processed.

@nickpoorman it is my understanding that adding all transactions in the batch to the "duplicate list" will not be sufficient to prevent the replay attack.

If the bad actor is a consensus node (community node), then the child transactions may be extracted and executed without the other transactions in the batch.

I believe child transactions must have their signatures modified to only be valid as child transactions under the specific batch. This way the child transaction cannot be extracted after being submitted to a consensus node.

@bugbytesinc
Copy link
Contributor

Path 1 creates a Replay Attack vulnerability. If one TX fails, an actor could crack open the batched TX and replay individual signed transactions without the consent of the original signer.

@bugbytesinc I believe we would add all the transactions inside the batch to the "duplicate list" whether they were successfully processed or not. That way, any transactions that come in while those would still be valid (max is probably around 3 minutes) would be considered duplicates and would not be processed.

But that's not how it works for prechecks, if they fail the TX "never happened" it will never reach this list to be considered a de-dup. Failed precheck TXs are not gossiped.

and then, if you made an exception for this use case, you open yourself up to a very cheap way to DOS the network.

@pwoosauce
Copy link

@rbair23 thank you for the well-thought response

It must not be possible to execute an inner transaction outside the context of the batch in which it is located

I agree, it must not be possible to execute an inner transaction outside the context of the batch in which it was submitted to a consensus node.

I believe there is some nuance here. You should have the ability build a transaction separately from the batch transaction, then put it in a batch transaction before submitting it to a consensus node.

It must not be possible for the inner transactions of a batch to be rearranged after the individual transactions have been signed

I agree that the inner transactions of a batch must not be able to be rearranged. But I believe they should be able to be rearranged up until the point that the batch transaction is frozen (or signed)

Each transaction must be individually signed. Any signatures on the batch itself relate to the batch, not the transactions within the batch.

I agree with this statement 100%


100% we need to ensure the immutable integrity of batch transactions before they are sent to a consensus nodes. But just before submitting the batch transaction to a consensus node, it is ideal to have the flexibility to compose batch transactions from different sources.

For example:

  • I want to be able to take a ContractExecuteTransaction from NFTier's marketplace and put it in a batch transaction with another ContractExecuteTransaction that I wrote, which will verify that I will receive the NFT that I am purchasing.
  • I want to be able to execute a series of ContractExecuteTransaction that perform token swaps from different DEXs, but revert if there was no arbitrage opportunity
  • I want to purchase an NFT on Zuse using my hot wallet and send the NFT immediately to my cold wallet. So I take the TokenTransferTransaction and add my own TokenTransferTransaction to the batch to send the purchased NFT to my cold wallet.

Please let me know if you have any question or concerns with what I said above. 😊

@nickpoorman
Copy link
Contributor

nickpoorman commented Jul 14, 2023

@pwoosam @pwoosauce

I want to be able to take a ContractExecuteTransaction from NFTier's marketplace and put it in a batch transaction with another ContractExecuteTransaction that I wrote, which will verify that I will receive the NFT that I am purchasing.

What if Alice is a malicious user that wants to kill the marketplace? In this case, Alice could take a ContractExecuteTransaction from the Marketplace and put it in a batch transaction guaranteed to fail because Alice has included a contract she wrote, which will always fail. The Marketplace would be forced to pay the node+network+service fees for their part of the transaction, and Alice could repeat the attack over and over again until the Marketplace has no more funds left.

If the issue was only with fees, I suppose, if you wanted to include a transaction in a batch that was not aware it was in the batch, then you could, as the one organizing the batch, designate yourself as the payer for all the fees of the transactions in the batch.

However, transactions in a batch need to know they are in a batch for other reasons. As @rbair23 pointed out:

It must not be possible to execute an inner transaction outside the context of the batch in which it is located

It must not be possible for a dishonest consensus node (or any program between the user who signed it and the consensus node) to disassemble a batch and reassemble it or modify it. Since inner transactions cannot be executed separately, it is not necessary to include them in the deduplication check IF, and only if, we can determine the transaction belongs in a batch before submitting to the network.

@pwoosauce
Copy link

pwoosauce commented Jul 14, 2023

@nickpoorman the ContractExecuteTransaction created by the Marketplace should use the user's account id in the transaction id, so the user will pay the network fees, not the marketplace.

It is bad practice to allow a user to execute transactions that the dapp pays for.
If the dapp is going to pay fees for the transaction, they should request a signature from the user, validate the transaction on their server, then sign it with their dapp's private key, and the dapp should execute the transaction.

For the other two concerns you mentioned, I proposed a potential solution earlier that satisfies those concerns:

Please consider another option for implementing batch transactions that is not susceptible to the replay attack. For example, if a transaction is in a batch, sign the signature with the batch transaction's signature or some variation of this. Do not submit the original signature for individual transactions.

@rbair23
Copy link
Member

rbair23 commented Jul 14, 2023

Please consider another option for implementing batch transactions that is not susceptible to the replay attack. For example, if a transaction is in a batch, sign the signature with the batch transaction's signature or some variation of this. Do not submit the original signature for individual transactions.

I don't understand this. Can you explain what you mean? Do you mean "sign the signature" or "sign the transaction"? I tried to work through this yesterday and I believe this would leave you wide open to attack. But maybe I am not understanding the idea right.

@rbair23
Copy link
Member

rbair23 commented Jul 14, 2023

It must not be possible to execute an inner transaction outside the context of the batch in which it is located

I agree, it must not be possible to execute an inner transaction outside the context of the batch in which it was submitted to a consensus node.

I believe there is some nuance here. You should have the ability build a transaction separately from the batch transaction, then put it in a batch transaction before submitting it to a consensus node.

If I have transaction T1, signed by user U1, and batch signed by user U2, then T1 must have a reference to the batch, or it is possible for an intermediary to disassemble and reassemble a batch.

It must not be possible for the inner transactions of a batch to be rearranged after the individual transactions have been signed

I agree that the inner transactions of a batch must not be able to be rearranged. But I believe they should be able to be rearranged up until the point that the batch transaction is frozen (or signed)

I agree, once the inner transaction has been signed, its position within the batch is fixed and the batch it is part of is fixed.

For example:

  • I want to be able to take a ContractExecuteTransaction from NFTier's marketplace and put it in a batch transaction with another ContractExecuteTransaction that I wrote, which will verify that I will receive the NFT that I am purchasing.
  • I want to be able to execute a series of ContractExecuteTransaction that perform token swaps from different DEXs, but revert if there was no arbitrage opportunity
  • I want to purchase an NFT on Zuse using my hot wallet and send the NFT immediately to my cold wallet. So I take the TokenTransferTransaction and add my own TokenTransferTransaction to the batch to send the purchased NFT to my cold wallet.

In these cases, as long as the user signing these transactions signs them in such a way that they fix them within the batch in their order, then I think we're good. If the expectation is that somebody else beside the user is signing it, then the user cannot subsequently add it to a batch. Otherwise, I think we're opening ourselves up to some attacks.

Thanks!

@pwoosauce
Copy link

@rbair23 Yes, I will break this down a bit. But I'll make a change because I see a flaw with my previous method.

Let's define some common language:

  • TransactionSignature: A byte array representing a the transaction's body bytes hashed by a private key
  • BatchedTransactionSignature: A byte array representing a TransactionSignature that has been hashed by the BatchTransaction's private key, a number representing the order that the transaction should be executed in a batch, and the BatchTransaction's transactionId
  • BatchTransactionSignature: A TransactionSignature for a BatchTransaction
  • SignatureMap: A map of key-value pairs where the key is a public key and the value is a signature

Before creating a batch transaction, a new public/private keypair should be created. When creating a BatchTransaction, the batch transaction should be initialized with a public key. The batch transaction should be populated with valid, signed inner transactions. Before submitting the batch transaction to the network, the batch transaction should be finalized with the private key we created earlier. The TransactionSignature of all inner transactions will be replaced with a BatchedTransactionSignature. This will prevent reordering of inner transactions, execution of the inner transaction from another BatchTransaction context, and execution from outside of a BatchTransaction.

This would look something like:

const client = Client.forPreviewNet();
client.setOperator('', '');

const tx1 = await getArbitrarySignedTransaction1();
const tx2 = await getArbitrarySignedTransaction2();

const privateKey = PrivateKey.generateED25519();
const batchTransaction = await new BatchTransaction()
    .setPublicKey(privateKey.publicKey)
    .addTransaction(tx1)
    .addTransaction(tx2)
    .finalizeBatch(privateKey) // this will replace all inner transaction's `TransactionSignatures` with `BatchedTransactionSignatures`;

const txResponse = await batchTransaction.execute(client);

@bugbytesinc
Copy link
Contributor

Goodness, this is getting complicated, I fear the level of effort required by wallet makers to successfully sort this all out. If you have something that "does not need to know its in a batch" - then it should just be submitted to the network on its own without being put inside a batch. If you have a series of all-or-nothing commands, then, it points to the easiest and most attainable option for wallet makers and system designers to require everyone to sign the outer envelope of all of the batched unsigned commands (XXXBodyBytes). If that solves 80% of the problem, and is decent forward progress on the matter, the other use cases can be sorted out after we get traction on some sort of atomic batching (particularly now with the allowance TX requirement for contracts)

... and if a wallet can't parse this new new Batched Transaction BodyBytes protobuf format, it has no business signing such a TX, because doing so could harm the key owner.

@nickpoorman
Copy link
Contributor

easiest and most attainable option for wallet makers and system designers to require everyone to sign the outer envelope of all of the batched unsigned commands (XXXBodyBytes).

@bugbytesinc - I believe we must have the inner transactions signed individually. A signature at the top level should not be an indication that that key authorizes everything within the batch, this could open us up to attacks. Generally, we want the finest-grained transaction signed to provide the most security and flexibility.

@rbair23
Copy link
Member

rbair23 commented Jul 18, 2023

I was talking with some others offline about how to do this. @netopyr convinced me some of my concerns were not valid. Let me see if I can explain.

Let's say:

  1. Each individual transaction within the batch is signed.
    a. Each transaction within the batch pays for itself (node + network + service fees)
    b. Each transaction within the batch has its own payer
    c. Each transaction within the batch is deduplicated individually
    d. The keys on the inner transaction are the only keys used to authorize that transaction (batch keys are not used)
    e. In other words, each inner transaction is a whole, complete transaction. It doesn't know anything about the batch.
  2. The batch itself has its own payer and its own signatures.
    a. The inner transactions are part of the signed bytes
    b. Each key holder on each inner transaction also signs the batch
    c. The batch payer pays the node+network fees for handling the batch -- but not for the inner transactions
    d. The batch will be deduplicated by its own transaction ID
  3. If an inner transaction fails, all preceding inner transactions that succeeded still have to pay, even though their effects are not committed.

In other words, inner transactions are normal transactions. The batch is signed by all participants.

I had claimed:

  • It must not be possible to execute an inner transaction outside the context of the batch in which it is located
  • It must not be possible for the inner transactions of a batch to be rearranged after the individual transactions have been > signed
  • Each transaction must be individually signed. Any signatures on the batch itself relate to the batch, not the transactions > within the batch.

The first two requirements were meant to prohibit a man-in-the-middle, or a dishonest node, from being able to break open and rearrange the batch, or break up a batch and create a new one, with a mind to preventing front-running. But @netopyr made a strong case that this is not something we should solve for, because a man-in-the-middle or a dishonest node can already front run you with normal transactions just by inspecting the transaction and holding it up and submitting their own transaction first. If you want to prevent front-running, you should use TLS to connect to the node and you should choose a node you trust.

That last requirement is really important. Signatures are used to determine authorization, so we want to use the signatures per-inner-transaction for authorization for that transaction.

Now, by requiring the signers of each inner transaction to also sign the batch, we make it so that the person who assembles the batch cannot fool the signers of the inner transactions. For example, as a batch creator I may say that you send me 100 HBAR and I will send you this fancy NFT worth $1M. You sign it. Then I (the batch creator), remove the second transaction and just take your 100 HBAR. It feels very "scammy". Instead, if the user has to sign the batch as well, then we are locking in place the batch and the agreement by all participants that the batch they're going to execute cannot be modified after they sign.

What do you think?

@se7enarianelabs
Copy link
Contributor Author

Now, by requiring the signers of each inner transaction to also sign the batch, we make it so that the person who assembles the batch cannot fool the signers of the inner transactions. For example, as a batch creator I may say that you send me 100 HBAR and I will send you this fancy NFT worth $1M. You sign it. Then I (the batch creator), remove the second transaction and just take your 100 HBAR. It feels very "scammy". Instead, if the user has to sign the batch as well, then we are locking in place the batch and the agreement by all participants that the batch they're going to execute cannot be modified after they sign.

@rbair23
I don't believe it is necessary to require the signers of each inner transaction to also sign the batch. Once a person signs a transaction to send you an NFT, they have already agreed to the action. If they want added protection, they should sign an atomic transaction instead.

Requiring signers of each inner transaction to also sign the batch implies that the context of the transaction is crucial for execution, rather than the signature itself.

When someone signs a transaction, they individually agree to the outcome. Batch transactions are not designed for retail users but rather for developers, dApps, etc.

mgarbs and others added 13 commits October 11, 2023 13:00
Co-authored-by: Nick Poorman <[email protected]>
Signed-off-by: Michael Garber <[email protected]>
Co-authored-by: Nick Poorman <[email protected]>
Signed-off-by: Michael Garber <[email protected]>
Co-authored-by: Nick Poorman <[email protected]>
Signed-off-by: Michael Garber <[email protected]>
Co-authored-by: Nick Poorman <[email protected]>
Signed-off-by: Michael Garber <[email protected]>
Co-authored-by: Nick Poorman <[email protected]>
Signed-off-by: Michael Garber <[email protected]>
Co-authored-by: Nick Poorman <[email protected]>
Signed-off-by: Michael Garber <[email protected]>
Co-authored-by: Nick Poorman <[email protected]>
Signed-off-by: Michael Garber <[email protected]>
Co-authored-by: Nick Poorman <[email protected]>
Signed-off-by: Michael Garber <[email protected]>
Co-authored-by: Nick Poorman <[email protected]>
Signed-off-by: Michael Garber <[email protected]>
Co-authored-by: Nick Poorman <[email protected]>
Signed-off-by: Michael Garber <[email protected]>
Co-authored-by: Nick Poorman <[email protected]>
Signed-off-by: Michael Garber <[email protected]>
Co-authored-by: Nick Poorman <[email protected]>
Signed-off-by: Michael Garber <[email protected]>
@mgarbs mgarbs merged commit 169753b into hashgraph:main Oct 11, 2023
kimbor pushed a commit to kimbor/hedera-improvement-proposal that referenced this pull request Jun 24, 2024
Signed-off-by: Michael Garber <[email protected]>
Signed-off-by: Piotr Swierzy <[email protected]>
Co-authored-by: Michael Garber <[email protected]>
Co-authored-by: Nick Poorman <[email protected]>
Signed-off-by: Kim Rader <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.