Filter empty quorums when confirming batch #581
Conversation
ian-shim
force-pushed
the
batch-quorum-filter
branch
from
62afa3b to
f96b9cf
Compare
disperser/batcher/batcher.go
Outdated
| @@ -579,23 +587,25 @@ func (b *Batcher) getBatchID(ctx context.Context, txReceipt *types.Receipt) (uin | |||
| return batchID, nil | |||
| } | |||
|
|
|||
| // numBlobsAttested returns the number of blobs that have been successfully attested by the given quorums | |||
| func numBlobsAttested(signedQuorums map[core.QuorumID]*core.QuorumResult, headers []*core.BlobHeader) int { | |||
| // numBlobsAttestedByQuorum returns the number of blobs that have been successfully attested by the given quorums | |||
There was a problem hiding this comment.
nit: the number of blobs that have been attested by all given quorums is still returned
disperser/batcher/batcher.go
Outdated
| nonEmptyQuorums := []core.QuorumID{} | ||
| for quorumID, count := range numPassedByQuorum { | ||
| log.Info("Blobs attested by quorum", "quorumID", quorumID, "count", count) | ||
| if count > 0 { |
There was a problem hiding this comment.
Should this filter out quorums that have a signing rate below confirmation threshold?
There was a problem hiding this comment.
This should be already filtering out quorums that have a signing rate below the confirmation threshold.
count here in numPassedByQuorum is the number of blobs that have signing rate above the confirmation threshold, and we're filtering out quorums with count == 0
There was a problem hiding this comment.
Since all blobs are in the same batch, they have the same signing rate for a given quorum. It looks this doesn't need to count num blobs (it's going to be either 0 or Num-Blobs), just whether the quorum collected sufficient signing rate
There was a problem hiding this comment.
Sure, what you're suggesting is that we can make this logic simpler by simply comparing the quorum result vs. the confirmation threshold required at each quorum which is hardcoded in contracts. Correct?
But then the batcher needs to be aware of those thresholds and make an assumption that all blobs will be dispersed using the same thresholds for each quorum.
Comparing the security params for each blobs with the quorums results allows the batcher to just use the security params set at each blob and filter quorums more generally.
There was a problem hiding this comment.
I think numBlobsAttestedByQuorum may just return a set of quorumIDs that have collected sufficient signatures. It's semantically more clear but cutting away the count since it's 0 or all.
core/aggregation.go
Outdated
| // including those that did not sign | ||
| QuorumAggPubKey map[QuorumID]*G1Point | ||
| // SignersAggPubKey is the aggregated public key for all of the operators that signed the message for each quorum, | ||
| // further aggregated across the quorums; operators signing for multiple quorums will be included in |
There was a problem hiding this comment.
nit: this doesn't aggregate across quorums now (it's per quorum)
| // SignersAggPubKey is the aggregated public key for all of the operators that signed the message for each quorum, | ||
| // further aggregated across the quorums; operators signing for multiple quorums will be included in | ||
| // the aggregation multiple times | ||
| SignersAggPubKey map[QuorumID]*G2Point |
There was a problem hiding this comment.
It aggregates operator's PubKeyG2
| // QuorumResults contains the quorum ID and the amount signed for each quorum | ||
| QuorumResults map[QuorumID]*QuorumResult | ||
| // SignerMap contains the operator IDs that signed the message | ||
| SignerMap map[OperatorID]bool |
There was a problem hiding this comment.
This is not per quorum, should it be left at in SignatureAggregation?
There was a problem hiding this comment.
Yeah it's a bit awkward because it's the only field not divided into quorums. But it's easier to track this map at the stage of receiving signatures. I added a comment in the struct to clarify what this field is
ian-shim
force-pushed
the
batch-quorum-filter
branch
from
f96b9cf to
3056bcc
Compare
Why are these changes needed?
With additional (custom) quorums, blobs may fail to get attested by some quorums and the current implementation would still post the batch including all the non-signers in the failed quorums.
This PR makes batcher inspect the quorum results and filter out empty quorums when submitting the batch confirmation transaction onchain.
Checks