-
Notifications
You must be signed in to change notification settings - Fork 24
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
Add initial specification for block stream data. #342
base: main
Are you sure you want to change the base?
Conversation
ab5cec7
to
c49a233
Compare
This draft PR is not intended to be merged. This is a vehicle to gather more detailed and extensive feedback and review, which will then be incorporated into a future PR with a much wider audience. |
7c15751
to
af0ddd0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1st pass comments.
Thanks for the cleanup and I like the idea to add MD files
* The block node SHALL send one `ItemAcknowledgement` for each `BlockItem` | ||
* received and verified.<br/> | ||
*/ | ||
message ItemAcknowledgement { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: What's the use case for an item level acknowledgement?
Doesn't a block contain multiple BlockItems? So shouldn't a single Block acknowledgement be sufficient?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are sending individual block items in a stream, so logically the acknowledgement should be per item. If we wait to only acknowledge the entire block, then both sides are required to retain the full block in memory until it's fully sent, and that imposes costs and requirements we may not want to impose in all cases.
We should also have block level acknowledgement, but that should not be the only acknowledgement.
* <li>Any transfers caused by the creation of threshold records</li> | ||
* </ul> | ||
*/ | ||
proto.TransferList transfer_list = 7; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
transfer_list
seems like it would be better placed in a general TransactionOutput
Same for token_transfer_lists
and automatic_token_associations
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only issue I see with the transfer lists is that it's present for every transaction, as that's where charged fees are recorded. Now, with state changes in separate block items, it's possible we won't have that (though certain huge accounts will be in every transaction state changes instead, which has its own issues in the short term).
This is something that was undecided at the last discussion I was invited to.
* >> TokenTransfer output would also need custom fees, and we may wish | ||
* >> to add custom fees to other transactions in the future. | ||
*/ | ||
message TransactionResult { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TransactionOutput
vs TransactionResult
.
It's not clear what each should contain.
TransactionResult
feels like it's transaction metadata for the networks point at the time of processing.
TransactionOutput
feels like it's newly created state as a result of the transaction that is not in the body.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neither should be state, as we have state changes in separate structure.
The rest is design that's in flux. This PR is far from final; it's just a step along the road to get to a final specification.
It is entirely possible that all, or nearly all, of TransactionOutput
is subsumed in StateChanges
and no longer relevant.
### Block | ||
A single complete Hedera block chain block. | ||
|
||
This is a single block structure and SHALL NOT represent the primary |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the goal here saying BlockItems
are streamed not Block
objects.
So to a client streaming they wouldn't know they have a complete block without the BlockHeader
and BlockStateProof
delineators?
Is the reasoning size or something else?
The impact is clients need to keep track of the separator to create a meaningful Block aggregation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely.
The stream is individual block items, not entire blocks.
Jasper has a good description for why we chose that approach, but it comes down to efficiency and resilience, for the most part.
A client that is streaming will only handle full blocks if the client chooses to do so, but if it does it will require both header and proof to delimit the block.
Clients that handle full blocks need to handle validating the block proof and may need to maintain an entire mirror state as well (if it's a stateful node), so tracking when a block starts and ends isn't hard in context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note, for "casual" clients, who only want full blocks, the expectation is that the client will just request a single block at a time from a block node, rather than requesting a stream from the block node.
(We expect push streams to be restricted to specific consensus nodes sending to specific block nodes, at least initially).
* Block node implementations SHOULD charge increased fees for such | ||
* "future" streams. | ||
*/ | ||
uint64 end_block_number = 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should consider adding a limit field. A subscriber would define one or the other if at all
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would a limit look like, if not the block number at which to end the stream?
block/block_service.proto
Outdated
* The reported status SHALL reflect the success of the request, or | ||
* a detailed reason the request failed. | ||
*/ | ||
SingleBlockResponse.ResponseCode status = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In SubscribeStreamResponse
you had a oneof
here. Should the two rsponses be similar in this case - either a oneof
or a separation of fields
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stream responses are sent many times for a single request, and there are a couple different response types possible because we need to send data items and result codes, but cannot join the two together (they're sent at different times).
The Single block response is sent exactly once per request, and has only one possible response type which includes both data and result code.
* of the request as possible in the event payment is not sufficient to | ||
* complete the request. | ||
*/ | ||
rpc subscribeBlockStream(stream SubscribeStreamRequest) returns (stream SubscribeStreamResponse); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: why a bi-directional stream for the subscribe?
Would a client make a single request and receive the stream, why would it keep sending a SubscribeStreamRequest
if blocks are delivered in order?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The subscriber is receiving BlockItem
s, not Block
s and, though design is not complete for this, will likely need to acknowledge items as they're received. Also, a requester might request blocks 1-5, then request 6-9, and so forth, continuing a stream as long as needed.
Streams aren't sent all at once, and can last a rather long (perhaps hours) time.
* the latest available, but MUST clearly document this behavior. | ||
*/ | ||
rpc stateSnapshot(StateSnapshotRequest) returns (StateSnapshotResponse); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: We probably need an API here to request a single item or subtree from "current" state (do we need "historical state"?).
Likely also need an API for "how big is block stream from block G to block Y".
20f1c85
to
2ad0221
Compare
4a16bf1
to
5028483
Compare
c9bc6df
to
efdafef
Compare
* add block stream protos * add new lines at the end of files * add v7 to package path * use releative path for proto imports * use releative path for proto imports * use releative path for proto imports * comment out empty proto messages * change the property to match for other token calls * Move all Block level props into BlockItem so it can be streamed. Add rpc for blocksPutIfAbsent. * Import BlockHeader into BlockItem * Import BlockStateProof into BlockItem * Fixed imports * Maybe if they are in a different file? * That didn't work. Going to have to look at the template code * I suspect it's because we don't have any streaming requests yet * Let's try making it the same as the others * Found an issue saying maybe change the file name * Adding to java_multiple_files option to see if that works * Needs to be a stream of BlockItems * Fixed typo for running hashes * Updated streams v7 protos from working notion doc * Updated streams v7 protos from working notion doc * Added start running hash to the BlockHeader * Added versions of block, hapi, services, and platform to the BlockHeader * Extracted sidecar output to match current outputs which are multiple sidecars * Using the already created TransactionSidecarRecord * Added SystemTransaction to BlockItem * Renaming System transaction messages to avoid collision * Fix StateSignatureSystemTransaction to use remove duplicate hash value * Optomized by including BlockHashAlgorithm and BlockSignatureAlgorithm in the BlockHeader for the whole block * Optomized by removing SignatureObject and replacing with hash and signature bytes property * Guess protobuf doesn't like having the same enum values * Fixed comment for birth_round to be accurate * Updated state change protos to surface state change block items * Add the delete operations * Fix numbers * Change type to plural version * Prefix enum values * We seem to be serializing string values in tests * Need to verify if AccountID should be a key type or not. Putting it in for now so I can get tests passing * Need to verify if AccountID should be a key type or not. Putting it in for now so I can get tests passing * Worked out all the protos needed for various state changes * import exchange_rate.proto * import recordcache.proto * add ProtoString to store types * fix typo * Make slot_value consistent * Fix QueuePushChange types * fixed change_operation * Switched to using element instead of value for the elements in a queue * Add missing type to SingletonUpdateChange * Add missing type to SingletonUpdateChange * Add missing type to SingletonUpdateChange * Add missing type to SingletonUpdateChange * Add the round which the state proof was constructed for * Working on block node API design * Cleaned it up Signed-off-by: Nick Poorman <nick@swirldslabs.com>
* Corrected bad copy/paste of copyright header * Added file prefix comment with keyword section. * Adjusted names to remove redundant suffix * Moved files from `streams/v7` to `block/stream`. * Commented out unused imports * Fixed java_package and pbj.java_package to both be `com.hedera.hapi.block.stream`. There is no legacy code that needs a different package for block stream. * Changed package directive to `com.hedera.hapi.block.stream` as well. * "main" messages prefixed with `proto` due to poor package choice in legacy design. Hopefully we can fix that soon. * Started on service outputs * `consensus_service.proto` * `util_service.proto` * `crypto_service.proto` * `file_service.proto` * `network_service.proto` -- renamed from misc_output.proto * `schedule_service.proto` * `smart_contract_Service.proto` * `token_service.proto` -- Much of this is yet to be designed * Cleaned up related items * `transaction_result.proto` --- Adjust and progress the design. * Updated proto design and documentation * `state_changes.proto` * `system_transaction.proto` * `block.proto` * `block_item.proto` * `block_header.proto` * `block_state_proof.proto` * `block_service.proto` * Added `block_info_state_proof.proto` for the block info state proof which is intended to replace the running hashes state proof in `block_state_proof.proto`. * Old version removed and new version renamed to `block_state_proof.proto` after discussion on 2024-04-11. * Changed most int64 values to uint64 as they are never permitted to be negative. * Changed one int32 value to uint32 as it is never negative. * Removed substantially everything from services outputs * These are in state changes, and outputs should not include anything from state changes. * Added a server status request to `block_service.proto`. * This proivides clients a mechanism to learn what blocks are available on a block node, whether the node offers historical snapshots, and the current price list for that block node server. --- Work In Progress * Updates to design _and_ other details * `transaction_output` * Added three options for communicating "sibling" hash _order_ in the state proof (`block_state_proof.proto`). * Rework of `event_metdata` * Platform is creating protobufs for events, and we need to reuse those here as they are completed. --- Temporary Changes for review purposes * Committed generated documentation for review purposes Signed-off-by: Joseph Sinclair <joseph.sinclair@swirldslabs.com>
Block service payment and authentication are separate from the block service API and not normative. We will define a _recommended_ authentication and payment process for a block node separately from the block stream specifications. Signed-off-by: Joseph Sinclair <joseph.sinclair@swirldslabs.com>
* Cleaned up `transaction_output.proto`. * Removed all currently unused options. * Adjusted ordering for better efficiency and clarity. * Reserved field numbers of all remaining transaction types for future use. * Documented, in an HTML comment, field types, names, and numbers for all remaining outputs. * Cleaned up remaining items in several other block stream files. * This is a general "editing" update to clean up and clarify the text. This may include small structural changes for both clarity and data efficiency. Signed-off-by: Joseph Sinclair <joseph.sinclair@swirldslabs.com>
* Modified all copyright notices to remove unnecessary text and match general guidelines. * Fixed a few items noted in offline review prior to DevCon-24a * Reformat to match current guidelines for line length and field descriptions. Signed-off-by: Joseph Sinclair <joseph.sinclair@swirldslabs.com>
* Moved "input" item to "input" folder and package. * Moved "output" items to "output" folder and package. * Specified the "merkle" approach to generating the block hash in the block header. * Added Jasper's diagram for this process in documents. Signed-off-by: Joseph Sinclair <joseph.sinclair@swirldslabs.com>
* Updated block header and items with review changes * Updated state changes to support add and remove for named states * Updated block service with review changes * Major rebuild of block state proof to work with both the "block merkle tree" concept and a single TSS-BLS signature instead of 30+ individual RSA signatures. Signed-off-by: Joseph Sinclair <joseph.sinclair@swirldslabs.com>
efdafef
to
ef61ded
Compare
Update specification to current standards
streams/v7
toblock/stream
.com.hedera.hapi.block.stream
. There is no legacy codethat needs a different package for block stream.
com.hedera.hapi.block.stream
as well.proto
due to poor package choice in legacydesign. Hopefully we can fix that soon.
consensus_service.proto
util_service.proto
crypto_service.proto
file_service.proto
network_service.proto
-- renamed from misc_output.protoschedule_service.proto
smart_contract_Service.proto
token_service.proto
-- Much of this is yet to be designedtransaction_result.proto
transaction_output.proto
.for future use.
for all remaining outputs.
the text. This may include small structural changes for
both clarity and data efficiency.
Adjust and progress the design.
state_changes.proto
system_transaction.proto
block.proto
block_item.proto
block_header.proto
block_state_proof.proto
block_service.proto
block_info_state_proof.proto
for the block info state proofwhich is intended to replace the running hashes state proof in
block_state_proof.proto
.block_state_proof.proto
after discussion on 2024-04-11.
permitted to be negative.
include anything from state changes.
block_service.proto
.available on a block node, whether the node offers
historical snapshots, and the current price list for that
block node server.
Remove payment from block_service.proto.
and not normative. We will define a recommended authentication and payment
process for a block node separately from the block stream specifications.
Non-Normative Documents
documents/api/block/
folder for ease of reviewfrom the codebase before merging this PR.
Work completed by Nick Poorman
Document Block Node Messages.