Verify headers and blocks only when needed #3023
Conversation
There was a problem hiding this comment.
We read "compact blocks" in protocol.rs -
let b: core::CompactBlock = msg.body()?;
Should this be untrusted as well to ensure we run the necessary validation on the header for the compact block?
Actually the only time we do anything with compact blocks is when we receive them via p2p so maybe these are always untrusted?
core/src/core/block.rs
Outdated
| // We want to do this here because blocks can be pretty large | ||
| // and we want to halt processing as early as possible. | ||
| // If we receive an invalid block version then the peer is not on our hard-fork. | ||
| if !consensus::valid_header_version(height, version) { |
There was a problem hiding this comment.
Is the valid_header_version check worth moving into UntrustedBlockHeader?
It takes a read lock on the global CHAIN_TYPE internally and we presumably don't need to do this check every time we read a header from the db.
There was a problem hiding this comment.
Sounds reasonable, will move it there
Curretnly we have some lightweigt validation implemented as part of entity deserialization, which is safer and allows us to not parse the entire object if some part is invalid. At the same time this logic always applies when we read an entity, eg when reading from DB. This PR introduces UntrustedHeader/Block which is used when we read from the network. It does partial validation during read, then it is supposed to be converted into regular header/block which doesn't validate itself. Also this PR adds "lightweight" validation to block header read like we have for block body, so we don't parse block body if the header is invalid. Fixes mimblewimble#1642
hashmap
force-pushed
the
header_verify_read
branch
from
7cff552 to
719ef01
Compare
I ended up having an additional Untrusted type, for symmetry, also some code relies on the existing structure of CompactBlock, so it's cleaner to read UntrustedCompactBlock and then convert it |
| @@ -365,7 +365,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" | |||
|
|
|||
| [[package]] | |||
| name = "grin_core" | |||
| version = "2.1.0-beta.1" | |||
| version = "3.0.0-alpha.1" | |||
There was a problem hiding this comment.
These changes should not be necessary on master.
|
|
||
| /// Deserialization of an untrusted block header | ||
| impl Readable for UntrustedBlockHeader { | ||
| fn read(reader: &mut dyn Reader) -> Result<UntrustedBlockHeader, ser::Error> { |
There was a problem hiding this comment.
Wondering if we never actually pass an UntrustedBlockHeader around what this might look like if we modified this signature to actually return the trusted version once we had successfully read it -
impl Readable for UntrustedBlockHeader {
fn read(reader: &mut dyn Reader) -> Result<BlockHeader, ser::Error> {
...
}
...
}
I assume we cannot do this easily because of how Readable is defined but it would potentially allow us to not need the additonal explicit .into() to do conversions later.
Maybe being explicit like this is better though.
| return Err(ErrorKind::LowEdgebits.into()); | ||
| } | ||
| let edge_bits = header.pow.edge_bits(); | ||
| if !(ctx.pow_verifier)(header).is_ok() { |
There was a problem hiding this comment.
@hashmap Where does this check happen now?
There was a problem hiding this comment.
The only place I see similar code is in validate_pow_only() but this is only used for orphan blocks.
Currently we have some lightweight validation implemented as part of
entity deserialization, which is safer and allows us to not parse the
entire object if some part is invalid. At the same time this logic
always applies when we read an entity, eg when reading from DB.
This PR introduces UntrustedHeader/Block which is used when we read from
the network. It does partial validation during read, then it is supposed
to be converted into regular header/block which doesn't validate itself.
Also this PR adds "lightweight" validation to block header read like we have
for block body, so we don't parse block body if the header is invalid.
Fixes #1642