Move spend limit check to finalize phase#3242
Conversation
| // Determine the block spend limit. | ||
| let block_spend_limit = if let Authority::Quorum(subdag) = block.authority() { | ||
| Some( | ||
| subdag.values().map(|certificates| certificates.len() as u64).sum::<u64>() |
There was a problem hiding this comment.
This doesn't exactly translate to batch_spend_limit for each batch.
Now it's that the entire block can't exceed num_certificates * batch_spend_limit, so some batches can have more as long as the avg is <= batch_spend_limit
There was a problem hiding this comment.
Indeed, it is a bit more lenient. I don't see issues with it though. An alternative would be to compute and pass around a mapping of {proposal => {limit, transactions}} which seems like a lot of work.
There was a problem hiding this comment.
Also note that since my last commits, I'm now also tracking compute_spend even for transactinos which may not be ultimately accepted. Seems like a very clean setup to me.
PrepareSpeculateResult::Finalize(compute_spend) => {
if consensus_version >= ConsensusVersion::V15 {
// Track the compute_spend used so far.
block_spend = block_spend.saturating_add(compute_spend);
}
}
There was a problem hiding this comment.
Can this be attacked by a malicious validator?
A validator could just fill their batch with to exceed the batch_spend_limit, and then everyone else's transactions will be aborted. Granted, this would require them to spend the fee.
There was a problem hiding this comment.
Gave it some more thought and tried some prototyping. Sticking to a batch_spend_limit would really be better, but also a lot more refactoring, so I'd suggest we wrap this up and leave it as future work. This PR + the snarkOS PR is already an improvement over the status quo in that it prevents a halt under honest conditions.
Added one more commit e026b3c which clarifies the TODOs, tightly integrates fn batch_spend_limit again, and to reduce the need for further regression analysis it keeps the batch spend limit equal to the transaction spend limit.
There was a problem hiding this comment.
Can you make an issue for this?
|
LGTM! Deferring final approval to @raychu86. |
| /// The block-specific random seed. | ||
| random_seed: [u8; 32], | ||
| /// The block spend limit. | ||
| block_spend_limit: Option<u64>, |
There was a problem hiding this comment.
Is there a reason why this needs to live in FinalizeGlobalState instead of just being declared during VM::finalize?
The objects in FinalizeGlobalState were originally intended for objects that the actual finalize operations could pull from or execute on.
There was a problem hiding this comment.
The block_spend_limit also is not included in the pre-image of the seed. And it should not need to be.
There was a problem hiding this comment.
TL;DR: to reduce refactoring.
I'm computing the spend limit based on the number of certificates in the block in order to achieve more fairness. Our consensus algorithm moves in mysterious ways - under heavy load sometimes anchors may be skipped, a block contains 3x the amount of certificates, and I think we'd rather want to optimize for guaranteed transaction inclusion than stable blocktimes at this point in time. We don't have access to the number of certificates in atomic_speculate.
With that background in mind, the other IMO inferior options are:
- pass a new block_spend_limit variable along to
atomic_speculate,speculate, and likely more call sites, which touches 10+ files and 30+ functions. - similar to the
pending_rejection_reasons(which carries over data fromatomic_speculatetoadd_next_block, introduce apending_block_spend, which carries over data intoatomic_speculate.
Antonio95
force-pushed
the
move_spend_limits
branch
from
957ac8a to
a73d81e
Compare
|
@vicsn Fixes look good. My only remaining concern is - #3242 (comment) |
| if height >= N::CONSENSUS_HEIGHT(ConsensusVersion::V15).unwrap() { | ||
| consensus_config_value!(N, TRANSACTION_SPEND_LIMIT, height).unwrap() |
There was a problem hiding this comment.
I don't understand this. This is now saying that after ConsensusVersion::V15, a batch can't spend more than a singular transaction's TRANSACTION_SPEND_LIMIT
There was a problem hiding this comment.
In the previous versions of this PR it was much lower, but in order to reduce the scope of this PR and to avoid further regression analysis, I think a minimum of TRANSACTION_SPEND_LIMIT suffices. Documented this in more detail in the follow-up issue: #3264
3 participants
Motivation
This is a lenient version of the block spend limit check, allowing for
num_certificates * proposal_spend_limitworth of compute (same as snarkOS allows at the moment).Test Plan
Besides CI, we should ensure:
Backwards compatibility
The new check is guarded by a consensus version.