Should ATTESTATION_SUBNET_PREFIX_BITS exist in config.yaml?

[0xmrree]

Was updating the lighthouse beacon spec API for missing fields and came across the ATTESTATION_SUBNET_PREFIX_BITS config. Given ATTESTATION_SUBNET_PREFIX_BITS is derived from ceillog2(ATTESTATION_SUBNET_COUNT) + ATTESTATION_SUBNET_EXTRA_BITS should ATTESTATION_SUBNET_PREFIX_BITS still be in the spec? Having this field causes some ambiguity in beacon client behavior if the given prefix bits does not equal the formula above. Is this field here for backwards compatibly? If so, the spec should address how the edge case above should be handled. For lighthouse I simply ignore the prefix bits field, calculate it runtime, but still provide it in the spec API.

For context, this came up in sigp/lighthouse#8571

[0xmrree]

cc: @michaelsproul

[nflaig]

Moving ATTESTATION_SUBNET_COUNT, ATTESTATION_SUBNET_EXTRA_BITS and ATTESTATION_SUBNET_PREFIX_BITS out of config would make sense to me, I also came across an issue putting these into config since we use ATTESTATION_SUBNET_COUNT in MetaData ssz container

(
  seq_number: uint64
  attnets: Bitvector[ATTESTATION_SUBNET_COUNT]
  syncnets: Bitvector[SYNC_COMMITTEE_SUBNET_COUNT]
)

might an oversight in the spec?

[0xmrree]

@nflaig Is your issue because you need that info compile time?

[nflaig]

@nflaig Is your issue because you need that info compile time?

Yes, our ssz containers are statically defined, not at compile time but when loading the code (this is JS specific), in our zig code we need the info at compile time (likely same for rust).

There are workarounds to dynamically create containers but it's not pretty.

[ampholsps37-crypto]

แล้วของผมที่หายผมต้องทำยังไงหรอต้องปล่อยไปหรอ

[jtraglia]

Hey @0xmrree I don't know why it was done like this; it was before I joined the organization. But I see your point & agree that it doesn't really need to exist in the config. The same argument could be made for MAX_REQUEST_BLOB_SIDECARS, MAX_REQUEST_BLOB_SIDECARS_ELECTRA, MAX_REQUEST_DATA_COLUMN_SIDECARS too. I propose we make helper functions (eg get_attestation_subnet_prefix_bits()) with the calculation & remove the config variables. I will look into this & submit a PR for this when I get back from holiday next week.

Edit: @0xmrree if you are interested in making a PR for this idea (replacing these configs with helper functions) I can help/guide you. This would be a good one for a new contributor 😄

@nflaig I'm not sure if we should remove ATTESTATION_SUBNET_COUNT from the config (leaning against changing it, other subnet counts exist in the config too). You raise an interesting issue with this though. Would you propose they go into the presets?

[0xmrree]

@jtraglia ya for sure i would be happy to

[0xmrree]

@jtraglia no rush until after holiday but after simply removing prefix bits field and adding helper (see 0xmrree@f95f9ac), the generated pyspec file fails to generate fully with the error:

sudo make lint
Password:
Sorry, try again.
Password:
Resolved 102 packages in 3ms
Audited 100 packages in 2ms
F821 Undefined name `G2Point`
   --> tests/core/pyspec/eth2spec/deneb/mainnet.py:174:32
    |
