-
Notifications
You must be signed in to change notification settings - Fork 490
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
AVM: Unify inner transaction ID calculation #3927
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3927 +/- ##
==========================================
- Coverage 50.09% 50.09% -0.01%
==========================================
Files 394 394
Lines 68456 68489 +33
==========================================
+ Hits 34294 34309 +15
- Misses 30465 30477 +12
- Partials 3697 3703 +6
Continue to review full report at Codecov.
|
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 think this looks good. One thing that should maybe be noted somewhere - I had previously hoped that if this hits mainnet and there were still no examples of any code grabbing an inner txn id, we'd be able to remove this code. But even if that doesn't happen, group ids have been calculated, and appear in blocks using these ids as their input, so we're stuck forever, right?
That's right, the new consensus param affects GroupID calculations, so we unfortunately have to keep at least some of the existing code around. But we could in theory fix the caching issue and different respones from |
Summary
Unifies the inner transaction ID calculation with a new consensus parameter,
UnifyInnerTxIDs
.Supersedes #3806
Test Plan
Unit tests added for new and current behavior.