bound the maximum number of validators considered for withdrawals per sweep #3095

ralexstokes · 2022-11-11T17:48:50Z

this PR does a few things:

bound the size of the sweep in the worst case to the constant MAX_VALIDATORS_PER_WITHDRAWALS_SWEEP
implement some notion of "fairness" if there were no validators found in the current sweep

the rationale for (2) is without it the processing will just stay at the same place in the validator set until there is some withdrawal to advance the state of the sweep

(2) adds some complexity to the implementation and I am not convinced it pulls its weight -- if you don't see the value please chime in below and I'll remove from this PR

if/when there is general consensus on this change then I'll add some testing

potuz · 2022-11-11T17:59:14Z

I like this change and I think 2) is actually necessary cause it may be that all validators are withrawn in the sweep interval and then we would be stuck if we don't advance. I suggest changing the check to when there were less than the maximum, instead of none

dapplion

LGTM, 2**17 sounds like a reasonable choice for capella

djrtwo

This needs consensus tests

I'm also loosely against the proposal. I don't think there has been a strong case made for the additional complexity and the selected current value

specs/capella/beacon-chain.md

djrtwo · 2022-11-17T23:35:21Z

re discussion on latest cl call: my preference order is

Do nothing if doing a full sweep will not introduce any requisite complexity to ensure load is fine (e.g. precomputes, caches, etc)
If we'll have to add caching/precompute for this edge case (if this PR doesn't get added), then add this logic and a lower per-slot bound so we don't have to really think about it at all -- e.g. to 2^10 or something. Just make it trivially costly to avoid having to bench or optimize or worry at all.

Based on convos, I think (1) is uncertain so I'm now in camp (2)

rolfyone · 2022-11-22T20:34:41Z

I like this change and I think 2) is actually necessary cause it may be that all validators are withrawn in the sweep interval and then we would be stuck if we don't advance. I suggest changing the check to when there were less than the maximum, instead of none

I think the suggestion @potuz made about advancing the sweep if less than the max were found is important, or everyone after the last index gets another crack at it... More importantly, everyone in the range after the last validator are clearly not selectable as at the previous slot...

So if max is 4, and we start from 0, and get 0,1,2 in the list, then scan the rest of the range and don't find another one, we know that we should start after that sweep range, because 3..2^15 (or whatever the number becomes) won't have had a partial/full withdrawal that could be processed when we scanned the last slot... so really we'd just be scanning the first 3 as real potential additions... which is close enough to stalling the scan...

Overall the change makes sense, especially once we fix this potential stall issue.

dapplion · 2022-11-23T10:23:39Z

Run some benchmarks for Lodestar's implementation. Each case has 4 parameters

eb: chance next index has excess balance
eth1: chance next index has eth1 credentials
we: chance next index is withdrawable
wn: chance next index is withdrawn

This does not reflect mainnet since clusters of keys will act together controlled by the same operator. But it forces different counts of total sampled validators smpl. nocache means requiring a tree traversal per validator sample, absence of the tag means 1 random vector access per validator sample.

