feat(tests): cover a null EIP-8070 custodyColumns and repin to the current EIP blob - #3622
spencer-tb wants to merge 2 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## forks/amsterdam #3622 +/- ##
===================================================
+ Coverage 94.47% 94.49% +0.02%
===================================================
Files 624 624
Lines 36969 36964 -5
Branches 3334 3331 -3
===================================================
+ Hits 34925 34930 +5
+ Misses 1443 1435 -8
+ Partials 601 599 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
LouisTsai-Csie
left a comment
There was a problem hiding this comment.
I'd like to understand how we verify the no-op semantics here. After sending custodyColumns = null, how do we confirm that the previously configured custody columns remain unchanged?
It seems like this state is not directly observable within our current test framework. Is this behavior verified somewhere outside this framework, and if so, where is the verification?
| rejected with `-32602: Invalid params`. | ||
| """ | ||
| assert self.custody_columns is not None | ||
| assert self.custody_columns is not None or self.custody_columns_null |
There was a problem hiding this comment.
The current logic allows both custody_columns to be set and custody_columns_null to be True at the same time. These two states should be mutually exclusive.
In that case, the implementation silently gives precedence to custody_columns and sends the bitmap, ignoring custody_columns_null. I don't think this combination should exist by the test framework in the first place.
For example, a test like the following should not be possible:
blobs_test(
pre=pre,
txs=txs,
get_blobs_version=4,
cell_mask=ALL_CELLS_MASK,
custody_columns=custody_columns,
custody_columns_null=True,
)It would be preferable to explicitly reject this combination, e.g. with an assertion that custody_columns and custody_columns_null cannot both be set.
…null `custody_columns` and `custody_columns_null` were both accepted, and the executor silently sent the bitmap. A validator on `BlobsTest` now rejects the combination, with a unit test that fails without it, and the executor asserts that exactly one of the two is set. Narrow the null arm's docstring to what the Engine API can observe: `null` is well-formed, the forkchoice update completes with VALID, and `getBlobsV4` still serves every cell. Whether the client keeps a previously configured custody set only steers devp2p sampling and is not observable here. Addresses review feedback on ethereum#3622.
30ed076 to
85a8111
Compare
…er a custody set Co-authored-by: 蔡佳誠 Louis Tsai <72684086+LouisTsai-Csie@users.noreply.github.com>
85a8111 to
38e234b
Compare
Description
Add
test_fcu_custody_columns_null, pinning the 2026-08-12 EIP-8070 clarification that anullcustodyColumnsonengine_forkchoiceUpdatedV4is a blobpool no-op accepted with VALID, both on a client with no custody set and after one was configured, andtest_fcu_custody_columns_identicalfor the other no-op case in the same rule, resending the current set. The blobs test now takescustody_columns_updates, an ordered list ofcustodyColumnsvalues sent one forkchoice update each beforegetBlobsV4, whereNonesends an explicitnull; leaving it unset sends no update, and an empty list is rejected by a validator with a unit test. Repin the suite to the current blob, which also carries the RLP empty-list clarification for elided blobs; the framework already encodes that. Execute-only suite, socheck_eip_versionsnever runs its version item; the collection shows 9 cases andjust staticis clean.These arms pin what the Engine API can observe: the value is well-formed, the forkchoice update completes with VALID, and
getBlobsV4still serves every cell afterwards. The resulting custody set only steers devp2p sampling of peer-announced transactions and is not observable here; that needs a devp2p eth/72 harness, which hive does not have yet.Related Issues or PRs
#3217 (EIP-8070 section). Follows #2948, #3344, #3359.
Checklist
just static<type>(<area>): <title>, where<type>and<area>come from an appropriateC-<type>, respectivelyA-<area>, label. The title should match the target squash commit message.Cute Animal Picture