subscan-explorer / subscan-issue-tracker

The issue tracker for Subscan.io.
3 stars 5 forks source link

Incorrect extraction of block author in last block in session, Aura #54

Closed DamianStraszak closed 1 year ago

DamianStraszak commented 1 year ago

Confirmation

Affected Network(s)

Aleph Zero, and all other using Aura

Steps to reproduce

See the actual explanation below

Expected output

-

Actual output

-

Additional factoids or references

This is a kind of nasty "off-by-one" bug that results in the author of the last block in a session to be computed incorrectly.

  1. Example: On Aleph Zero Mainnet (which uses Aura) the author of this block https://alephzero.subscan.io/block/44913600 is this 5EFD4z93FCt8Vv9YRJRvf1Sy4N3FTeMSdCQEJ8y5f6cxB5Pw account (it's the validator at index 12 (zero-based) among validators in session 49903). However subscan reports the author as 5EbbxSnnXA5GE8fKUZhpzkn7pi2SkiscAJX7pk7fAS5NF68M (as seen in the link). The issue here is that 44913600 is the last block of session 49903, and subscan reports that the author is a validator from session 49904.

  2. Where does the bug come from: Without going deep into details, the way how the block author is calculated is as follows:

    The slot number slot is computed from a PreRuntime digests in a block header, The validators are queried from the session pallet, i.e., a query Session Validators is made that returns an array of AccountIds. The author is computed as validators[slot % validators.len()] While this is indeed the correct way to compute the slot author in Aura, the issue is how validators are computed. In fact, in the example above the block 44913600 ends session 49903, but when querying validators from pallet Session, already the new validators, i.e., from session 49904 are returned (because they have been initialized in this very block). The correct way to compute validators would be to query them from the parent of the current block, i.e., block 44913599, and not the current block. This would then give the correct author.

  3. Why what I'm saying is correct: You might ask -- Ok, but how to confirm that this report is legit, and not just some bullshit. This unfortunately is not very simple, there are two ways: a) Checking that the signature under block 44913600 does not actually verify, for the block author claimed by subscan, but it does for the block author claimed by me. Honestly, I haven't done that, because this would require a lot of work for me, as I'm not really sure how to get the signature and then verify it, but I could probably do that, if there is no other way to prove that this issue is valid :). b) Reading substrate, specifically Aura and Slots. This is what I have done. Let me go over this very quickly. So for choosing the block author there is a function called slot_author which takes the slot number and the authorities list, and essentially executes the logic as above authorities[slot % authorities.len()]. The question is what are the authorities the function is called with. To see this, we need to go here and then we need to find how aux_data is computed, which leads us back to aura, to this place and then here. Then finally we see that the authorities are computed as

    runtime_api .authorities(parent_hash) .ok() .ok_or(ConsensusError::InvalidAuthoritiesSet)

where parent_hash is the hash of the block we are trying to build on. This means that indeed one must query Session -> Validators from the previous block, to learn the author of the current block.

  1. This is actually a repost of a similar issue to polkadot.js/api which was promptly fixed already https://github.com/polkadot-js/api/issues/5597
freehere107 commented 1 year ago

@DamianStraszak Thanks for your feedback. I will fix it as soon.

freehere107 commented 1 year ago

@DamianStraszak This issue has been fixed.