-
Notifications
You must be signed in to change notification settings - Fork 378
fix: resolve incorrect author caused by multiple preRuntime logs #6230
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
base: master
Are you sure you want to change the base?
Conversation
TarikGul
left a comment
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.
👍
|
cc: @voliva |
| if (accountId) { | ||
| return accountId; | ||
| } |
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 could be mistaken, but from a distance this fix seems a bit brittle. Are we certain that performing engine.extractAuthor on an entry that doesn't actually have an author will always return undefined? Aren't we chancing it a bit? Are we certain that the underlying encoded data can't yield a false-positive? What if one day the extractAuthor implementation changes?
Again, I'm not familiar with the implementation of extractAuthor. However, I think that it would be best to be more assertive and to first find out which one is the correct entry from where to extract the author, and only then get the author using the extractAuthor method.
It's just a thought. Please take it with a pinch of salt, because I really am not familiar with this logic in PJS. 🙂
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.
Great question! Your concern about relying on undefined returns is valid. However, the current approach is safe because extractAuthor() validates the engine identifier first, before interpreting any payload data:
api/packages/types/src/generic/ConsensusEngineId.ts
Lines 92 to 107 in 241a1a3
| public extractAuthor (bytes: Bytes, sessionValidators: AccountId[]): AccountId | undefined { | |
| if (sessionValidators?.length) { | |
| if (this.isAura) { | |
| return getAuraAuthor(this.registry, bytes, sessionValidators); | |
| } else if (this.isBabe) { | |
| return getBabeAuthor(this.registry, bytes, sessionValidators); | |
| } | |
| } | |
| // For pow & Nimbus, the bytes are the actual author | |
| if (this.isPow || this.isNimbus) { | |
| return getBytesAsAuthor(this.registry, bytes); | |
| } | |
| return undefined; | |
| } |
Example: When we call extractAuthor() on a CMLS preRuntime log:
- Engine = 'CMLS' (4 bytes: 0x434d4c53)
- Doesn't match 'aura', 'BABE', 'pow_' ...
- Returns undefined without decoding the payload
- Loop continues to the next preRuntime log
This means the engine ID acts as a gatekeeper -- random bytes in unrecognized engine payloads can't produce false positives because the engine string is checked before any data interpretation.
fixes #6229
Problem
Block author extraction fails on parachains and chains with multiple preRuntime digest logs (e.g., Kreivo, Asset Hub, and other CMLS based chains).
Solution