Worst case is not great but the node could survive with it. Note that this tests are for an old state with 250k vals. Mainnet should ~ double the worst case results. Would be interesting to analyze mainnet and understand how interlaced or not are indexes of independent actors.

  getExpectedWithdrawals
    ✔ vc - 250000 eb 1 eth1 1 we 0 wn 0 - smpl 15                         23630.61 ops/s    42.31800 us/op        -       2419 runs   5.45 s
    ✔ vc - 250000 eb 0.95 eth1 0.1 we 0.05 wn 0 - smpl 219                11216.92 ops/s    89.15100 us/op        -        304 runs   2.93 s
    ✔ vc - 250000 eb 0.95 eth1 0.3 we 0.05 wn 0 - smpl 42                 16537.95 ops/s    60.46700 us/op        -        703 runs   3.08 s
    ✔ vc - 250000 eb 0.95 eth1 0.7 we 0.05 wn 0 - smpl 18                 20744.31 ops/s    48.20600 us/op        -        317 runs   2.50 s
    ✔ vc - 250000 eb 0.1 eth1 0.1 we 0 wn 0 - smpl 1020                   3250.098 ops/s    307.6830 us/op        -       1120 runs   4.12 s
    ✔ vc - 250000 eb 0.03 eth1 0.03 we 0 wn 0 - smpl 11777                876.3505 ops/s    1.141096 ms/op        -        813 runs   3.77 s
    ✔ vc - 250000 eb 0.01 eth1 0.01 we 0 wn 0 - smpl 141069               93.73472 ops/s    10.66841 ms/op        -         64 runs   2.07 s
    ✔ vc - 250000 eb 0 eth1 0 we 0 wn 0 - smpl 250000                     55.27471 ops/s    18.09146 ms/op        -         31 runs   1.93 s
    ✔ vc - 250000 eb 0 eth1 0 we 0 wn 0 nocache - smpl 250000             13.51712 ops/s    73.98023 ms/op        -         13 runs   2.33 s
    ✔ vc - 250000 eb 0 eth1 1 we 0 wn 0 - smpl 250000                     28.66458 ops/s    34.88626 ms/op        -         19 runs   2.07 s
    ✔ vc - 250000 eb 0 eth1 1 we 0 wn 0 nocache - smpl 250000             9.634900 ops/s    103.7894 ms/op        -         12 runs   2.58 s

specs/capella/beacon-chain.md

rolfyone · 2022-11-25T20:46:27Z

I had a quick look at mainnet...
Currently the first validator id with a 0x01 is 120374, and there's about 182k validators that have a 0x01 withdrawal address...

So in around 1.5 days we'd be doing all the withdrawals....

Given the number of 0x01 withdrawal addresses already set, we probably don't need to be too concerned with limiting the scan distance, as that first leap of 120374 is not that huge...

I guess that means every 1.5 days we'll be doing that large scan, until some of those update their withdrawal addresses, which probably won't take very long.

My point, I guess, is that maybe this scan limit isn't really needed?

specs/capella/beacon-chain.md

dapplion · 2022-12-05T05:51:49Z

My point, I guess, is that maybe this scan limit isn't really needed?

Yes, it's great to know that block processing time worst case is bounded. This is a guarantee I really want to have unless the bounded sweep has drawbacks.

presets/minimal/capella.yaml

rolfyone · 2022-12-05T20:51:34Z

If we do this, our upper limit should be more like 2^17, which would ensure under current mainnet conditions we don't have 120 blocks per sweep with no withdrawals (1k per sweep, first 120k ish are 0x00) ....
My mainnet test from a state i forwarded to capella had around 8ms to select from that, which i think is worth it to not have several epochs of blocks with no withdrawals.

specs/capella/beacon-chain.md

djrtwo

I need to do a bit of a test pass still but wanted to get the initial comments out.

thanks!

tests/core/pyspec/eth2spec/test/capella/block_processing/test_process_withdrawals.py

djrtwo · 2022-12-08T13:55:17Z

tests/core/pyspec/eth2spec/test/capella/block_processing/test_process_withdrawals.py

    elif len(expected_withdrawals) < spec.MAX_WITHDRAWALS_PER_PAYLOAD:
-        assert len(spec.get_expected_withdrawals(state)) == 0
+        bound = min(spec.MAX_VALIDATORS_PER_WITHDRAWALS_SWEEP, spec.MAX_WITHDRAWALS_PER_PAYLOAD)


this seems like a weird code path. I think we should just consider configs (we make very few) in which the withdrawals per payload is less than the sweep bound as invalid.

this is less about the size of the sweep and more just about the scenario where you do the full sweep (of whatever size) and only have a small number of withdrawals

I think all three arms are realistic cases -- do you see it differently?

but isn't the bound always going to be MAX_WITHDRAWALS_PER_PAYLOAD here? I mean there is no sense in defining a config with MAX_VALIDATOR_PER_WITHDRAWAL_SWEEP being smaller since then we would never attain the MAX_WITHDRAWAL_PER_PAYLOAD and therefore is equivalent to simply lower it.

you are right that for the current parameterization across mainnet and minimal the bound will always be MAX_WITHDRAWALS_PER_PAYLOAD although that is not necessarily the case

we can separately make the commitment that we will never do this but its the type of thing that is hard to track and then enforce...

