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

Update hip-551.md #1090

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

ty-swirldslabs
Copy link
Contributor

Add new updates to HIP-551 after implementation discussions

Description:
Updated information and implementation description into HIP-551

Add new updates to HIP-551 after implementation discussions
Copy link

netlify bot commented Dec 19, 2024

Deploy Preview for hedera-hips ready!

Name Link
🔨 Latest commit bf64a9f
🔍 Latest deploy log https://app.netlify.com/sites/hedera-hips/deploys/67a63bdc47c5b900085d4777
😎 Deploy Preview https://deploy-preview-1090--hedera-hips.netlify.app
📱 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.

@mgarbs mgarbs requested a review from netopyr January 7, 2025 17:08
HIP/hip-551.md Outdated Show resolved Hide resolved
@mgarbs mgarbs requested a review from SimiHunjan January 23, 2025 00:14
@mgarbs
Copy link
Collaborator

mgarbs commented Feb 3, 2025

Hi team,

Hope you're all doing well! Just wanted to follow up on PR #Update hip-551.md. It's fantastic to see the engagement so far, thank you for all of your inputs.

There are still 3 unresolved discussions that require your perspectives. The insights which each of you hold are extremely valuable to shaping this project. If you could find some time to go through these discussions and share your views, that would be highly appreciated.

Thanks again and looking forward to your inputs!

Best,
Michael
@SimiHunjan - There's an unresolved discussion from 2025-01-17. Could you please take a look and either resolve or provide additional context? Link to discussion

@SimiHunjan - There's an unresolved discussion from 2025-01-17. Could you please take a look and either resolve or provide additional context? Link to discussion

@SimiHunjan
Copy link
Contributor

I resolved the conversations. Thanks for the follow-up!

kimbor and others added 3 commits February 4, 2025 15:55
- The batch transaction will be processed as a single unit. If any inner transaction fails, the entire batch will fail.
- The batch transaction will return a single transaction response as usual. In general, inner transactions are not checked during pre-check.
- The batch transaction and all executed inner transactions create TransactionReceipts. In order to see response codes for inner transactions, the user must query the individual inner transactions receipts.
- Transaction records for inner transactions will have the `parentConsensusTimestamp` set to the `consensusTimestamp` of the containing AtomicBatch transaction. The `parentConsensusTimestamp` can be used to correlate all transaction records for the batch. Block streams are handled similarly.
Copy link
Contributor

Choose a reason for hiding this comment

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

It isn't clear to me how a transaction record is accessed for an inner transaction. I don't see any protobuf updates in this design to support this. Currently there is only a single transaction record per stream item

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure if I understand the question.

4. Are deduplicated on an individual basis.
5. Are authorized solely by their keys; batch keys are not used.
6. If one of the inner transactions has a child transaction, the parent transaction is in the batch, and the batch isn't successful, then the child transaction should also be reverted.
4. Are deduplicated on an individual basis. If two inner transactions within the same batch have the same transactionID, the transaction will be rejected.
Copy link
Member

Choose a reason for hiding this comment

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

A TransactionID has many components. To be clear, they each should provide a unique combination of accountID plus transactionValidStart? They should still continue to set scheduled=false and nonce=0, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. Inner transaction are regular transactions with the only difference that the batchKey is set and nodeAccountID is 0.0.0.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can add a short comment to the HIP to clarify that scheduled=false and nonce=0 for all inner transactions.

5. Are authorized solely by their keys; batch keys are not used for authorization.
6. Cannot be batch transactions. A batch transactions cannot contain another batch transaction.
7. Cannot be network freeze transactions, but may be other privileged transactions.
8. Must have nodeAccountID set to 0.0.0. This acts as a marker to indicate that an inner transaction is part of a batch, and it allows resubmitting a batch transaction without the need to re-sign the inner transactions.
Copy link
Member

@steven-sheehy steven-sheehy Feb 12, 2025

Choose a reason for hiding this comment

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

This will be troublesome or at least confusing for non-zero realms. Can't we just say that its nodeAccountID should not be set instead of using some sentinel value?

Copy link
Contributor

Choose a reason for hiding this comment

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

Why? It should never be used (except for checking that it is set to 0.0.0.


The batch:
The AtomicBatch outer transaction:

1. Has its own payer and signatures.
2. Considers inner transactions as part of its signed bytes.
3. Makes the batch payer cover node+network fees for handling the batch, excluding inner transaction fees.
4. Has deduplication based on its unique transaction ID.
Copy link
Member

Choose a reason for hiding this comment

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

So there's no relation between the TransactionID of the outer and its inners? So on hashscan or mirror node API if I look up a transaction by its TransactionID is it expected that only the single inner or outer transaction that it refers to shows up? Or should it somehow show all?

- The batch transaction will be processed as a single unit. If any inner transaction fails, the entire batch will fail.
- The batch transaction will return a single transaction response as usual. In general, inner transactions are not checked during pre-check.
- The batch transaction and all executed inner transactions create TransactionReceipts. In order to see response codes for inner transactions, the user must query the individual inner transactions receipts.
- Transaction records for inner transactions will have the `parentConsensusTimestamp` set to the `consensusTimestamp` of the containing AtomicBatch transaction. The `parentConsensusTimestamp` can be used to correlate all transaction records for the batch. Block streams are handled similarly.
Copy link
Member

@steven-sheehy steven-sheehy Feb 12, 2025

Choose a reason for hiding this comment

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

It's still not quite clear. So will each outer and inner be assigned a distinct consensus timestamp? And will each inner consensus timestamp each be offset by one nanosecond from the outer just like regular child transactions? What will occur if inner transaction 3 has 10 synthetic child transactions and inner 5 has 2 synthetic children?

Copy link
Contributor

Choose a reason for hiding this comment

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

The consensus timestamp is counted up for all transactions as they appear in the stream. The hierarchy has no influence. If inner transaction 3 has nanosecond n, then the 10 synthetic child transactions will have n+1...n+10, and the next inner transaction 4 has n+11.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for clarifying, but I meant these sort of details should be in the HIP since this is a new concept of having inners with children.

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.

7 participants