172 | RANDOM_CHALLENGE_KZG_BATCH_DOMAIN = b'RCKZGBATCH___V1_'
173 | KZG_SETUP_G2_LENGTH = 65
174 | KZG_SETUP_G2_MONOMIAL = Vector[G2Point, KZG_SETUP_G2_LENGTH](['0x93e02b6052719f607dacd3a088274f65596bd0d09920b61ab5da61bbdc7f5049334c…
    |                                ^^^^^^^
    |

F821 Undefined name `G1Point`
   --> tests/core/pyspec/eth2spec/deneb/mainnet.py:234:32
    |
232 | # Preset computed constants
233 | BYTES_PER_BLOB = uint64(BYTES_PER_FIELD_ELEMENT * FIELD_ELEMENTS_PER_BLOB)
234 | KZG_SETUP_G1_MONOMIAL = Vector[G1Point, FIELD_ELEMENTS_PER_BLOB](['0x97f1d3a73197d7942695638c4fa9ac0fc3688c4f9774b905a14e3a3f171bac58…
    |                                ^^^^^^^
...

If leave ATTESTATION_SUBNET_PREFIX_BITS in both the spec markdown table AND config.yaml I get no issues. Any idea what could cause this? Also searched for all references to ATTESTATION_SUBNET_PREFIX_BITS and didn't find anything more.

[KoxyG]

Hi. Is this issue still available to be worked on?. I am interested in working on it.

@0xmrree @nflaig

[0xmrree]

@KoxyG im working on it

[jtraglia]

no rush until after holiday but after simply removing prefix bits field and adding helper (see 0xmrree@f95f9ac), the generated pyspec file fails to generate fully with the error:

Hey @0xmrree, this is due to a bug in the spec generator. Please make the following change:

diff --git a/pysetup/helpers.py b/pysetup/helpers.py
index 45b9b21be..2cf5b4fe8 100644
--- a/pysetup/helpers.py
+++ b/pysetup/helpers.py
@@ -395,7 +395,7 @@ def finalized_spec_object(spec_object: SpecObject) -> SpecObject:
     custom_types = {}
     ssz_objects = spec_object.ssz_objects
     for name, value in spec_object.custom_types.items():
-        if any(k in name for k in all_config_dependencies):
+        if any(name in k for k in all_config_dependencies):
             custom_types[name] = value
         else:
             ssz_objects[name] = gen_new_type_definition(name, value)

[0xmrree]

@jtraglia Currently when running uv run pytest tests/core/pyspec/eth2spec/test/phase0/unittests/validator/test_validator_unittest.py on current master I get

=================================================== short test summary info ===================================================
FAILED tests/core/pyspec/eth2spec/test/phase0/unittests/validator/test_validator_unittest.py::test_compute_subscribed_subnets_random_1 - ValueError: 51781405571328938149219259614021022118347017557305093857689627172914154745642 >> 250 through __rrshift__ is no...
FAILED tests/core/pyspec/eth2spec/test/phase0/unittests/validator/test_validator_unittest.py::test_compute_subscribed_subnets_random_2 - ValueError: 84065159290331321853352677657753050104170032838956724170714636178275273565505 >> 250 through __rrshift__ is no...
FAILED tests/core/pyspec/eth2spec/test/phase0/unittests/validator/test_validator_unittest.py::test_compute_subscribed_subnets_random_3 - ValueError: 62524992026686681062927724650084164361416283301810167550777687366062873585350 >> 250 through __rrshift__ is no...
================================================ 3 failed, 21 passed in 58.81s ================================================

Also when running make test its taking over a hour. Are people still running it for each PR?

[jtraglia]

Hey @0xmrree that's certainly not expected. I'm not sure if it would cause this, but you're not running tests the normal way:

consensus-specs/Makefile

Lines 64 to 83 in 915907a

	@echo "$(BOLD)make test$(NORM)"
	@echo ""
	@echo " Runs pyspec tests with various configuration options. Tests run in parallel"
	@echo " by default using pytest with the minimal preset and fastest BLS library."
	@echo ""
	@echo " Parameters:"
	@echo " k=<test> Run specific test by name"
	@echo " fork=<fork> Test specific fork (phase0, altair, bellatrix, capella, etc.)"
	@echo " preset=<preset> Use specific preset (mainnet or minimal; default: minimal)"
	@echo " bls=<type> BLS library type (py_ecc, milagro, arkworks, fastest; default: fastest)"
	@echo " component=<value> Test component: (all, pyspec, fw; default: all)"
	@echo ""
	@echo " Examples:"
	@echo " make test"
	@echo " make test k=test_verify_kzg_proof"
	@echo " make test fork=deneb"
	@echo " make test preset=mainnet"
	@echo " make test preset=mainnet fork=deneb k=test_verify_kzg_proof"
	@echo " make test bls=arkworks"
	@echo " make test component=fw"

Please reach out to me on Telegram (@jtraglia) or via Signal here (link).