1.1.5 merge spec tests #2781

realbigsean · 2021-11-04T19:50:08Z

Issue Addressed

1.1.5 spec tests

Proposed Changes

Got rid of Union wrapper around Transaction Remove Union from ExecutionPayload transaction type ethereum/consensus-specs#2684
Updated tx sizes fix gossip and tx size limits for the merge ethereum/consensus-specs#2686
Updated penalty constants values update penalty params for Merge ethereum/consensus-specs#2698
increment spec test version to v1.1.5
Removed Gas validation remove gas validations from CL ethereum/consensus-specs#2699
Made TERMINAL_BLOCK_HASH_ACTIVATION_EPOCH available but not used anywhere yet add TBH_ACTIVATION_EPOCH ethereum/consensus-specs#2682
Remove an extraneous gossip condition: remove extraneous p2p condition ethereum/consensus-specs#2687

Additional Info

Does not include ssz static fixes
Does not include any fork choice related changes

…nt to use based on the state variant

paulhauner

Awesome work! I made one subjective suggestion, lmk what you think.

I also had a go at removing the Transaction new-type, which is possible because the enum was removed. Code is here paulhauner@63b01d5. Let me know what you think, it's nice to avoid the ssz/tree_hash manual impls.

consensus/state_processing/src/per_epoch_processing/altair/rewards_and_penalties.rs

realbigsean · 2021-11-05T13:40:11Z

I also had a go at removing the Transaction new-type, which is possible because the enum was removed. Code is here paulhauner/lighthouse@63b01d5. Let me know what you think, it's nice to avoid the ssz/tree_hash manual impls.

I did attempt to go this route after finishing the implementation in the PR. But I got hung up on generics in the Serialize/Deserialize implementations so gave up. My issue might have been that I was attempting to type Transaction on EthSpec instead of the var list length -> type Transaction<T> = VariableList<u8, T::MaxBytesPerTransaction>.

Anyways, I think this is much better! Was also wondering if adding #[ssz(transparent)] might be worth doing in the future? Was thinking of it as another route around the manual impls for a new-type.

…1.4-merge-specs � Conflicts: � consensus/types/src/execution_payload.rs

paulhauner · 2021-11-05T22:52:22Z

Anyways, I think this is much better! Was also wondering if adding #[ssz(transparent)] might be worth doing in the future?

Yeah, I think that would be great. That would have been a nice solution here, but involves some dank macro work that I didn't have an appetite for.

paulhauner

Very nice! Only a few very minor changes. Feel free to squash-merge after you've addressed them!

paulhauner · 2021-11-10T02:58:12Z

consensus/state_processing/src/per_block_processing.rs

