MX-17306 Tooling for incorrect cross shard execution#7419
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feat/testnet-fixes #7419 +/- ##
===================================================
Coverage 77.68% 77.68%
===================================================
Files 885 885
Lines 125904 125916 +12
===================================================
+ Hits 97809 97822 +13
Misses 21613 21613
+ Partials 6482 6481 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
mradian1
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR adds enhanced logging capabilities for debugging cross-shard miniblock execution by logging header details at key consensus stages using the PrettifyStruct utility.
Key changes:
- Added debug logging for headers when they are proposed, received, and committed across BLS consensus versions (v1 and v2)
- Added comprehensive test coverage for
PrettifyStructwith HeaderV3 and MetaBlockV3 types - Created mock helper functions to support the new tests
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| consensus/spos/bls/v2/subroundEndRound.go | Logs header details when a block is finalized and committed |
| consensus/spos/bls/v1/subroundEndRound.go | Logs header details when blocks are committed by both leader and participant nodes |
| consensus/spos/bls/v1/subroundBlock.go | Logs header details when blocks are sent and received during consensus |
| common/common_test.go | Adds test cases for HeaderV3/MetaBlockV3 and helper functions to create mock headers |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| NotarizedInRound: hdrRound - 1, | ||
| }, | ||
| ExecutionResults: []*block.ExecutionResult{ | ||
| &block.ExecutionResult{ |
There was a problem hiding this comment.
The code uses the unidiomatic composite literal syntax &block.ExecutionResult{...} instead of {...}. When the slice element type is already a pointer ([]*block.ExecutionResult), you should omit the & operator. This applies to all three entries in the slice (lines 848, 857, 866).
|
|
||
| meta := createMockMetaHeaderV3() | ||
| hdr = meta | ||
| prettified, err = common.PrettifyStruct(meta) |
There was a problem hiding this comment.
Inconsistency in the variable passed to PrettifyStruct. Line 510 assigns meta to hdr, but line 511 passes meta directly instead of using hdr. For consistency with the pattern used for the shard header (lines 504-505), this should be common.PrettifyStruct(hdr).
| prettified, err = common.PrettifyStruct(meta) | |
| prettified, err = common.PrettifyStruct(hdr) |
axenteoctavian
left a comment
axenteoctavian
left a comment
There was a problem hiding this comment.
- Copilot suggestions
…cross-shard-execution-analysis
mariusmihaic
left a comment
mariusmihaic
left a comment
There was a problem hiding this comment.
Is this extra log intended/expected to be here forever?
All this extra data will increase the log size pretty much
| PrevHash: []byte("prev hash"), | ||
| PrevRandSeed: []byte("prev rand seed"), | ||
| RandSeed: []byte("rand seed"), | ||
| ShardID: hdrShardId, |
There was a problem hiding this comment.
You could use "1" here directly, without extra variable declaration
| headerOutput, err := common.PrettifyStruct(headerHandler) | ||
| if err != nil { | ||
| log.Error("Proposed header sent v1", "error", err) | ||
| } else { | ||
| log.Debug("Proposed header sent v1", "header", headerOutput) | ||
| } |
There was a problem hiding this comment.
- Perhaps we should make a func here with input param being the
message string - This should not be logged with log.Error, since it is not critical for debugging. Debug level should work too
There was a problem hiding this comment.
done, added a new method in common to log a header
| // log the header output for debugging purposes | ||
| common.LogPrettifiedHeader(sr.GetHeader(), "committed", "v2") |
There was a problem hiding this comment.
this will print the same header info as in subroundBlock
There was a problem hiding this comment.
or this was intended for some extra tool?
There was a problem hiding this comment.
this will be used by our logs checker for alarms trigger for issues on committed blocks during consensus, not on the failed consensuses (can generate false positives otherwise).
|
✅ Integration Tests passed successfully! 📊 MultiversX Automated Test Report: View Report 🔄 Build Details:
🚀 Environment Variables:
|
| // LogPrettifiedHeader logs the prettified representation of the provided header or an error if prettification fails | ||
| func LogPrettifiedHeader(header data.HeaderHandler, sentOrReceived string, version string, configsHandler CommonConfigsHandler) { | ||
| if !configsHandler.PrintPrettifiedHeader() { | ||
| return | ||
| } | ||
|
|
||
| headerOutput, err := PrettifyStruct(header) | ||
| message := fmt.Sprintf("Proposed header %s %s", sentOrReceived, version) | ||
| if err != nil { | ||
| log.Debug(message, "error", err) | ||
| } else { | ||
| log.Debug(message, "header", headerOutput) | ||
| } | ||
| } |
Reasoning behind the pull request
Using the prettify block logging, improve the logs analysis tool to validate that cross shard miniblocks are executed (and proposed) in strict order, without gaps or duplications.
Proposed changes
Testing procedure
Pre-requisites
Based on the Contributing Guidelines the PR author and the reviewers must check the following requirements are met:
featbranch created?featbranch merging, do all satellite projects have a proper tag insidego.mod?