Skip to content

Eval: Move heartbeat verification earlier#6194

Merged
jannotti merged 10 commits intoalgorand:feature/heartbeatsfrom
jannotti:early-crypto
Dec 11, 2024
Merged

Eval: Move heartbeat verification earlier#6194
jannotti merged 10 commits intoalgorand:feature/heartbeatsfrom
jannotti:early-crypto

Conversation

@jannotti
Copy link
Copy Markdown
Contributor

@jannotti jannotti commented Dec 9, 2024

Adds needed fields to HeartbeatTxnFields so that the HbProof can be checked with other signatures (early, and concurrently).

Summary

Test Plan

Adds needed fields to HeartbeatTxnFields so that the HbProof can be
checked with other signatures (early, and concurrently).
Comment thread data/basics/userBalance.go Outdated
Comment thread ledger/apply/heartbeat.go
}
if tx.HbKeyDilution == 0 {
return errors.New("tx.HbKeyDilution is zero")
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idk, maybe it makes sense to reject HB with empty HbSeed/HbVoteID earlier right here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, I'm adding empty checks.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reminder for myself; will look for most of the "is this is a free heartbeat, go draconian" checks (the must be empties) to move here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check now.

Copy link
Copy Markdown
Contributor Author

@jannotti jannotti Dec 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I use the tx.<FieldName> construction in error messages universally? It's common in WellFormed.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be fine with that.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See if you like the new error messages.

Comment thread data/transactions/verify/txn.go
Comment thread data/transactions/transaction.go
Copy link
Copy Markdown
Contributor

@gmalouf gmalouf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm good pending the checks moving into `WellFormed" (and test cases around them).

@codecov
Copy link
Copy Markdown

codecov bot commented Dec 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 51.99%. Comparing base (d5286e3) to head (120d544).

Additional details and impacted files
@@                  Coverage Diff                   @@
##           feature/heartbeats    #6194      +/-   ##
======================================================
+ Coverage               50.94%   51.99%   +1.05%     
======================================================
  Files                     642      642              
  Lines                   85831    85852      +21     
======================================================
+ Hits                    43723    44640     +917     
+ Misses                  39298    38375     -923     
- Partials                 2810     2837      +27     

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

Comment thread data/transactions/transaction.go
@jannotti jannotti merged commit 573ec73 into algorand:feature/heartbeats Dec 11, 2024
@jannotti jannotti deleted the early-crypto branch July 15, 2025 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants