Fix error from `CalculateNonceAndVUB` #3134

End-rey · 2025-02-14T19:01:25Z

The alphabetic key change event comes from the main chain with its hash, and when calculating vub, an error is returned because there is no hash in the fs chain. Ignore the error and calculate the value in another way.

codecov · 2025-02-14T19:03:34Z

Codecov Report

Attention: Patch coverage is 6.25000% with 15 lines in your changes missing coverage. Please review.

Project coverage is 23.03%. Comparing base (653d9f8) to head (0ddb375).
Report is 12 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/morph/client/notary.go	0.00%	8 Missing ⚠️
pkg/innerring/state.go	0.00%	7 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3134      +/-   ##
==========================================
- Coverage   23.04%   23.03%   -0.01%     
==========================================
  Files         756      756              
  Lines       60202    60212      +10     
==========================================
- Hits        13872    13870       -2     
- Misses      45334    45345      +11     
- Partials      996      997       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

roman-khimov

Can we somehow do this only for this special main chain case? The problem is that in the vast majority of cases the original error is more appropriate, there has to be a transaction. Maybe we can pass zero hash in this case and then fallback to this scheme without transaction height query?

pkg/morph/client/notary.go

carpawell

Agree that we need to separate this logic for events from main and FS chains. For FS chains it is a real problem if no TX found while for main it should not be even tried. TBH I even cannot say if we have any non-notary notifications from FS chain but it should not change anything, they may appear at any time, morph client was meant to be for every chain (at least it is now, without additional chages i would keep this).

pkg/morph/client/notary.go

cthulhu-rider · 2025-02-19T11:30:09Z

pkg/innerring/state.go

