Implement withdrawals in capella #4730
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
25 tasks
g11tech
force-pushed
the
g11tech/without-epoch-queue
branch
from
ec17f64
to
8101d98
Compare
g11tech
changed the title
g11tech
force-pushed
the
g11tech/without-epoch-queue
branch
from
6b45667
to
b6fdeff
Compare
twoeths
reviewed
Nov 11, 2022
twoeths
reviewed
Nov 11, 2022
g11tech
changed the title
Performance Report✔️ no performance regression detected Full benchmark results
|
|
Note from @g11tech : "Tests (e2e + sim) won't pass until ethereum/consensus-specs does a new release including latest spec tests and content. Promised data from researchers is ~ end of the week Nov 14 - Nov 18 |
dapplion
reviewed
Nov 18, 2022
| @@ -59,6 +62,11 @@ export function specTestIterator(configDirpath: string, testRunners: Record<stri | |||
| } | |||
|
|
|||
| for (const testHandler of readdirSyncSpec(testRunnerDirpath)) { | |||
| if ( | |||
| ["full_withdrawals", "partial_withdrawals", "bls_to_execution_change", "withdrawals"].includes(testHandler) | |||
There was a problem hiding this comment.
This will be removed once new spec tests land right?
There was a problem hiding this comment.
ummm, new test runners for them need to be written, is part of the TODO tracker here #4680
dapplion
suggested changes
Nov 18, 2022
packages/state-transition/src/block/processBlsToExecutionChange.ts
Outdated
Show resolved
Hide resolved
packages/state-transition/src/block/processBlsToExecutionChange.ts
Outdated
Show resolved
Hide resolved
packages/state-transition/src/block/processBlsToExecutionChange.ts
Outdated
Show resolved
Hide resolved
packages/state-transition/src/block/processBlsToExecutionChange.ts
Outdated
Show resolved
Hide resolved
dapplion
reviewed
Nov 21, 2022
packages/state-transition/test/perf/block/processWithdrawals.test.ts
Outdated
Show resolved
Hide resolved
dapplion
reviewed
Nov 21, 2022
packages/state-transition/test/perf/block/processWithdrawals.test.ts
Outdated
Show resolved
Hide resolved
|
@g11tech The test name is too long, could you drop "capella"? "getExpectedWithdrawals" is enough for now. Also remove "normalcase" tags for the long names for something that fits the print. Just use acronyms or something short that can be mapped to its meaning. Why does the |
twoeths
reviewed
Nov 22, 2022
packages/state-transition/src/block/processBlsToExecutionChange.ts
Outdated
Show resolved
Hide resolved
|
from latest run``` |
dapplion
suggested changes
Nov 23, 2022
There was a problem hiding this comment.
Something is wrong in the benchmarks still @g11tech the time to complete should grow progressively on each degraded case. If all times are the same this PR cannot be merged
getExpectedWithdrawals
✔ vc - 250000 eb 1 eth1 1 we 0 wn 0 121285.6 ops/s 8.245000 us/op - 646 runs 4.07 s
✔ vc - 250000 eb 0.95 eth1 0.1 we 0.05 wn 0 15963.99 ops/s 62.64100 us/op - 443 runs 3.01 s
✔ vc - 250000 eb 0.95 eth1 0.3 we 0.05 wn 0 21.34639 ops/s 46.84633 ms/op - 31 runs 3.35 s
✔ vc - 250000 eb 0.95 eth1 0.7 we 0.05 wn 0 21.09553 ops/s 47.40340 ms/op - 37 runs 3.59 s
✔ vc - 250000 eb 0 eth1 0 we 0 wn 0 21.35403 ops/s 46.82957 ms/op - 12 runs 3.60 s
✔ vc - 250000 eb 0 eth1 0 we 0 wn 0 nocache 7.729008 ops/s 129.3827 ms/op - 12 runs 3.13 s
|
@g11tech Benchmarks fixed 👍 reverted to using a rand() function but deterministic |
g11tech
force-pushed
the
g11tech/without-epoch-queue
branch
from
84f2544
to
5e708e2
Compare
g11tech
force-pushed
the
g11tech/without-epoch-queue
branch
from
eedd868
to
c49bf1a
Compare
process partial full withdrawals block processWithdrawals and processBlsToExecutionChange handle serialize/deserialize for execution api stub interop with the geth add withdrawals CI test run update spec version fix some of tests skip unsupported spec tests fix unit tests fix issues and some of specs as a result code improvement fix the credential change fix spec tests, skip the ones which are confirmed not correct reset yarn lock cleanup added libs improve wording Co-authored-by: Lion - dapplion <[email protected]> improve wording Co-authored-by: Lion - dapplion <[email protected]> add blsToExecutionChanges empty list while producing block fix the genesis state init and genesis spec tests for capella fix the amount conversion lint handle capella state for test/utils/state remove epoch queue and process withdrawals on block processing remove WithdrawalQueue from types lint add withdrawals and txs to the log fix amount in tests update the fixed geth image and fix tests interop with ethereumjs add nethermind image run to the withdrawals ci run fix the log gen fix killing error lint simplify eth1 prefix address check add interop have deposits with withdrawals enabled for testing lint handle withdrawal null response replace latest validator index by next validator index logic trigger saturate balance decrease to 0 use validdators.getReadonly remove recipient update the latest to next validator index in the test file use genesisTemplate as the template var Co-authored-by: Lion - dapplion <[email protected]> improve wording of error Co-authored-by: Lion - dapplion <[email protected]> incorporate feedback fixes fix test add perf test for getExpectedWithdrawals fix perf test pass config for phase0 to create cache fix perf test update balance for withdraw fix the random generation fix the epoch in expected withdrawals refac perf test data gen comment cleanup shorten the log further shorten fix data gen improve comment Fix performance tests lint fix prep for bls to execution changes validation signature test Fix benchmarks fix build fix test fix type error reset the test fix the pubkey for withdrawal signature validation export capella types individually disable nethermind withdrawals port back to previous spec version fix build fix spec name fix val
g11tech
force-pushed
the
g11tech/without-epoch-queue
branch
from
c49bf1a
to
4649ebc
Compare
dapplion
reviewed
Nov 26, 2022
dapplion
reviewed
Nov 26, 2022
dapplion
reviewed
Nov 26, 2022
dapplion
reviewed
Nov 26, 2022
dapplion
force-pushed
the
g11tech/without-epoch-queue
branch
from
66f7c90
to
fa85faf
Compare
dapplion
approved these changes
Nov 26, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
TODO:
new spec tests release awaited for integration, else they pass (tested locally from the shared google drive tar)