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 1 commit 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. 🙂)
| - the builder takes the first proof. | ||
| - the builder takes the second proof for its one uncovered voter. | ||
| - the builder folds both proofs into one attestation. | ||
| - block_2 holds 1 aggregated attestation covering V0, V1, V2, V3. |
There was a problem hiding this comment.
Small docstring-convention nit: these bullets describe the builder's internal selection steps, but the Then section is meant to mirror the assertions one-to-one — and nothing here actually observes selection order or "one uncovered voter" (the checks are just block_attestation_count=1, participants={0,1,2,3}, and head at slot 2).
Could we swap them for outcome-only bullets, like the sibling has at lines 43-44? Something like:
- block_2 holds 1 aggregated attestation.
- that aggregation covers V0, V1, V2, V3.
- head is block_2 at slot 2.
That also naturally splits the count and the participant set onto their own lines, which matches the one-fact-per-line style. 🙂
| Two overlapping proofs (enforced by 2 separate gossips) for one target | ||
| fold into a single in-block attestation. |
There was a problem hiding this comment.
Tiny one: the test-vector convention only allows a parenthetical when it glosses a number, so "(enforced by 2 separate gossips)" is slightly off here (and uses "2" as a word). Since the Given section already says both proofs arrive by gossip, I think we can just drop the parenthetical and keep the summary as a clean one-liner. 👍
| Timing | ||
| ------ | ||
| - the proofs are gossipped at slot 1, interval 3 (past the aggregate phase). |
There was a problem hiding this comment.
Optional: this Timing line mostly restates the Given bullet about the clock ticking past the aggregate phase. No strong opinion — maybe keep the concrete interval here and drop the Given line (or vice versa), just so the fact lives in one place. Happy either way. 🙂
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.