Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

all: nuke total difficulty #30744

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

all: nuke total difficulty #30744

wants to merge 11 commits into from
189 −672

Conversation

karalabe
Copy link
Member

@karalabe karalabe commented Nov 11, 2024 •
edited
Loading

The total difficulty is the sum of all block difficulties from genesis to a certain block. This value was used in PoW for deciding which chain is heavier, and thus which chain to select. Since PoS has a different fork selection algorithm, all blocks since the merge have a difficulty of 0, and all total difficulties are the same for the past 2 years.

Whilst the TDs are mostly useless nowadays, there was never really a reason to mess around removing them since they are so tiny. This reasoning changes when we go down the path of pruned chain history. In order to reconstruct any TD, we must retrieve all the headers from chain head to genesis and then iterate all the difficulties to compute the TD.

In a world where we completely prune past chain segments (bodies, receipts, headers), it is not possible to reconstruct the TD at all. In a world where we still keep chain headers and prune only the rest, reconstructing it possible as long as we process (or download) the chain forward from genesis, but trying to snap sync the head first and backfill later hits the same issue, the TD becomes impossible to calculate until genesis is backfilled.

All in all, the TD is a messy out-of-state, out-of-consensus computed field that is overall useless nowadays, but code relying on it forces the client into certain modes of operation and prevents other modes or other optimizations. This PR completely nukes out the TD from the node. It doesn't compute it, it doesn't operate on it, it's as if it didn't even exist.

Caveats:

  • Whenever we have APIs that return TD (devp2p handshake, tracer, etc.) we return a TD of 0.
  • For era files, we recompute the TD during export time (fairly quick) to retain the format content.
  • It is not possible to "verify" the merge point (i.e. with TD gone, TTD is useless). Since we're not verifying PoW any more, just blindly trust it, not verifying but blindly trusting the many year old merge point seems just the same trust model.
  • Our tests still need to be able to generate pre and post merge blocks, so they need a new way to split the merge without TTD. The PR introduces a settable ttdBlock field on the consensus object which is used by tests as the block where originally the TTD happened. This is not needed for live nodes, we never want to generate old blocks.
  • One merge transition consensus test was disabled. With a non-operational TD, testing how the client reacts to TTD is useless, it cannot react.

Questions:

  • Should we also drop total terminal difficulty from the genesis json? It's a number we cannot react on any more, so maybe it would be cleaner to get rid of even more concepts.

holiman
holiman reviewed Nov 12, 2024
View reviewed changes
core/blockchain.go
@@ -1447,20 1437,14 @@ func (bc *BlockChain) writeKnownBlock(block *types.Block) error {
// writeBlockWithState writes block, metadata and corresponding state data to the
// database.
func (bc *BlockChain) writeBlockWithState(block *types.Block, receipts []*types.Receipt, statedb *state.StateDB) error {
// Calculate the total difficulty of the block
ptd := bc.GetTd(block.ParentHash(), block.NumberU64()-1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note, this is a backwards-incompatible change. Once blocks without TD have been written, users cannot switch back to an earlier version, right?
And if so, perhaps we need to version-bump after merging this PR ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also bump the db version number?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If so, i would like to include my freezer pr as well. It also needs a db version bump

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is 100% a one directional move, we should bump both.

@karalabe karalabe closed this Nov 15, 2024
@holiman
Copy link
Contributor

holiman commented Nov 16, 2024

Hope you don't mind if I reopen this? Pretty sure we want it...

@holiman holiman reopened this Nov 16, 2024
@rjl493456442
Copy link
Member

Yeah, we need it and i will review it next week.

holiman
holiman reviewed Nov 18, 2024
View reviewed changes
core/rawdb/database.go Outdated Show resolved Hide resolved
@rjl493456442
Copy link
Member

rjl493456442 commented Nov 18, 2024 via email

@rjl493456442
Copy link
Member 

 ----------------------- --------------------------- ------------ -------- 
|       DATABASE        |         CATEGORY          |    SIZE    | ITEMS  |
 ----------------------- --------------------------- ------------ -------- 
| Key-Value store       | Headers                   | 576.00 B   |      1 |
| Key-Value store       | Bodies                    | 44.00 B    |      1 |
| Key-Value store       | Receipt lists             | 42.00 B    |      1 |
| Key-Value store       | Difficulties (deprecated) | 212.00 B   |      3 |
| Key-Value store       | Block number->hash        | 453.00 B   |      3 |
| Key-Value store       | Block hash->number        | 11.19 MiB  | 286075 |
| Key-Value store       | Transaction index         | 0.00 B     |      0 |
| Key-Value store       | Bloombit index            | 5.82 MiB   | 141382 |
| Key-Value store       | Contract codes            | 1.30 MiB   |    743 |
| Key-Value store       | Hash trie nodes           | 11.45 MiB  |  95278 |
| Key-Value store       | Path trie state lookups   | 0.00 B     |      0 |
| Key-Value store       | Path trie account nodes   | 0.00 B     |      0 |
| Key-Value store       | Path trie storage nodes   | 0.00 B     |      0 |
| Key-Value store       | Verkle trie nodes         | 0.00 B     |      0 |
| Key-Value store       | Verkle trie state lookups | 0.00 B     |      0 |
| Key-Value store       | Trie preimages            | 547.13 KiB |   8893 |
| Key-Value store       | Account snapshot          | 1.69 MiB   |  36542 |
| Key-Value store       | Storage snapshot          | 2.52 MiB   |  35341 |
| Key-Value store       | Beacon sync headers       | 0.00 B     |      0 |
| Key-Value store       | Clique snapshots          | 0.00 B     |      0 |
| Key-Value store       | Singleton metadata        | 697.91 KiB |     11 |
| Light client          | CHT trie nodes            | 14.92 MiB  | 139827 |
| Light client          | Bloom trie nodes          | 237.77 KiB |   1777 |
| Ancient store (Chain) | Headers                   | 82.31 MiB  | 286075 |
| Ancient store (Chain) | Hashes                    | 10.37 MiB  | 286075 |
| Ancient store (Chain) | Bodies                    | 33.41 MiB  | 286075 |
| Ancient store (Chain) | Receipts                  | 10.95 MiB  | 286075 |
 ----------------------- --------------------------- ------------ -------- 
|                                   TOTAL           | 187.37 MIB |        |
 ----------------------- --------------------------- ------------ --------

The db inspector has been fixed. Check the output format above

rjl493456442
rjl493456442 previously approved these changes Nov 19, 2024
View reviewed changes
@rjl493456442 rjl493456442 added this to the 1.15.0 milestone Nov 20, 2024
@fjl
Copy link
Contributor

fjl commented Nov 21, 2024

We need to ensure that downgrading to a previous version of Geth remains possible. The change will be backwards-incompatible if we delete the TD freezer table. So we should keep this table and write zero for now. At the next major release, v1.15.0, we can drop the TD freezer table.

The downgrade path needs to be tested w.r.t. this change to database, and peering with older versions also needs to be tested.

@fjl fjl removed the status:triage label Nov 21, 2024
@barnabasbusa
Copy link
Member

Should we also drop total terminal difficulty from the genesis json? It's a number we cannot react on any more, so maybe it would be cleaner to get rid of even more concepts.

lots of other clients also rely on genesis.json file to init a custom network, like erigon and reth. Even if you drop this value, we would need to coordinate with other clients that also use genesis.json format to also drop this value.

@fjl
Copy link
Contributor

fjl commented Nov 21, 2024

@barnabasbusa 'dropping' just means we would ignore it.

@rjl493456442 rjl493456442 modified the milestones: 1.15.0, 1.14.13 Nov 22, 2024
fjl
fjl reviewed Nov 22, 2024
View reviewed changes
core/rawdb/accessors_chain.go Outdated Show resolved Hide resolved
@rjl493456442
Copy link
Member

Tests are failing, i will fix them next week

@rjl493456442
Copy link
Member

@fjl I still prefer to bump the database version for this PR.

For freezer TD table, it's for sure we can write zero into it, however, once we downgrade
to legacy version, the TD in key-value store won't be found and blockchain will report
return consensus.ErrUnknownAncestor.

Personally I think it's not worthwhile to add the complexity for backward compatibility
and we can include it in the next major release.

@fjl fjl modified the milestones: 1.14.13, 1.15.0 Nov 28, 2024
@fjl fjl added status:marinating PR hasn't been open long enough to get merged and removed status:marinating PR hasn't been open long enough to get merged labels Dec 3, 2024
@Rjected Rjected mentioned this pull request Dec 19, 2024
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@s1na s1na s1na left review comments

@holiman holiman holiman left review comments

@maoueh maoueh maoueh left review comments

@fjl fjl fjl left review comments

@rjl493456442 rjl493456442 rjl493456442 left review comments

@lightclient lightclient Awaiting requested review from lightclient lightclient is a code owner

@gballet gballet Awaiting requested review from gballet

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
status:marinating PR hasn't been open long enough to get merged
Projects
None yet
Milestone
1.15.0
Development

Successfully merging this pull request may close these issues.
7 participants
@karalabe @holiman @rjl493456442 @fjl @barnabasbusa @maoueh @s1na