-
Notifications
You must be signed in to change notification settings - Fork 111
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
HIP-1056 Add block streams design #9716
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Edwin Greene <[email protected]>
### Block protobuf to mirror node database mapping | ||
|
||
#### Contract Create Transaction | ||
|
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.
More details on the missing entries still to come.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9716 +/- ##
============================================
- Coverage 92.32% 92.26% -0.06%
- Complexity 7413 7771 +358
============================================
Files 911 951 +40
Lines 31050 32472 +1422
Branches 3978 4118 +140
============================================
+ Hits 28667 29961 +1294
- Misses 1450 1546 +96
- Partials 933 965 +32 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Edwin Greene <[email protected]>
Signed-off-by: Edwin Greene <[email protected]>
Signed-off-by: Edwin Greene <[email protected]>
Signed-off-by: Edwin Greene <[email protected]>
Signed-off-by: Edwin Greene <[email protected]>
Signed-off-by: Edwin Greene <[email protected]>
Signed-off-by: Edwin Greene <[email protected]>
Signed-off-by: Edwin Greene <[email protected]>
Signed-off-by: Edwin Greene <[email protected]>
Signed-off-by: Edwin Greene <[email protected]>
``` | ||
|
||
### Database | ||
|
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.
Currently it is expected that there is a single round
per block
. The round
has a round_number
value that currently equals the block number. In the future multiple rounds could be included in a single block. I am not sure if we want to capture this somewhere.
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.
Rounds are sequential right? If so, can future proof by adding round_start, round_end columns.
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.
Sounds good, I have added them.
docs/design/block-streams.md
Outdated
|
||
## REST API | ||
|
||
-Deprecate the REST API's State Proof Alpha. The record files and signature files will no longer be provided in the cloud buckets. |
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.
-Deprecate the REST API's State Proof Alpha. The record files and signature files will no longer be provided in the cloud buckets. | |
- Deprecate the REST API's State Proof Alpha. The record files and signature files will no longer be provided in the cloud buckets. |
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.
Fixed
|
||
### Data Flow | ||
|
||
![Data Flow](images/blockstream.png) |
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.
There's a StreamFileNotifier in between.
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.
Thanks I have added it
docs/design/block-streams.md
Outdated
```java | ||
package com.hedera.mirror.importer.downloader.block; | ||
|
||
public class BlockStreamVerifier implements StreamFileNotifier, Closable { |
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 StreamFileNotifier
is an existing component to batch and send stream files. The BlockStreamVerifier
should leverage it, not implement it.
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.
Got it, I have updated that part.
docs/design/block-streams.md
Outdated
public abstract class StreamPoller<StreamFile> { | ||
public abstract void poll(); |
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.
Should prefer interfaces
public abstract class StreamPoller<StreamFile> { | |
public abstract void poll(); | |
public interface StreamPoller<StreamFile> { | |
void poll(); |
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.
Sounds good, I have updated it to an interface.
```java | ||
package com.hedera.mirror.common.domain.transaction; | ||
|
||
public class BlockItem implements StreamItem { |
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.
I assume this won't do any parsing like RecordItem currently does and instead just contain the Transaction, results, output, and state changes? Maybe we can show those fields and make it a simple record?
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.
It corresponds to the protobuf BlockItem. (Maybe it should be removed in favor of just using the protobuf class directly). The block item will only have an eventTransaction
or a transactionOutput
or one of the other types.
``` | ||
|
||
### Database | ||
|
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.
Rounds are sequential right? If so, can future proof by adding round_start, round_end columns.
docs/design/block-streams.md
Outdated
```java | ||
package com.hedera.mirror.importer.downloader.block; | ||
|
||
public class BlockStreamVerifier implements StreamFileNotifier, Closable { |
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.
Closable is misspelled. But why is it needed? This should be a singleton bean.
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.
Thanks I have removed that part, it will not be needed if StreamFileNotifier is used rather than implemented.
docs/design/block-streams.md
Outdated
| name | Name of the blk file | | ||
| node_id | | | ||
| prev_hash | block_header.previous_block_hash | | ||
| sidecar_count | Count of all transaction*output.\_transaction type*.sidecars in the blk file | |
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.
Should be zero. Sidecars should go away.
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.
Got it I have updated it with a note.
docs/design/block-streams.md
Outdated
| prev_hash | block_header.previous_block_hash | | ||
| sidecar_count | Count of all transaction*output.\_transaction type*.sidecars in the blk file | | ||
| size | byte size of the blk file | | ||
| version | Record stream version - Perhaps a new version should be added to indicate "Translated from Block" | |
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.
Just assume it's 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.
Sounds good I have updated this to 7
docs/design/block-streams.md
Outdated
| Database | Block Item | | ||
| -------------- | ------------------------------------------------------ | | ||
| entity.id | state_changes[i].state_change.map_update.value.topicId | | ||
| entity.deleted | state_changes[i].state_change.map_update.value.deleted | |
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.
This value doesn't exist. This should be true i map_delete present or false for any other update.
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.
Thanks for spotting, I have updated it to map_delete/key/value
Signed-off-by: Edwin Greene <[email protected]>
Quality Gate passedIssues Measures |
Description:
Adds the design for phase one, support for block stream files.
Related issue(s):
Fixes #9573
Notes for reviewer:
Checklist