feat: Validate conway block KES signatures #2
Conversation
AndrewWestberg
force-pushed
the
amw/validate_block_kes_signature
branch
2 times, most recently
from
174c9b1 to
98e7a94
Compare
|
Converting to draft to add additional check for opcert counter. Also re-factoring Validator to pass back errors instead of just boolean. |
AndrewWestberg
force-pushed
the
amw/validate_block_kes_signature
branch
from
98e7a94 to
0d80a63
Compare
|
Code is ~20% faster at block validation by using parallel code. |
AndrewWestberg
force-pushed
the
amw/validate_block_kes_signature
branch
from
0d80a63 to
c3ff6a2
Compare
| FixedDecimal::from(5u64) / FixedDecimal::from(100u64); | ||
| let c = (FixedDecimal::from(1u64) - active_slots_coeff).ln(); | ||
| let conway_block_tag: u8 = 6; | ||
| let multi_era_header = MultiEraHeader::decode(conway_block_tag, None, test_block).unwrap(); |
There was a problem hiding this comment.
Shouldn't the decoding of the header be outside of the bench? If that bench is specifically about measuring the validation, it sounds to me that it's reasonable to assume already decoded blocks?
| validation_checks | ||
| .into_par_iter() | ||
| .try_for_each(|check| check())?; |
There was a problem hiding this comment.
It would actually be interesting to benchmark whether the parallel validation here does provide a benefit over a sequential validation? Threads creation and context switching may perhaps dwarf the benefits gained from doing the validation in parallel?
It's not obvious to me that this is necessarily faster.
| #[error("InvalidByteLength: {0}")] | ||
| InvalidByteLength(String), | ||
| #[error("Invalid VRF key for pool: expected: {0}, was: {1}")] | ||
| InvalidVrfKeyForPool(String, String), | ||
| #[error("Invalid block hash, expected: {0}, was: {1}")] | ||
| InvalidBlockHash(String, String), | ||
| #[error("Ledger error: {0}")] | ||
| LedgerError(#[from] crate::ledger::Error), | ||
| #[error("VrfVerificationError: {0}")] | ||
| VrfVerificationError(#[from] pallas_crypto::vrf::VerificationError), | ||
| #[error("InvalidVrfProofHash, expected: {0}, was: {1}")] | ||
| InvalidVrfProofHash(String, String), | ||
| #[error("InvalidVrfLeaderHash, expected: {0}, was: {1}")] | ||
| InvalidVrfLeaderHash(String, String), | ||
| #[error("InvalidOpcertSequenceNumber: {0}")] | ||
| InvalidOpcertSequenceNumber(String), | ||
| #[error("InvalidOpcertSignature")] | ||
| InvalidOpcertSignature, | ||
| #[error("KesVerificationError: {0}")] | ||
| KesVerificationError(String), | ||
| #[error("InsufficientPoolStake")] | ||
| InsufficientPoolStake, |
There was a problem hiding this comment.
It doesn't feel right to have hex-encoded strings here as part of the ValidationError types for many of those fields. It is IMO a mismatch of responsibility / representation. Having an hex-encoded string is useful for display yet should be the responsibility of the Debug or Display trait's implementation.
To give an concrete example where it can be problematic; if you end up returning the error and handling it from another place, then you end up paying the cost of hex-encoding the data for nothing. It wouldn't be problematic in a lazy language, but in Rust that'd be wasted CPU.
There was a problem hiding this comment.
I would decouple the representation of the error from its definition: leave raw types in the definition and provide different representations elsewhere (a Display trait is one option, but there's also the case of rendering errors as JSON or any other representation as part of structured logging)
AndrewWestberg
force-pushed
the
amw/validate_block_kes_signature
branch
from
c3ff6a2 to
5d914a7
Compare
|
Merging so Arnaud can have minimal changes to his pr that built on top of this. We can correct the open comments in a follow-up. |
This code adds validation of both the Operational Certificate included in each block as well as the hot key KES signature in the block header.
dependent on pallas changes: txpipe/pallas#518