separately there is the point about this arm of this conditional block and I am imagining a scenario where there are a handful of validators, less than MAX_WITHDRAWALS_PER_PAYLOAD, which are withdrawable e.g. going into an inactivity leak

tests/core/pyspec/eth2spec/test/capella/block_processing/test_process_withdrawals.py

specs/capella/beacon-chain.md

tests/core/pyspec/eth2spec/test/capella/block_processing/test_process_withdrawals.py

specs/capella/beacon-chain.md

… sweep

tests/core/pyspec/eth2spec/test/capella/block_processing/test_process_withdrawals.py

djrtwo

Okay, let's get this merged and released. I suggest a 2^14 for mainnet value as a compromise

specs/capella/beacon-chain.md

lucassaldanha

LGTM. Teku has already implemented the new logic in a branch and is ready to go! 🚀

This PR implements ethereum/consensus-specs#3095

* Implement bound on expected withdrawals This PR implements ethereum/consensus-specs#3095 * Terence's suggestion Co-authored-by: terencechain <terence@prysmaticlabs.com> * Update spectests * fix minimal preset * Clear committee cache during header tests to ensure active validators ain't leaking * Rm debug log * Rm bad init * Update consensus spec sha * Fix bad tests and remove redundant casting * Fix build * MAX_VALIDATORS_PER_WITHDRAWALS_SWEEP typo Co-authored-by: terencechain <terence@prysmaticlabs.com> Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com>

ralexstokes force-pushed the bound-withdrawals-sweep branch from 156468c to 57fdbfa Compare November 11, 2022 17:56

dapplion approved these changes Nov 11, 2022

View reviewed changes

hwwhww added the Capella label Nov 11, 2022

ralexstokes mentioned this pull request Nov 14, 2022

Consensus-layer Call 98 ethereum/pm#660

Closed

djrtwo reviewed Nov 15, 2022

View reviewed changes

specs/capella/beacon-chain.md Show resolved Hide resolved

ralexstokes force-pushed the bound-withdrawals-sweep branch from 57fdbfa to 920b3d0 Compare November 22, 2022 15:58

dapplion reviewed Nov 23, 2022

View reviewed changes

specs/capella/beacon-chain.md Outdated Show resolved Hide resolved

ralexstokes force-pushed the bound-withdrawals-sweep branch 8 times, most recently from a0796b9 to a05cb87 Compare November 30, 2022 21:01

ralexstokes mentioned this pull request Nov 30, 2022

Consensus-layer Call 99 ethereum/pm#667

Closed

ralexstokes closed this Nov 30, 2022

ralexstokes reopened this Nov 30, 2022

ralexstokes force-pushed the bound-withdrawals-sweep branch from a05cb87 to 1eaa441 Compare November 30, 2022 21:23

potuz reviewed Dec 1, 2022

View reviewed changes

specs/capella/beacon-chain.md Outdated Show resolved Hide resolved

ralexstokes commented Dec 5, 2022

View reviewed changes

presets/minimal/capella.yaml Outdated Show resolved Hide resolved

ralexstokes force-pushed the bound-withdrawals-sweep branch 2 times, most recently from 4431dae to 24a8e22 Compare December 5, 2022 20:44

potuz reviewed Dec 8, 2022

View reviewed changes

specs/capella/beacon-chain.md Outdated Show resolved Hide resolved

djrtwo reviewed Dec 8, 2022

View reviewed changes

specs/capella/beacon-chain.md Outdated Show resolved Hide resolved

djrtwo reviewed Dec 8, 2022

View reviewed changes

ralexstokes force-pushed the bound-withdrawals-sweep branch 4 times, most recently from b631e70 to 240ba48 Compare December 8, 2022 20:38

djrtwo reviewed Dec 8, 2022

View reviewed changes

specs/capella/beacon-chain.md Outdated Show resolved Hide resolved

djrtwo reviewed Dec 8, 2022

View reviewed changes

tests/core/pyspec/eth2spec/test/capella/block_processing/test_process_withdrawals.py Show resolved Hide resolved

dapplion reviewed Dec 9, 2022

View reviewed changes

specs/capella/beacon-chain.md Show resolved Hide resolved

