-
Notifications
You must be signed in to change notification settings - Fork 20.2k
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.
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
all: nuke total difficulty #30744
base: master
Are you sure you want to change the base?
all: nuke total difficulty #30744
Conversation
@@ -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) |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
Hope you don't mind if I reopen this? Pretty sure we want it... |
Yeah, we need it and i will review it next week. |
Yep, I can add it back
Thanks and Best regards Gary rong
Martin HS ***@***.***>于2024年11月18日 周一下午3:43写道:
… ***@***.**** commented on this pull request.
------------------------------
In core/rawdb/database.go
<#30744 (comment)>
:
> @@ -409,7 +408,8 @@ func InspectDatabase(db ethdb.Database, keyPrefix, keyStart []byte) error {
case bytes.HasPrefix(key, blockReceiptsPrefix) && len(key) == (len(blockReceiptsPrefix)+8+common.HashLength):
receipts.Add(size)
case bytes.HasPrefix(key, headerPrefix) && bytes.HasSuffix(key, headerTDSuffix):
- tds.Add(size)
+ // The TD has been removed, but we still count it here
IMO better leave the tds.Add too, so we actually count the data, not just
ignore it. Otherwise the numbers won't match up
—
Reply to this email directly, view it on GitHub
<#30744 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABNO6OIOY2GCK5CFQE73OOT2BGLD5AVCNFSM6AAAAABRSNXYNWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDINBRG42TIMRXGM>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
The db inspector has been fixed. Check the output format above |
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. |
@@ -333,9 +328,6 @@ func (f *chainFreezer) freezeRange(nfdb *nofreezedb, number, limit uint64) (hash | |||
if err := op.AppendRaw(ChainFreezerReceiptTable, number, receipts); err != nil { | |||
return fmt.Errorf("can't write receipts to Freezer: %v", err) | |||
} | |||
if err := op.AppendRaw(ChainFreezerDifficultyTable, number, td); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we need to keep writing zero.
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. |
@barnabasbusa 'dropping' just means we would ignore it. |
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:
Questions: