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
Consolidate and cleanup tx aggregation #1332
Conversation
* Include commitments non-duplicate checks in aggregate * Remove said check from the pool * Block building now uses tx aggregation to reduce duplication
Created #1333 for the block and transaction validation cleanup. |
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.
Looks good.
Couple of comments.
A question about how we handle coinbase outputs during aggregation/cut-through.
core/src/core/transaction.rs
Outdated
@@ -431,14 +431,16 @@ impl Transaction { | |||
/// Validates all relevant parts of a fully built transaction. Checks the | |||
/// excess value against the signature as well as range proofs for each | |||
/// output. | |||
pub fn validate(&self) -> Result<(), Error> { | |||
pub fn validate(&self, as_block: bool) -> Result<(), Error> { |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
core/src/core/transaction.rs
Outdated
if self.inputs.len() > consensus::MAX_BLOCK_INPUTS { | ||
return Err(Error::TooManyInputs); | ||
} | ||
self.verify_features()?; | ||
if !as_block { |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
||
let in_set = inputs | ||
.iter() | ||
.map(|inp| inp.commitment()) | ||
.collect::<HashSet<_>>(); | ||
|
||
let out_set = outputs | ||
.iter() | ||
.filter(|out| !out.features.contains(OutputFeatures::COINBASE_OUTPUT)) |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
* Include commitments non-duplicate checks in aggregate * Remove said check from the pool * Block building now uses tx aggregation to reduce duplication
Didn't want to get too deep in the rabbit hole but I think we're at a point where there's enough common validation code between
Block
andTransaction
to introduce a thing that both would include. The thing could have theVec
s ofInput
,Output
andKernel
and all the validation in common.