-
Notifications
You must be signed in to change notification settings - Fork 0
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
Update to support casper 2.0.0-rc4 #9
Conversation
Start introducing the V2 block format Fix clippy lints Make BlockV2 serde prop tests Make Era stuff work Fix tests Fix end era v2 BlockHeaderV2 proptests More Tests
Notably replacing JsonBlock with casper_types::JsonBlockWithSignatures
We could just get rid of src/crypto.rs and use |
@xcthulhu All deleted |
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.
LGTM
Looks good, can we merge it? Avi, Matt |
src/kernel.rs
Outdated
// impl From<SignatureVerificationError> for BlockSignaturesValidationError { | ||
// fn from(signature_verification_error: SignatureVerificationError) -> Self { | ||
// BlockSignaturesValidationError::SignatureVerificationError(signature_verification_error) | ||
// } | ||
// } |
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.
We can delete this I think.
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 deleted this in the last commit.
pub fn validate( | ||
&self, | ||
block_header_with_signatures: &BlockHeaderWithSignatures, | ||
JsonBlockWithSignatures { block, proofs }: JsonBlockWithSignatures, |
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.
@uchu Does this work with what the new side-care outputs when you try to get block by height?
src/kernel.rs
Outdated
.proofs() | ||
.iter() | ||
{ | ||
let mut signature_data = block.hash().inner().into_vec(); |
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.
So the hash
on a block could possibly be forged (this accessor doesn't compute the hash, it accesses it from the Block
data structure).
You need to add a check that block.block_header().block_hash() == block.hash()
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.
You would also want to check the hash of the block body to make sure it matches the body hash in the header.
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.
Fixed
Deletes a bunch of stuff.
Makes a new version.
Other than using upstream types the main breaking change is replacing
casper_node::JsonBlock
withcasper_types::JsonBlockWithSignatures
.JsonBlock
has been removed fromcasper_node
, it was briefly added tocasper_sidecar
, but then it was removed. I believe everything now usesJsonBlockWithSignatures
.JsonBlockWithSignatures
sounds like it should be the same type when serialized, but it's rep is very different.Upstream users of this library are unlikely to be able to continue getting
JsonBlock
s from the l1, and should now update to useJsonBlockWithSignatures
.This change has been made in litmus-wasm, but the service worker will also have to update.
I did attempt to support an updated V2 compatible version of
JsonBlock
, but it's a pain, and is still a breaking change so I stopped, it's best if that change is done in the JS.