@@ -128,7 +128,11 @@ func (s *Server) voteForFSChainValidator(validators keys.PublicKeys, trigger *ut
 	if trigger != nil {
 		nonce, vub, err = s.morphClient.CalculateNonceAndVUB(*trigger)
 		if err != nil {
-			return fmt.Errorf("could not calculate nonce and `validUntilBlock` values: %w", err)
+			s.log.Debug("could not calculate nonce and `validUntilBlock` values", zap.Error(err))
+			nonce, vub, err = s.morphClient.CalculateNonceAndVUB(util.Uint256{})


commit states

... an error is returned because there is no hash in the FSchain. Ignore the error and calculate the value in another way

but we switch to zero hash on any failure. Shouldnt we check the exact case?

IIUC we should always call CalculateNonceAndVUB with util.Uint256{} in voteForFSChainValidator if trigger == nil, because the only user of this method (not counting validator predefined in config) is governance processor reacting to main chain events.

cthulhu-rider · 2025-02-19T11:30:41Z

pkg/innerring/state.go

@@ -128,7 +128,11 @@ func (s *Server) voteForFSChainValidator(validators keys.PublicKeys, trigger *ut
 	if trigger != nil {
 		nonce, vub, err = s.morphClient.CalculateNonceAndVUB(*trigger)
 		if err != nil {
-			return fmt.Errorf("could not calculate nonce and `validUntilBlock` values: %w", err)
+			s.log.Debug("could not calculate nonce and `validUntilBlock` values", zap.Error(err))


the hash can also be useful

cthulhu-rider · 2025-02-19T11:32:31Z

pkg/innerring/state.go

+			s.log.Debug("could not calculate nonce and `validUntilBlock` values", zap.Error(err))
+			nonce, vub, err = s.morphClient.CalculateNonceAndVUB(util.Uint256{})
+			if err != nil {
+				return fmt.Errorf("could not calculate nonce and `validUntilBlock` values for zero hash: %w", err)


how about wrapping both errors?

roman-khimov · 2025-02-19T14:23:29Z

pkg/innerring/state.go

@@ -128,7 +128,11 @@ func (s *Server) voteForFSChainValidator(validators keys.PublicKeys, trigger *ut
 	if trigger != nil {
 		nonce, vub, err = s.morphClient.CalculateNonceAndVUB(*trigger)
 		if err != nil {
-			return fmt.Errorf("could not calculate nonce and `validUntilBlock` values: %w", err)
+			s.log.Debug("could not calculate nonce and `validUntilBlock` values", zap.Error(err))
+			nonce, vub, err = s.morphClient.CalculateNonceAndVUB(util.Uint256{})


IIUC we should always call CalculateNonceAndVUB with util.Uint256{} in voteForFSChainValidator if trigger == nil, because the only user of this method (not counting validator predefined in config) is governance processor reacting to main chain events.

pkg/morph/client/notary.go

The alphabetic key change event comes from the main chain with its hash, and when calculating vub, an error is returned because there is no hash in the FS chain. So for designation event of the mainnet RoleManagement contract transaction hash is set to zero. `CalculateNonceAndVUB` can now handle zero hashes and calculate vub rounded. Signed-off-by: Andrey Butusov <[email protected]>

Signed-off-by: Andrey Butusov <[email protected]>

End-rey · 2025-02-19T18:36:47Z

Designation event of the mainnet RoleManagement contract contains a hash from the main chain, so I just made it zero, so that later it calculated vub as for the zero hash. Did I get it right?

roman-khimov · 2025-02-20T13:59:13Z

pkg/morph/event/rolemanagement/designate.go

@@ -40,6 +40,6 @@ func ParseDesignate(e *state.ContainedNotificationEvent) (event.Event, error) {

 	return Designate{
 		Role:   noderoles.Role(bi.Int64()),
-		TxHash: e.Container,
+		TxHash: util.Uint256{},


I'm not excited about this one because I'd expect this "real->fake" translation at some other layer dealing with the event. But it should work.

End-rey self-assigned this Feb 14, 2025

End-rey requested review from roman-khimov, carpawell and cthulhu-rider as code owners February 14, 2025 19:01

End-rey marked this pull request as draft February 14, 2025 19:01

End-rey force-pushed the 3113-designation-contract-changes-are-not-reflected-in-ir-configuration branch from 2257c3c to 8fa1b74 Compare February 17, 2025 18:11

End-rey marked this pull request as ready for review February 17, 2025 19:07

roman-khimov reviewed Feb 18, 2025

View reviewed changes

pkg/morph/client/notary.go Show resolved Hide resolved

pkg/morph/client/notary.go Outdated Show resolved Hide resolved

carpawell reviewed Feb 18, 2025

View reviewed changes

pkg/morph/client/notary.go Show resolved Hide resolved

End-rey force-pushed the 3113-designation-contract-changes-are-not-reflected-in-ir-configuration branch 2 times, most recently from 3aae148 to 435f64d Compare February 19, 2025 08:00

End-rey requested review from roman-khimov and carpawell February 19, 2025 10:20

cthulhu-rider reviewed Feb 19, 2025

View reviewed changes

roman-khimov reviewed Feb 19, 2025

View reviewed changes

End-rey force-pushed the 3113-designation-contract-changes-are-not-reflected-in-ir-configuration branch from 435f64d to b1cfbb4 Compare February 19, 2025 14:26

carpawell reviewed Feb 19, 2025

View reviewed changes

pkg/morph/client/notary.go Outdated Show resolved Hide resolved

End-rey added 2 commits February 19, 2025 21:29

morph: fix a typo in error

0ddb375

Signed-off-by: Andrey Butusov <[email protected]>

End-rey force-pushed the 3113-designation-contract-changes-are-not-reflected-in-ir-configuration branch from b1cfbb4 to 0ddb375 Compare February 19, 2025 18:32

End-rey requested review from roman-khimov, carpawell and cthulhu-rider February 19, 2025 19:03

carpawell approved these changes Feb 20, 2025

View reviewed changes

roman-khimov approved these changes Feb 20, 2025

View reviewed changes

roman-khimov merged commit d540de0 into master Feb 20, 2025
20 of 22 checks passed

roman-khimov deleted the 3113-designation-contract-changes-are-not-reflected-in-ir-configuration branch February 20, 2025 13:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix error from `CalculateNonceAndVUB` #3134

Fix error from `CalculateNonceAndVUB` #3134

End-rey commented Feb 14, 2025

codecov bot commented Feb 14, 2025 •

edited

Loading

roman-khimov left a comment

carpawell left a comment

cthulhu-rider Feb 19, 2025

roman-khimov Feb 19, 2025

cthulhu-rider Feb 19, 2025

cthulhu-rider Feb 19, 2025

roman-khimov Feb 19, 2025

End-rey commented Feb 19, 2025

roman-khimov Feb 20, 2025

Fix error from CalculateNonceAndVUB #3134

Fix error from CalculateNonceAndVUB #3134

Conversation

End-rey commented Feb 14, 2025

codecov bot commented Feb 14, 2025 • edited Loading

Codecov Report

roman-khimov left a comment

Choose a reason for hiding this comment

carpawell left a comment

Choose a reason for hiding this comment

cthulhu-rider Feb 19, 2025

Choose a reason for hiding this comment

roman-khimov Feb 19, 2025

Choose a reason for hiding this comment

cthulhu-rider Feb 19, 2025

Choose a reason for hiding this comment

cthulhu-rider Feb 19, 2025

Choose a reason for hiding this comment

roman-khimov Feb 19, 2025

Choose a reason for hiding this comment

End-rey commented Feb 19, 2025

roman-khimov Feb 20, 2025

Choose a reason for hiding this comment

Fix error from `CalculateNonceAndVUB` #3134

Fix error from `CalculateNonceAndVUB` #3134

codecov bot commented Feb 14, 2025 •

edited

Loading