test: add test vector for block production with merging aggregates#897
test: add test vector for block production with merging aggregates#897unnawut wants to merge 3 commits into
Conversation
|
Thanks a lot, can you just rebase after #895 and follow the new doc writer conventions for the documentation of the test vectors just for uniformity? :) Also I haven't checked in details but would be nice to ensure that this test doesn't already exist with the new coverage added in the PR. |
unnawut
force-pushed
the
builder-recursive-fold-vector
branch
from
a8a9f9e to
2d82838
Compare
|
Accidentally pushed. I want to do a bit of touch up before marking for review again |
unnawut
force-pushed
the
builder-recursive-fold-vector
branch
4 times, most recently
from
62810fc to
3219764
Compare
…roofs The block builder's collapse-by-data branch in build_block merges multiple single-message aggregates for the same AttestationData via SingleMessageAggregate.aggregate(children=..., raw_xmss=[]). This is the only proposal-time path that constructs a recursive single-message aggregate, and no existing fork-choice vector reached it because the local aggregator absorbs same-data proofs before block build time. Gossiping two aggregated proofs with overlapping participant sets at slot 2 interval 3 (past the aggregate phase) bypasses the local aggregator, so both proofs migrate side by side into the known pool at slot 2 interval 4. The greedy picker takes both because the second still adds one uncovered validator on top of the first, exercising the merge branch on overlapping children rather than the unrealistic fully disjoint shape.
unnawut
force-pushed
the
builder-recursive-fold-vector
branch
from
3219764 to
269529e
Compare
@tcoratger I checked and yep there's I've moved my new test into the same file as yours though (test_signature_aggregation.py) so they stay close together. |
tcoratger
left a comment
tcoratger
left a comment
There was a problem hiding this comment.
Thanks for adding this! 🙏 I went through it carefully and the scenario is really nicely constructed. The TickStep to slot 1 / interval 3 is a neat touch: it pushes the gossips past the aggregate phase so the two overlapping proofs land in the known pool unmerged, which is exactly what forces the recursive in-block fold — so this genuinely covers a different path from the leaf-merge sibling. 👍
Just a few small things below, mostly around the test-vector docstring conventions, plus one question about crypto coverage. Nothing blocking — feel free to push back on any of them. 🚀
| ) | ||
|
|
||
|
|
||
| def test_overlapping_proofs_same_target_recursively_merge_into_one( |
There was a problem hiding this comment.
One thing worth double-checking: the leaf-merge sibling just above carries @pytest.mark.real_crypto(smoke=True), but this new test doesn't. In the default mocked lane the prover is stubbed out, so the real recursive fold/split only runs in the slower real-crypto lane — which means the fast lane here is really validating the bitfield bookkeeping (overlapping voters collapsing into one attestation) rather than the actual recursive proof.
Since the recursion is kind of the whole point of this vector, would it make sense to add @pytest.mark.real_crypto(smoke=True) to mirror the sibling, so the fast lane exercises the real fold too?
(If you intentionally left it off because real recursive fold isn't smoke-lane-stable yet — related to the leanVM bump you mentioned — that's totally fair, I just wanted to flag the asymmetry. 🙂)
There was a problem hiding this comment.
I overlooked this. I enabled real crypto now and impressed the test filler is still low 4 minutes so I think yes it makes sense to use real proof for this.
|
@tcoratger Applied your comments and re-reviewed. Please have another look! I'm still not too familiar with Given/When/Then format so feel free to guide me more |
2 participants
🗒️ Description
Add a test vector that asserts if a block producer has 2 aggregates of the same message, they get merged successfully for block inclusion.
Since proof bytes are not deterministic, this vector checks that all attesters are accounted for in the produced block.
I was also trying to add split and merge but seems like it needs a leanVM commit bump so I'll defer that to later.