Skip to content
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

Byron block hash with prefix bytes #316

Merged
merged 1 commit into from
Mar 7, 2024
Merged

Conversation

rooooooooob
Copy link
Contributor

Fixes #315

Comment on lines +243 to +250
ByronBlock::EpochBoundary(ebb) => {
tagged_bytes.push(0x00);
tagged_bytes.extend(&ebb.header.to_bytes());
}
ByronBlock::Main(mb) => {
tagged_bytes.push(0x01);
tagged_bytes.extend(&mb.header.to_bytes());
}
Copy link
Contributor

@SebastienGllmt SebastienGllmt Mar 6, 2024

Choose a reason for hiding this comment

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

Why do we have to add 0 or 1 manually here? It's already part of the CDDL definition of a block

block = [0, eb_block] ; @name ebb_wrapper

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was just looking at the byron CDDL we generate from:

; this doesn't match what we're getting. we're getting the inner block instead directly
;byron_block = [
;     ; @name ebb_wrapper
;     tag: 0, block: byron_eb_block //
;     ; @name main_block_wrapper
;     tag: 1, block: byron_main_block
;]
byron_block = byron_eb_block ; @name epoch_boundary
                       / byron_main_block ; @name main

when I was testing I was getting byron blocks from cardano-sync or net-fetcher that did not have the outer wrapper.

However, this hash that lines up would actually be something different entirely as that code would be [0, ebb_head // 1, byron_block_header] (hashing the header not the entire block). I don't remember how Byron does it so I am just going by that other link to blockchain-source for how it was done there.

Copy link
Contributor

@SebastienGllmt SebastienGllmt Mar 6, 2024

Choose a reason for hiding this comment

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

so should I be merging this PR as-is or do we need to do more investigation on this?

when I was testing I was getting byron blocks from cardano-sync or net-fetcher that did not have the outer wrapper.

in my experience a lot of tools don't support epoch boundary blocks so maybe those were stripped out for compatibility reasons? Not sure stripping is the intended behavior or something that was done for usability with something specific in mind

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't remember how Byron does it so I am just going by that other link to blockchain-source for how it was done there.

I did some investigation when I did that, btw. The links I put in the dcspark-core PR seem to be dead since the code was moved, but there are some docs about it in the haskell code.

https://cardano-ledger.cardano.intersectmbo.org/cardano-ledger-byron/src/Cardano.Chain.Block.Header.html#wrapHeaderBytes

-- | These bytes must be prepended when hashing raw boundary header data
--
--   In the Byron release, hashes were taken over a data type that was never
--   directly serialized to the blockchain, so these magic bytes cannot be
--   determined from the raw header data.
--
--   These bytes are from `encodeListLen 2 <> encCBOR (1 :: Word8)`

https://cardano-ledger.cardano.intersectmbo.org/cardano-ledger-byron/src/Cardano.Chain.Block.Header.html#wrapBoundaryBytes

-- | These bytes must be prepended when hashing raw boundary header data
--
--   In the Byron release, hashes were taken over a data type that was never
--   directly serialized to the blockchain, so these magic bytes cannot be
--   determined from the raw header data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in my experience a lot of tools don't support epoch boundary blocks so maybe those were stripped out for compatibility reasons? Not sure stripping is the intended behavior or something that was done for usability with something specific in mind

So I just synced mainnet and was looking at it and they do not skip the EBB blocks, but it looks like I was also just stripping the first 2 bytes since I wrote the tool when I was only parsing from Shelley onwards (and those types don't have those bytes). I updated MultiEraBlock:;from_explicit_network_cbor_bytes() here: #318 and tested it on mainnet since that didn't accept EBBs (the tool was directly using MultiEraBlock::deserialize() and thus bypassing this before hence why they parsed before).

It still seems unrelated to this however as this is calculating the hash on the wrapped block header, not the entire block.

-- In the Byron release, hashes were taken over a data type that was never
-- directly serialized to the blockchain, so these magic bytes cannot be
-- determined from the raw header data.

I guess that confirms the above so I'm going to merge this. I think we'll want to re-add those bits into the byron and figure out how we want to actually structure MultiEraBlock's (de)serialization. I made an issue here: #317

@rooooooooob rooooooooob merged commit 0c3c937 into develop Mar 7, 2024
1 check failed
@rooooooooob rooooooooob deleted the byron-block-hash-prefix branch July 18, 2024 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Byron block hash wrong
3 participants