ralexstokes force-pushed the bound-withdrawals-sweep branch 2 times, most recently from ae79349 to 4cae27a Compare December 9, 2022 17:47

bound the maximum number of validators considered for withdrawals per…

37e504e

… sweep

ralexstokes force-pushed the bound-withdrawals-sweep branch from 4cae27a to 37e504e Compare December 9, 2022 17:51

djrtwo reviewed Dec 12, 2022

View reviewed changes

tests/core/pyspec/eth2spec/test/capella/block_processing/test_process_withdrawals.py Show resolved Hide resolved

djrtwo reviewed Dec 12, 2022

View reviewed changes

tests/core/pyspec/eth2spec/test/capella/block_processing/test_process_withdrawals.py Show resolved Hide resolved

lint

478b437

djrtwo approved these changes Dec 12, 2022

View reviewed changes

update mainnet preset value for sweep size

a062757

djrtwo reviewed Dec 12, 2022

View reviewed changes

specs/capella/beacon-chain.md Outdated Show resolved Hide resolved

Update specs/capella/beacon-chain.md

1f943f0

dapplion approved these changes Dec 13, 2022

View reviewed changes

lucassaldanha mentioned this pull request Dec 13, 2022

Limit number of validators checked during withdrawals lookup Consensys/teku#6592

Closed

lucassaldanha approved these changes Dec 13, 2022

View reviewed changes

djrtwo merged commit 06d6d38 into ethereum:dev Dec 13, 2022

ralexstokes deleted the bound-withdrawals-sweep branch December 13, 2022 04:03

potuz added a commit to prysmaticlabs/prysm that referenced this pull request Dec 13, 2022

Implement bound on expected withdrawals

52a39f3

This PR implements ethereum/consensus-specs#3095

potuz mentioned this pull request Dec 13, 2022

Implement bound on expected withdrawals prysmaticlabs/prysm#11760

Merged

twoeths mentioned this pull request Dec 17, 2022

Update to spec tests v1.3.0 alpha.2 ChainSafe/lodestar#4917

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bound the maximum number of validators considered for withdrawals per sweep #3095

bound the maximum number of validators considered for withdrawals per sweep #3095

ralexstokes commented Nov 11, 2022 •

edited

Loading

potuz commented Nov 11, 2022

dapplion left a comment

djrtwo left a comment

djrtwo commented Nov 17, 2022 •

edited

Loading

rolfyone commented Nov 22, 2022

dapplion commented Nov 23, 2022

rolfyone commented Nov 25, 2022 •

edited

Loading

dapplion commented Dec 5, 2022

rolfyone commented Dec 5, 2022

djrtwo left a comment

djrtwo Dec 8, 2022

ralexstokes Dec 8, 2022

potuz Dec 8, 2022

ralexstokes Dec 8, 2022

djrtwo left a comment

lucassaldanha left a comment

bound the maximum number of validators considered for withdrawals per sweep #3095

bound the maximum number of validators considered for withdrawals per sweep #3095

Conversation

ralexstokes commented Nov 11, 2022 • edited Loading

potuz commented Nov 11, 2022

dapplion left a comment

Choose a reason for hiding this comment

djrtwo left a comment

Choose a reason for hiding this comment

djrtwo commented Nov 17, 2022 • edited Loading

rolfyone commented Nov 22, 2022

dapplion commented Nov 23, 2022

rolfyone commented Nov 25, 2022 • edited Loading

dapplion commented Dec 5, 2022

rolfyone commented Dec 5, 2022

djrtwo left a comment

Choose a reason for hiding this comment

djrtwo Dec 8, 2022

Choose a reason for hiding this comment

ralexstokes Dec 8, 2022

Choose a reason for hiding this comment

potuz Dec 8, 2022

Choose a reason for hiding this comment

ralexstokes Dec 8, 2022

Choose a reason for hiding this comment

djrtwo left a comment

Choose a reason for hiding this comment

lucassaldanha left a comment

Choose a reason for hiding this comment

ralexstokes commented Nov 11, 2022 •

edited

Loading

djrtwo commented Nov 17, 2022 •

edited

Loading

rolfyone commented Nov 25, 2022 •

edited

Loading