Filter out redundant attestations in input builder #100
Conversation
… to make threshold as set in config
host/src/input_builder.rs
Outdated
| #[error("Error in called verify method: {0}")] | ||
| VerifyError(#[from] z_core::VerifyError), | ||
| #[error("Arithmetic error")] | ||
| Arith(safe_arith::ArithError), |
There was a problem hiding this comment.
Usu
| #[error("Error in called verify method: {0}")] | |
| VerifyError(#[from] z_core::VerifyError), | |
| #[error("Arithmetic error")] | |
| Arith(safe_arith::ArithError), | |
| #[error("Error in called verify method")] | |
| VerifyError(#[from] z_core::VerifyError), | |
| #[error("Arithmetic error: {0:?}")] | |
| ArithError(safe_arith::ArithError), |
Usually the best practice is to not use any {} when there is a #[from] or #[source], because then std error chaining can and will be used.
Otherwise use it.
we call this ArithError anywhere else, so we should do the same here.
| } | ||
| } | ||
|
|
||
| pub struct InputBuilder<'a, E, CR, SR> { |
There was a problem hiding this comment.
Since this has a lifetime now anyways, lets also use chain_reader: &'a CR,
| ) -> Result<Vec<Attestation<E>>, InputBuilderError> { | ||
| let active_validators: BTreeMap<_, _> = self | ||
| .state_reader | ||
| .active_validators(target_epoch) |
There was a problem hiding this comment.
This is not correct in general. Only if the ssz reader or an equivalent reader is used. The host reader uses the active balance information about the actual epoch and not only the trusted epoch, so this might significantly diviate.
There was a problem hiding this comment.
Ah right yes. I have never been happy about how the expected behavior of StateReader is different depending on the type and this is a great example of why it is kind of dangerous. I think using the SszReader and then also sharing the committee cache is a workable solution for now
There was a problem hiding this comment.
I've changed how the host state reader operates so now it has a trusted epoch the same as the others. On the plus side this allowed me to remove the hack in AssertStateReader as the results from reading the validators matches now.
There was a problem hiding this comment.
Alternative approach. This just grabs the balances and slashed status from the trusted state which replicates the SSZ Reader approach for the values used in this function. It is also much simpler than trying to modify the Host reader
There was a problem hiding this comment.
I am not a fan of modifying the active validators returned by the HostStateReader. I liked the idea that it always returns the actual, correct values of the state. Removing this property seems error-prone and inconsistent to me.
One compromise I could accept is modifying the returned iterator in the input_builder for this purpose only.
Signed-off-by: Willem Olding <willemolding@gmail.com>
Modifies the input builder to be more intelligent and skip adding attestations once it has detected enough to meet the super-majority requirement.
This requires the input builder to receive a little more information, namely the config that the guest is using and a state reader.
No changes to the guest code are included in this PR except for exporting some functions from
core.This will make quite a significant different to both the input size and the cycle count as extra attestations means more expensive signature checks. When the chain is receiving near 100% participation (as is typical) and our threshold is set to 85% we expect close to a 15% improvement with this one simple trick.