Monorepo: Removal all TTD Logic from Libraries #3518

holgerd77 · 2024-07-18T11:55:25Z

Too radical? 😂

Could anyone already give this a first look, removed all TTD stuff from Commoon, lots of tests falling off the roof, but I would say: so what?

(so Common would be just a first step, this would go up the libraries then)

packages/common/src/common.ts

codecov · 2024-07-21T18:55:45Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 0.00%. Comparing base (fb50628) to head (394ae31).
Report is 17 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (fb50628) and HEAD (394ae31). Click for more details.

HEAD has 2 uploads less than BASE

Flag BASE (fb50628) HEAD (394ae31)

tx 1 0

client 1 0

Additional details and impacted files

Flag	Coverage Δ
client	`?`
tx	`?`
wallet	`0.00% <ø> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

acolytec3 · 2024-07-22T12:30:19Z

packages/client/src/miner/pendingBlock.ts

    if (typeof vm.blockchain.getTotalDifficulty !== 'function') {
      throw new Error('cannot get iterator head: blockchain has no getTotalDifficulty function')
    }


Do we still need this check?

I would find it nice if we are still able to at least run ethash blockchains, and this check might be necessary for it?

You are probably right. On a related note: should this still work npm run client:start:ts -- --dev=pow?

No idea about this dev flags

…l/420/files\#diff-556d0544d517a1fe1242173c7d23c7cb5f3153bad5eeec9293865df9eefd349aR164) exchangeTransitionConfigurationV1 engine API RPC method from client

jochem-brouwer

Some small-ish comments.

The mergeForkIdTransition: should we remove this? This is a special fork id, which specifically has to do with the PoW -> PoS transition logic which we essentially have removed here (see: https://eips.ethereum.org/EIPS/eip-3675#eip-2124-fork-identifier). Should we remove this hardfork as well? It has lead numerous times to confusion 😅 Since we don't support PoW -> PoS transition I think it is fine to remove it as well (can be done in a new PR)

Also side note: we will now thus fail any tests which have to do with PoW -> PoS transitions (there likely are not many, but there likely will be some in the ethereum/tests and in hive)

jochem-brouwer · 2024-08-03T14:05:12Z

packages/block/test/header.spec.ts

-        }),
-      'A base fee',
-      undefined,
-      'throws when RLP serialized block with no base fee on default hardfork (london) and setHardfork left undefined',


This test is removed but I don't think this has anything to do with TTD, so we should likely keep this in?

If I interpret this test correctly, this is testing some weird half-defined case for the more-complex TD including setHardfork option (which we now removed).

So, now, with the surrounding test setup, both when we set setHardfork to true as well as false we are going the normal ways to a minimally-London common (common provided is initialized with London) and so this test setup should not throw in both cases (also re-tested this now).

That's why I removed the test case, should be not testing anything meaningful now.

packages/client/src/rpc/modules/engine/engine.ts

packages/client/test/integration/merge.spec.backup.ts

packages/common/src/common.ts

packages/common/src/constructors.ts

jochem-brouwer · 2024-08-03T14:38:39Z

packages/common/test/utils.spec.ts

@@ -57,122 +55,24 @@ describe('[Utils/Parse]', () => {
    assert.equal(params.genesis.timestamp, '0x10', 'timestamp parsed correctly')
  })

-  it('should successfully parse kiln genesis and set forkhash', async () => {
-    const common = createCommonFromGethGenesis(gethGenesisKilnJSON, {


I don't think this one should be removed as well? Correctly setting the hardfork order? Maybe the mergeForkIdTransition should be removed here?

I tried to re-add the two tests you mentioned, but I am getting a "Error parsing parameters file: nonzero terminal total difficulty is not supported" error now when trying to re-add the Kiln config file, so I guess this is not so easy to re-create in this setup.

I would generally pledge that there are more simple test setups for HF order as well as getHardforkBy() functionality and this is sufficiently tested in other places and we therefore shouldn't stick to this super-outdated Kiln setup (I was somewhat happy to throw the Kiln config out) and just throw it out here.

(side mention: guess nothing urgent to ad hoc re-add, but I do wonder if we would rather want to keep the "Geth config with post-Merge transition but a dedicated Merge HF block included" case (so this would also match what we keep in other places) instead of fully throwing this out. Guess THAT case also should not produce significant overhead and still migth be useful. Again, but in doubt we can "re-add on demand" I guess).

jochem-brouwer · 2024-08-03T14:39:20Z

packages/common/test/utils.spec.ts

-
-    assert.equal(common.getHardforkBy({ blockNumber: 0n }), Hardfork.London, 'london at genesis')
-    assert.equal(
-      common.getHardforkBy({ blockNumber: 1n, td: 2n }),


Same for this test, we can remove all checks with have to do with td checks, but the getHardforkBy via either block or timestamp should still work.

See comment above

holgerd77 · 2024-08-05T09:36:20Z

Some small-ish comments.

The mergeForkIdTransition: should we remove this? This is a special fork id, which specifically has to do with the PoW -> PoS transition logic which we essentially have removed here (see: https://eips.ethereum.org/EIPS/eip-3675#eip-2124-fork-identifier). Should we remove this hardfork as well? It has lead numerous times to confusion 😅 Since we don't support PoW -> PoS transition I think it is fine to remove it as well (can be done in a new PR)

Let's be semantically precise here: we are only removing the Live transition from PoW -> PoS (at least that's my understanding from the basis of what we are doing and that's also what I would be suggesting), so that is this first-time transition where the fork block is not yet known and needs to be determined by TTD in a complicated manner.

We still do support a static transition (post Merge PIT, merge block known), and I personally would strongly pledge to keep it this way, since the remaining logic is easy to handle and not a substantial overhead and otherwise we would e.g. finally loose on a theoretical/practical mainnet full sync capability (still dreaming of this 😂).

For a static transition the mergeForkId HF is also needed, otherwise one would branch off with a missing mergeForkId HF during sync.

Ah, and while writing about it: the subsequent fork hashes would also be wrong, so we would loose the correct fork hash calculation (since in-between HFs are calculated in into newer ones). So we should definitely keep "as is" and live with some mild technical debt here (can remember a somewhat longer discussion/thread from Martin Holst Swende where he also complained about this, that this forkk would now be "in forever", but that's just how it is now. 🙂 )

Also side note: we will now thus fail any tests which have to do with PoW -> PoS transitions (there likely are not many, but there likely will be some in the ethereum/tests and in hive)

So, since ethereum/tests are passing here, this does not seem to be a problem at least for ethereum/tests (?).

acolytec3

LGTM. Lets fix that bracket later.

jochem-brouwer · 2024-08-05T14:07:44Z

So, since ethereum/tests are passing here, this does not seem to be a problem at least for ethereum/tests (?).

@holgerd77 This is because we do not run certain forks on the CI, in this case that would be the fork which actually has the Merge transition. (Just like we don't run ethereum/tests for Chainstart for instance on the CI)

holgerd77 · 2024-08-05T14:20:07Z

@jochem-brouwer ah, ok, thanks!

holgerd77 added PR state: WIP type: refactor package: common breaking labels Jul 18, 2024

g11tech reviewed Jul 18, 2024

View reviewed changes

packages/common/src/common.ts Show resolved Hide resolved

acolytec3 reviewed Jul 22, 2024

View reviewed changes

holgerd77 and others added 10 commits July 31, 2024 15:11

Remove all TTD from Common

ad20ebc

remove td references in comments

ed06604

Update block, simplify setHardfork option (boolean only)

e3bd18e

Fix Blockchain, remove td from checkAndTransitionHardforkByNumber()

43c3358

VM fixes

7159393

Remove unnecessary test scenarios from Blockchain

696daab

Remove deprecated (see https://github.com/ethereum/execution-apis/pul…

431bf48

…l/420/files\#diff-556d0544d517a1fe1242173c7d23c7cb5f3153bad5eeec9293865df9eefd349aR164) exchangeTransitionConfigurationV1 engine API RPC method from client

Remove terminal block validation from client

c2d35bc

Additional client fixes and td/ttd simplifications

4ba52d9

Clean up comments again

19df223

acolytec3 force-pushed the monorepo-remove-ttd-logic branch from 5ae8ba2 to 19df223 Compare July 31, 2024 19:17

acolytec3 and others added 12 commits August 1, 2024 17:20

accomodate ttdPassed flag in geth genesis [no ci]

27b6393

type [no ci]

ce4f242

disallow nonzero ttd in geth genesis params

88dfccc

Add ttdPassed to genesis params files

0f3f546

Revise skeleton sync tests

bc56242

remove obsolete tests [no ci]

556d93f

more fixes [no ci]

2940734

add todo on forkhash [no ci]

ce6c8a7

typo [no ci]

12ff16c

update newPayload

646979f

update setHardfork typing [no ci]

cf41f77

client: fix test

ba5a61b

jochem-brouwer added 2 commits August 2, 2024 22:01

client: lint

229975a

client: remove it.only

394ae31

jochem-brouwer reviewed Aug 3, 2024

View reviewed changes

holgerd77 added 4 commits August 5, 2024 12:38

Common constructor doc fix

585a23c

Remove Client merge test backup file

dcb211f

Minor

8f25c59

Lint fixes

958545d

holgerd77 added PR state: needs review and removed PR state: WIP labels Aug 5, 2024

acolytec3 approved these changes Aug 5, 2024

View reviewed changes

holgerd77 merged commit 3810f1d into master Aug 5, 2024
34 checks passed

holgerd77 deleted the monorepo-remove-ttd-logic branch August 5, 2024 13:35

acolytec3 mentioned this pull request Aug 5, 2024

Adjust ttd checks in parseGethParams #3556

Merged

holgerd77 added package: block package: blockchain package: vm package: client labels Oct 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Monorepo: Removal all TTD Logic from Libraries #3518

Monorepo: Removal all TTD Logic from Libraries #3518

holgerd77 commented Jul 18, 2024 •

edited

Loading

codecov bot commented Jul 21, 2024 •

edited

Loading

acolytec3 Jul 22, 2024

holgerd77 Jul 22, 2024

acolytec3 Jul 22, 2024

holgerd77 Jul 22, 2024

jochem-brouwer left a comment

jochem-brouwer Aug 3, 2024

holgerd77 Aug 5, 2024

jochem-brouwer Aug 3, 2024

holgerd77 Aug 5, 2024

jochem-brouwer Aug 3, 2024

holgerd77 Aug 5, 2024

holgerd77 commented Aug 5, 2024 •

edited

Loading

acolytec3 left a comment

jochem-brouwer commented Aug 5, 2024

holgerd77 commented Aug 5, 2024

Monorepo: Removal all TTD Logic from Libraries #3518

Monorepo: Removal all TTD Logic from Libraries #3518

Conversation

holgerd77 commented Jul 18, 2024 • edited Loading

codecov bot commented Jul 21, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jochem-brouwer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holgerd77 commented Aug 5, 2024 • edited Loading

acolytec3 left a comment

Choose a reason for hiding this comment

jochem-brouwer commented Aug 5, 2024

holgerd77 commented Aug 5, 2024

holgerd77 commented Jul 18, 2024 •

edited

Loading

codecov bot commented Jul 21, 2024 •

edited

Loading

holgerd77 commented Aug 5, 2024 •

edited

Loading