-            .gas_limit
-            .safe_add(parent.gas_limit.safe_div(T::gas_limit_denominator())?)?
-    {
-        return Err(BlockProcessingError::ExecutionInvalidGasLimitIncrease {


We can remove these ExecutionInvalidGas* error variants, too.

paulhauner · 2021-11-10T02:59:36Z

consensus/state_processing/src/per_block_processing.rs

-                    .latest_execution_payload_header()?
-                    .block_number
-                    .safe_add(1)?,
-            BlockProcessingError::ExecutionBlockNumberIncontiguous {


We can also remove this error variant.

paulhauner · 2021-11-10T03:10:13Z

beacon_node/src/cli.rs

+                       that the broad Ethereum community has elected to override the terminal PoW block. \
+                       Incorrect use of this flag will cause your node to experience a consensus
+                       failure. Be extremely careful with this flag.")
+                .takes_value(true)


I think we should make a "requires" dependency between both "terminal-block-hash-override" and "terminal-block-hash-epoch-override" (i.e. you can't supply one without the other).

I can't see how they'd be effective without each other and it would be nice to fail early.

paulhauner · 2021-11-10T03:11:59Z

beacon_node/beacon_chain/src/block_verification.rs

-        // Gas used is less than the gas limit
-        if execution_payload.gas_used > execution_payload.gas_limit {
-            return Err(BlockError::ExecutionPayloadError(
-                ExecutionPayloadError::GasUsedExceedsLimit,


We can remove this GasUsedExceedsLimit error variant.

paulhauner · 2021-11-10T03:12:27Z

beacon_node/beacon_chain/src/block_verification.rs

-        // The execution payload block hash is not equal to the parent hash
-        if execution_payload.block_hash == execution_payload.parent_hash {
-            return Err(BlockError::ExecutionPayloadError(
-                ExecutionPayloadError::BlockHashEqualsParentHash,


We can remove this BlockHashEqualsParentHash error variant.

paulhauner · 2021-11-10T04:55:09Z

It looks like there's a consistent stack overflow on Windows 🤔

 test engine_api::http::test::execute_payload_request ... ok
test engine_api::http::test::consensus_validated_request ... ok
test engine_api::http::test::forkchoice_updated_request ... ok
test engine_api::http::test::get_block_by_hash_request ... ok
test engine_api::http::test::get_block_by_number_request ... ok
test engine_api::http::test::get_payload_request ... ok
test engine_api::http::test::prepare_payload_request ... ok
test engine_api::http::test::geth_test_vectors ... ok
test test::finds_valid_terminal_block_hash ... ok
test test::produce_three_valid_pos_execution_blocks ... ok
test test::rejects_invalid_terminal_block_hash ... ok
test test::rejects_unknown_terminal_block_hash ... ok
test test::verifies_valid_terminal_block_hash ... ok
test test_utils::execution_block_generator::test::pow_chain_only ... ok
memory allocation of 2147483650 bytes failed
error: test failed, to rerun pass '-p execution_layer --lib'

Caused by:
  process didn't exit successfully: `D:\a\lighthouse\lighthouse\target\release\deps\execution_layer-c989491669370dbf.exe` (exit code: 0xc0000409, STATUS_STACK_BUFFER_OVERRUN)
make: *** [Makefile:88: test-release] Error -1073740791

I'm in the middle of some things, but I'll try have a look. I assume it's due to recursion somewhere.

paulhauner · 2021-11-10T05:27:36Z

It looks like there's a consistent stack overflow on Windows thinking

Feel free to just leave this, we can pick it up in another PR.

- Require both "terminal-block-hash-epoch-override" and "terminal-block-hash-override" when either flag is used

* Fix arbitrary check kintsugi * Add merge chain spec fields, and a function to determine which constant to use based on the state variant * increment spec test version * Remove `Transaction` enum wrapper * Remove Transaction new-type * Remove gas validations * Add `--terminal-block-hash-epoch-override` flag * Increment spec tests version to 1.1.5 * Remove extraneous gossip verification ethereum/consensus-specs#2687 * - Remove unused Error variants - Require both "terminal-block-hash-epoch-override" and "terminal-block-hash-override" when either flag is used * - Remove a couple more unused Error variants Co-authored-by: Paul Hauner <paul@paulhauner.com>

realbigsean added 4 commits November 4, 2021 10:25

Fix arbitrary check kintsugi

baee9da

Add merge chain spec fields, and a function to determine which consta…

9c44e72

…nt to use based on the state variant

increment spec test version

2c6f1ad

Remove Transaction enum wrapper

b9ee00e

realbigsean added kintsugi 🍵 ready-for-review The code is ready for review labels Nov 4, 2021

realbigsean mentioned this pull request Nov 4, 2021

Kintsugi 🍵 #2768

Merged

realbigsean changed the title ~~1.1.4 merge specs~~ 1.1.4 merge spec tests Nov 4, 2021

pawanjay176 mentioned this pull request Nov 5, 2021

Fix Uint256 deserialization #2786

Merged

paulhauner reviewed Nov 5, 2021

View reviewed changes

consensus/state_processing/src/per_epoch_processing/altair/rewards_and_penalties.rs Show resolved Hide resolved

paulhauner and others added 3 commits November 5, 2021 09:45

Remove Transaction new-type

c21db0a

Merge branch 'kintsugi' of https://github.com/sigp/lighthouse into 1.…

561ff49

…1.4-merge-specs � Conflicts: � consensus/types/src/execution_payload.rs

Remove gas validations

d46d5d5

realbigsean added work-in-progress PR is a work-in-progress and removed ready-for-review The code is ready for review labels Nov 5, 2021

realbigsean added 2 commits November 6, 2021 10:24

Add --terminal-block-hash-epoch-override flag

3443ae8

Increment spec tests version to 1.1.5

1973082

realbigsean changed the title ~~1.1.4 merge spec tests~~ 1.1.5 merge spec tests Nov 9, 2021

realbigsean added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels Nov 9, 2021

Remove extraneous gossip verification ethereum/consensus-specs#2687

1762767

paulhauner requested changes Nov 10, 2021

View reviewed changes

paulhauner added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Nov 10, 2021

realbigsean added 2 commits November 10, 2021 09:34

- Remove unused Error variants

c73e2e5

- Require both "terminal-block-hash-epoch-override" and "terminal-block-hash-override" when either flag is used

- Remove a couple more unused Error variants

1eaf351

realbigsean merged commit efe0947 into sigp:kintsugi Nov 10, 2021

realbigsean deleted the 1.1.4-merge-specs branch November 21, 2023 16:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1.1.5 merge spec tests #2781

1.1.5 merge spec tests #2781

realbigsean commented Nov 4, 2021 •

edited

Loading

paulhauner left a comment

realbigsean commented Nov 5, 2021

paulhauner commented Nov 5, 2021 •

edited

Loading

paulhauner left a comment

paulhauner Nov 10, 2021

paulhauner Nov 10, 2021

paulhauner Nov 10, 2021

paulhauner Nov 10, 2021

paulhauner Nov 10, 2021

paulhauner commented Nov 10, 2021

paulhauner commented Nov 10, 2021

1.1.5 merge spec tests #2781

1.1.5 merge spec tests #2781

Conversation

realbigsean commented Nov 4, 2021 • edited Loading

Issue Addressed

Proposed Changes

Additional Info

paulhauner left a comment

Choose a reason for hiding this comment

realbigsean commented Nov 5, 2021

paulhauner commented Nov 5, 2021 • edited Loading

paulhauner left a comment

Choose a reason for hiding this comment

paulhauner Nov 10, 2021

Choose a reason for hiding this comment

paulhauner Nov 10, 2021

Choose a reason for hiding this comment

paulhauner Nov 10, 2021

Choose a reason for hiding this comment

paulhauner Nov 10, 2021

Choose a reason for hiding this comment

paulhauner Nov 10, 2021

Choose a reason for hiding this comment

paulhauner commented Nov 10, 2021

paulhauner commented Nov 10, 2021

realbigsean commented Nov 4, 2021 •

edited

Loading

paulhauner commented Nov 5, 2021 •

edited

Loading