-
Notifications
You must be signed in to change notification settings - Fork 204
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
Fixes outport refactor #5135
Fixes outport refactor #5135
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## feat/outport-refactor #5135 +/- ##
======================================================
Coverage 70.81% 70.82%
======================================================
Files 675 675
Lines 87822 87850 +28
======================================================
+ Hits 62193 62218 +25
- Misses 20939 20940 +1
- Partials 4690 4692 +2
... and 3 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
@@ -173,7 +173,7 @@ func (s *sorter) getInvalidTxsExecutedInCurrentBlock(scheduledMbsFromPreviousBlo | |||
for _, hash := range mb.TxHashes { | |||
_, found := allScheduledTxs[string(hash)] | |||
if found { | |||
scheduledExecutedInvalidTxsHashesPrevBlock = append(scheduledExecutedInvalidTxsHashesPrevBlock, string(hash)) | |||
scheduledExecutedInvalidTxsHashesPrevBlock = append(scheduledExecutedInvalidTxsHashesPrevBlock, hex.EncodeToString(hash)) |
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 this still backward compatible?
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 is not a problem. because this part is used only in indexer
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.
agree, did not noticed that the package was actually outport...
@@ -275,9 +277,10 @@ func extractSCRsFromMap(txsHashes [][]byte, scrs map[string]*outport.SCRInfo) ([ | |||
func extractRewardsFromMap(txsHashes [][]byte, rewards map[string]*outport.RewardInfo) ([]data.TxWithExecutionOrderHandler, error) { | |||
result := make([]data.TxWithExecutionOrderHandler, 0, len(txsHashes)) | |||
for _, txHash := range txsHashes { | |||
reward, found := rewards[string(txHash)] | |||
txHashHex := hex.EncodeToString(txHash) |
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.
why do we keep the hash in hex format? This degrades the performance of the node as the hex.EncodeToString is more CPU intensive than the regular cast to string.
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 need the transaction hash hex-encoded in order can send it over to the WS.
If I will let it non hex I will have problems with the marshaller because the hash is used as a string key in map structures.
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.
agree, this does not degrade the performance of the validators as the code impacted is in outport...
package
go.mod
Outdated
@@ -13,9 +13,9 @@ require ( | |||
github.com/google/gops v0.3.18 | |||
github.com/gorilla/websocket v1.5.0 | |||
github.com/mitchellh/mapstructure v1.5.0 | |||
github.com/multiversx/mx-chain-core-go v1.2.1-0.20230322093158-35195fa155c0 | |||
github.com/multiversx/mx-chain-core-go v1.2.1-0.20230329082847-b78e96c3ad5a |
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 not the latest commit hash
I think this should be 932a718276f65ff8f820720adf5e98c4cfdd3074
.
Check here: multiversx/mx-chain-core-go#171
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
outport/notifier/eventNotifier.go
Outdated
@@ -46,10 +48,15 @@ func NewEventNotifier(args ArgsEventNotifier) (*eventNotifier, error) { | |||
if err != nil { | |||
return nil, err | |||
} | |||
blockContainer, err := createBlockCreatorsContainer() |
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.
Please move it at the same level as factory: CreateEventNotifier
, just like you did on indexer
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.
done
Reasoning behind the pull request
Pre-requisites
Based on the Contributing Guidelines the PR author and the reviewers must check the following requirements are met:
feat
branch created?feat
branch merging, do all satellite projects have a proper tag insidego.mod
?