-
Notifications
You must be signed in to change notification settings - Fork 4
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
feat: simulator generation mode improvements #682
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: georgi-l95 <[email protected]>
Signed-off-by: georgi-l95 <[email protected]>
Signed-off-by: georgi-l95 <[email protected]>
Signed-off-by: georgi-l95 <[email protected]>
Signed-off-by: georgi-l95 <[email protected]>
Signed-off-by: georgi-l95 <[email protected]>
Signed-off-by: georgi-l95 <[email protected]>
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.
Looks good
import com.hedera.block.common.hasher.Hashes; | ||
import com.hedera.block.common.hasher.HashingUtilities; | ||
import com.hedera.block.common.hasher.NaiveStreamingTreeHasher; | ||
import com.hedera.block.common.hasher.StreamingTreeHasher; |
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.
Hey Georgi, we talked about this in a DM. We'll need to talk through whether the Hashing really belongs in the common
module since it forces the Simulator to use PBJ types. I'm bringing it up for visibility. We can fix it with a future PR if the team wants to remove the dep here. Feel free to resolve this comment to merge.
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.
Yes, thank you for bringing attetion to this, I was not aware that the PBJ types was dep non grata
in common
module at the time. we can discuss how to decouple the hasher from PBJ or maybe just create new hasher algorithm for simulator and return this one to server
there are many alternatives to consider and at the time, I appreciate that you don't consider this to be a blocker 🙏 👍 💯
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 support creating a different hasher for the simulator and moving the current hasher back to server.
It seemed a good choice, at the time, to share the code, but it's looking like we are better off keeping the two systems more separated in this specific case.
Thanks for the suggestions, @AlfredoG87.
@@ -0,0 +1,27 @@ | |||
// SPDX-License-Identifier: Apache-2.0 | |||
package com.hedera.block.simulator.generator.itemhandler; |
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 looks like all the classes in this package are helpers to deal with the PBJ transitive dep. Hopefully we can figure out a way to remove that dep since you already have parsed the full object via protoc
. We shouldn't have to use classes like *Unparsed
@ConfigProperty(defaultValue = "") String folderRootPath, | ||
@ConfigProperty(defaultValue = "BlockAsFileBlockStreamManager") String managerImplementation, | ||
@ConfigProperty(defaultValue = "36") int paddedLength, | ||
@ConfigProperty(defaultValue = ".blk.gz") String fileExtension, | ||
// Optional block number range for the BlockAsFileLargeDataSets manager | ||
@ConfigProperty(defaultValue = "1") @Min(1) int startBlockNumber, | ||
@ConfigProperty(defaultValue = "1") @Min(0) int startBlockNumber, |
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.
Good catch on this min value
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.
Where did we land on this?
I recall that blocks were supposed to always start at 1
for genesis, and if a client requests a block range it needs to run from 1 to n (where if n is 0 then it means stream forever).
Did we find that the genesis block is numbered 0
?
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 is great work!! thank you @georgi-l95 💯
note: all comments are just comments and feel free to resolve them.
import com.hedera.block.common.hasher.Hashes; | ||
import com.hedera.block.common.hasher.HashingUtilities; | ||
import com.hedera.block.common.hasher.NaiveStreamingTreeHasher; | ||
import com.hedera.block.common.hasher.StreamingTreeHasher; |
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.
Yes, thank you for bringing attetion to this, I was not aware that the PBJ types was dep non grata
in common
module at the time. we can discuss how to decouple the hasher from PBJ or maybe just create new hasher algorithm for simulator and return this one to server
there are many alternatives to consider and at the time, I appreciate that you don't consider this to be a blocker 🙏 👍 💯
if ("BlockAsDirBlockStreamManager".equalsIgnoreCase(managerImpl)) { | ||
yield new BlockAsDirBlockStreamManager(config); |
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 might consider to get rid of this implementation in the future to reduce maintenance footprint, I mean, if is no longer needed.
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 simulator may find benefit from having the dir option.
For instance, we might want to have an input folder with blocks (in block as dir) as a source and appreciate the ability to swap out individual block items without needing to rewrite entire blocks (or make changes on the fly). The option to use hard links for the parts that don't change is also valuable, at least to some extent.
That said, if it's costing a lot of maintenance, it's fine to remove now, and we can always restore the code or rewrite if it's worthwhile.
.setRealmNum(0) | ||
.setShardNum(0) |
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.
Could we add a story to support non-zero (and non-static) shard and realm in a future sprint?
Consensus is already adding this, and it would be nice to be able to test with the simulator.
new ConfigMapping("generator.minNumberOfEventsPerBlock", "GENERATOR_MIN_NUMBER_OF_EVENTS_PER_BLOCK"), | ||
new ConfigMapping("generator.maxNumberOfEventsPerBlock", "GENERATOR_MAX_NUMBER_OF_EVENTS_PER_BLOCK"), | ||
new ConfigMapping( | ||
"generator.minNumberOfTransactionsPerEvent", "GENERATOR_MIN_NUMBER_OF_TRANSACTIONS_PER_EVENT"), | ||
new ConfigMapping( | ||
"generator.maxNumberOfTransactionsPerEvent", "GENERATOR_MAX_NUMBER_OF_TRANSACTIONS_PER_EVENT"), |
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.
Would it makes sense to elide the NUMBER_OF_
part of these names?
Description
With this PR we aim to introduce new generator mode for the block stream, by introducing new implementation for the
BlockStreamManager
. The following changes are included in this pull request:CraftBlockStreamManager
implementation with the purpose of crafting blocks on the fly and on demand. Currently we are adding random amount of items(transactions) in the block, according to the defined configuration.ADHOC
mode inGenerationMode
renamed toCRAFT
, to better explain the purpose and function of this mode.BlockHeaderHandler
responsible for crafting block header for each new block with needed fields.BlockProofHandler
responsible for crafting block proof with adequte signature(for now) to pass verification.EventHeader
andEventTransaction
to represent the start of each new event and it's respective transctions.TransactionResult
to represent the result of the transactions included in the block. For now we've included only HBARTransferList
and fungibleTokenTransferList
as result with randomly generatedAccountAmounts
.Related Issue(s)
Fixes #502