Improve `get_custody_columns` validation, caching and error handling by jimmygchen · Pull Request #6308 · sigp/lighthouse

jimmygchen · 2024-08-27T06:58:24Z

Issue Addressed

This PR addresses two issues:

Add validation to get_custody_columns to prevent infinite loop when supplied custody subnet count is invalid (> total number of data column subnets).
Refactor error handling of network_globals.custody_columns to the caller site (see @pawanjay176's comment here)
Store the node's custody subnets and columns on NetworkGlobals initialization rather than computing it every time.

Error handling

For node's custody_subnet_count from local metadata:

if custody subnet count is not set or invalid after PeerDAS is enabled, we just panic on startup - this indicates a bug in the code, and we should detect this immediately on testing

For peer's custody_subnet_count from ENR & metadata:

if custody_subnet_count is invalid, fall back to custody_requirement
I think it's fine to not penalise peer, if they can still serve the columns from custody_requirement, they are still useful to us

jimmygchen · 2024-08-29T07:46:30Z

I don't really like the complexity in error handling here, will work on simplifying it tomorrow.

jimmygchen · 2024-08-30T13:11:06Z

@pawanjay176 I've re-written the PR and I think it's in a better place now and ready for review.

# Conflicts: # beacon_node/lighthouse_network/src/discovery/subnet_predicate.rs # beacon_node/lighthouse_network/src/peer_manager/peerdb.rs # beacon_node/lighthouse_network/src/peer_manager/peerdb/peer_info.rs # beacon_node/lighthouse_network/src/types/globals.rs # beacon_node/network/src/service.rs # consensus/types/src/data_column_subnet_id.rs

…ute_custody_requirement_subnets` function.

…d update logs.

…onnection since we get them on metadata anyway.

dapplion · 2024-09-04T13:04:29Z

+    try_create_int_gauge_vec(
+        "peers_per_custody_subnet_count",
+        "The current count of peers by custody subnet count",
+        &["custody_subnet_count"],


I would consider doing a histogram here as we can have buckets instead of exact values. Say we have a peer with csc 128 and another with 127, can grafana bucket these two labels into the same range?

Yeah I think buckets are useful, but historgrams accumulate observersations over time and it doesn't have the ability to track the current count for each bucket at a specific moment. I feel like gauge is more suitable but if you have a way to represent the metric in histogram I'm interested to hear.

In Lodestar we reset the histogram buckets before every scrape. It feels weird but renders nicely on Grafana.

I think it's a good idea. I've tried it and unfortunately couldn't find a way to set it with the prometheus crate though.

Closest thing that I could find was histogram.metric().clear_histogram(); but it doesn't do anything to the actual metric.

I'm going to merge this now, if we find any better way to do it, happy to do it in a follow up PR.

dapplion

LGTM!

jimmygchen · 2024-09-06T06:56:09Z

@mergify queue

mergify · 2024-09-06T06:56:46Z

queue

✅ The pull request has been merged automatically

Details

The pull request has been merged automatically at c0b4f01

Add retropgf funding (sigp#6324) Migrate from `ethereum-types` to `alloy-primitives` (sigp#6078) * Remove use of ethers_core::RlpStream * Merge branch 'unstable' of https://github.com/sigp/lighthouse into remove_use_of_ethers_core * Remove old code * Simplify keccak call * Remove unused package * Merge branch 'unstable' of https://github.com/ethDreamer/lighthouse into remove_use_of_ethers_core * Merge branch 'unstable' into remove_use_of_ethers_core * Run clippy * Merge branch 'remove_use_of_ethers_core' of https://github.com/dospore/lighthouse into remove_use_of_ethers_core * Check all cargo fmt * migrate to alloy primitives init * fix deps * integrate alloy-primitives * resolve dep issues * more changes based on dep changes * add TODOs * Merge branch 'unstable' of https://github.com/sigp/lighthouse into remove_use_of_ethers_core * Revert lock * Add BeaconBlocksByRange v3 * continue migration * Revert "Add BeaconBlocksByRange v3" This reverts commit e3ce7fc. * impl hash256 extended trait * revert some uneeded diffs * merge conflict resolved * fix subnet id rshift calc * rename to FixedBytesExtended * debugging * Merge branch 'unstable' of https://github.com/sigp/lighthouse into migrate-to-alloy-primitives * fix failed test * fixing more tests * Merge branch 'unstable' of https://github.com/sigp/lighthouse into remove_use_of_ethers_core * introduce a shim to convert between the two u256 types * move alloy to wrokspace * align alloy versions * update * update web3signer test certs * refactor * resolve failing tests * linting * fix graffiti string test * fmt * fix ef test * resolve merge conflicts * remove udep and revert cert * cargo patch * cyclic dep * fix build error * Merge branch 'unstable' of https://github.com/sigp/lighthouse into migrate-to-alloy-primitives * resolve conflicts, update deps * merge unstable * fmt * fix deps * Merge branch 'unstable' of https://github.com/sigp/lighthouse into migrate-to-alloy-primitives * resolve merge conflicts * resolve conflicts, make necessary changes * Remove patch * fmt * remove file * merge conflicts * sneaking in a smol change * bump versions * Merge remote-tracking branch 'origin/unstable' into migrate-to-alloy-primitives * Updates for peerDAS * Update ethereum_hashing to prevent dupe * updated alloy-consensus, removed TODOs * cargo update * endianess fix * Merge branch 'unstable' of https://github.com/sigp/lighthouse into migrate-to-alloy-primitives * fmt * fix merge * fix test * fixed_bytes crate * minor fixes * convert u256 to i64 * panic free mixin to_low_u64_le * from_str_radix * computbe_subnet api and ensuring we use big-endian * Merge branch 'unstable' of https://github.com/sigp/lighthouse into migrate-to-alloy-primitives * fix test * Simplify subnet_id test * Simplify some more tests * Add tests to fixed_bytes crate * Merge branch 'unstable' into migrate-to-alloy-primitives update libp2p to version 0.54 (sigp#6249) * update libp2p to version 0.54.0 * address review * Merge branch 'unstable' of github.com:sigp/lighthouse into update-libp2p * Merge branch 'update-libp2p' of github.com:sigp/lighthouse into update-libp2p Drop block data from BlockError and BlobError (sigp#5735) * Drop block data from BlockError and BlobError * Debug release tests * Fix release tests * Revert unnecessary changes to lighthouse_metrics Update manual checkpoint sync content in Lighthouse book (sigp#6338) * Update manual checkpiont sync * Update faq * Minor revision * Minor revision Light Client Bug Fix (sigp#6299) * Light Client Bug Fix Ignore Rust 1.82 warnings about void patterns (sigp#6357) * Ignore Rust 1.82 warnings about void patterns Enable `large_stack_frames` lint (sigp#6343) * Enable `large_stack_frames` lint Add blob count label to `DATA_COLUMN_SIDECAR_COMPUTATION` metric (sigp#6340) * Add blob count label to `DATA_COLUMN_SIDECAR_COMPUTATION` metric, and move metrics into the compute function, recording only successful computation. * Move `discard_timer_on_break` usage to caller site. * Merge branch 'unstable' into compute-data-column-metric * Merge branch 'unstable' into compute-data-column-metric Metadata request ordering (sigp#6336) * Send metadata request ordering * Merge branch 'unstable' into metadata-order Clarify validator monitor block log (sigp#6342) * Clarify validator monitor block log * Merge branch 'unstable' into clarify-block-log Check known parent on rpc blob process (sigp#5893) * Check known parent on rpc blob process * fix test * Merge branch 'unstable' of https://github.com/sigp/lighthouse into blob-unknown-parent Remove beta tag from gossipsub 1.2 (sigp#6344) * Remove the beta tag from gossipsub v1.2 * fix clippy * Merge branch 'unstable' into remove-beta-tag Fix lints for Rust 1.81 (sigp#6363) * Fix lints for Rust 1.81 Use tikv-jemallocator instead of jemallocator (sigp#6354) * Use tikv-jemallocator instead of jemallocator * Merge branch 'unstable' into tikv-jemallocator * Bump tikv-jemallocator and tikv-jemalloc-ctl Improve `get_custody_columns` validation, caching and error handling (sigp#6308) * Improve `get_custody_columns` validation, caching and error handling. * Merge branch 'unstable' into get-custody-columns-error-handing * Fix failing test and add more test. * Fix failing test and add more test. * Merge branch 'unstable' into get-custody-columns-error-handing * Add unit test to make sure the default specs won't panic on the `compute_custody_requirement_subnets` function. * Add condition when calling `compute_custody_subnets_from_metadata` and update logs. * Validate `csc` when returning from enr. Remove `csc` computation on connection since we get them on metadata anyway. * Add `peers_per_custody_subnet_count` to track peer csc and supernodes. * Disconnect peers with invalid metadata and find other peers instead. * Fix sampling tests. * Merge branch 'unstable' into get-custody-columns-error-handing * Merge branch 'unstable' into get-custody-columns-error-handing Delete legacy payload reconstruction (sigp#6213) * Delete legacy payload reconstruction * Delete unneeded failing test * Merge remote-tracking branch 'origin/unstable' into remove-more-ethers * Merge remote-tracking branch 'origin/unstable' into remove-more-ethers * Cleanups simplify rpc codec logic (sigp#6304) * simplify rpc codec logic * Merge branch 'unstable' of github.com:sigp/lighthouse into simplify-rpc-codec * Merge branch 'unstable' of github.com:sigp/lighthouse into simplify-rpc-codec * Merge branch 'unstable' of github.com:sigp/lighthouse into simply-rpc-codec * Merge branch 'unstable' into simplify-rpc-codec * Merge branch 'unstable' into simplify-rpc-codec

…igp#6308) * Improve `get_custody_columns` validation, caching and error handling. * Merge branch 'unstable' into get-custody-columns-error-handing * Fix failing test and add more test. * Fix failing test and add more test. * Merge branch 'unstable' into get-custody-columns-error-handing # Conflicts: # beacon_node/lighthouse_network/src/discovery/subnet_predicate.rs # beacon_node/lighthouse_network/src/peer_manager/peerdb.rs # beacon_node/lighthouse_network/src/peer_manager/peerdb/peer_info.rs # beacon_node/lighthouse_network/src/types/globals.rs # beacon_node/network/src/service.rs # consensus/types/src/data_column_subnet_id.rs * Add unit test to make sure the default specs won't panic on the `compute_custody_requirement_subnets` function. * Add condition when calling `compute_custody_subnets_from_metadata` and update logs. * Validate `csc` when returning from enr. Remove `csc` computation on connection since we get them on metadata anyway. * Add `peers_per_custody_subnet_count` to track peer csc and supernodes. * Disconnect peers with invalid metadata and find other peers instead. * Fix sampling tests. * Merge branch 'unstable' into get-custody-columns-error-handing * Merge branch 'unstable' into get-custody-columns-error-handing

jimmygchen added the das Data Availability Sampling label Aug 27, 2024

jimmygchen mentioned this pull request Aug 27, 2024

Implement Metadatav3 #6303

Merged

jimmygchen commented Aug 27, 2024

View reviewed changes

Comment thread beacon_node/lighthouse_network/src/discovery/subnet_predicate.rs Outdated

jimmygchen force-pushed the get-custody-columns-error-handing branch 2 times, most recently from b9a2cfd to b455de6 Compare August 30, 2024 13:08

jimmygchen changed the title ~~Add validation to get_custody_columns and improve error handling~~ Improve get_custody_columns validation, caching and error handling Aug 30, 2024

jimmygchen marked this pull request as ready for review August 30, 2024 13:11

jimmygchen added the ready-for-review The code is ready for review label Aug 30, 2024

Improve get_custody_columns validation, caching and error handling.

3e040e5

jimmygchen force-pushed the get-custody-columns-error-handing branch from b4c6f0d to 3e040e5 Compare August 30, 2024 13:19

Merge branch 'unstable' into get-custody-columns-error-handing

a51dd65

jimmygchen requested a review from pawanjay176 August 30, 2024 13:20

Fix failing test and add more test.

ce9020c

jimmygchen force-pushed the get-custody-columns-error-handing branch from f3e99e9 to 73a340c Compare August 30, 2024 14:12

Fix failing test and add more test.

82a578f

jimmygchen force-pushed the get-custody-columns-error-handing branch from 73a340c to 82a578f Compare August 30, 2024 14:30

dapplion reviewed Sep 2, 2024

View reviewed changes

jimmygchen added 3 commits September 3, 2024 12:30

Add unit test to make sure the default specs won't panic on the `comp…

763ea03

…ute_custody_requirement_subnets` function.

Add condition when calling compute_custody_subnets_from_metadata an…

e8b9d76

…d update logs.

jimmygchen added work-in-progress PR is a work-in-progress and removed ready-for-review The code is ready for review labels Sep 3, 2024

Validate csc when returning from enr. Remove csc computation on c…

0a717ff

…onnection since we get them on metadata anyway.

jimmygchen force-pushed the get-custody-columns-error-handing branch from 5def040 to 0a717ff Compare September 3, 2024 07:59

dapplion reviewed Sep 3, 2024

View reviewed changes

Comment thread beacon_node/lighthouse_network/src/peer_manager/peerdb/peer_info.rs Outdated

jimmygchen added 3 commits September 4, 2024 20:53

Add peers_per_custody_subnet_count to track peer csc and supernodes.

d82c806

Disconnect peers with invalid metadata and find other peers instead.

067307a

Fix sampling tests.

502436d

jimmygchen added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels Sep 4, 2024

jimmygchen requested a review from dapplion September 4, 2024 12:50

Merge branch 'unstable' into get-custody-columns-error-handing

906e2a2

dapplion reviewed Sep 4, 2024

View reviewed changes

Merge branch 'unstable' into get-custody-columns-error-handing

889c9da

dapplion approved these changes Sep 5, 2024

View reviewed changes

jimmygchen added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Sep 6, 2024

mergify Bot added a commit that referenced this pull request Sep 6, 2024

Merge of #6308

2d13023

mergify Bot mentioned this pull request Sep 6, 2024

merge queue: embarking unstable (0c5e25b) and #6308 together #6367

Closed

5 tasks

mergify Bot merged commit c0b4f01 into sigp:unstable Sep 6, 2024

jimmygchen mentioned this pull request Sep 9, 2024

Investigate flaky sampling tests #6319

Closed

Conversation

jimmygchen commented Aug 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue Addressed

Error handling

Uh oh!

Uh oh!

jimmygchen commented Aug 29, 2024

Uh oh!

jimmygchen commented Aug 30, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dapplion Sep 4, 2024

Choose a reason for hiding this comment

Uh oh!

jimmygchen Sep 5, 2024

Choose a reason for hiding this comment

Uh oh!

dapplion Sep 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimmygchen Sep 6, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dapplion left a comment

Choose a reason for hiding this comment

Uh oh!

jimmygchen commented Sep 6, 2024

Uh oh!

mergify Bot commented Sep 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ The pull request has been merged automatically

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jimmygchen commented Aug 27, 2024 •

edited

Loading

dapplion Sep 5, 2024 •

edited

Loading

mergify Bot commented Sep 6, 2024 •

edited

Loading